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
8 changes: 3 additions & 5 deletions pkg/cli/login/loginoptions.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,9 @@ func (o *LoginOptions) getClientConfig() (*restclient.Config, error) {
}
o.Server = serverNormalized
clientConfig.Host = o.Server
clientConfig.Insecure = o.InsecureTLS
clientConfig.Insecure = o.InsecureTLS || hasExistingInsecureCluster(*clientConfig, *o.StartingKubeConfig)

if !o.InsecureTLS {
if !clientConfig.Insecure {
// use specified CA or find existing CA
if len(o.CAFile) > 0 {
clientConfig.CAFile = o.CAFile
Expand All @@ -188,9 +188,7 @@ func (o *LoginOptions) getClientConfig() (*restclient.Config, error) {
// connection or if we already have a cluster stanza that tells us to
// connect to this particular server insecurely
case x509.UnknownAuthorityError, x509.HostnameError, x509.CertificateInvalidError:
if o.InsecureTLS ||
hasExistingInsecureCluster(*clientConfig, *o.StartingKubeConfig) ||
promptForInsecureTLS(o.In, o.Out, err) {
if clientConfig.Insecure || promptForInsecureTLS(o.In, o.Out, err) {
Copy link
Member

@ardaguclu ardaguclu Nov 19, 2025

Choose a reason for hiding this comment

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

What was the leftover line that caused an issue in previous version. I'm wondering why did I miss it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The one right below was removed:

clientConfig.Insecure = true

clientConfig.Insecure = true
clientConfig.CAFile = ""
clientConfig.CAData = nil
Expand Down
73 changes: 73 additions & 0 deletions pkg/cli/login/loginoptions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,79 @@ func TestDialToHTTPSServer(t *testing.T) {
}
}

func TestGetClientConfig_InsecureSkipTLSVerify(t *testing.T) {
// Test that insecure-skip-tls-verify setting from kubeconfig is respected
// when logging in without the --insecure-skip-tls-verify flag.

server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
}))
defer server.Close()

testCases := map[string]struct {
insecureFlag bool
insecureKubeconfig bool
expectedInsecureClientConfig bool
}{
"command flag set": {
insecureFlag: true,
expectedInsecureClientConfig: true,
},
"kubeconfig flag set": {
insecureKubeconfig: true,
expectedInsecureClientConfig: true,
},
"no flag set": {
insecureFlag: false,
insecureKubeconfig: false,
expectedInsecureClientConfig: false,
},
"both command and kubeconfig flag set": {
insecureFlag: true,
insecureKubeconfig: true,
Copy link
Member

Choose a reason for hiding this comment

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

I'd expect insecureFlag and insecureKubeconfig values differ and we explicitly state which one we should choose. Flags are higher priority than implicit assignments (kubeconfig)

Copy link
Contributor Author

@tchap tchap Nov 10, 2025

Choose a reason for hiding this comment

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

I think that the logic is that there is no priority, but setting insecure anywhere just makes the whole thing accept insecure connection. In other words, insecure=false flag does not overwrite kubeconfig with insecure=true. Otherwise we would need to explicitly check whether the CLI flag was actually set and use that value in any case, in other words, treat default flag value and explicitly set flag value to false differently.

Copy link
Member

Choose a reason for hiding this comment

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

My suggestion basically is to assure whatever the expected outcome if they differ. So that in the future if that test fails, we will be notified something has changed.

Copy link
Member

Choose a reason for hiding this comment

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

So if assigning one of them is true makes the outcome as true, let's reflect it to the test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is reflected in the test 🙂

Copy link
Member

Choose a reason for hiding this comment

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

I assume the ones you pointed out is line:394 and line:398. They exercise, for example, oc login (with insecure in kubeconfig), vs. But they do not exercise oc login --insecure-tls-verify=false (with insecure in kubeconfig).

I'm totally aware that they are identical. But we need to test this case too, because from user point of view oc login and oc login --insecure-tls-verify are not identical (former means run this with whatever value, latter means run this command with my explicit definition). So in my opinion, adding 2 additional tests would make sense to me not in terms of functionality but in terms of semantically.

Copy link
Member

Choose a reason for hiding this comment

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

On the other hand, test does not have notion of explicit, implicit flag. So we can leave the tests as is

Copy link
Member

Choose a reason for hiding this comment

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

I appreciate adding unit tests every time. Thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the way :mandalorian:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is not so great is that the tests are passing even when I roll back the change 😅

expectedInsecureClientConfig: true,
},
}

for name, test := range testCases {
t.Run(name, func(t *testing.T) {
startingConfig := &kclientcmdapi.Config{
Clusters: map[string]*kclientcmdapi.Cluster{},
}
if test.insecureKubeconfig {
startingConfig.Clusters["test-cluster"] = &kclientcmdapi.Cluster{
Server: server.URL,
InsecureSkipTLSVerify: true,
}
}

options := &LoginOptions{
Server: server.URL,
InsecureTLS: test.insecureFlag,
StartingKubeConfig: startingConfig,
}

clientConfig, err := options.getClientConfig()
if err != nil {
if test.expectedInsecureClientConfig {
t.Fatalf("Expected to succeed with insecure connection, but got error: %v", err)
} else {
// If we expect secure connection and get a TLS error, that's expected
// since we're using a test server with a self-signed cert.
if err.Error() != certificateAuthorityUnknownMsg {
t.Fatalf("Expected to fail with insecure connection, but got another error: %v", err)
}
return
}
}

if clientConfig.Insecure != test.expectedInsecureClientConfig {
t.Errorf("expected Insecure=%v, got %v", test.expectedInsecureClientConfig, clientConfig.Insecure)
}
})
}
}

func TestPreserveExecProviderOnUsernameLogin(t *testing.T) {
// Test that when using -u flag with existing OIDC credentials,
// the ExecProvider configuration is preserved
Expand Down