From ddbf5dece4edc045fb0cea2beb1dd450c6e8c599 Mon Sep 17 00:00:00 2001 From: Luther Monson Date: Thu, 1 Oct 2026 22:54:28 -0700 Subject: [PATCH] fix(proxies): rewrite GOPROXY's fallback list, not just single URLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #208 rewrote the advertised host for every URL-valued proxy var — except the one that matters most on a Go codebase. GOPROXY is not a URL, it is a fallback LIST: Go accepts elements separated by "," or "|" mixed with the literals "direct" and "off". Production emits GOPROXY=http://10.88.0.1:8082|direct and url.Parse on that whole string yields no usable authority, so the single-URL rewrite passed it through untouched. Read off mfl-mac-arm64 minutes after rolling v0.2.17: GOPROXY=http://10.88.0.1:8082|direct <- NOT rewritten RUSTUP_DIST_SERVER=http://192.168.64.1:8083/rustup <- ok GHREL_PROXY=http://192.168.64.1:8087 <- ok COMPOSER_REPO_PACKAGIST=http://192.168.64.1:8088 <- ok Three of four correct, and the log line said "cache proxies advertised to jobs on the VM-visible host address" — success by every signal except the one that counts. Go kept pointing at an unreachable address. Why the tests missed it: the fixture used a bare GOPROXY=http://host:port, an input invented rather than observed. It is now copied verbatim from the live node, with a comment not to tidy it. Also fixes the probe: EnvHostPorts parsed the whole value, so it never saw the Go proxy's address and would have advertised a host it never checked — defeating the gate's purpose. Separators and literals are preserved exactly: "a|direct", "a,direct", multiple proxies, "off", and a trailing separator all round-trip. Revert-verified: restoring the single-URL call fails both the fleet-env test and the new list table. --- pkg/proxies/advertise.go | 96 ++++++++++++++++++++++++++--------- pkg/proxies/advertise_test.go | 44 ++++++++++++++-- 2 files changed, 114 insertions(+), 26 deletions(-) diff --git a/pkg/proxies/advertise.go b/pkg/proxies/advertise.go index 7a64608..7d32e6e 100644 --- a/pkg/proxies/advertise.go +++ b/pkg/proxies/advertise.go @@ -46,19 +46,57 @@ func RewriteEnvHost(env []string, host string) []string { out = append(out, e) continue } - u, err := url.Parse(v) - if err != nil || u.Host == "" || (u.Scheme != "http" && u.Scheme != "https") { - out = append(out, e) + out = append(out, k+"="+rewriteURLList(v, host)) + } + return out +} + +// rewriteURLList rewrites every URL element of a value that may be a LIST. +// +// GOPROXY is not a URL, it is a fallback list: Go accepts elements separated by +// "," or "|", mixed with the literals "direct" and "off". Production emits +// +// GOPROXY=http://10.88.0.1:8082|direct +// +// and url.Parse on that whole string yields no usable host:port, so the naive +// single-URL version silently left GOPROXY alone while correctly rewriting +// RUSTUP_DIST_SERVER, GHREL_PROXY and COMPOSER_REPO_PACKAGIST. Three of four +// vars fixed looks like success in a log line and leaves Go — the heaviest +// consumer on a Go codebase — still pointed at an unreachable address. +// +// Caught only by reading the real env off a live node. The original test used a +// bare GOPROXY=http://host:port, an input invented rather than observed, and it +// passed. +func rewriteURLList(v, host string) string { + var b strings.Builder + start := 0 + for i := 0; i <= len(v); i++ { + if i < len(v) && v[i] != ',' && v[i] != '|' { continue } - if port := u.Port(); port != "" { - u.Host = net.JoinHostPort(host, port) - } else { - u.Host = host + b.WriteString(rewriteOneURL(v[start:i], host)) + if i < len(v) { + b.WriteByte(v[i]) // keep the caller's separator exactly } - out = append(out, k+"="+u.String()) + start = i + 1 } - return out + return b.String() +} + +// rewriteOneURL swaps the host of a single http(s) URL, preserving port and +// path. Anything that is not such a URL — "direct", "off", "sparse", a bare +// value — is returned untouched. +func rewriteOneURL(s, host string) string { + u, err := url.Parse(s) + if err != nil || u.Host == "" || (u.Scheme != "http" && u.Scheme != "https") { + return s + } + if port := u.Port(); port != "" { + u.Host = net.JoinHostPort(host, port) + } else { + u.Host = host + } + return u.String() } // HostAddrVisibleTo returns the local address that peerIP can reach this host @@ -156,25 +194,37 @@ func EnvHostPorts(env []string) []string { if !ok { continue } - u, err := url.Parse(v) - if err != nil || u.Host == "" { - continue - } - hp := u.Host - if u.Port() == "" { - switch u.Scheme { - case "http": - hp = net.JoinHostPort(u.Hostname(), "80") - case "https": - hp = net.JoinHostPort(u.Hostname(), "443") - default: + // Walk LIST elements, not the whole value: GOPROXY is + // "http://host:port|direct", and parsing that as one URL yields no + // usable authority — so the probe would skip the Go proxy entirely and + // happily advertise an address it never checked. + for _, el := range strings.FieldsFunc(v, func(r rune) bool { return r == ',' || r == '|' }) { + hp, ok := hostPortOf(el) + if !ok || seen[hp] { continue } - } - if !seen[hp] { seen[hp] = true out = append(out, hp) } } return out } + +// hostPortOf returns the dialable authority of an http(s) URL, defaulting the +// port from the scheme when absent. +func hostPortOf(s string) (string, bool) { + u, err := url.Parse(s) + if err != nil || u.Host == "" { + return "", false + } + if u.Port() != "" { + return u.Host, true + } + switch u.Scheme { + case "http": + return net.JoinHostPort(u.Hostname(), "80"), true + case "https": + return net.JoinHostPort(u.Hostname(), "443"), true + } + return "", false +} diff --git a/pkg/proxies/advertise_test.go b/pkg/proxies/advertise_test.go index 58ba42f..e4c375a 100644 --- a/pkg/proxies/advertise_test.go +++ b/pkg/proxies/advertise_test.go @@ -9,10 +9,17 @@ import ( "time" ) -// The exact env the fleet's proxies emit, with the exact host that broke it. +// The env the fleet's proxies ACTUALLY emit, copied verbatim off +// mfl-mac-arm64's log on 2026-10-02 — note GOPROXY carries Go's "|direct" +// fallback list, which the first version of this test did not. +// +// That omission is why a half-working fix shipped: with a bare +// GOPROXY=http://host:port the tests passed, while production left GOPROXY +// unrewritten and pointed at an unreachable address. Keep this fixture +// byte-identical to observed output; do not "tidy" it. func fleetEnv() []string { return []string{ - "GOPROXY=http://10.88.0.1:8082", + "GOPROXY=http://10.88.0.1:8082|direct", "GOSUMDB=off", "CARGO_REGISTRIES_CRATES_IO_PROTOCOL=sparse", "RUSTUP_DIST_SERVER=http://10.88.0.1:8083/rustup", @@ -25,7 +32,7 @@ func TestRewriteEnvHostFixesTheAdvertisedAddress(t *testing.T) { got := RewriteEnvHost(fleetEnv(), "192.168.64.1") want := []string{ - "GOPROXY=http://192.168.64.1:8082", + "GOPROXY=http://192.168.64.1:8082|direct", "GOSUMDB=off", "CARGO_REGISTRIES_CRATES_IO_PROTOCOL=sparse", "RUSTUP_DIST_SERVER=http://192.168.64.1:8083/rustup", @@ -187,3 +194,34 @@ func TestHostAddrInSubnet(t *testing.T) { t.Error("HostAddrInSubnet(nil) matched") } } + +// Regression for the half-working fix shipped on 2026-10-02: GOPROXY is a +// fallback LIST, not a URL. Production emits "http://host:port|direct"; the +// single-URL rewrite silently skipped it, so Go — the heaviest consumer in a Go +// codebase — kept pointing at an unreachable proxy while three other vars were +// correctly rewritten and the log line claimed success. +func TestRewriteEnvHostHandlesGoProxyLists(t *testing.T) { + for _, tc := range []struct{ name, in, want string }{ + {"pipe direct", "http://10.88.0.1:8082|direct", "http://192.168.64.1:8082|direct"}, + {"comma direct", "http://10.88.0.1:8082,direct", "http://192.168.64.1:8082,direct"}, + {"two proxies then direct", "http://10.88.0.1:8082|https://proxy.golang.org|direct", + "http://192.168.64.1:8082|https://192.168.64.1|direct"}, + {"off literal", "off", "off"}, + {"direct only", "direct", "direct"}, + {"trailing separator kept", "http://10.88.0.1:8082|", "http://192.168.64.1:8082|"}, + } { + got := RewriteEnvHost([]string{"GOPROXY=" + tc.in}, "192.168.64.1") + if got[0] != "GOPROXY="+tc.want { + t.Errorf("%s:\n got %q\nwant %q", tc.name, got[0], "GOPROXY="+tc.want) + } + } +} + +// The probe must see the Go proxy's address too, or it advertises a host it +// never verified — defeating the entire point of the gate. +func TestEnvHostPortsSeesListElements(t *testing.T) { + got := EnvHostPorts([]string{"GOPROXY=http://10.88.0.1:8082|direct"}) + if len(got) != 1 || got[0] != "10.88.0.1:8082" { + t.Errorf("got %q, want [10.88.0.1:8082] — the probe is blind to list elements", got) + } +}