feat: make oidc discovery url configurable #8145
Conversation
knative-prow
bot
added
the
size/M
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Cali0707 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow
bot
added
the
approved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8145 +/- ##
==========================================
- Coverage 67.47% 67.37% -0.10%
==========================================
Files 371 371
Lines 18036 18068 +32
==========================================
+ Hits 12169 12173 +4
- Misses 5088 5114 +26
- Partials 779 781 +2 ☔ View full report in Codecov by Sentry. |
pkg/auth/token_verifier.go
Outdated
| @@ -57,14 +53,14 @@ type IDToken struct { | |||
| AccessTokenHash string | |||
| } | |||
|
|
|||
| func NewOIDCTokenVerifier(ctx context.Context) *OIDCTokenVerifier { | |||
| func NewOIDCTokenVerifier(ctx context.Context, features feature.Flags) *OIDCTokenVerifier { | |||
There was a problem hiding this comment.
Is there a reason, for not getting the feature store from the given context?
There was a problem hiding this comment.
This made it easier to re-create the verifier in the callback functions when the config-features cm changes
|
/cc @creydr |
knative-prow-robot
added
needs-rebase
|
/assign @creydr Can you take a look? |
|
@creydr can you take a look? |
|
/cc |
creydr
force-pushed
the
add-oidc-discovery-config
branch
from
6ff7de7
to
3f2dfa6
Compare
|
Rebased after #8195 |
Signed-off-by: Calum Murray <[email protected]>
creydr
force-pushed
the
add-oidc-discovery-config
branch
from
3f2dfa6
to
837d926
Compare
knative-prow
bot
added
size/L
|
@Cali0707, on testing with This was because the used HTTP client of the TokenVerifier (now only Verifier) only took the CA certs from the API server. I addressed this in eee1320 (which should support Trustbundles too). |
|
I can also check on #8145 (comment) and get the features from the ctx in |
Would this work if the features were updated to have a new url? |
When we make sure we pass a context, which has the features (e.g. featureStore.ToContext(ctx)), I'd guess so Anyhow not totally sure about it, as having it as a parameter shows directly, that this is required for the function call 🤔 |
|
@pierDipi As I also committed to this PR, could you give it a quick review from your side too? |
| featureStore := feature.NewStore(logging.FromContext(ctx).Named("feature-config-store"), func(name string, value interface{}) { | ||
| logger.Info("Updated", zap.String("name", name), zap.Any("value", value)) | ||
| if flags, ok := value.(feature.Flags); ok && h != nil { | ||
| h.authVerifier = auth.NewVerifier(ctx, eventpolicyinformer.Get(ctx).Lister(), trustBundleConfigMapLister, flags) |
There was a problem hiding this comment.
This is changing a variable which is shared across goroutines, we might want to use a mutex or atomic handles
Now that I think more, maybe adding this "update logic" to the verifier might encapsulate that and we won't duplicate "multi-threading" everywhere this is used
There was a problem hiding this comment.
@Cali0707 just as an idea, you could pass a configmapwatcher to auth.NewVerifier(), which then retriggers the initOIDCProvider() on changes
There was a problem hiding this comment.
I like that idea @creydr - let me try this tomorrow
Fixes #8121
Proposed Changes
Release Note