OCPBUGS-64619: oc login: Respect insecure flag from kubeconfig #2134

tchap · 2025-11-07T11:43:44Z

When running oc login, the insecure flag from kubeconfig is not consulted properly when calling getClientConfig.
This is now fixed.

openshift-ci-robot · 2025-11-07T11:43:50Z

@tchap: This pull request references Jira Issue OCPBUGS-64619, which is invalid:

expected the bug to target the "4.21.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

When running oc login, the insecure flag from kubeconfig is not consulted properly when calling getClientConfig.
This is now fixed.

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 openshift-eng/jira-lifecycle-plugin repository.

coderabbitai · 2025-11-07T11:44:06Z

Walkthrough

Centralizes the insecure TLS decision by computing clientConfig.Insecure (OR of flag and existing kubeconfig detection), updates CA handling and certificate-error prompt logic to use that value, and adds a test validating kubeconfig-based insecure-skip-tls-verify behavior.

Changes

Cohort / File(s)	Summary
Core login logic refactor `pkg/cli/login/loginoptions.go`	`getClientConfig` now sets `clientConfig.Insecure` to `o.InsecureTLS
Test coverage expansion `pkg/cli/login/loginoptions_test.go`	Added `TestGetClientConfig_InsecureSkipTLSVerify` to verify behavior when `--insecure-skip-tls-verify` is absent but kubeconfig marks the cluster insecure. Test exercises flag/kubeconfig combinations against a TLS server and asserts resulting `Insecure` or TLS error outcomes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

Review TLS/security implications of switching guards to clientConfig.Insecure.
Verify CA file/CAData clearing and preservation across branches.
Confirm prompt logic for certificate errors still matches intended UX (no unintended bypass).
Inspect the new test for deterministic behavior and sufficient coverage of edge cases.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

Provide your own instructions using the high_level_summary_instructions setting.
Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

📝 Description — Summarize the main change in 50–60 words, explaining why this PR is needed, why this solution was chosen, and what was done.

📓 References — List relevant issues, discussions, documentation, or related PRs.

📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.

📊 Contributor Summary — Include a Markdown table showing contributions:
| Contributor | Lines Added | Lines Removed | Files Changed |

✔️ Additional Notes — Add any extra reviewer context.
Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

tchap · 2025-11-07T11:44:37Z

/jira refresh

openshift-ci-robot · 2025-11-07T11:44:46Z

@tchap: This pull request references Jira Issue OCPBUGS-64619, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.21.0) matches configured target version for branch (4.21.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

ardaguclu · 2025-11-07T14:20:11Z

This one #2131 is also trying to fix same bug I suppose

tchap · 2025-11-07T14:22:11Z

Gosh, people don't link to Jira issues 🥲 I would personally keep this one as it also contains tests, but...

ardaguclu · 2025-11-07T14:27:52Z

Gosh, people don't link to Jira issues 🥲 I would personally keep this one as it also contains tests, but...

The PRs that contain tests will always have higher priority :).

tchap · 2025-11-10T08:58:04Z

/retest

ardaguclu

Dropped a comment about test cases. Logical changes are nice to me. Thank you for fixing this.

ardaguclu · 2025-11-10T16:12:16Z

pkg/cli/login/loginoptions_test.go

+		},
+		"both command and kubeconfig flag set": {
+			insecureFlag:                 true,
+			insecureKubeconfig:           true,


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)

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.

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.

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

It is reflected in the test 🙂

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.

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

I appreciate adding unit tests every time. Thank you

This is the way :mandalorian:

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

ardaguclu · 2025-11-10T20:58:23Z

/lgtm

ardaguclu · 2025-11-12T13:10:11Z

I believe that it is better to perform pre-merge tests. cc: @zhouying7780

When running oc login, the insecure flag from kubeconfig is not consulted properly when calling getClientConfig(). This is now fixed. Assisted-by: Claude Code

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

pkg/cli/login/loginoptions.go (1)

163-174: Centralizing Insecure handling looks good; consider guarding StartingKubeConfig usage.

Using clientConfig.Insecure as the single source of truth (OR of o.InsecureTLS and hasExistingInsecureCluster) simplifies the TLS decision and keeps the error-path logic in sync with initial config, which is a nice cleanup. The only thing I’d double‑check is whether o.StartingKubeConfig is guaranteed non‑nil on all call sites: the new hasExistingInsecureCluster(*clientConfig, *o.StartingKubeConfig) call runs unconditionally now, including when the CLI flag is set, so if that invariant is ever broken this will panic. If the invariant isn’t watertight, a small guard like falling back to clientConfig.Insecure = o.InsecureTLS when StartingKubeConfig == nil would make this safer.

Also applies to: 191-201

pkg/cli/login/loginoptions_test.go (1)

380-451: Solid coverage of flag/kubeconfig combinations; tweak failure message for clarity.

The table cases nicely exercise the combinations of CLI and kubeconfig insecure sources and assert the resulting clientConfig.Insecure. One tiny readability nit: in the branch where expectedInsecureClientConfig is false, the fatal message says “Expected to fail with insecure connection…”, which sounds opposite to what the test is asserting (we actually expect a secure connection to fail TLS validation). Renaming that part of the message would avoid confusion for future readers, especially when debugging failures.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 17ab6e4 and 0215a6c.

📒 Files selected for processing (2)

pkg/cli/login/loginoptions.go (2 hunks)
pkg/cli/login/loginoptions_test.go (1 hunks)

🧰 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_test.go
pkg/cli/login/loginoptions.go

openshift-ci · 2025-11-18T03:04:29Z

@tchap: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

ardaguclu · 2025-11-19T13:59:08Z

pkg/cli/login/loginoptions.go

-			if o.InsecureTLS ||
-				hasExistingInsecureCluster(*clientConfig, *o.StartingKubeConfig) ||
-				promptForInsecureTLS(o.In, o.Out, err) {
+			if clientConfig.Insecure || promptForInsecureTLS(o.In, o.Out, err) {


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

The one right below was removed:

clientConfig.Insecure = true

ardaguclu · 2025-11-20T09:17:52Z

/lgtm
we will need another round of verification

openshift-ci · 2025-11-20T09:18:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, tchap

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ardaguclu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ardaguclu · 2025-11-28T11:14:05Z

Basically this PR still requires to be tested to merge it.

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Nov 7, 2025

tchap changed the title ~~https://issues.redhat.com/browse/OCPBUGS-64619: oc login: Respect insecure flag from kubeconfig~~ OCPBUGS-64619: oc login: Respect insecure flag from kubeconfig Nov 7, 2025

openshift-ci bot requested review from ardaguclu and ingvagabund November 7, 2025 11:44

openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Nov 7, 2025

openshift-ci-robot removed the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Nov 7, 2025

ardaguclu mentioned this pull request Nov 7, 2025

Do not verify TLS if cluster is configured as insecure #2131

Closed

ardaguclu reviewed Nov 10, 2025

View reviewed changes

openshift-ci bot assigned ardaguclu Nov 10, 2025

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 10, 2025

oc login: Respect insecure flag from kubeconfig

0215a6c

When running oc login, the insecure flag from kubeconfig is not consulted properly when calling getClientConfig(). This is now fixed. Assisted-by: Claude Code

tchap force-pushed the login-skip-tls-validation-kubeconfig branch from 17ab6e4 to 0215a6c Compare November 17, 2025 23:10

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2025

coderabbitai bot reviewed Nov 17, 2025

View reviewed changes

ardaguclu reviewed Nov 19, 2025

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2025

OCPBUGS-64619: oc login: Respect insecure flag from kubeconfig #2134

Are you sure you want to change the base?

OCPBUGS-64619: oc login: Respect insecure flag from kubeconfig #2134

Uh oh!

Conversation

tchap commented Nov 7, 2025

Uh oh!

openshift-ci-robot commented Nov 7, 2025

Uh oh!

coderabbitai bot commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Uh oh!

tchap commented Nov 7, 2025

Uh oh!

openshift-ci-robot commented Nov 7, 2025

Uh oh!

ardaguclu commented Nov 7, 2025

Uh oh!

tchap commented Nov 7, 2025

Uh oh!

ardaguclu commented Nov 7, 2025

Uh oh!

tchap commented Nov 10, 2025

Uh oh!

ardaguclu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tchap Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ardaguclu commented Nov 10, 2025

Uh oh!

ardaguclu commented Nov 12, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Nov 18, 2025

Uh oh!

ardaguclu Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ardaguclu commented Nov 20, 2025

Uh oh!

openshift-ci bot commented Nov 20, 2025

Uh oh!

ardaguclu commented Nov 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

coderabbitai bot commented Nov 7, 2025 •

edited

Loading

tchap Nov 10, 2025 •

edited

Loading

ardaguclu Nov 19, 2025 •

edited

Loading