HTTP proxy support for SSH in GitOps#4781
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds HTTP CONNECT proxy support for go-git SSH operations in Fleet GitOps flows by wiring go-git ProxyOptions from standard proxy environment variables and registering a custom x/net/proxy dialer for http:// (and https://) proxy URLs.
Changes:
- Add
ProxyOptionsplumbing to go-git remote listing and thegitclonerCLI clone paths. - Introduce
pkg/git/proxy.goimplementing an HTTP CONNECT dialer andProxyOptsFromEnvironment(...)helper. - Add unit tests for proxy option selection and CONNECT dialer behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| pkg/git/remote.go | Passes go-git ProxyOptions into ListOptions and initializes them from env in NewRemote. |
| pkg/git/proxy.go | Registers proxy dialer types and implements CONNECT tunneling + env-based proxy option selection. |
| pkg/git/proxy_test.go | Adds tests for CONNECT tunneling and proxy env parsing. |
| internal/cmd/cli/gitcloner/cloner.go | Wires proxy options into go-git CloneOptions for CLI cloning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
p-se
force-pushed
the
issue-3595
branch
4 times, most recently
from
842891b to
cb12598
Compare
| return nil, fmt.Errorf("http connect proxy: write CONNECT request: %w", err) | ||
| } | ||
|
|
||
| resp, err := http.ReadResponse(bufio.NewReader(conn), req) |
There was a problem hiding this comment.
bufio.NewReader (4096 bytes) could buffer bytes beyond the 200 response.
A size-1 reader only buffers what http.ReadResponse strictly needs and can't buffer ahead of the response end.
| resp, err := http.ReadResponse(bufio.NewReader(conn), req) | |
| resp, err := http.ReadResponse(bufio.NewReaderSize(conn, 1), req) |
| func TestHTTPConnectDialer_ContextCancelled(t *testing.T) { | ||
| // Use a forward dialer that blocks until the context is done. | ||
| blocking := &recordingDialer{dial: func(network, addr string) (net.Conn, error) { | ||
| time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
Can' we use an Eventually here or at least a much lower sleep time because this is static and quite high?
| } | ||
| done <- struct{}{} | ||
| }() | ||
| <-done |
There was a problem hiding this comment.
nit: Only <-done once despite two piping goroutines sending - drain both or use WaitGroup.
| t.Fatalf("Write: %v", err) | ||
| } | ||
| buf := make([]byte, len(msg)) | ||
| if _, err := io.ReadFull(conn, buf); err != nil && !errors.Is(err, io.ErrUnexpectedEOF) { |
There was a problem hiding this comment.
Tolerating io.ErrUnexpectedEOF means the buffer could be partially filled (e.g., all zeros), and string(buf) != msg would still fire - so the test doesn't silently pass wrong data. However, dropping the ErrUnexpectedEOF carve-out and asserting err == nil would make the test stronger as a signal that the TLS tunnel is working.
If the test server's connection teardown before io.ReadFull completes could result in a race condition, a sync.WaitGroup in startFakeTLSProxy to wait for the echo goroutines to finish first should be better than tolerating the partial-read error.
TestHTTPConnectDialer_Dial timed out after 10 minutes because handleProxyConn waited for both piping goroutines to signal done, but the target→client goroutine blocked on targetConn.Read indefinitely when the client closed its side. Added a sync.Once-guarded closeBoth() that closes both connections when either direction finishes, so the other goroutine unblocks immediately.
Replace the manual read/write loops and sync.Once/closeBoth with io.Copy and a simple targetConn.Close() after the client→target direction finishes — the same pattern already used in startFakeTLSProxy in the same file.
Refers to #3595
Additional Information
Checklist
fleet-docs repository.