Use both lookup.ClusterNameFrom and request.ClusterNameFrom to retreive cluster name#4064
Conversation
kcp-ci-bot
added
release-note-none
There was a problem hiding this comment.
Pull request overview
This PR fixes TokenReview “extra” fields for per-workspace authentication by retrieving the logical cluster name from the apiserver request context (available during authentication), rather than from a front-proxy/middleware-specific context key.
Changes:
- Switch cluster name lookup in the workspace authenticator to
request.ClusterNameFrom. - Extend the OIDC TokenReview e2e test to assert the expected cluster-related
User.Extrafields.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
pkg/authentication/authenticators.go |
Uses apiserver request context (request.ClusterNameFrom) when populating cluster-scoped TokenReview extras. |
test/e2e/authentication/workspace_test.go |
Adds assertions that TokenReview responses include expected authentication.kcp.io/scopes and authentication.kcp.io/cluster-name extras. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ntnn
force-pushed
the
fix-user-extra
branch
2 times, most recently
from
14f2c34 to
168feac
Compare
|
LGTM label has been added. DetailsGit tree hash: 7cc23d52316429e01faf1e5d086b5758dee39c5f |
kcp-ci-bot
added
the
approved
|
/cherry-pick 0.31 |
|
@ntnn: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: 53c13535dcb1bb68b25ba5d9a08107c191005b9d |
kcp-ci-bot
added
size/M
|
/cherry-pick release-0.31 |
|
@ntnn: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ve cluster name withClusterScope is used both in the front-proxy and shards. Both set the cluster name on different context keys due to differing handler chains and middlewares. On top of that synthetic requests like a TokenReview only pass the authentication chain only have the cluster on the context key from the request package - not from the lookup package.
ntnn
changed the title
|
/test pull-kcp-test-e2e-multiple-runs flake on unrelated tests |
|
LGTM label has been added. DetailsGit tree hash: b4c549d92f1d7f6118bca0357e3da75a7f32c5c6 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mjudeikis The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@ntnn: cannot checkout DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@ntnn: new pull request created: #4068 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Summary
withClusterScope is used both in the front-proxy and shards.
Both set the cluster name on different context keys due to
differing handler chains and middlewares.
On top of that synthetic requests like a TokenReview only
pass the authentication chain only have the cluster on the
context key from the request package - not from the lookup
package.
Related: #4061
What Type of PR Is This?
/kind bug
Related Issue(s)
Fixes #
Release Notes