Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/cli/login/login.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ var (
# Log in to the given server with the given credentials (will not prompt interactively)
oc login localhost:8443 --username=myuser --password=mypass

# Log in to the given server as myuser and use a custom kubeconfig context name.
oc login localhost:8443 --username=myuser --context=local

# Log in to the given server through a browser
oc login localhost:8443 --web --callback-port 8280

Expand Down Expand Up @@ -162,6 +165,7 @@ func (o *LoginOptions) Complete(f kcmdutil.Factory, cmd *cobra.Command, args []s
o.Token = kcmdutil.GetFlagString(cmd, "token")

o.DefaultNamespace, _, _ = f.ToRawKubeConfigLoader().Namespace()
o.Context = kcmdutil.GetFlagString(cmd, "context")
Comment thread
tchap marked this conversation as resolved.

o.PathOptions = kclientcmd.NewDefaultPathOptions()
// we need to set explicit path if one was specified, since NewDefaultPathOptions doesn't do it for us
Expand Down
43 changes: 42 additions & 1 deletion pkg/cli/login/loginoptions.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ type LoginOptions struct {
StartingKubeConfig *kclientcmdapi.Config
DefaultNamespace string
Config *restclient.Config
Context string

// cert data to be used when authenticating
CertFile string
Expand Down Expand Up @@ -570,7 +571,7 @@ func (o *LoginOptions) SaveConfig() (bool, error) {
return false, err
}

configToWrite, err := cliconfig.MergeConfig(*o.StartingKubeConfig, *newConfig)
configToWrite, err := o.mergeConfig(*newConfig)
if err != nil {
return false, err
}
Expand All @@ -593,6 +594,46 @@ func (o *LoginOptions) SaveConfig() (bool, error) {
return created, nil
}

// mergeConfig merges StartingKubeConfig with additionalConfig,
// which must contain just a single context, cluster and authInfo.
func (o *LoginOptions) mergeConfig(additionalConfig kclientcmdapi.Config) (*kclientcmdapi.Config, error) {
if o.Context == "" {
return cliconfig.MergeConfig(*o.StartingKubeConfig, additionalConfig)
}

// Set the custom context name in additionalConfig. This needs to happen in any case.
var newContext *kclientcmdapi.Context
for _, v := range additionalConfig.Contexts {
newContext = v
additionalConfig.Contexts = map[string]*kclientcmdapi.Context{
o.Context: v,
}
}
if newContext == nil {
panic(errors.New("no context found in additionalConfig"))
}
Comment thread
tchap marked this conversation as resolved.

// If there is a matching context, replace the linked cluster and authInfo.
existingContext, ok := o.StartingKubeConfig.Contexts[o.Context]
if ok {
for _, v := range additionalConfig.Clusters {
additionalConfig.Clusters = map[string]*kclientcmdapi.Cluster{
existingContext.Cluster: v,
}
newContext.Cluster = existingContext.Cluster
}
for _, v := range additionalConfig.AuthInfos {
additionalConfig.AuthInfos = map[string]*kclientcmdapi.AuthInfo{
existingContext.AuthInfo: v,
}
newContext.AuthInfo = existingContext.AuthInfo
}
Comment thread
tchap marked this conversation as resolved.
}

additionalConfig.CurrentContext = o.Context
return cliconfig.MergeConfig(*o.StartingKubeConfig, additionalConfig)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

func (o *LoginOptions) whoAmI(clientConfig *restclient.Config) (*userv1.User, error) {
if o.whoAmIFunc != nil {
return o.whoAmIFunc(clientConfig)
Expand Down
184 changes: 184 additions & 0 deletions pkg/cli/login/loginoptions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -669,3 +669,187 @@ func newTLSServer(certString, keyString string) (*httptest.Server, error) {
}
return server, nil
}

func TestMergeConfig(t *testing.T) {
testCases := []struct {
name string
startingConfig *kclientcmdapi.Config
additionalConfig kclientcmdapi.Config
contextFlag string
expectedConfig *kclientcmdapi.Config
}{
{
name: "context name unspecified",
startingConfig: &kclientcmdapi.Config{
Clusters: map[string]*kclientcmdapi.Cluster{},
AuthInfos: map[string]*kclientcmdapi.AuthInfo{},
Contexts: map[string]*kclientcmdapi.Context{},
},
additionalConfig: kclientcmdapi.Config{
Clusters: map[string]*kclientcmdapi.Cluster{
"new-cluster": {Server: "https://new-server:6443"},
},
AuthInfos: map[string]*kclientcmdapi.AuthInfo{
"new-user": {Token: "token"},
},
Contexts: map[string]*kclientcmdapi.Context{
"new-context": {Cluster: "new-cluster", AuthInfo: "new-user", Namespace: "default"},
},
CurrentContext: "new-context",
},
expectedConfig: &kclientcmdapi.Config{
Clusters: map[string]*kclientcmdapi.Cluster{
"new-cluster": {Server: "https://new-server:6443"},
},
AuthInfos: map[string]*kclientcmdapi.AuthInfo{
"new-user": {Token: "token"},
},
Contexts: map[string]*kclientcmdapi.Context{
"new-context": {Cluster: "new-cluster", AuthInfo: "new-user", Namespace: "default"},
},
CurrentContext: "new-context",
},
},
{
name: "custom context name without matching existing context",
startingConfig: &kclientcmdapi.Config{
Clusters: map[string]*kclientcmdapi.Cluster{},
AuthInfos: map[string]*kclientcmdapi.AuthInfo{},
Contexts: map[string]*kclientcmdapi.Context{},
},
additionalConfig: kclientcmdapi.Config{
Clusters: map[string]*kclientcmdapi.Cluster{
"new-cluster": {Server: "https://new-server:6443"},
},
AuthInfos: map[string]*kclientcmdapi.AuthInfo{
"new-user": {Token: "token"},
},
Contexts: map[string]*kclientcmdapi.Context{
"auto-generated-context": {Cluster: "new-cluster", AuthInfo: "new-user", Namespace: "default"},
},
CurrentContext: "auto-generated-context",
},
contextFlag: "my-custom-context",
expectedConfig: &kclientcmdapi.Config{
Clusters: map[string]*kclientcmdapi.Cluster{
"new-cluster": {Server: "https://new-server:6443"},
},
AuthInfos: map[string]*kclientcmdapi.AuthInfo{
"new-user": {Token: "token"},
},
Contexts: map[string]*kclientcmdapi.Context{
"my-custom-context": {Cluster: "new-cluster", AuthInfo: "new-user", Namespace: "default"},
},
CurrentContext: "my-custom-context",
},
},
{
name: "custom context name matches existing context",
startingConfig: &kclientcmdapi.Config{
Clusters: map[string]*kclientcmdapi.Cluster{
"existing-cluster": {Server: "https://old-server:6443"},
},
AuthInfos: map[string]*kclientcmdapi.AuthInfo{
"existing-user": {Token: "old-token"},
},
Contexts: map[string]*kclientcmdapi.Context{
"my-custom-context": {Cluster: "existing-cluster", AuthInfo: "existing-user", Namespace: "old-namespace"},
},
},
additionalConfig: kclientcmdapi.Config{
Clusters: map[string]*kclientcmdapi.Cluster{
"new-cluster": {Server: "https://new-server:6443"},
},
AuthInfos: map[string]*kclientcmdapi.AuthInfo{
"new-user": {Token: "new-token"},
},
Contexts: map[string]*kclientcmdapi.Context{
"auto-generated-context": {Cluster: "new-cluster", AuthInfo: "new-user", Namespace: "default"},
},
CurrentContext: "auto-generated-context",
},
contextFlag: "my-custom-context",
expectedConfig: &kclientcmdapi.Config{
Clusters: map[string]*kclientcmdapi.Cluster{
"existing-cluster": {Server: "https://new-server:6443"},
},
AuthInfos: map[string]*kclientcmdapi.AuthInfo{
"existing-user": {Token: "new-token"},
},
Contexts: map[string]*kclientcmdapi.Context{
"my-custom-context": {Cluster: "existing-cluster", AuthInfo: "existing-user", Namespace: "default"},
},
CurrentContext: "my-custom-context",
},
},
{
name: "custom context name replaces matching context but keeps other contexts",
startingConfig: &kclientcmdapi.Config{
Clusters: map[string]*kclientcmdapi.Cluster{
"cluster-a": {Server: "https://server-a:6443"},
"cluster-b": {Server: "https://server-b:6443"},
"existing-cluster": {Server: "https://old-server:6443"},
},
AuthInfos: map[string]*kclientcmdapi.AuthInfo{
"user-a": {Token: "token-a"},
"user-b": {Token: "token-b"},
"existing-user": {Token: "old-token"},
},
Contexts: map[string]*kclientcmdapi.Context{
"context-a": {Cluster: "cluster-a", AuthInfo: "user-a", Namespace: "ns-a"},
"context-b": {Cluster: "cluster-b", AuthInfo: "user-b", Namespace: "ns-b"},
"my-custom-context": {Cluster: "existing-cluster", AuthInfo: "existing-user", Namespace: "old-namespace"},
},
CurrentContext: "context-a",
},
additionalConfig: kclientcmdapi.Config{
Clusters: map[string]*kclientcmdapi.Cluster{
"new-cluster": {Server: "https://new-server:6443"},
},
AuthInfos: map[string]*kclientcmdapi.AuthInfo{
"new-user": {Token: "new-token"},
},
Contexts: map[string]*kclientcmdapi.Context{
"auto-generated-context": {Cluster: "new-cluster", AuthInfo: "new-user", Namespace: "default"},
},
CurrentContext: "auto-generated-context",
},
contextFlag: "my-custom-context",
expectedConfig: &kclientcmdapi.Config{
Clusters: map[string]*kclientcmdapi.Cluster{
"cluster-a": {Server: "https://server-a:6443"},
"cluster-b": {Server: "https://server-b:6443"},
"existing-cluster": {Server: "https://new-server:6443"},
},
AuthInfos: map[string]*kclientcmdapi.AuthInfo{
"user-a": {Token: "token-a"},
"user-b": {Token: "token-b"},
"existing-user": {Token: "new-token"},
},
Contexts: map[string]*kclientcmdapi.Context{
"context-a": {Cluster: "cluster-a", AuthInfo: "user-a", Namespace: "ns-a"},
"context-b": {Cluster: "cluster-b", AuthInfo: "user-b", Namespace: "ns-b"},
"my-custom-context": {Cluster: "existing-cluster", AuthInfo: "existing-user", Namespace: "default"},
},
CurrentContext: "my-custom-context",
},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
options := &LoginOptions{
StartingKubeConfig: tc.startingConfig,
Context: tc.contextFlag,
}

result, err := options.mergeConfig(tc.additionalConfig)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
if !cmp.Equal(result, tc.expectedConfig) {
t.Errorf("Unexpected merged config:\n%s", cmp.Diff(tc.expectedConfig, result))
}
})
}
}
48 changes: 38 additions & 10 deletions pkg/cli/project/project.go
Original file line number Diff line number Diff line change
Expand Up @@ -276,19 +276,47 @@ func (o ProjectOptions) Run() error {
userNameInUse = user.Name
}

kubeconfig, err := cliconfig.CreateConfig(projectName, userNameInUse, o.RESTConfig)
if err != nil {
return err
// Check if the current context has a custom name (doesn't match the auto-generated pattern).
// If so, and the server matches, just update its namespace instead of creating a new context.
// This preserves custom context names set via "oc login --context=<name>".
contextUpdated := false
if currentContext != nil {
defaultContextName := cliconfig.GetContextNickname(currentContext.Namespace, currentContext.Cluster, currentContext.AuthInfo)
if config.CurrentContext != defaultContextName {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic in this PR makes sense to me. However, historically ProjectName always aligns with contextName (

if context, contextExists := o.GetContextFromName(argument); contextExists {
). This is used in routes, services, Consoles, etc. If we officially allow custom context names in the kubeconfig, that means we'll have to fix unforeseen misalignments between different components. Therefore, this requires a collaborative work between teams.

I understand the inconvenience filed in the issue. But it is more about annoyance not an actual bug.

Initially I was supportive of this work. But now I think I would prefer waiting from official RFE from customers to be driven by BU that support us to change the flow. This PR is already mergeable for me.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, didn't know the context name is so important to match. Thanks for the clarification.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me neither until seeing openshift/origin#16161 (comment). But it is good to have your PR in case we need it in the future.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just landed on this PR by doing a quick google search if oc supports naming for context. I'm atm doing some integrations with Konflux and it requires our tooling to talk with more than one cluster in the same process, having static naming for the credential will save us some troubleshooting time in CI. Do we really need one RFE for this? It's been almost 9 years since openshift/origin#16161 (comment) was done, this was before OCP 4.x (feeling old), a lot changed on how we view Openshift.

if cluster := config.Clusters[currentContext.Cluster]; cluster != nil {
currentServer, err := cliconfig.NormalizeServerURL(cluster.Server)
if err != nil {
return fmt.Errorf("invalid server URL %q in kubeconfig: %v", cluster.Server, err)
}
targetServer, err := cliconfig.NormalizeServerURL(clientCfg.Host)
if err != nil {
return fmt.Errorf("invalid server URL %q: %v", clientCfg.Host, err)
}
if currentServer == targetServer {
currentContext.Namespace = projectName
namespaceInUse = projectName
contextInUse = config.CurrentContext
contextUpdated = true
}
}
}
}

merged, err := cliconfig.MergeConfig(config, *kubeconfig)
if err != nil {
return err
}
config = *merged
if !contextUpdated {
kubeconfig, err := cliconfig.CreateConfig(projectName, userNameInUse, o.RESTConfig)
if err != nil {
return err
}

namespaceInUse = projectName
contextInUse = merged.CurrentContext
merged, err := cliconfig.MergeConfig(config, *kubeconfig)
if err != nil {
return err
}
config = *merged

namespaceInUse = projectName
contextInUse = merged.CurrentContext
}
}

if err := kclientcmd.ModifyConfig(o.PathOptions, config, true); err != nil {
Expand Down