OCPBUGS-64619: oc login: Respect insecure flag from kubeconfig #2134
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@tchap: This pull request references Jira Issue OCPBUGS-64619, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
tchap
changed the title
WalkthroughgetClientConfig's TLS/CA resolution was refactored: CA sourcing can now fall back to a kubeconfig cluster via findCluster; Insecure handling and certificate-error branching use clientConfig.Insecure; combining insecure-skip-tls-verify with certificate-authority/-data now returns a conflict error. A unit test and an error constant were added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)pkg/cli/login/loginoptions_test.go (1)
🔇 Additional comments (4)
Comment |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@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
The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
removed
the
jira/invalid-bug
|
This one #2131 is also trying to fix same bug I suppose |
|
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 :). |
|
/retest |
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
Dropped a comment about test cases. Logical changes are nice to me. Thank you for fixing this.
|
/lgtm |
openshift-ci
bot
added
lgtm
|
I believe that it is better to perform pre-merge tests. cc: @zhouying7780 |
tchap
force-pushed
the
login-skip-tls-validation-kubeconfig
branch
from
17ab6e4 to
0215a6c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/cli/login/loginoptions.go (1)
163-174: CentralizingInsecurehandling looks good; consider guardingStartingKubeConfigusage.Using
clientConfig.Insecureas the single source of truth (OR ofo.InsecureTLSandhasExistingInsecureCluster) 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 whethero.StartingKubeConfigis guaranteed non‑nil on all call sites: the newhasExistingInsecureCluster(*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 toclientConfig.Insecure = o.InsecureTLSwhenStartingKubeConfig == nilwould 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
insecuresources and assert the resultingclientConfig.Insecure. One tiny readability nit: in the branch whereexpectedInsecureClientConfigis 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
📒 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.gopkg/cli/login/loginoptions.go
|
/lgtm |
|
Basically this PR still requires to be tested to merge it. |
|
I'll do pre-merge test with the latest code tomorrow morning. |
tchap
force-pushed
the
login-skip-tls-validation-kubeconfig
branch
from
0215a6c to
cbd832a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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
📒 Files selected for processing (3)
pkg/cli/login/helpers.go(1 hunks)pkg/cli/login/loginoptions.go(2 hunks)pkg/cli/login/loginoptions_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cli/login/loginoptions_test.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/helpers.go
🔇 Additional comments (2)
pkg/cli/login/helpers.go (1)
33-48: LGTM!The new
findClusterandfindClusterWithCAhelper functions are well-designed. The genericfindClusterwith an optional predicate provides good flexibility, andfindClusterWithCAcorrectly encapsulates the CA-presence check. This refactor improves reusability over the previousfindExistingClientCAapproach.pkg/cli/login/loginoptions.go (1)
200-214: Core fix looks correct.Checking
clientConfig.Insecurefirst at line 201 ensures that if the kubeconfig already specifiedinsecure-skip-tls-verify: truefor this cluster (set at lines 172 or 176), the user won't be prompted unnecessarily. This is the essence of the bug fix.
tchap
force-pushed
the
login-skip-tls-validation-kubeconfig
branch
from
cbd832a to
298b544
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/cli/login/loginoptions.go (1)
165-184: CA/insecure handling now correctly honors kubeconfig, with one suggested robustness tweakThe new cascade looks solid:
- When not already insecure, you first honor an explicit
CAFile, then fall back to kubeconfig viafindClusterWithCA, and finally tofindClusterfor picking upInsecureSkipTLSVerify.- Using
clientConfig.Insecure(instead of onlyo.InsecureTLS) in both the CA‑application block and the x509 error branch means kubeconfig‑driveninsecure-skip-tls-verifyis respected end‑to‑end.- Unsetting
CAFile/CADatawheneverclientConfig.Insecureis true avoids conflicting TLS settings during transport initialization.One robustness suggestion: both kubeconfig lookups here dereference
*o.StartingKubeConfig. That was already assumed non‑nil elsewhere, but this makes the assumption more central in the login path. If there is any code path (tests or future usages) that might callgetClientConfigbefore initializingStartingKubeConfig, a small guard would prevent panics:- } else if cluster := findClusterWithCA(clientConfig.Host, *o.StartingKubeConfig); cluster != nil { - clientConfig.Insecure = cluster.InsecureSkipTLSVerify - clientConfig.CAFile = cluster.CertificateAuthority - clientConfig.CAData = cluster.CertificateAuthorityData - } else if cluster := findCluster(clientConfig.Host, *o.StartingKubeConfig, nil); cluster != nil { - clientConfig.Insecure = cluster.InsecureSkipTLSVerify - } + } else if o.StartingKubeConfig != nil { + if cluster := findClusterWithCA(clientConfig.Host, *o.StartingKubeConfig); cluster != nil { + clientConfig.Insecure = cluster.InsecureSkipTLSVerify + clientConfig.CAFile = cluster.CertificateAuthority + clientConfig.CAData = cluster.CertificateAuthorityData + } else if cluster := findCluster(clientConfig.Host, *o.StartingKubeConfig, nil); cluster != nil { + clientConfig.Insecure = cluster.InsecureSkipTLSVerify + } + }If you’re confident
StartingKubeConfigis always non‑nil beforegetClientConfigis called, this remains optional, but adding the check would make the code more future‑proof.Also applies to: 201-201
📜 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
📒 Files selected for processing (3)
pkg/cli/login/helpers.go(1 hunks)pkg/cli/login/loginoptions.go(2 hunks)pkg/cli/login/loginoptions_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cli/login/loginoptions_test.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/helpers.go
🔇 Additional comments (1)
pkg/cli/login/helpers.go (1)
33-47: Generic cluster lookup helpers look correct and improve reuseThe new
findCluster/findClusterWithCAhelpers correctly handle the optional predicate, returnnilwhen no match is found, and cleanly encapsulate the CA‑presence filter. This makes the kubeconfig lookup logic more maintainable without changing the existing “first matching cluster wins” behavior.
tchap
force-pushed
the
login-skip-tls-validation-kubeconfig
branch
2 times, most recently
from
09cc2b7 to
7c307ce
Compare
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
7c307ce to
4ba0db7
Compare
|
Thank you |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@tchap: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/jira refresh The requirements for Jira bugs have changed (Jira issues linked to PRs on main branch need to target different OCP), recalculating validity. |
openshift-ci-robot
added
jira/invalid-bug
|
@openshift-bot: This pull request references Jira Issue OCPBUGS-64619, which is invalid:
Comment 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 openshift-eng/jira-lifecycle-plugin repository. |
5 participants
When running oc login, the insecure flag from kubeconfig is not consulted properly when calling
getClientConfig.This is now fixed.