Fix for 'oc login should avoid clobbering kubeconfig contexts #283' #2185
Conversation
WalkthroughAdds a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes ✨ Finishing touches
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pietergobin The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @pietergobin. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
openshift-ci
bot
added
the
needs-ok-to-test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/cli/login/loginoptions.go`:
- Around line 562-565: The current remap under KeepCtxName runs even when
o.StartingKubeConfig.CurrentContext is empty, causing newConfig.CurrentContext
to be set to an empty string; change the block in the KeepCtxName branch of
loginoptions (the code manipulating newConfig.Contexts and
newConfig.CurrentContext) to first check that
o.StartingKubeConfig.CurrentContext is non-empty before performing the remap —
only assign newContext into newConfig.Contexts and set newConfig.CurrentContext
when o.StartingKubeConfig.CurrentContext != "".
- Line 33: Remove the duplicate unaliased import of
"k8s.io/client-go/tools/clientcmd/api" and keep the existing aliased import
kclientcmdapi; then update any code that references the package with the
unaliased name (e.g., occurrences of api.SomeType or api.SomeFunc around the
login options usage) to use the alias kclientcmdapi.SomeType /
kclientcmdapi.SomeFunc so the single aliased import is used consistently.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
pkg/cli/login/login.gopkg/cli/login/loginoptions.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/cli/login/loginoptions.gopkg/cli/login/login.go
🔇 Additional comments (2)
pkg/cli/login/loginoptions.go (1)
74-74: LGTM: KeepCtxName is surfaced in LoginOptions.pkg/cli/login/login.go (1)
98-98: Flag wiring looks good.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| "k8s.io/client-go/pkg/apis/clientauthentication" | ||
| restclient "k8s.io/client-go/rest" | ||
| kclientcmd "k8s.io/client-go/tools/clientcmd" | ||
| "k8s.io/client-go/tools/clientcmd/api" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file
find . -name "loginoptions.go" -type fRepository: openshift/oc
Length of output: 87
🏁 Script executed:
# Read the imports section (first ~50 lines) to see the imports
head -50 ./pkg/cli/login/loginoptions.go | cat -nRepository: openshift/oc
Length of output: 1864
🏁 Script executed:
# Check the usage around line 562-565 to see if api.Context is being used
sed -n '555,570p' ./pkg/cli/login/loginoptions.go | cat -nRepository: openshift/oc
Length of output: 623
🏁 Script executed:
# Let's verify if this file can compile by checking if there are any compilation errors
# Try to get the full file to see the context better
wc -l ./pkg/cli/login/loginoptions.goRepository: openshift/oc
Length of output: 91
🏁 Script executed:
# Let's verify the complete imports and usage context
# Check if there's a go.mod file and try to understand the module structure
head -n 100 ./pkg/cli/login/loginoptions.go | tail -n 50Repository: openshift/oc
Length of output: 1338
🏁 Script executed:
# Search for usage of "api." in the file to see what's using the non-aliased import
grep -n "api\." ./pkg/cli/login/loginoptions.goRepository: openshift/oc
Length of output: 506
🏁 Script executed:
# Let's verify this is indeed a Go compilation error by checking if there's a way to test this
# Search for any other uses of the non-aliased "api" that might be imported
grep -n " api\." ./pkg/cli/login/loginoptions.go | head -20Repository: openshift/oc
Length of output: 38
🏁 Script executed:
# Let's search for the exact error by looking at the grep output more carefully
# Check if line 564 specifically uses "api.Context"
sed -n '564p' ./pkg/cli/login/loginoptions.go | cat -nRepository: openshift/oc
Length of output: 158
🏁 Script executed:
# Verify: Is there any use of just "api." (not kclientcmdapi.) in the file?
grep -E '\bapi\.[A-Z]' ./pkg/cli/login/loginoptions.goRepository: openshift/oc
Length of output: 151
Duplicate import will cause compilation failure.
Lines 33-34 both import k8s.io/client-go/tools/clientcmd/api—line 33 without an alias and line 34 aliased as kclientcmdapi. Go forbids the same package path twice. Remove the unaliased import on line 33 and update line 564 to use the existing alias.
🛠️ Suggested fix
- "k8s.io/client-go/tools/clientcmd/api"
@@
- newConfig.Contexts = map[string]*api.Context{o.StartingKubeConfig.CurrentContext: newContext}
+ newConfig.Contexts = map[string]*kclientcmdapi.Context{o.StartingKubeConfig.CurrentContext: newContext}🤖 Prompt for AI Agents
In `@pkg/cli/login/loginoptions.go` at line 33, Remove the duplicate unaliased
import of "k8s.io/client-go/tools/clientcmd/api" and keep the existing aliased
import kclientcmdapi; then update any code that references the package with the
unaliased name (e.g., occurrences of api.SomeType or api.SomeFunc around the
login options usage) to use the alias kclientcmdapi.SomeType /
kclientcmdapi.SomeFunc so the single aliased import is used consistently.
| if o.KeepCtxName { | ||
| newContext := newConfig.Contexts[newConfig.CurrentContext] | ||
| newConfig.Contexts = map[string]*api.Context{o.StartingKubeConfig.CurrentContext: newContext} | ||
| newConfig.CurrentContext = o.StartingKubeConfig.CurrentContext |
There was a problem hiding this comment.
Guard --no-clobber when there is no current context.
If StartingKubeConfig.CurrentContext is empty (e.g., first-time login), this block sets CurrentContext to empty and writes a kubeconfig with no active context. Gate the remap to only run when a current context exists.
🛠️ Suggested fix
- if o.KeepCtxName {
+ if o.KeepCtxName && o.StartingKubeConfig != nil && o.StartingKubeConfig.CurrentContext != "" {
newContext := newConfig.Contexts[newConfig.CurrentContext]
newConfig.Contexts = map[string]*api.Context{o.StartingKubeConfig.CurrentContext: newContext}
newConfig.CurrentContext = o.StartingKubeConfig.CurrentContext
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if o.KeepCtxName { | |
| newContext := newConfig.Contexts[newConfig.CurrentContext] | |
| newConfig.Contexts = map[string]*api.Context{o.StartingKubeConfig.CurrentContext: newContext} | |
| newConfig.CurrentContext = o.StartingKubeConfig.CurrentContext | |
| if o.KeepCtxName && o.StartingKubeConfig != nil && o.StartingKubeConfig.CurrentContext != "" { | |
| newContext := newConfig.Contexts[newConfig.CurrentContext] | |
| newConfig.Contexts = map[string]*api.Context{o.StartingKubeConfig.CurrentContext: newContext} | |
| newConfig.CurrentContext = o.StartingKubeConfig.CurrentContext | |
| } |
🤖 Prompt for AI Agents
In `@pkg/cli/login/loginoptions.go` around lines 562 - 565, The current remap
under KeepCtxName runs even when o.StartingKubeConfig.CurrentContext is empty,
causing newConfig.CurrentContext to be set to an empty string; change the block
in the KeepCtxName branch of loginoptions (the code manipulating
newConfig.Contexts and newConfig.CurrentContext) to first check that
o.StartingKubeConfig.CurrentContext is non-empty before performing the remap —
only assign newContext into newConfig.Contexts and set newConfig.CurrentContext
when o.StartingKubeConfig.CurrentContext != "".
| cmds.Flags().BoolVarP(&o.WebLogin, "web", "w", o.WebLogin, "Login with web browser. Starts a local HTTP callback server to perform the OAuth2 Authorization Code Grant flow. Use with caution on multi-user systems, as the server's port will be open to all users.") | ||
| cmds.Flags().Int32VarP(&o.CallbackPort, "callback-port", "c", o.CallbackPort, "Port for the callback server when using --web. Defaults to a random open port") | ||
|
|
||
| cmds.Flags().BoolVar(&o.KeepCtxName, "no-clobber", o.KeepCtxName, "Experimental: Keep the current context name in the kubeconfig file.") |
There was a problem hiding this comment.
Thank you for opening a PR to fix this. But we would not prefer a new flag for this functionality.
|
I guess a viable solution would be to just try to find an existing context name based on |
|
And would you like that to become default behaviour? Otherwise it might make more sense to put it in the get cluster nickname function than where I put it. I didn't fully look into the usage of that function in the project part of the CLI. If I find some 10 minutes spare time, I might look into the option you described then |
|
We could actually just check here: oc/pkg/cli/login/loginoptions.go Line 573 in 4ba0db7 Instead of just merging, we could check the contexts in the existing kubeconfig. In case there is a match on cluster and user, I would merge by replacing the context in the existing kubeconfig with the new object while keeping the name. |
|
Alright, let me give it a go and see what I can come up with. Thanks for the feedback already :) |
That would be reasonable option. But we need to carefully evaluate the backwards compatibility. There might be a user that rely on recreation of the cluster stanza instead of using the existing one. So feel free to work on it, but we can't guarantee it will be accepted. |
Yeah... But I guess this is pretty annoying for people as I can see from the linked issue. Generally this cannot be solved 100% without introducing a new flag, I would say, but somebody depending on actually always using the stable context name, that seems pretty unlikely. But generally speaking you can have N contexts linking to the same cluster and user, so this kind of config manipulation is always bit of a mess, because you don't know what people manage to come up with. But we could replace the context iff there is just one matching context in the kubeconfig, otherwise use the standard name as currently present. I don't know. |
|
Alright, thinking about this some more, the alternative way would be to use client authentication in the kubeconfig file (similar to kubelogin for aks) and then maybe put the population of this behind a flag or subcommand in the cli so users that want the behaviour from the issue can execute it once, and then the client authentication can do its thing. I tried to get this to work using oc get-token but never quite got there on my setup. |
|
dummy kubeconfig for clarity: apiVersion: v1
kind: Config
clusters:
- cluster:
server: localhost:8883
name: localhost-k8s
contexts:
- context:
cluster: localhost-k8s
namespace: default
user: localhost-user
name: localhost-k8s
current-context: localhost-k8s
users:
- name: localhost-user
user:
exec:
apiVersion: client.authentication.k8s.io/v1beta1
command: oc
args:
- get-token
env: null
interactiveMode: IfAvailable
provideClusterInfo: false
|
@pietergobin we discussed this with @tchap internally. Considering that how many people are unhappy about the default behavior, we believe that we need to fix this (probably by introducing a flag as opposed to what I mentioned above). But it would be ideal if @tchap open a new PR, since he knows the internals of |
|
More than happy to, thanks for taking this up :) |
|
just checking, it's a no-go on the client authentication method then? |
Thanks for your understanding. We'll tag you when we have a PR for you have a look at too. |
That would delegate the functionality to another command/plugin just for this purpose. It may theoretically work but in my opinion would be an overkill. Since we lean towards introducing a new flag within |
|
Alright! thanks for the quick responses everyone! looking forward to the pr :) |
| return false, err | ||
| } | ||
| if o.KeepCtxName { | ||
| newContext := newConfig.Contexts[newConfig.CurrentContext] |
There was a problem hiding this comment.
Could this take the context from the --context global option, if provided; and default to current context?
Thanks!
|
I am closing this now as this is superseded by #2187 /close |
|
@tchap: Closed this PR. 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. |
5 participants
Added overwriting of context in newConfig with the original kubeconfig entry for the current context. Should still update all other auth info but keep the friendly user specified cluster name as-is.
Reference to original issue: #283