flyte icon indicating copy to clipboard operation
flyte copied to clipboard

chore: handle empty or invalid secrets nicely

Open trutx opened this issue 1 year ago • 6 comments

Tracking issue

Closes #4800

Why are the changes needed?

So flyteadmin errors out nicely and with information about the error instead of with a stack trace, which is difficult to follow for people not used to code.

What changes were proposed in this pull request?

I'm adding some missing error handling.

How was this patch tested?

Having both an empty and an invalid PrivateKeyPEM variable resulted in the stack trace below:

time="2024-01-31T13:19:12Z" level=info msg="Using config file: [/etc/flyte/config/flyteadmin_config.yaml]"
{"data":{"src":"service.go:71"},"message":"setting metrics keys to [project domain wf task phase tasktype runtime_type runtime_version app_name]","severity":"INFO","timestamp":"2024-01-31T13:19:12Z"}
{"data":{"src":"service.go:302"},"message":"Serving Flyte Admin Insecure","severity":"INFO","timestamp":"2024-01-31T13:19:12Z"}
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x205939c]

goroutine 1 [running]:
github.com/flyteorg/flyte/flyteadmin/auth/authzserver.NewProvider({_, _}, {{0x0, 0x0}, {0x1a3185c5000}, {0x34630b8a000}, {0x45d964b800}, {0xc000bfdd10, 0x13}, {0xc000bfdd28, ...}, ...}, ...)
	/go/src/github.com/flyteorg/flyteadmin/auth/authzserver/provider.go:163 +0x33c
github.com/flyteorg/flyte/flyteadmin/pkg/server.serveGatewayInsecure({0x31c9678?, 0xc00013a000}, 0x4871340?, 0xc000225680, 0xc0001d2600, 0xc000f55bc0?, 0xc000f55c28?, {0x31e5328, 0xc0013ee370})
	/go/src/github.com/flyteorg/flyteadmin/pkg/server/service.go:316 +0x1f2
github.com/flyteorg/flyte/flyteadmin/pkg/server.Serve({0x31c9678, 0xc00013a000}, 0x0?, 0x0?)
	/go/src/github.com/flyteorg/flyteadmin/pkg/server/service.go:62 +0x19f
github.com/flyteorg/flyte/flyteadmin/cmd/entrypoints.glob..func7(0x4882b60?, {0x2bbada5?, 0x2?, 0x2?})
	/go/src/github.com/flyteorg/flyteadmin/cmd/entrypoints/serve.go:44 +0x1f2
github.com/spf13/cobra.(*Command).execute(0x4882b60, {0xc0001a7c00, 0x2, 0x2})
	/go/pkg/mod/github.com/spf13/[email protected]/command.go:940 +0x862
github.com/spf13/cobra.(*Command).ExecuteC(0x4883f80)
	/go/pkg/mod/github.com/spf13/[email protected]/command.go:1068 +0x3bd
github.com/spf13/cobra.(*Command).Execute(...)
	/go/pkg/mod/github.com/spf13/[email protected]/command.go:992
github.com/flyteorg/flyte/flyteadmin/cmd/entrypoints.Execute(0x60?)
	/go/src/github.com/flyteorg/flyteadmin/cmd/entrypoints/root.go:49 +0x3a
main.main()
	/go/src/github.com/flyteorg/flyteadmin/cmd/main.go:12 +0x85

This refers to these fields in the flyteadmin_config.yaml file:

  • .auth.appAuth.selfAuthServer.tokenSigningRSAKeySecretName
  • .clusters.clusterConfigs[].auth.certPath

Setting valid PEMs in both cases solved the issue. Values were read from files mounted in the pod from a Kubernetes Secret, so the actual fix was to correct the value in the Secret.

Setup process

Set an empty environment variable or an invalid value in the Secret.

Check all the applicable boxes

  • [ ] I updated the documentation accordingly.
  • [x] All new and existing tests passed.
  • [x] All commits are signed-off.

trutx avatar Jan 31 '24 16:01 trutx

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

welcome[bot] avatar Jan 31 '24 16:01 welcome[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 61.10%. Comparing base (ccf9473) to head (0238be8).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4801      +/-   ##
==========================================
+ Coverage   61.08%   61.10%   +0.02%     
==========================================
  Files         794      794              
  Lines       51203    51213      +10     
==========================================
+ Hits        31277    31295      +18     
+ Misses      17043    17037       -6     
+ Partials     2883     2881       -2     
Flag Coverage Δ
unittests-datacatalog 69.31% <ø> (ø)
unittests-flyteadmin 58.90% <100.00%> (+0.08%) :arrow_up:
unittests-flytecopilot 17.79% <ø> (ø)
unittests-flytectl 68.30% <ø> (ø)
unittests-flyteidl 79.30% <ø> (ø)
unittests-flyteplugins 61.94% <ø> (ø)
unittests-flytepropeller 57.32% <ø> (ø)
unittests-flytestdlib 65.75% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 05 '24 06:02 codecov[bot]

Cc @katrogan

kumare3 avatar Feb 05 '24 07:02 kumare3

Hey @pingsutw is there anything you need to get this PR merged?

trutx avatar Feb 27 '24 11:02 trutx

@trutx sorry I missed it. Could you merge the master into your branch?

pingsutw avatar Apr 17 '24 06:04 pingsutw

I did, but tests seem to be stuck? Maybe you need to reapprove @pingsutw ?

trutx avatar Apr 17 '24 08:04 trutx

Wow this should be small but has a lot of files impacted?

kumare3 avatar May 06 '24 14:05 kumare3

Wow this should be small but has a lot of files impacted?

Used the github "sync fork" button to merge from upstream/master. I guess a rebase was a better option. 1 sec.

trutx avatar May 06 '24 14:05 trutx

@neverett , do you know what's up with this failure in docs?

The error:

/home/runner/work/flyte/flyte/flyte/docs/flytesnacks/examples/pandera_plugin/basic_schema_example.md:10006:undefined label: 'pandera:dataframe_models'

That label hasn't changed in a while.

eapolinario avatar May 06 '24 15:05 eapolinario

This CI check failure is going to be fixed by https://github.com/flyteorg/flytesnacks/pull/1670.

eapolinario avatar May 06 '24 17:05 eapolinario

Congrats on merging your first pull request! 🎉

welcome[bot] avatar May 06 '24 18:05 welcome[bot]