NO-JIRA: Add --proxy-url support to oc login - #2360
Conversation
Allow login to use and persist a per-cluster kubeconfig proxy-url so re-login no longer drops existing proxy settings when HTTPS_PROXY is used. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review. WalkthroughThe login command accepts validated ChangesLogin proxy support
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The proxy URL change is localized to login configuration and persistence, with no actionable current-head merge-blocking risk identified. It is merge-ready after normal checks. Sequence Diagram(s)sequenceDiagram
participant User
participant LoginCommand
participant LoginOptions
participant RESTConfig
participant Kubeconfig
User->>LoginCommand: provide --proxy-url
LoginCommand->>LoginOptions: validate ProxyURL
LoginOptions->>RESTConfig: configure rest.Config.Proxy
LoginOptions->>Kubeconfig: persist cluster proxy
Kubeconfig-->>User: updated kubeconfig
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/test |
|
@kchawlani19: The The following commands are available to trigger optional jobs: Use 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kchawlani19 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 |
kchawlani19
changed the title
|
Updated the PR title to |
openshift-ci-robot
added
the
jira/valid-reference
|
@kchawlani19: This pull request explicitly references no jira issue. 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. |
![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
🧹 Nitpick comments (2)
pkg/cli/login/helpers.go (1)
23-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap the URL parse error.
Line 25 discards
err. Return contextual error text with%wso callers retain the parse cause.Proposed fix
if err != nil { - return nil, fmt.Errorf("could not parse: %v", proxyURL) + return nil, fmt.Errorf("parse proxy URL %q: %w", proxyURL, err) }As per coding guidelines, “Wrap errors with meaningful context before returning or logging them.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/login/helpers.go` around lines 23 - 26, Update the error return in the proxy URL parsing flow around url.Parse to wrap the original err with %w while retaining meaningful context; do not substitute proxyURL for the parse error, so callers can inspect the underlying cause.Source: Coding guidelines
pkg/cli/login/loginoptions_test.go (1)
661-661: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd descriptive comments to the new test functions.
pkg/cli/login/loginoptions_test.go#L661-L661: add a comment forTestLoginProxyURLFlagAccepted.pkg/cli/login/loginoptions_test.go#L672-L672: add a comment forTestValidateProxyURL.pkg/cli/login/loginoptions_test.go#L731-L731: add a comment forTestGetClientConfigUsesProxyURL.pkg/cli/login/loginoptions_test.go#L789-L789: add a comment forTestGetClientConfigPreservesExistingClusterProxyURL.pkg/cli/login/loginoptions_test.go#L835-L835: add a comment forTestGetClientConfigProxyURLFlagOverridesExisting.pkg/cli/login/loginoptions_test.go#L889-L889: add a comment forTestGetClientConfigLeavesProxyNilWithoutFlagOrClusterProxy.pkg/cli/login/loginoptions_test.go#L909-L909: add a comment forTestSaveConfigPersistsAndPreservesProxyURL.pkg/cli/login/loginoptions_test.go#L981-L981: add a comment forTestSaveConfigDoesNotPersistEnvProxyWhenUnset.pkg/helpers/kubeconfig/smart_merge_test.go#L102-L102: add a comment forTestCreateConfigPersistsProxyURL.pkg/helpers/kubeconfig/smart_merge_test.go#L126-L126: add a comment forTestCreateConfigDoesNotPersistProxyWhenUnset.pkg/helpers/kubeconfig/smart_merge_test.go#L144-L144: add a comment forTestMergeConfigKeepsDistinctClusterProxyURLs.As per coding guidelines, “Add descriptive comments to all exported and unexported Go types, functions, and methods.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/login/loginoptions_test.go` at line 661, Each listed test function lacks the required descriptive Go comment. Add a concise comment immediately before every named function in pkg/cli/login/loginoptions_test.go at lines 661-661, 672-672, 731-731, 789-789, 835-835, 889-889, 909-909, and 981-981, and in pkg/helpers/kubeconfig/smart_merge_test.go at lines 102-102, 126-126, and 144-144, describing the behavior or scenario that test verifies; make each comment begin with its corresponding function name.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cli/login/helpers.go`:
- Around line 22-33: Update parseProxyURL to reject parsed URLs with an empty
u.Host, returning an appropriate validation error before the scheme-based
success path; extend TestValidateProxyURL to cover both http:// and http:/proxy
as invalid inputs.
In `@pkg/cli/login/loginoptions.go`:
- Around line 171-180: Update the cluster lookup used by the proxy-selection
flow around findCluster and CreateConfig so it resolves the stable canonical
cluster identity that will be replaced or required by CreateConfig, rather than
an arbitrary entry sharing the same host. Preserve explicit --proxy-url
precedence, and add coverage for duplicate server entries with different
ProxyURL values to verify the selected proxy belongs to the canonical cluster.
---
Nitpick comments:
In `@pkg/cli/login/helpers.go`:
- Around line 23-26: Update the error return in the proxy URL parsing flow
around url.Parse to wrap the original err with %w while retaining meaningful
context; do not substitute proxyURL for the parse error, so callers can inspect
the underlying cause.
In `@pkg/cli/login/loginoptions_test.go`:
- Line 661: Each listed test function lacks the required descriptive Go comment.
Add a concise comment immediately before every named function in
pkg/cli/login/loginoptions_test.go at lines 661-661, 672-672, 731-731, 789-789,
835-835, 889-889, 909-909, and 981-981, and in
pkg/helpers/kubeconfig/smart_merge_test.go at lines 102-102, 126-126, and
144-144, describing the behavior or scenario that test verifies; make each
comment begin with its corresponding function name.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e90dc3f-4365-41e6-a3a3-de209af6b73f
📒 Files selected for processing (5)
pkg/cli/login/helpers.gopkg/cli/login/login.gopkg/cli/login/loginoptions.gopkg/cli/login/loginoptions_test.gopkg/helpers/kubeconfig/smart_merge_test.go
Address review feedback so invalid proxy URLs fail early and duplicate server entries reuse the CreateConfig cluster nickname. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/cli/login/helpers.go (1)
34-36: 🎯 Functional Correctness | 🟡 MinorReject proxy URLs with a port but no hostname.
len(u.Host) != 0does not prove that the URL has a hostname.url.Parse("http://:8080")producesu.Host == ":8080"andu.Hostname() == "", so this invalid value passes validation and reaches proxy configuration. Checku.Hostname() == ""instead. Add regression cases forhttp://:8080andsocks5://:1080.Proposed fix
- if len(u.Host) == 0 { + if u.Hostname() == "" { return nil, fmt.Errorf("host must be specified") }#!/bin/bash set -euo pipefail cat >/tmp/proxy_host_probe.go <<'EOF' package main import ( "fmt" "net/url" ) func main() { for _, raw := range []string{"http://:8080", "socks5://:1080"} { u, err := url.Parse(raw) fmt.Printf("%q: err=%v host=%q hostname=%q\n", raw, err, u.Host, u.Hostname()) } } EOF go run /tmp/proxy_host_probe.go🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/login/helpers.go` around lines 34 - 36, Update the proxy URL validation in the visible helper to check u.Hostname() rather than len(u.Host), rejecting URLs with a port but no hostname. Add regression coverage for http://:8080 and socks5://:1080, while preserving acceptance of valid proxy URLs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@pkg/cli/login/helpers.go`:
- Around line 34-36: Update the proxy URL validation in the visible helper to
check u.Hostname() rather than len(u.Host), rejecting URLs with a port but no
hostname. Add regression coverage for http://:8080 and socks5://:1080, while
preserving acceptance of valid proxy URLs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 78349c43-4015-4b14-8a53-ce07e23b7f07
📒 Files selected for processing (2)
pkg/cli/login/helpers.gopkg/cli/login/loginoptions_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cli/login/loginoptions_test.go
|
Hi @ardaguclu @atiratree — could you take a look when you have a chance? This adds
First-stage CI is green. Happy to address any feedback. |
|
Thanks for spending time on this. But I think, we don't have any capacity to review these changes for now. |
|
Thanks @ardaguclu for the update — understood. Happy to keep the PR open and follow up later when you have bandwidth. Please let me know if there's a better time or another reviewer I should ping. |
Address review feedback by replacing channel/select hit tracking with atomic loads for clearer assertions. Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid type-asserting an unset atomic.Value when checking that dial went through the proxy. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Match validation errors exactly, clarify HTTPS_PROXY docs, and document dial/canonical test intent. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@kchawlani19: 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. |
4 participants
Summary
--proxy-urltooc loginso login uses a per-cluster proxy and persists it to kubeconfigcluster.proxy-url.proxy-urlwhen the flag is omitted, so re-login no longer drops it (including whenHTTPS_PROXYis set).rest.Config.Proxyunset when neither flag nor kubeconfig proxy is present, so env proxies still apply without being written into kubeconfig.Fixes #2314
Test plan
--proxy-urlflag accepted and validated (http/https/socks5; reject invalid schemes)--proxy-urlduring dialproxy-urlis reused when flag is omitted--proxy-urloverrides an existing cluster proxySaveConfig/CreateConfigpersistproxy-url; unrelated clusters keep distinct proxiesHTTPS_PROXYalone is not written into kubeconfigunit/verify/verify-depsgreenoc login --proxy-url=http://<proxy>:3128 ...and confirm kubeconfigclusters.*.proxy-urlSummary by CodeRabbit
New Features
--proxy-urlsupport to the login command.Bug Fixes