Fix regressions in Configuration.get_api_key_with_prefix bearer-token prefix lookup#2618
Conversation
|
|
|
Welcome @HaimLC! |
PR 2604 restored the api_key['authorization'] fallback in auth_settings()
by calling get_api_key_with_prefix('BearerToken', alias='authorization'),
but get_api_key_with_prefix only applied `alias` to the api_key lookup, not
the api_key_prefix lookup. Callers that set the prefix under the legacy
'authorization' key (rather than embedding "Bearer " directly in the token)
get a bearer token with no "Bearer " prefix, which most API servers (and
GKE's anonymous-auth fallback) don't recognize as a valid Authorization
header -> system:anonymous.
Apply the same alias fallback to the prefix lookup, symmetrically in the
sync (kubernetes/client/configuration.py) and async
(kubernetes/aio/client/configuration.py) variants, matching the pattern
PR 2604 already established for the key lookup.
See kubernetes-client#2592
Signed-off-by: HaimLC <110099998+HaimLC@users.noreply.github.com>
1b5a8f8 to
5a8b6a5
Compare
There was a problem hiding this comment.
Pull request overview
Fixes a regression in Configuration.get_api_key_with_prefix() where the alias= fallback was applied to api_key but not to api_key_prefix, causing legacy bearer-token configurations (token and "Bearer" prefix stored separately under 'authorization') to emit an unprefixed Authorization header value.
Changes:
- Apply
aliasfallback toapi_key_prefixlookup in the syncConfiguration.get_api_key_with_prefix(). - Apply the same
aliasfallback toapi_key_prefixlookup in the asyncaioConfiguration.get_api_key_with_prefix(). - Add a regression test covering the legacy split token/prefix case for the sync configuration.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| kubernetes/client/configuration.py | Adds alias fallback to the prefix lookup so legacy 'authorization' prefix keys are honored. |
| kubernetes/aio/client/configuration.py | Mirrors the same alias fallback behavior in the async configuration implementation. |
| kubernetes/test/test_api_client.py | Adds a regression test ensuring split api_key/api_key_prefix under 'authorization' produces a properly prefixed bearer value. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| key = self.api_key.get(identifier, self.api_key.get(alias) if alias is not None else None) | ||
| if key: | ||
| prefix = self.api_key_prefix.get(identifier) | ||
| prefix = self.api_key_prefix.get( | ||
| identifier, self.api_key_prefix.get(alias) if alias is not None else None) | ||
| if prefix: |
There was a problem hiding this comment.
could you please add a test case for async, similar to the sync case?
There was a problem hiding this comment.
sorry didnt saw it in conversation :) fixing.
|
please sign the CLA |
|
signed |
|
@kramaranya could you review this PR ? |
Mirror the sync test_auth_settings_with_authorization_key_and_prefix added in this PR, covering the same path in the async variant of get_api_key_with_prefix() via auth_settings(). Also fixes InClusterConfigTest base class from unittest.TestCase to IsolatedAsyncioTestCase so the existing async test_refresh_token actually runs. Signed-off-by: HaimLC <110099998+HaimLC@users.noreply.github.com>
5e2c78c to
c77c246
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: HaimLC, yliaog 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 |
|
thanks for the PR |
e003e85
into
kubernetes-client:master
What type of PR is this?
/kind bug
/kind regression
What this PR does / why we need it:
#2604 restored the
api_key['authorization']fallback inauth_settings()by callingget_api_key_with_prefix('BearerToken', alias='authorization'), butget_api_key_with_prefixonly applies thealiasfallback to theapi_keylookup, not theapi_key_prefixlookup:So callers that set the prefix under the legacy
'authorization'key (rather than embedding"Bearer "directly in the token string) get a bearer token with no"Bearer "prefix in the resultingAuthorizationheader. Most API servers — and specifically GKE's anonymous-auth fallback — don't recognize an unprefixed token as a valid bearer credential, so the request is treated assystem:anonymous.This reproduces with a plain manual
Configuration(), noload_incluster_config()/load_kube_config()involved:This PR applies the same alias fallback already used for the key lookup to the prefix lookup, symmetrically in the sync (
kubernetes/client/configuration.py) and async (kubernetes/aio/client/configuration.py) variants.Verified against a real cluster in production: a service using this exact pattern (manual
Configurationwithapi_key/api_key_prefixunder'authorization', talking to an external GKE cluster) was hittingsystem:anonymouson 36.0.2. Built this fix into that service via agit+dependency pointing at this branch and confirmed it now authenticates correctly against the live cluster with zero code changes on the caller's side.Which issue(s) this PR fixes:
Fixes #2592
load_incluster_config()/load_kube_config(), matching this root cause.test_auth_settings_with_authorization_key_and_prefix, covering the split key/prefix case that the existing Restore bearer-token fallback in Configuration.auth_settings() #2604 tests didn't exercise (those set the full'Bearer abc123'string directly inapi_key['authorization'], which doesn't touch the prefix lookup at all).scripts/apply-hotfixes.shin this PR since it needs this commit's final SHA — happy to follow up with that once this merges, same as was done for 5621a4c after Restore bearer-token fallback in Configuration.auth_settings() #2604.Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: