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 1 commit 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
📝 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8550 +/- ##
==========================================
+ Coverage 40.13% 40.37% +0.24%
==========================================
Files 753 755 +2
Lines 93045 93227 +182
==========================================
+ Hits 37345 37644 +299
+ Misses 53003 52879 -124
- Partials 2697 2704 +7
... and 28 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Remove the deprecated Dial field from http.Transport and replace httpProxy.Tr.Dial with a wrapper around httpProxy.Tr.DialContext in dialDirectFunc so golangci-lint passes without nolint suppressions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 |
|
Now I have all the data. Let me compile the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll three failures are unrelated to the PR's code changes. The PR is a correct, mechanical refactor that replaces the deprecated Root Causecodecov/patch (0.00% of diff hit): Konflux / hypershift-operator-main-enterprise-contract: Konflux / hypershift-operator-enterprise-contract: Recommendations
Evidence
|
2 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