OCPBUGS-55621: Replace konnectivity Dial with DialContext in konnectivity-https-proxy/cmd.go#8550
OCPBUGS-55621: Replace konnectivity Dial with DialContext in konnectivity-https-proxy/cmd.go#8550PoornimaSingour wants to merge 3 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-low
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@PoornimaSingour: This pull request references Jira Issue OCPBUGS-55621, 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. |
openshift-ci-robot
added
the
jira/invalid-bug
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: PoornimaSingour 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 |
openshift-ci
Bot
added
area/documentation
|
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:
📝 WalkthroughWalkthroughThis PR replaces deprecated transport Dial usage with context-aware DialContext. It adds a context import, updates the HTTP transport wiring to use konnectivityDialer.DialContext, and changes the direct-dial wrapper to call httpProxy.Tr.DialContext with context.Background(). 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
PoornimaSingour
force-pushed
the
DialwithDialContext
branch
from
d61d5d8 to
48f1889
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: 1
🧹 Nitpick comments (1)
konnectivity-https-proxy/cmd.go (1)
184-189: 💤 Low valueContext parameter is discarded in proxy dial function due to goproxy API limitation.
The
dialThroughProxyFuncwrapper correctly maps the context-aware signature butNewConnectDialToProxyWithHandlerreturns a function without context support, so request cancellation and timeouts won't affect proxy connection establishment. This is a limitation of goproxy's convenience API, not a regression—dialDirectFuncproperly handles context viahttpProxy.Tr.DialContext.Improving this would require either using a different goproxy mechanism or implementing a custom proxy dialer with context awareness, which may not be worth the complexity for this use case.
🤖 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 `@konnectivity-https-proxy/cmd.go` around lines 184 - 189, dialThroughProxyFunc discards the incoming context because goproxy.NewConnectDialToProxyWithHandler returns a non‑context dialer; update dialThroughProxyFunc to document this limitation and point to the alternative used elsewhere: mention that dialDirectFunc uses httpProxy.Tr.DialContext for context-aware dialing, and note that making proxy dialing context-aware would require replacing NewConnectDialToProxyWithHandler with a custom dialer or different goproxy mechanism; keep the wrapper as-is but add a clear comment in the dialThroughProxyFunc body referencing NewConnectDialToProxyWithHandler, dialDirectFunc, and the reason for not implementing a context-aware proxy dialer.
🤖 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 `@docs/content/how-to/automated-machine-management/nodepool-lifecycle.md`:
- Around line 89-99: The example uses a fenced YAML block but the repo requires
indented code blocks (markdownlint MD046); replace the fenced block with an
indented code block by removing the triple backticks and indenting each YAML
line (e.g., the lines starting with "apiVersion:
hypershift.openshift.io/v1beta1" and "kind: NodePool" through "# ...other
fields...") by four spaces so the NodePool example is rendered as an indented
code block in nodepool-lifecycle.md.
---
Nitpick comments:
In `@konnectivity-https-proxy/cmd.go`:
- Around line 184-189: dialThroughProxyFunc discards the incoming context
because goproxy.NewConnectDialToProxyWithHandler returns a non‑context dialer;
update dialThroughProxyFunc to document this limitation and point to the
alternative used elsewhere: mention that dialDirectFunc uses
httpProxy.Tr.DialContext for context-aware dialing, and note that making proxy
dialing context-aware would require replacing NewConnectDialToProxyWithHandler
with a custom dialer or different goproxy mechanism; keep the wrapper as-is but
add a clear comment in the dialThroughProxyFunc body referencing
NewConnectDialToProxyWithHandler, dialDirectFunc, and the reason for not
implementing a context-aware proxy dialer.
🪄 Autofix (Beta)
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: Enterprise
Run ID: a140c514-9429-49e2-9037-a4ca9b0206eb
⛔ Files ignored due to path filters (1)
docs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.md
📒 Files selected for processing (2)
docs/content/how-to/automated-machine-management/nodepool-lifecycle.mdkonnectivity-https-proxy/cmd.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@konnectivity-https-proxy/cmd.go`:
- Around line 185-188: The current dialThroughProxyFunc wrapper ignores the
passed context (it discards ctx and returns a func that calls dialFn(network,
addr)), breaking cancellation/timeouts; replace this with a context-aware
CONNECT dialer by using or implementing a DialContext-capable CONNECT function:
either swap httpProxy.NewConnectDialToProxyWithHandler for a library that
exposes DialContext (e.g., github.com/elastic/proxy-connect-dialer-go) and call
its DialContext with the incoming ctx, or implement a custom CONNECT handshake
inside dialThroughProxyFunc that performs dial + CONNECT while selecting on
ctx.Done() to cancel the operation; ensure you still add basic auth via
addBasicAuthHeader(proxyURLUser) and keep the returned signature func(ctx
context.Context, network, addr string) (net.Conn, error) so context cancellation
and timeouts propagate.
🪄 Autofix (Beta)
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: Enterprise
Run ID: 62e90aab-65d5-498c-aab9-984b6d139230
📒 Files selected for processing (1)
konnectivity-https-proxy/cmd.go
| dialFn := httpProxy.NewConnectDialToProxyWithHandler(proxyURL, addBasicAuthHeader(proxyURLUser)) | ||
| return func(_ context.Context, network, addr string) (net.Conn, error) { | ||
| return dialFn(network, addr) | ||
| } |
There was a problem hiding this comment.
❓ Verification inconclusive
In github.com/elazarl/goproxy v1.7.2, is there a context-aware alternative to NewConnectDialToProxyWithHandler (or a recommended pattern) to support context cancellation/timeouts during CONNECT proxy dialing?
In github.com/elazarl/goproxy v1.7.2, the NewConnectDialToProxyWithHandler function does not natively support context.Context for cancellation or timeouts during the CONNECT dialing phase [1]. The function signature returns a standard func(network, addr string) (net.Conn, error) [2][1], which lacks the context parameter required for modern Go cancellation patterns. To achieve context-aware dialing in goproxy, the recommended pattern is to manually assign a custom dialer to the ProxyHttpServer.ConnectDial field [3][4]. While goproxy evolved in later versions to support context-aware signatures (e.g., func(ctx context.Context, network string, addr string) (net.Conn, error)) [3][4], in v1.7.2, you can implement a wrapper that respects context by using a third-party library or a custom implementation that performs the CONNECT handshake while respecting ctx.Done() [5][6][7]. Recommended approach: 1. Do not use NewConnectDialToProxyWithHandler if context support is required. 2. Implement a custom dialer that satisfies the context.Context requirement. You can use libraries like github.com/elastic/proxy-connect-dialer-go [5][6] or github.com/vladvalkov/connectdial [7], which provide DialContext methods specifically designed for this purpose. 3. Assign this custom DialContext logic to the proxy.ConnectDial field of your ProxyHttpServer instance [3]. If you are able to upgrade, later versions of goproxy have improved support for context-aware dialing [3][4]. If you must remain on v1.7.2, you are effectively responsible for implementing the CONNECT handshake (sending the CONNECT request, reading the response, and handling the context) within your own custom dialer function [2][5].
Citations:
- 1: https://github.com/elazarl/goproxy/tree/v1.7.2
- 2: ywww/goproxy-1@a9cf76a
- 3: https://github.com/elazarl/goproxy2/blob/master/proxy.go
- 4: https://github.com/elazarl/goproxy/blob/v1.8.3/ctx.go
- 5: https://pkg.go.dev/github.com/michel-laterman/proxy-connect-dialer-go
- 6: https://github.com/elastic/proxy-connect-dialer-go
- 7: https://pkg.go.dev/github.com/vladvalkov/connectdial
Script executed:
# Find the file and context
find . -type f -name "cmd.go" | grep -i konnectivityRepository: openshift/hypershift
Repository: openshift/hypershift
Exit code: 0
stdout:
./konnectivity-https-proxy/cmd.go
Script executed:
# Search for goproxy imports and version
rg "github.com/elazarl/goproxy" --type go -A 2 -B 2Repository: openshift/hypershift
Repository: openshift/hypershift
Exit code: 0
stdout:
vendor/github.com/elazarl/goproxy/doc.go-Example use cases:
vendor/github.com/elazarl/goproxy/doc.go-
vendor/github.com/elazarl/goproxy/doc.go:1. https://github.com/elazarl/goproxy/tree/master/examples/goproxy-avgsize
vendor/github.com/elazarl/goproxy/doc.go-
vendor/github.com/elazarl/goproxy/doc.go-To measure the average size of an Html served in your site. One can ask
--
vendor/github.com/elazarl/goproxy/doc.go-request, it'll send a warning email.
vendor/github.com/elazarl/goproxy/doc.go-
vendor/github.com/elazarl/goproxy/doc.go:3. https://github.com/elazarl/goproxy/blob/master/examples/goproxy-httpdump/
vendor/github.com/elazarl/goproxy/doc.go-
vendor/github.com/elazarl/goproxy/doc.go-Generate a real traffic to your website by real users using through
vendor/github.com/elazarl/goproxy/doc.go-proxy. Record the traffic, and try it again for more real load testing.
vendor/github.com/elazarl/goproxy/doc.go-
vendor/github.com/elazarl/goproxy/doc.go:4. https://github.com/elazarl/goproxy/tree/master/examples/goproxy-no-reddit-at-worktime
vendor/github.com/elazarl/goproxy/doc.go-
vendor/github.com/elazarl/goproxy/doc.go-Will allow browsing to reddit.com between 8:00am and 17:00pm
vendor/github.com/elazarl/goproxy/doc.go-
vendor/github.com/elazarl/goproxy/doc.go:5. https://github.com/elazarl/goproxy/tree/master/examples/goproxy-jquery-version
vendor/github.com/elazarl/goproxy/doc.go-
vendor/github.com/elazarl/goproxy/doc.go-Will warn if multiple versions of jquery are used in the same domain.
vendor/github.com/elazarl/goproxy/doc.go-
vendor/github.com/elazarl/goproxy/doc.go:6. https://github.com/elazarl/goproxy/blob/master/examples/goproxy-upside-down-ternet/
vendor/github.com/elazarl/goproxy/doc.go-
vendor/github.com/elazarl/goproxy/doc.go-Modifies image files in an HTTP response via goproxy's image extension found in ext/.
--
vendor/github.com/elazarl/goproxy/https.go- "sync/atomic"
vendor/github.com/elazarl/goproxy/https.go-
vendor/github.com/elazarl/goproxy/https.go: "github.com/elazarl/goproxy/internal/http1parser"
vendor/github.com/elazarl/goproxy/https.go: "github.com/elazarl/goproxy/internal/signer"
vendor/github.com/elazarl/goproxy/https.go-)
vendor/github.com/elazarl/goproxy/https.go-
--
konnectivity-https-proxy/cmd.go- "sigs.k8s.io/controller-runtime/pkg/log/zap"
konnectivity-https-proxy/cmd.go-
konnectivity-https-proxy/cmd.go: "github.com/elazarl/goproxy"
konnectivity-https-proxy/cmd.go- "github.com/spf13/cobra"
konnectivity-https-proxy/cmd.go- "go.uber.org/zap/zapcore"
Script executed:
# Check go.mod for goproxy version
cat go.mod | grep -i goproxyRepository: openshift/hypershift
Repository: openshift/hypershift
Exit code: 0
stdout:
github.com/elazarl/goproxy v1.7.2
Script executed:
# Examine the full function context around lines 185-188
fd "cmd.go" -p "*konnectivity*" --exec head -c 20000 {} \;Repository: openshift/hypershift
Repository: openshift/hypershift
Exit code: 1
stderr:
[fd error]: Search path '*konnectivity*' is not a directory.
[fd error]: No valid search paths given.
Script executed:
cat -n ./konnectivity-https-proxy/cmd.go | head -200Repository: openshift/hypershift
Repository: openshift/hypershift
Exit code: 0
stdout:
1 package konnectivityhttpsproxy
2
3 import (
4 "context"
5 "crypto/tls"
6 "encoding/base64"
7 "fmt"
8 "net"
9 "net/http"
10 "net/url"
11 "os"
12
13 "github.com/openshift/hypershift/support/konnectivityproxy"
14 "github.com/openshift/hypershift/support/netutil"
15 "github.com/openshift/hypershift/support/supportedversion"
16
17 "k8s.io/apimachinery/pkg/util/sets"
18 "k8s.io/utils/ptr"
19
20 ctrl "sigs.k8s.io/controller-runtime"
21 "sigs.k8s.io/controller-runtime/pkg/client"
22 "sigs.k8s.io/controller-runtime/pkg/log"
23 "sigs.k8s.io/controller-runtime/pkg/log/zap"
24
25 "github.com/elazarl/goproxy"
26 "github.com/spf13/cobra"
27 "go.uber.org/zap/zapcore"
28 "golang.org/x/net/http/httpproxy"
29 )
30
31 func NewStartCommand() *cobra.Command {
32 zLogger := zap.New(
33 zap.JSONEncoder(func(o *zapcore.EncoderConfig) {
34 o.EncodeTime = zapcore.RFC3339TimeEncoder
35 }),
36 )
37 log.SetLogger(zLogger)
38 l := log.Log.WithName("konnectivity-https-proxy")
39 cmd := &cobra.Command{
40 Use: "konnectivity-https-proxy",
41 Short: "Runs the konnectivity https proxy server.",
42 Long: ` Runs the konnectivity https proxy server.
43 This proxy accepts request and tunnels them through the designated Konnectivity Server.`,
44 }
45
46 opts := konnectivityproxy.Options{
47 ResolveBeforeDial: true,
48 ResolveFromGuestClusterDNS: true,
49 }
50
51 var servingPort uint32
52 var httpProxyURL string
53 var httpsProxyURL string
54 var noProxy string
55
56 cmd.Flags().StringVar(&opts.KonnectivityHost, "konnectivity-hostname", "konnectivity-server-local", "The hostname of the konnectivity service.")
57 cmd.Flags().Uint32Var(&opts.KonnectivityPort, "konnectivity-port", 8090, "The konnectivity port that https proxy should connect to.")
58 cmd.Flags().Uint32Var(&servingPort, "serving-port", 8090, "The port that https proxy should serve on.")
59
60 cmd.Flags().StringVar(&opts.CAFile, "ca-cert-path", "/etc/konnectivity/proxy-ca/ca.crt", "The path to the konnectivity client's ca-cert.")
61 cmd.Flags().StringVar(&opts.ClientCertFile, "tls-cert-path", "/etc/konnectivity/proxy-client/tls.crt", "The path to the konnectivity client's tls certificate.")
62 cmd.Flags().StringVar(&opts.ClientKeyFile, "tls-key-path", "/etc/konnectivity/proxy-client/tls.key", "The path to the konnectivity client's private key.")
63
64 cmd.Flags().StringVar(&httpProxyURL, "http-proxy", "", "HTTP proxy to use on hosted cluster requests")
65 cmd.Flags().StringVar(&httpsProxyURL, "https-proxy", "", "HTTPS proxy to use on hosted cluster requests")
66 cmd.Flags().StringVar(&noProxy, "no-proxy", "", "URLs that should not use the provided http-proxy and https-proxy")
67
68 cmd.Flags().BoolVar(&opts.ConnectDirectlyToCloudAPIs, "connect-directly-to-cloud-apis", false, "If true, bypass konnectivity to connect to cloud APIs while still honoring management proxy config")
69
70 cmd.Run = func(cmd *cobra.Command, args []string) {
71 l.Info("Starting proxy", "version", supportedversion.String())
72 c, err := client.New(ctrl.GetConfigOrDie(), client.Options{})
73 if err != nil {
74 fmt.Fprintf(os.Stderr, "Error: failed to get kubernetes client: %v", err)
75 os.Exit(1)
76 }
77 opts.Client = c
78 opts.Log = l
79
80 var proxyTLS *tls.Config
81 var proxyURLHostPort *string
82 var proxyURLUser *url.Userinfo
83 proxyHostNames := sets.New[string]()
84
85 if len(httpsProxyURL) > 0 {
86 u, err := url.Parse(httpsProxyURL)
87 if err != nil {
88 fmt.Fprintf(os.Stderr, "Error: failed to parse HTTPS proxy URL: %v", err)
89 os.Exit(1)
90 }
91 proxyURLUser = u.User
92 hostName, _, err := net.SplitHostPort(u.Host)
93 if err == nil {
94 proxyHostNames.Insert(hostName)
95 }
96 l.V(4).Info("Data plane HTTPS proxy is set", "hostname", hostName, "url", u.String())
97 proxyURLHostPort = ptr.To(u.Host)
98 }
99 if len(httpProxyURL) > 0 {
100 u, err := url.Parse(httpProxyURL)
101 if err != nil {
102 fmt.Fprintf(os.Stderr, "Error: failed to parse HTTP proxy URL: %v", err)
103 os.Exit(1)
104 }
105 hostName, _, err := net.SplitHostPort(u.Host)
106 if err == nil {
107 proxyHostNames.Insert(hostName)
108 }
109 l.V(4).Info("Data plane HTTP proxy is set", "hostname", hostName, "url", u.String())
110 if proxyURLHostPort == nil {
111 proxyURLHostPort = ptr.To(u.Host)
112 }
113 }
114 l.V(4).Info("Excluding API hosts from isCloudAPI check", "hosts", sets.List(proxyHostNames))
115 opts.ExcludeCloudAPIHosts = sets.List(proxyHostNames)
116
117 konnectivityDialer, err := konnectivityproxy.NewKonnectivityDialer(opts)
118 if err != nil {
119 fmt.Fprintf(os.Stderr, "Error: failed to initialize konnectivity dialer: %v", err)
120 os.Exit(1)
121 }
122
123 userProxyConfig := &httpproxy.Config{
124 HTTPProxy: httpProxyURL,
125 HTTPSProxy: httpsProxyURL,
126 NoProxy: noProxy,
127 }
128 userProxyFunc := userProxyConfig.ProxyFunc()
129
130 httpProxy := goproxy.NewProxyHttpServer()
131 httpProxy.Verbose = true
132
133 if proxyURLHostPort != nil {
134 host, _, err := net.SplitHostPort(*proxyURLHostPort)
135 if err != nil {
136 fmt.Fprintf(os.Stderr, "Error: failed to split proxy URL host port (%s): %v", *proxyURLHostPort, err)
137 }
138 proxyTLS = &tls.Config{
139 MinVersion: tls.VersionTLS12,
140 ServerName: host,
141 }
142 }
143 httpProxy.Tr = &http.Transport{
144 TLSClientConfig: proxyTLS,
145 Proxy: func(req *http.Request) (*url.URL, error) {
146 l.V(4).Info("Determining whether request should be proxied", "url", req.URL)
147 u, err := userProxyFunc(req.URL)
148 if err != nil {
149 l.V(4).Error(err, "failed to determine whether request should be proxied")
150 return nil, err
151 }
152 l.V(4).Info("Should proxy", "url", u)
153 return u, nil
154 },
155 DialContext: konnectivityDialer.DialContext,
156 }
157 if httpsProxyURL != "" {
158 httpProxy.ConnectDialWithReq = connectDialFunc(
159 shouldDialDirectFunc(opts.ConnectDirectlyToCloudAPIs, konnectivityDialer.IsCloudAPI, userProxyFunc),
160 dialDirectFunc(httpProxy),
161 dialThroughProxyFunc(httpProxy, httpsProxyURL, proxyURLUser),
162 )
163 } else {
164 httpProxy.ConnectDial = nil
165 httpProxy.ConnectDialWithReq = nil
166 }
167 err = http.ListenAndServe(fmt.Sprintf(":%d", servingPort), httpProxy)
168 if err != nil {
169 fmt.Fprintf(os.Stderr, "Error: %v", err)
170 os.Exit(1)
171 }
172 }
173
174 return cmd
175 }
176
177 type dialFunc func(ctx context.Context, network, addr string) (net.Conn, error)
178 type dialRequestFunc func(req *http.Request, network, addr string) (net.Conn, error)
179
180 func dialDirectFunc(httpProxy *goproxy.ProxyHttpServer) dialFunc {
181 return httpProxy.Tr.DialContext
182 }
183
184 func dialThroughProxyFunc(httpProxy *goproxy.ProxyHttpServer, proxyURL string, proxyURLUser *url.Userinfo) dialFunc {
185 dialFn := httpProxy.NewConnectDialToProxyWithHandler(proxyURL, addBasicAuthHeader(proxyURLUser))
186 return func(_ context.Context, network, addr string) (net.Conn, error) {
187 return dialFn(network, addr)
188 }
189 }
190
191 func shouldDialDirectFunc(connectDirectlyToCloudAPIs bool, isCloudAPI func(string) bool, userProxyFunc func(*url.URL) (*url.URL, error)) func(*url.URL) (bool, error) {
192 return func(u *url.URL) (bool, error) {
193 if connectDirectlyToCloudAPIs {
194 hostName, err := netutil.HostFromURL(u.String())
195 if err != nil {
196 return false, err
197 }
198 if isCloudAPI(hostName) {
199 return true, nil
200 }
Script executed:
# Find the dialThroughProxyFunc function and understand how it's used
rg "dialThroughProxyFunc" ./konnectivity-https-proxy/ -A 10 -B 5Repository: openshift/hypershift
Repository: openshift/hypershift
Exit code: 0
stdout:
./konnectivity-https-proxy/cmd.go- }
./konnectivity-https-proxy/cmd.go- if httpsProxyURL != "" {
./konnectivity-https-proxy/cmd.go- httpProxy.ConnectDialWithReq = connectDialFunc(
./konnectivity-https-proxy/cmd.go- shouldDialDirectFunc(opts.ConnectDirectlyToCloudAPIs, konnectivityDialer.IsCloudAPI, userProxyFunc),
./konnectivity-https-proxy/cmd.go- dialDirectFunc(httpProxy),
./konnectivity-https-proxy/cmd.go: dialThroughProxyFunc(httpProxy, httpsProxyURL, proxyURLUser),
./konnectivity-https-proxy/cmd.go- )
./konnectivity-https-proxy/cmd.go- } else {
./konnectivity-https-proxy/cmd.go- httpProxy.ConnectDial = nil
./konnectivity-https-proxy/cmd.go- httpProxy.ConnectDialWithReq = nil
./konnectivity-https-proxy/cmd.go- }
./konnectivity-https-proxy/cmd.go- err = http.ListenAndServe(fmt.Sprintf(":%d", servingPort), httpProxy)
./konnectivity-https-proxy/cmd.go- if err != nil {
./konnectivity-https-proxy/cmd.go- fmt.Fprintf(os.Stderr, "Error: %v", err)
./konnectivity-https-proxy/cmd.go- os.Exit(1)
./konnectivity-https-proxy/cmd.go- }
--
./konnectivity-https-proxy/cmd.go-
./konnectivity-https-proxy/cmd.go-func dialDirectFunc(httpProxy *goproxy.ProxyHttpServer) dialFunc {
./konnectivity-https-proxy/cmd.go- return httpProxy.Tr.DialContext
./konnectivity-https-proxy/cmd.go-}
./konnectivity-https-proxy/cmd.go-
./konnectivity-https-proxy/cmd.go:func dialThroughProxyFunc(httpProxy *goproxy.ProxyHttpServer, proxyURL string, proxyURLUser *url.Userinfo) dialFunc {
./konnectivity-https-proxy/cmd.go- dialFn := httpProxy.NewConnectDialToProxyWithHandler(proxyURL, addBasicAuthHeader(proxyURLUser))
./konnectivity-https-proxy/cmd.go- return func(_ context.Context, network, addr string) (net.Conn, error) {
./konnectivity-https-proxy/cmd.go- return dialFn(network, addr)
./konnectivity-https-proxy/cmd.go- }
./konnectivity-https-proxy/cmd.go-}
./konnectivity-https-proxy/cmd.go-
./konnectivity-https-proxy/cmd.go-func shouldDialDirectFunc(connectDirectlyToCloudAPIs bool, isCloudAPI func(string) bool, userProxyFunc func(*url.URL) (*url.URL, error)) func(*url.URL) (bool, error) {
./konnectivity-https-proxy/cmd.go- return func(u *url.URL) (bool, error) {
./konnectivity-https-proxy/cmd.go- if connectDirectlyToCloudAPIs {
./konnectivity-https-proxy/cmd.go- hostName, err := netutil.HostFromURL(u.String())
Script executed:
# Check if context is used elsewhere in this file for dial operations
rg "context\." ./konnectivity-https-proxy/cmd.go -B 2 -A 2Repository: openshift/hypershift
Repository: openshift/hypershift
Exit code: 0
stdout:
}
type dialFunc func(ctx context.Context, network, addr string) (net.Conn, error)
type dialRequestFunc func(req *http.Request, network, addr string) (net.Conn, error)
--
func dialThroughProxyFunc(httpProxy *goproxy.ProxyHttpServer, proxyURL string, proxyURLUser *url.Userinfo) dialFunc {
dialFn := httpProxy.NewConnectDialToProxyWithHandler(proxyURL, addBasicAuthHeader(proxyURLUser))
return func(_ context.Context, network, addr string) (net.Conn, error) {
return dialFn(network, addr)
}
Implement context-aware HTTPS proxy CONNECT dialing to support request cancellation.
The dialThroughProxyFunc wrapper accepts context.Context but ignores it by discarding the parameter with _, breaking context cancellation and timeout propagation on this proxy path. The underlying goproxy.NewConnectDialToProxyWithHandler (v1.7.2) does not natively support context during CONNECT dialing. To align with Kubernetes/Go patterns where the direct dial path (dialDirectFunc) properly respects context via DialContext, implement a custom context-aware CONNECT dialer. Recommended approaches: (1) implement a custom CONNECT handshake that respects ctx.Done(), or (2) use a third-party library like github.com/elastic/proxy-connect-dialer-go that provides context-aware DialContext support for proxy CONNECT operations.
🤖 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 `@konnectivity-https-proxy/cmd.go` around lines 185 - 188, The current
dialThroughProxyFunc wrapper ignores the passed context (it discards ctx and
returns a func that calls dialFn(network, addr)), breaking
cancellation/timeouts; replace this with a context-aware CONNECT dialer by using
or implementing a DialContext-capable CONNECT function: either swap
httpProxy.NewConnectDialToProxyWithHandler for a library that exposes
DialContext (e.g., github.com/elastic/proxy-connect-dialer-go) and call its
DialContext with the incoming ctx, or implement a custom CONNECT handshake
inside dialThroughProxyFunc that performs dial + CONNECT while selecting on
ctx.Done() to cancel the operation; ensure you still add basic auth via
addBasicAuthHeader(proxyURLUser) and keep the returned signature func(ctx
context.Context, network, addr string) (net.Conn, error) so context cancellation
and timeouts propagate.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8550 +/- ##
==========================================
+ Coverage 41.44% 41.45% +0.01%
==========================================
Files 756 756
Lines 93644 93644
==========================================
+ Hits 38807 38821 +14
+ Misses 52114 52100 -14
Partials 2723 2723
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
PoornimaSingour
force-pushed
the
DialwithDialContext
branch
from
48f1889 to
424c65a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
konnectivity-https-proxy/cmd.go (1)
181-183:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
context.Background()on Line 182 drops request cancellation/timeout propagation.Using a background context here disconnects dialing from the incoming request lifecycle. This can leave dials running after client cancellation and weakens the DialContext migration.
Suggested fix
-type dialFunc func(network, addr string) (net.Conn, error) +type dialFunc func(ctx context.Context, network, addr string) (net.Conn, error) func dialDirectFunc(httpProxy *goproxy.ProxyHttpServer) dialFunc { - return func(network, addr string) (net.Conn, error) { - return httpProxy.Tr.DialContext(context.Background(), network, addr) - } + return httpProxy.Tr.DialContext } func dialThroughProxyFunc(httpProxy *goproxy.ProxyHttpServer, proxyURL string, proxyURLUser *url.Userinfo) dialFunc { - return httpProxy.NewConnectDialToProxyWithHandler(proxyURL, addBasicAuthHeader(proxyURLUser)) + dialFn := httpProxy.NewConnectDialToProxyWithHandler(proxyURL, addBasicAuthHeader(proxyURLUser)) + return func(ctx context.Context, network, addr string) (net.Conn, error) { + select { + case <-ctx.Done(): + return nil, ctx.Err() + default: + } + return dialFn(network, addr) + } } func connectDialFunc(shouldDialDirect func(*url.URL) (bool, error), dialDirectly dialFunc, dialThroughProxy dialFunc) dialRequestFunc { return func(req *http.Request, network, addr string) (net.Conn, error) { shouldDialDirectly, err := shouldDialDirect(req.URL) if err != nil { return nil, err } if shouldDialDirectly { - return dialDirectly(network, addr) + return dialDirectly(req.Context(), network, addr) } - return dialThroughProxy(network, addr) + return dialThroughProxy(req.Context(), network, addr) } }#!/bin/bash set -euo pipefail # Verify goproxy version and dial signatures in this repo rg -n 'github.com/elazarl/goproxy' go.mod rg -n --glob '*.go' 'ConnectDialWithReq|NewConnectDialToProxyWithHandler|type .*ConnectDial' vendor/github.com/elazarl/goproxy -C2 # Verify current context propagation in this file rg -n 'type dialFunc|dialDirectFunc|DialContext\(context.Background\)|connectDialFunc\(' konnectivity-https-proxy/cmd.go -C2Expected verification outcome: goproxy CONNECT helpers are non-context signatures in current version, and this file currently hardcodes
context.Background()at Line 182.🤖 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 `@konnectivity-https-proxy/cmd.go` around lines 181 - 183, The dialer currently calls httpProxy.Tr.DialContext(context.Background(), ...) which drops request cancellation; replace the background context with the incoming request's context so dials cancel with the client request: wherever the closure that returns func(network, addr string) (net.Conn, error) is created, capture the request context (e.g. req.Context()) from the goproxy CONNECT handler and call httpProxy.Tr.DialContext(req.Context(), network, addr) instead of context.Background(); if the CONNECT helper in use doesn't expose req, adapt the factory that builds this dial func (the connectDial/ConnectDialWithReq wrapper) to accept or capture a context and propagate it into httpProxy.Tr.DialContext.
🤖 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 `@konnectivity-https-proxy/cmd.go`:
- Around line 181-183: The dialer currently calls
httpProxy.Tr.DialContext(context.Background(), ...) which drops request
cancellation; replace the background context with the incoming request's context
so dials cancel with the client request: wherever the closure that returns
func(network, addr string) (net.Conn, error) is created, capture the request
context (e.g. req.Context()) from the goproxy CONNECT handler and call
httpProxy.Tr.DialContext(req.Context(), network, addr) instead of
context.Background(); if the CONNECT helper in use doesn't expose req, adapt the
factory that builds this dial func (the connectDial/ConnectDialWithReq wrapper)
to accept or capture a context and propagate it into httpProxy.Tr.DialContext.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 173ac80f-1b8f-400a-93ca-5fee6afc0bca
📒 Files selected for processing (1)
konnectivity-https-proxy/cmd.go
|
/jira-refresh |
openshift-ci-robot
added
jira/valid-bug
|
@PoornimaSingour: This pull request references Jira Issue OCPBUGS-55621, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (jiezhao@redhat.com), skipping review request. 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. |
PoornimaSingour
force-pushed
the
DialwithDialContext
branch
from
b4b8d11 to
993f4ea
Compare
|
@PoornimaSingour: This pull request references Jira Issue OCPBUGS-55621, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (jiezhao@redhat.com), skipping review request. 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
konnectivity-https-proxy/cmd_test.go (2)
155-162: ⚡ Quick winConsider returning a valid mock connection or an error in stub dialers.
The stub
directDialandproxyDialfunctions return(nil, nil), which violates Go's convention that a nil error should accompany a valid (non-nil) return value. While this works for tests that only verify which dialer is called, it's not realistic and could mask issues ifconnectDialFuncis later modified to use the returned connection.Consider returning a mock connection (e.g.,
&net.TCPConn{}cast tonet.Connvia a test helper, or a custom mock implementingnet.Conn) withnilerror to better match production behavior.Also applies to: 182-189
🤖 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 `@konnectivity-https-proxy/cmd_test.go` around lines 155 - 162, The stub dialers directDial and proxyDial currently return (nil, nil) which violates Go conventions; update these stubs to return a valid mock net.Conn and nil error (or return a non-nil error when simulating failure) so tests are realistic; implement or reuse a small test helper mock that satisfies net.Conn (e.g., a minimal struct implementing Read/Write/Close/etc.) and have directDial and proxyDial return that mock, and update the same pattern for the other stub block at lines 182-189; this ensures connectDialFunc and callers can safely use the returned connection.
108-218: ⚖️ Poor tradeoffConsider adding tests for context-aware dial behavior.
The current tests verify that
DialContextis wired correctly and that routing logic works as expected. As an optional enhancement, you might consider adding tests that exercise context-aware behavior such as dial cancellation or timeout via context. This would more thoroughly validate the benefits of theDialContextmigration.Example scenarios:
- Canceling a context before dial completes
- Context timeout during a slow dial
This is not required for the current refactoring but could strengthen confidence in context propagation through the dial path.
🤖 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 `@konnectivity-https-proxy/cmd_test.go` around lines 108 - 218, Add tests that exercise context-aware dialing by creating a transport with a DialContext implementation that blocks until ctx.Done() (or respects context deadlines) and then verifying that a canceled or timed-out context causes the dial to return promptly with the context error; apply this pattern to test the DialContext path used by dialDirectFunc and the connectDialFunc call paths (i.e., ensure you invoke the DialContext directly or wire a proxy transport into dialDirectFunc/connectDialFunc so the blocking DialContext is used) and assert the returned error is context.Canceled or context.DeadlineExceeded as appropriate.
🤖 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.
Nitpick comments:
In `@konnectivity-https-proxy/cmd_test.go`:
- Around line 155-162: The stub dialers directDial and proxyDial currently
return (nil, nil) which violates Go conventions; update these stubs to return a
valid mock net.Conn and nil error (or return a non-nil error when simulating
failure) so tests are realistic; implement or reuse a small test helper mock
that satisfies net.Conn (e.g., a minimal struct implementing
Read/Write/Close/etc.) and have directDial and proxyDial return that mock, and
update the same pattern for the other stub block at lines 182-189; this ensures
connectDialFunc and callers can safely use the returned connection.
- Around line 108-218: Add tests that exercise context-aware dialing by creating
a transport with a DialContext implementation that blocks until ctx.Done() (or
respects context deadlines) and then verifying that a canceled or timed-out
context causes the dial to return promptly with the context error; apply this
pattern to test the DialContext path used by dialDirectFunc and the
connectDialFunc call paths (i.e., ensure you invoke the DialContext directly or
wire a proxy transport into dialDirectFunc/connectDialFunc so the blocking
DialContext is used) and assert the returned error is context.Canceled or
context.DeadlineExceeded as appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 594733f7-1e01-437d-a803-03903fbdd62c
📒 Files selected for processing (1)
konnectivity-https-proxy/cmd_test.go
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
PoornimaSingour
force-pushed
the
DialwithDialContext
branch
from
993f4ea to
b43cd95
Compare
|
/verified by me Tested manually on a HyperShift hosted cluster E2E Cluster Validation — ✅ PASSED
Konnectivity Components Verified
|
openshift-ci-robot
added
the
verified
|
@PoornimaSingour: This PR has been marked as verified by 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. |
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!
| }) | ||
| } | ||
|
|
||
| func TestConnectDialFunc(t *testing.T) { |
There was a problem hiding this comment.
nit: this should be table-driven following Gherkin nomenclature, consistent with the other tests in this file (TestShouldDialDirectFunc, TestAddBasicAuthHeader). The three subtests share the same scaffold and only differ in the boolean + expectations.
| return true, nil | ||
| } | ||
|
|
||
| dialFn := connectDialFunc(shouldDirect, directDial, proxyDial) |
There was a problem hiding this comment.
nit: the error from http.NewRequest is silently discarded here (and on lines 191, 213). Maybe:
req, err := http.NewRequest("CONNECT", "https://example.com:443", nil)
g.Expect(err).NotTo(HaveOccurred())
|
|
||
| import ( | ||
| "context" | ||
| "crypto/tls" |
There was a problem hiding this comment.
Checked: removing Dial is safe — Go's http.Transport prefers DialContext when both are set, and goproxy's internal dial() also checks Tr.DialContext first. Good cleanup.
| // nolint:staticcheck | ||
| return httpProxy.Tr.Dial | ||
| return func(network, addr string) (net.Conn, error) { | ||
| return httpProxy.Tr.DialContext(context.Background(), network, addr) |
There was a problem hiding this comment.
In a separate work item, it'd be good to track the propagation of the original request context through the dial instead of introducing a context.Background() here.
Otherwise, lgtm
PoornimaSingour
force-pushed
the
DialwithDialContext
branch
from
b43cd95 to
c9da775
Compare
openshift-ci-robot
removed
the
verified
PoornimaSingour
force-pushed
the
DialwithDialContext
branch
3 times, most recently
from
bf5bd87 to
7fd8035
Compare
|
Due to version mismatch in CI and local, the lint and verify is failing in CI but passing in my local. |
|
Now I have all the evidence. The two lint errors are crystal clear:
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe lint job failed due to two golangci-lint violations in the newly added test file Root CauseTwo distinct lint rule violations in the new test code added by this PR:
Neither issue is in the production code ( Recommendations
Evidence
|
Replace the deprecated `net.Dial` with `DialContext` in konnectivity-https-proxy and propagate the original request context through the dial chain instead of using context.Background(). Introduce dialContextFunc type to carry context from connectDialFunc (which has access to req.Context()) through to dialDirectFunc. Add unit tests for dialDirectFunc covering success and failure scenarios. Co-Authored-By: Poornima Singour <psingour@redhat.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PoornimaSingour
force-pushed
the
DialwithDialContext
branch
from
3de6fa5 to
3cd8363
Compare
|
@PoornimaSingour: 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
What this PR does / why we need it:
Replacing deprecated Dial with DialContext in konnectivity-https-proxy/cmd.go so that golangci-lint passes without needing nolint suppressions.
If we uncomment nolint in cmd.go its giving below error:
konnectivity-https-proxy/cmd.go uses the deprecated Dial field on http.Transport (deprecated since Go 1.7) instead of DialContext. A // nolint:staticcheck comment on line 182 suppresses the lint warning.Which issue(s) this PR fixes:
Fixes ---> https://redhat.atlassian.net/browse/OCPBUGS-55621
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
Improvements
Tests