-
Notifications
You must be signed in to change notification settings - Fork 141
fix oidc and token exchange config for k8s #2537
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2537 +/- ##
==========================================
- Coverage 55.99% 55.95% -0.05%
==========================================
Files 316 316
Lines 29748 29766 +18
==========================================
- Hits 16658 16655 -3
- Misses 11636 11652 +16
- Partials 1454 1459 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull Request Overview
This PR enables consuming the resource URL for CRD (Custom Resource Definition) configurations by introducing a mechanism to append middleware configurations incrementally in the operator context.
- Introduced
WithAppendMiddlewareConfigfunction to append individual middleware configs instead of replacing the entire list - Updated
AddExternalAuthConfigOptionsto use the append pattern for token exchange middleware - Enhanced
AddOIDCConfigOptionsto create and append auth middleware configuration for OAuth discovery endpoint support
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/runner/config_builder.go | Added WithAppendMiddlewareConfig function to support incremental middleware configuration |
| cmd/thv-operator/pkg/controllerutil/tokenexchange.go | Changed to use WithAppendMiddlewareConfig instead of WithMiddlewareConfig to preserve existing middlewares |
| cmd/thv-operator/pkg/controllerutil/oidc.go | Added auth middleware creation to enable authInfoHandler for OAuth discovery endpoints and support ResourceURL field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@claude please review. wasn't there any other opportunity for code reuse, or any other optimization? |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
JAORMX
left a comment
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.
Uhm... shouldn't this come from the runconfig we form? Ideally it should already be formed that way from there.
ef9a7ac to
6207348
Compare
blkt
left a comment
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 won't block this because I don't get the full scope of the change, but I'm a bit concerned by some of the added code, could you clarify the 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.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
15329fc to
53f1133
Compare
|
@yrobla could it be that we're not seeing yo the middleware correctly in the proxyrunner? Then the changes would go there instead of the general runner package cc @blkt @ChrisJBurns |
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.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
proxyrunner should be picking the config from runconfig as well, right? |
451c4e3 to
bb4421a
Compare
9965f1d to
5c67232
Compare
5c67232 to
b56d807
Compare
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.
Pull Request Overview
Copilot reviewed 16 out of 18 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.