-
Notifications
You must be signed in to change notification settings - Fork 588
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: make oidc discovery url configurable #8145
base: main
Are you sure you want to change the base?
Conversation
[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 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8145 +/- ##
==========================================
- Coverage 67.47% 66.55% -0.92%
==========================================
Files 371 371
Lines 18036 18291 +255
==========================================
+ Hits 12169 12174 +5
- Misses 5088 5336 +248
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason, for not getting the feature store from the given context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This made it easier to re-create the verifier in the callback functions when the config-features cm changes
/cc @creydr |
/assign @creydr Can you take a look? |
@creydr can you take a look? |
/cc |
6ff7de7
to
3f2dfa6
Compare
Rebased after #8195 |
Signed-off-by: Calum Murray <cmurray@redhat.com>
3f2dfa6
to
837d926
Compare
@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? |
cmd/jobsink/main.go
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that idea @creydr - let me try this tomorrow
… of feature flags Signed-off-by: Cali0707 <calumramurray@gmail.com>
// provider is valid, update it | ||
v.provider = provider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we potentially update this from multiple goroutines, can we add a mutex around it?
Fixes #8121
Proposed Changes
Release Note