fix(cli): honor kubeconfig env and exec auth tokens - #1389
Conversation
|
|
|
/kind bug |
f9bd147 to
239f455
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes tkn-results CLI authentication to match standard client-go/kubectl behavior by honoring kubeconfig loading precedence (including $KUBECONFIG) and correctly resolving bearer tokens from kubeconfig-backed credential providers (notably exec plugins like oc get-token), with accompanying unit tests and updated flag help text.
Changes:
- Switch kubeconfig loading to
clientcmd.NewDefaultClientConfigLoadingRules()so precedence is--kubeconfig→$KUBECONFIG→~/.kube/config(including merged kubeconfigs). - Add bearer-token resolution that drives client-go’s auth transport so
exec/auth-provider credentials can mint a token for forwarding to the Results API. - Update CLI help text and add unit tests for kubeconfig precedence and token resolution paths.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/flags/flags.go | Updates --kubeconfig / --namespace help strings to match actual kubeconfig precedence/default behavior. |
| pkg/cli/config/config.go | Loads kubeconfig via default client-go rules and resolves tokens via the new token resolution helper. |
| pkg/cli/config/token.go | Introduces resolveBearerToken helper to obtain bearer tokens via client-go transport credential behavior. |
| pkg/cli/config/token_test.go | Adds unit tests for static token, nil config, wrap-transport token injection, and exec credential token resolution. |
| pkg/cli/config/config_auth_test.go | Adds end-to-end unit coverage for $KUBECONFIG precedence and exec credential token resolution through NewConfig(). |
Comments suppressed due to low confidence (1)
pkg/cli/config/token.go:79
- resolveBearerToken() claims to return a bearer token, but currently returns any non-empty Authorization header verbatim when it isn't "Bearer". That can accidentally forward Basic/custom auth material as a Results token; non-bearer schemes should resolve to an empty token.
authz := capture.authorization
if authz == "" {
return "", nil
}
// Strip the "Bearer " scheme prefix if present.
if parts := strings.SplitN(authz, " ", 2); len(parts) == 2 && strings.EqualFold(parts[0], "Bearer") {
return parts[1], nil
}
return authz, nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
239f455 to
0a15540
Compare
|
Addressed Copilot review comments in
|
aThorp96
left a comment
There was a problem hiding this comment.
This is great, thanks! Clever idea to extract token injected by middleware as well. I have one suggestion for simplifying the token resolution logic. It should also reduce the required tests a bit
|
Addressed @aThorp96 review feedback in |
|
I will do some testing with this against our Tekton clusters and mark as ready for review when I'm comfortable things are good to go |
Ah, good catch. I was wondering about that possibility as well but wasn't sure. |
Thank you for this PR. Can you please remove the draft/not ready indicator. |
|
@divyansh42 PTAL when you have a chance. |
|
I think I identified an issue with how I was testing this, so I marked it as draft. Will validate today or tomorrow... |
divyansh42
left a comment
There was a problem hiding this comment.
@dronenb Quick question on the config set interactive prompt: for exec/OIDC credentials (e.g. oc get-token), it now pre-fills the "Token" field with a live, short-lived token instead of leaving it blank. If a user just hits Enter, that token gets saved permanently in Extension.Token, and future commands will keep using that saved value instead of asking oc for a fresh one — so things would start failing again once it expires, until the user reruns config set.
Is that the intended behavior, or should the prompt stay blank by default for non-static credentials so the CLI keeps auto-refreshing the token each time?
@dronenb just wanted to check, if you were able to validate it? |
bf61273 to
5603dba
Compare
I have addressed this, PTAL
Just validated, should be g2g. I have rebased as well. |
divyansh42
left a comment
There was a problem hiding this comment.
Thanks @dronenb!
Could you please squash the commits into one?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: divyansh42 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 |
5603dba to
5cf8205
Compare
The Results CLI built its client config in a way that diverged from oc/kubectl/tkn in two ways: 1. NewConfig hardcoded clientcmd.RecommendedHomeFile and getRawKubeConfigLoader used a single ExplicitPath, so the KUBECONFIG environment variable was ignored unless --kubeconfig was set. 2. Token() read rest.Config.BearerToken directly. For contexts that authenticate via exec credential plugins or legacy auth-provider configuration, BearerToken is empty because credentials are injected through the transport chain. Load kubeconfig through NewDefaultClientConfigLoadingRules so KUBECONFIG, including merged files, is honored while --kubeconfig still takes precedence. Resolve bearer tokens by driving the configured credential round-tripper once when no static token is present, capturing the Authorization header produced by exec/auth-provider auth. Non-token auth continues to return an empty token. Add unit coverage for KUBECONFIG precedence, NewConfig behavior, static and nil token handling, exec credential resolution, and the CLI flag help text. Fixes tektoncd#1388 /kind bug Signed-off-by: Ben Dronen <dronenb@users.noreply.github.com>
5cf8205 to
90317de
Compare
Thanks, @divyansh42! I have squashed the commits and rebased, should be g2g! |
|
|
@divyansh42 @enarha @khrm any update on this? Anything needed from my end? Let me know! 🙂 |
Not for now. I want to perform a test on OpenShift cluster to verify if a smaller fix isn't sufficient. Please give me a day or max two to check that. |
Changes
Fixes #1388
tkn-resultsbuilt its Kubernetes client configuration differently fromoc,kubectl, andtkn, which broke common kubeconfig-based authentication flows.This PR fixes two related CLI authentication issues:
--kubeconfig, then$KUBECONFIG, then~/.kube/config, including merged kubeconfig files.rest.Config.BearerToken, so kubeconfigexeccredentials such asoc get-tokenare invoked and their bearer token can be forwarded to the Results API.--tokenand configured token overrides.--kubeconfigand--namespaceto match the actual precedence/default behavior.NewConfig, static token handling, nil config handling, non-token auth, exec credential token resolution, and CLI flag help text./kind bug
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you review them:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepTesting
go test ./pkg/cli/...AI Assistance
This PR was prepared with assistance from OpenCode using GPT-5.5. The submitter reviewed the changes and ran the testing noted above.
Release Notes