feat: Expose the OIDC JWKS tunables through the operator - #6690
Open
larrysingleton007 wants to merge 3 commits into
Open
feat: Expose the OIDC JWKS tunables through the operator#6690larrysingleton007 wants to merge 3 commits into
larrysingleton007 wants to merge 3 commits into
Conversation
Follow-up to feast-dev#6683, requested in its review. Add jwksCacheLifespanSeconds and jwksRequestTimeoutSeconds to OidcAuthz as CR fields rather than OIDC Secret keys: these are non-secret operational knobs, so they belong with verifySSL and caCertConfigMap rather than in the Secret bag that carries IdP-coupled credentials. Both are optional pointers with a Minimum=1 constraint mirroring the SDK's validation, and are omitted from the generated feature_store.yaml when unset so the SDK defaults apply rather than the operator asserting its own. Regenerates deepcopy, CRD bases, dist/install.yaml, and the API reference. Documents that the cache lifespan is not purely a performance setting: it also bounds how long a key the provider revoked keeps validating tokens. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
Code review findings on the JWKS tunables change. The OLM bundle CRD was not regenerated, so it lacked both new fields while config/crd/bases and dist/install.yaml carried them. Every other OidcAuthz field is present in all three copies, and no PR workflow runs make bundle, so CI would not have caught it: an OLM install would have pruned the settings silently rather than failing. Regenerated with make bundle; operator-sdk bundle validate passes. Assert the client repo config omits both keys. OidcClientAuthConfig inherits the same strict validation, so mirroring the forwarding into the client path would break every client pod, and nothing tested it. Also restore the neighbouring blocks' length assertion so a leaked parameter fails. Add CRD validation tests for the Minimum=1 constraints the docs promise. Docs: caCertConfigMap was documented as a bare string but the CRD requires an object with a name key, so the whole snippet failed to apply, including the lines added here. Name the required Feast version instead of implying any newer image works. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6690 +/- ##
=======================================
Coverage 46.77% 46.77%
=======================================
Files 414 414
Lines 50191 50191
Branches 7181 7181
=======================================
Hits 23475 23475
Misses 25077 25077
Partials 1639 1639
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it
Follow-up to #6683, which you asked for in that review. #6683 added
jwks_cache_lifespan_secondsandjwks_request_timeout_secondstoOidcAuthConfig; this exposes them through the operator so they are reachable from a FeatureStore CR.They are added as CR fields on
OidcAuthzrather than OIDC Secret keys, because they are non-secret operational knobs and belong withverifySSLandcaCertConfigMaprather than in the Secret that carries IdP credentials. Wiring follows the existingVerifySSLpattern.Both are optional pointers with a
Minimum=1constraint mirroring the SDK's validation, and both are omitted from the generatedfeature_store.yamlwhen unset so the SDK's own defaults apply rather than the operator asserting a competing set.Worth flagging for review:
jwksCacheLifespanSecondsis not purely a performance setting. It also bounds how long a key the provider has revoked continues to validate tokens, so the docs describe that tradeoff rather than presenting it as a throughput dial.Regenerated artifacts: deepcopy, CRD bases,
dist/install.yaml, the API reference, and the OLM bundle. The bundle is a separatemake bundlestep thatmake manifestsdoes not cover and no PR workflow runs, so it is easy to miss; without it an OLM install silently prunes the new fields rather than failing.Testing: unit coverage asserts the keys are absent when unset and forwarded with correct values when set, plus that the client config omits them (it inherits the same strict validation). CRD validation tests cover the
Minimum=1constraints.go build ./...,gofmt, and all four operator test packages pass.One docs fix included: the
caCertConfigMapexample in the same block was written as a bare string, but the CRD requires an object with anamekey, so that snippet failed to apply as written.Which issue(s) this PR fixes
Fixes #6686