Fix TokenReviews when using WorkspaceAuthentication #3624
Merged
Conversation
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
kcp-ci-bot
added
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
kcp-ci-bot
added
the
size/L
|
/test all |
|
/test all |
|
/retest |
1 similar comment
|
/retest |
kcp-ci-bot
removed
the
do-not-merge/work-in-progress
|
/retest |
|
what is going on /retest |
|
/retest |
1 similar comment
|
/retest |
|
LGTM label has been added. Git tree hash: ec0dd348203a2a35a800c02ad66cf4e57fc03d19
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: embik 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 |
kcp-ci-bot
added
the
approved
|
/retest |
kcp-ci-bot
added
the
needs-rebase
…eviews to be handled The original design for per-workspace-auth was in the front-proxy only, only later did I add it to each shard, primarily to make writing e2e tests easier. In the front-proxy, there is a chain of middleware handlers that * resolve the cluster path * find the authenticator for a workspace * inject the authenticator into the request context * the handle the "optional auth", where the authenticator is read from the context again This means the actual authenticator used (i.e. in genericConfig.Authentication.Authenticator) is one that simply reads the authenticator from step 3 (out of the context) and calls it. In the shard variant, this concept was kept the same, i.e. there are handlers that are involved to make the per-workspace-authenticator work. This however means that any other component in kcp that uses the .Authenticator has one that has no concept of per-workspace auth anymore, since the middlewares are not being called. Thankfully the concept of middlewares is not required to make per-workspace-auth working on the shard level, so this commit condenses the logic on the shards to instead provide a "standalone" authenticator that does all steps listed above in one big function. Required for this refactoring is untangling the previous LocalProxy initialisation routine. Now the cluster index is started separatedly (since the new standalone auther needs it) and then handed into the middleware. On-behalf-of: @SAP [email protected] Signed-off-by: Marvin Beckers <[email protected]>
This test will only work in sharded setups if and when we extend the local workspace authenticator to use replication, so it can see other shards. On-behalf-of: @SAP [email protected] Signed-off-by: Marvin Beckers <[email protected]>
kcp-ci-bot
removed
the
needs-rebase
|
/retest |
embik
approved these changes
Oct 16, 2025
|
LGTM label has been added. Git tree hash: 71602157249b683d0cd5ed0011b7bb80391cd73d
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Summary
The original design for per-workspace-auth was in the front-proxy only, only later did I add it to each shard, primarily to make writing e2e tests easier.
In the front-proxy, there is a chain of middleware handlers that
This means the actual authenticator used (i.e. in genericConfig.Authentication.Authenticator) is one that simply reads the authenticator from step 3 (out of the context) and calls it.
In the shard variant, this concept was kept the same, i.e. there are handlers that are involved to make the per-workspace-authenticator work. This however means that any other component in kcp that uses the .Authenticator has one that has no concept of per-workspace auth anymore, since the middlewares are not being called.
Thankfully the concept of middlewares is not required to make per-workspace-auth working on the shard level, so this commit condenses the logic on the shards to instead provide a "standalone" authenticator that does all steps listed above in one big function. This required the Kube patch introduced via #3622 (kcp-dev/kubernetes#180)
Required for this refactoring is untangling the previous LocalProxy initialisation routine. Now the cluster index is started separatedly (since the new standalone auther needs it) and then handed into the middleware.
What Type of PR Is This?
/kind bug
Related Issue(s)
Fixes #3614
Release Notes