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 |
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe login command accepts validated ChangesLogin proxy support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LoginCommand
participant LoginOptions
participant RESTClient
participant Kubeconfig
User->>LoginCommand: provide --proxy-url
LoginCommand->>LoginOptions: validate ProxyURL
LoginOptions->>RESTClient: configure rest.Config.Proxy
LoginOptions->>Kubeconfig: persist cluster proxy
Kubeconfig-->>User: updated kubeconfig
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 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. |
| func TestGetClientConfigUsesProxyURL(t *testing.T) { | ||
| apiHit := make(chan struct{}, 1) | ||
| apiServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| apiHit <- struct{}{} |
There was a problem hiding this comment.
I don't have time to review all of this right now, so I may continue later, but you could use an atomic type here so that you don't have to do select at the end, but just load a value and compare.
There was a problem hiding this comment.
Thanks @tchap — updated in the latest commit to use atomic.Bool / atomic.Value instead of channel+select.
There was a problem hiding this comment.
The cast to string will panic, though, if the value is not there. Not too great. Use atomic.Pointer[string] or check the cast worked and fail if not. Thanks.
There was a problem hiding this comment.
Good catch — switched to atomic.Pointer[string] and nil-check on Load(). Thanks!
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>
|
@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. |
| if cluster, ok := kubeconfig.Clusters[nick]; ok && cluster.Server == host { | ||
| return cluster | ||
| } | ||
| } |
There was a problem hiding this comment.
I am not sure this is needed, strictly speaking, but it also doesn't hurt...
|
|
||
| if len(o.ProxyURL) > 0 { | ||
| if _, err := parseProxyURL(o.ProxyURL); err != nil { | ||
| return fmt.Errorf("invalid --proxy-url %q: %w", o.ProxyURL, err) |
There was a problem hiding this comment.
I would improve the error messages in parseProxyURL so that we don't have to wrap here. The same goes for setClientConfigProxy call below. Wrapping it down there with invalid proxy URL leaks internals of setClientConfigProxy. How can you know the error is related to proxy URL? Better to generate meaningful and complete errors in parseProxyURL and just return it without wrapping...
| // ProxyURL, when set, is used for the login HTTP client and persisted to the | ||
| // kubeconfig cluster's proxy-url field. When empty, an existing cluster | ||
| // proxy-url is preserved; otherwise HTTPS_PROXY/HTTP_PROXY may still apply | ||
| // via the default transport when rest.Config.Proxy is nil. |
There was a problem hiding this comment.
We should rather be sure the usual HTTPS_PROXY/HTTP_PROXY still works. Does it still work? We should make the docstring more exact.
| } | ||
| if err := cmd.Flags().Set("proxy-url", "http://squid.example.com:3128"); err != nil { | ||
| t.Fatalf("expected --proxy-url to accept a value: %v", err) | ||
| } |
There was a problem hiding this comment.
Is this really testing anything useful? I mean the Set part...
| testCases := []struct { | ||
| name string | ||
| proxyURL string | ||
| expectErrSubstr string |
There was a problem hiding this comment.
I think that we know the exact full error message, so let's match against the full error message exactly and rename this to expectedErrMsg or something like that.
| t.Fatalf("expected proxy %q, got %v", proxyServer.URL, got) | ||
| } | ||
|
|
||
| // dialToServer requests the API root; absolute form may include a trailing slash. |
There was a problem hiding this comment.
Pls extend this to mention that getClientConfig is actually making the real request.
| } | ||
| if got == nil || got.String() != proxyServer.URL { | ||
| t.Fatalf("expected proxy %q, got %v", proxyServer.URL, got) | ||
| } |
There was a problem hiding this comment.
IMO this whole section can be dropped, it's not really testing anything relevant that is not tested by the following check.
| } | ||
| } | ||
|
|
||
| func TestGetClientConfigPrefersCanonicalClusterProxyURL(t *testing.T) { |
There was a problem hiding this comment.
This is not a particularly robust test, because even if we just iterate as we did before, we can accidentally find the right cluster since map iteration is non-deterministic.
| } | ||
| if got == nil || got.String() != proxyServer.URL { | ||
| t.Fatalf("expected preserved proxy %q, got %v", proxyServer.URL, got) | ||
| } |
There was a problem hiding this comment.
I don't really like that we are sending the real request in getClientConfig and then also calling Proxy directly and checking the result. We can store what server was called in the request handlers and then just check that, we don't need to create another request and call Proxy. We should simplify this in all tests where possible.
| } | ||
| } | ||
|
|
||
| func TestSaveConfigPersistsAndPreservesProxyURL(t *testing.T) { |
There was a problem hiding this comment.
I reviewed up until here for now. Can we pls restructure the tests so that we have a top-level wrapper TestProxyURL or TestProxyURLFlag and then we can use t.Run() for each test that is now a top-level thing? So that we can see these are all related.
Also pls add a doc comment to each test quickly explaining what that test is testing. This really helps with review.
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