chore: handle empty or invalid secrets nicely
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.
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).
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.
Cc @katrogan
Hey @pingsutw is there anything you need to get this PR merged?
@trutx sorry I missed it. Could you merge the master into your branch?
I did, but tests seem to be stuck? Maybe you need to reapprove @pingsutw ?
Wow this should be small but has a lot of files impacted?
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.
@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.
This CI check failure is going to be fixed by https://github.com/flyteorg/flytesnacks/pull/1670.
Congrats on merging your first pull request! 🎉