all: keep the first-attempt error when the direct-ip fallback also fails

Both API requests and binary downloads retry against a hard-coded IP when the
attempt via hostname fails. Both then overwrote the first error with the
fallback's, so only the last failure was reported.

That discarded the diagnosis. During the Firewall Mode incident the hostname
attempt was denied locally - WSAEACCES, "An attempt was made to access a socket
in a way forbidden by its access permissions", which means the host is blocking
ctrld - while the direct-ip fallback failed with an unreachable IPv6 route. What
surfaced to the operator was "dial tcp6: no route to host", pointing at a
routing problem that did not exist, while the WSAEACCES that named the real
cause was visible only in debug logs.

Report both failures instead, keeping the error chain intact so errors.Is still
matches either one. Also switch the final wrap in doWithRetry from %v to %w,
which had been flattening the chain even when a single error was reported.

Add coverage for both paths, including that the fallback is still attempted and
that a successful fallback returns no error.

This also changes retry classification, which is worth stating explicitly because
it is not obvious from "report both errors". processCDFlags decides whether to
keep backing off with errUrlNetworkError, which uses errors.As - and errors.As
returns the first match in the tree. Wrapping the hostname attempt first
therefore hands the predicate that attempt's failure, where previously only the
fallback's error survived to be classified.

The effect is intended. A locally denied socket (WSAEACCES) is not a transient
network error, so preflight now fails fast and reports instead of retrying
against a firewall that is not going to clear on its own - the incident logged
256 retry cycles doing exactly that. The case that justifies retrying forever, a
network unreachable on both attempts at boot, is unchanged. Both classifications
are pinned by tests, along with the wrap order they depend on at each composition
site, so reversing it fails loudly rather than silently restoring the old
behaviour.
This commit is contained in:
Cuong Manh Le
2026-08-14 15:29:32 +07:00
parent 891d5d9821
commit 052b057756
5 changed files with 489 additions and 17 deletions
+17 -6
View File
@@ -64,8 +64,13 @@ func httpClientWithFallback(timeout time.Duration) *http.Client {
// doWithRetry performs an HTTP request with retries
// This improves reliability by automatically retrying failed requests with exponential backoff
func doWithRetry(req *http.Request, maxRetries int, ip string) (*http.Response, error) {
return doWithRetryClient(httpClientWithFallback(defaultHTTPTimeout), req, maxRetries, ip)
}
// doWithRetryClient is doWithRetry with an injectable client, so the retry and
// error-composition behaviour can be tested without real network access.
func doWithRetryClient(client *http.Client, req *http.Request, maxRetries int, ip string) (*http.Response, error) {
var lastErr error
client := httpClientWithFallback(defaultHTTPTimeout)
var ipReq *http.Request
if ip != "" {
ipReq = req.Clone(req.Context())
@@ -82,22 +87,28 @@ func doWithRetry(req *http.Request, maxRetries int, ip string) (*http.Response,
if err == nil {
return resp, nil
}
// Keep the hostname attempt's error: it carries the diagnosis (on Windows,
// a local firewall denying the socket shows up here as WSAEACCES), while the
// direct-ip fallback often fails for an unrelated reason such as an
// unreachable IPv6 route.
attemptErr := err
if ipReq != nil {
mainLog.Load().Warn().Err(err).Msgf("Dial to %q failed", req.Host)
mainLog.Load().Warn().Msgf("Fallback to direct ip to download prod version: %q", ip)
resp, err = client.Do(ipReq)
if err == nil {
resp, fallbackErr := client.Do(ipReq)
if fallbackErr == nil {
return resp, nil
}
attemptErr = fmt.Errorf("%w; fallback to direct ip %s failed: %w", attemptErr, ip, fallbackErr)
}
lastErr = err
mainLog.Load().Debug().Err(err).
lastErr = attemptErr
mainLog.Load().Debug().Err(attemptErr).
Str("method", req.Method).
Str("url", req.URL.String()).
Msgf("HTTP request attempt %d/%d failed", attempt+1, maxRetries)
}
return nil, fmt.Errorf("failed after %d attempts to %s %s: %v", maxRetries, req.Method, req.URL, lastErr)
return nil, fmt.Errorf("failed after %d attempts to %s %s: %w", maxRetries, req.Method, req.URL, lastErr)
}
// Helper for making GET requests with retries
+242
View File
@@ -0,0 +1,242 @@
package cli
import (
"context"
"errors"
"fmt"
"net"
"net/http"
"net/url"
"syscall"
"testing"
"github.com/Control-D-Inc/ctrld/internal/controld"
)
// wsaEACCES is WSAEACCES (10013): "An attempt was made to access a socket in a way
// forbidden by its access permissions." This is what Windows reports when a WFP
// filter denies the connect. Used as a plain errno so the test runs everywhere.
const wsaEACCES = syscall.Errno(10013)
// denyingRoundTripper denies the hostname attempt with firstErr and the direct-ip
// attempt with fbErr, the shape seen during the Firewall Mode incident: the
// hostname attempt was denied by ctrld's own stale block-all filters, while the
// direct-ip fallback failed on an unreachable IPv6 route.
type denyingRoundTripper struct {
hostname string
firstErr error
fbErr error
}
func (rt *denyingRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
if req.URL.Host == rt.hostname {
return nil, &net.OpError{Op: "dial", Net: "tcp4", Err: rt.firstErr}
}
return nil, &net.OpError{Op: "dial", Net: "tcp6", Err: rt.fbErr}
}
func TestDoWithRetryPreservesHostnameError(t *testing.T) {
const hostname = "dl.controld.dev"
req, err := http.NewRequest(http.MethodGet, "https://"+hostname+"/v2/windows-amd64/ctrld.exe", nil)
if err != nil {
t.Fatal(err)
}
rt := &denyingRoundTripper{
hostname: hostname,
firstErr: wsaEACCES,
fbErr: syscall.EHOSTUNREACH,
}
_, err = doWithRetryClient(&http.Client{Transport: rt}, req, 1, "23.171.240.151")
if err == nil {
t.Fatal("expected doWithRetry to fail when both attempts are denied")
}
if !errors.Is(err, wsaEACCES) {
t.Errorf("hostname-attempt error (WSAEACCES) was lost, got: %v", err)
}
if !errors.Is(err, syscall.EHOSTUNREACH) {
t.Errorf("fallback error was lost, got: %v", err)
}
}
// composedAttemptErrors builds the error shape the two-attempt paths return: each
// attempt's *url.Error (as produced by http.Client.Do) wrapped by a single fmt.Errorf
// with two %w verbs, hostname attempt first. Mirrors doWithFallback in
// internal/controld and doWithRetryClient above.
func composedAttemptErrors(first, fallback error) error {
attempt := func(network string, cause error) error {
return &url.Error{
Op: "Post",
URL: "https://api.controld.com/utility",
Err: &net.OpError{Op: "dial", Net: network, Err: cause},
}
}
return fmt.Errorf("request failed: %w; fallback to direct ip %s failed: %w",
attempt("tcp4", first), "147.185.34.1", attempt("tcp6", fallback))
}
// TestComposedFallbackErrorRetryClassification pins which attempt decides whether
// preflight keeps retrying.
//
// Reporting both attempt errors is not purely diagnostic: processCDFlags decides
// retryability with errUrlNetworkError, which uses errors.As, and errors.As is
// order-sensitive - it returns the *first* matching error in the tree. Composing the
// hostname attempt first therefore hands the retry predicate the hostname failure,
// where previously only the fallback's error survived to be classified.
//
// The consequence is deliberate: a locally denied socket (WSAEACCES, a firewall
// blocking ctrld) is no longer treated as a transient network error, so preflight fails
// fast and reports instead of backing off - the incident logged 256 retry cycles
// against filters that were never going to clear on their own. The boot case that
// justifies the indefinite retry, a network unreachable on both attempts, is preserved.
//
// If the wrap order is ever reversed, this test fails rather than silently restoring
// indefinite retries against a host that is actively refusing.
func TestComposedFallbackErrorRetryClassification(t *testing.T) {
tests := []struct {
name string
hostname error
fallback error
wantRetryable bool
}{
{
// The incident's pair: denied locally, IPv6 route unusable.
name: "denied socket then unreachable fallback fails fast",
hostname: wsaEACCES,
fallback: syscall.EHOSTUNREACH,
wantRetryable: false,
},
{
// Boot with no network yet: must still retry indefinitely.
name: "network unreachable on both attempts still retries",
hostname: syscall.ENETUNREACH,
fallback: syscall.ENETUNREACH,
wantRetryable: true,
},
{
name: "connection refused still retries",
hostname: syscall.ECONNREFUSED,
fallback: syscall.EHOSTUNREACH,
wantRetryable: true,
},
{
name: "permission denied on both attempts fails fast",
hostname: syscall.EACCES,
fallback: syscall.EACCES,
wantRetryable: false,
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
err := composedAttemptErrors(tc.hostname, tc.fallback)
if got := errUrlNetworkError(err); got != tc.wantRetryable {
t.Errorf("errUrlNetworkError() = %v, want %v", got, tc.wantRetryable)
}
// Both attempts remain reportable regardless of classification.
if !errors.Is(err, tc.hostname) {
t.Error("hostname attempt error was lost")
}
if !errors.Is(err, tc.fallback) {
t.Error("fallback attempt error was lost")
}
})
}
}
// TestUnresolvedHostnameDefersToFallbackAttempt covers the asymmetric pair.
//
// Only the hostname attempt resolves DNS, and Go marks a *net.DNSError as temporary only
// for socket failures that reached the server - so a SERVFAIL or "no such host" answer is
// not temporary. At boot behind a captive portal, or before a router's forwarder is up,
// that is exactly how the hostname attempt fails while the network is merely not ready.
// Before the composed error existed only the fallback decided, so this pair retried;
// classifying the hostname attempt alone would fail it fast and reach Fatal.
//
// A name-resolution failure therefore carries no verdict: the fallback attempt decides.
// The locally-denied case above still fails fast, because a denied socket is definitive.
func TestUnresolvedHostnameDefersToFallbackAttempt(t *testing.T) {
dnsFailure := &url.Error{
Op: "Post",
URL: "https://api.controld.com/utility",
Err: &net.DNSError{Err: "server misbehaving", Name: "api.controld.com", IsTemporary: false},
}
attempt := func(cause error) error {
return &url.Error{
Op: "Post",
URL: "https://api.controld.com/utility",
Err: &net.OpError{Op: "dial", Net: "tcp6", Err: cause},
}
}
retryable := fmt.Errorf("request failed: %w; fallback to direct ip %s failed: %w",
dnsFailure, "147.185.34.1", attempt(syscall.ECONNREFUSED))
if !errUrlNetworkError(retryable) {
t.Error("an unresolved hostname with a retryable fallback must keep retrying: at boot the network is simply not up yet")
}
denied := fmt.Errorf("request failed: %w; fallback to direct ip %s failed: %w",
dnsFailure, "147.185.34.1", attempt(wsaEACCES))
if errUrlNetworkError(denied) {
t.Error("an unresolved hostname with a denied fallback must fail fast: nothing here clears on its own")
}
// A resolution failure alone still says nothing, so it must not be read as retryable.
if errUrlNetworkError(dnsFailure) {
t.Error("a bare name-resolution failure must not be classified as retryable")
}
}
// TestDoWithFallbackClassificationEndToEnd drives the real composition in
// internal/controld through the real predicate, instead of asserting a hand-written copy
// of its error shape against another hand-written copy. A change to either side's format
// string or wrap order is caught here.
func TestDoWithFallbackClassificationEndToEnd(t *testing.T) {
const hostname = "api.controld.com"
req, err := http.NewRequest(http.MethodPost, "https://"+hostname+"/utility", nil)
if err != nil {
t.Fatal(err)
}
rt := &denyingRoundTripper{
hostname: hostname,
firstErr: wsaEACCES,
fbErr: syscall.EHOSTUNREACH,
}
_, gotErr := controld.DoWithFallbackForTest(context.Background(), &http.Client{Transport: rt}, req, "147.185.34.1")
if gotErr == nil {
t.Fatal("expected both attempts to fail")
}
if errUrlNetworkError(gotErr) {
t.Errorf("the real composed error was classified as retryable: %v", gotErr)
}
if !errors.Is(gotErr, wsaEACCES) || !errors.Is(gotErr, syscall.EHOSTUNREACH) {
t.Errorf("the real composed error lost an attempt: %v", gotErr)
}
}
// TestDoWithRetryComposesHostnameAttemptFirst anchors the ordering assumption above to
// the real composition, so a reordering of the wrap in doWithRetryClient is caught here
// and not only in the hand-built shape.
func TestDoWithRetryComposesHostnameAttemptFirst(t *testing.T) {
const hostname = "dl.controld.dev"
req, err := http.NewRequest(http.MethodGet, "https://"+hostname+"/v2/windows-amd64/ctrld.exe", nil)
if err != nil {
t.Fatal(err)
}
rt := &denyingRoundTripper{hostname: hostname, firstErr: wsaEACCES, fbErr: syscall.EHOSTUNREACH}
_, gotErr := doWithRetryClient(&http.Client{Transport: rt}, req, 1, "23.171.240.151")
if gotErr == nil {
t.Fatal("expected both attempts to fail")
}
// errors.As must reach the hostname attempt first: that is what the retry
// predicate classifies.
var opErr *net.OpError
if !errors.As(gotErr, &opErr) {
t.Fatalf("no net.OpError in the chain: %v", gotErr)
}
if !errors.Is(opErr.Err, wsaEACCES) {
t.Errorf("first OpError in the chain is %v, want the hostname attempt (%v)", opErr.Err, wsaEACCES)
}
}
+45 -3
View File
@@ -1500,14 +1500,56 @@ var (
windowsEADDRINUSE = syscall.Errno(10048)
)
// errUrlNetworkError reports whether a failed HTTP attempt is worth retrying.
//
// The two-attempt paths compose one *url.Error per attempt - hostname first, then the
// direct-IP fallback - so this walks them in order rather than classifying only the first
// one errors.As happens to find. Each attempt can say one of three things:
//
// - retryable (unreachable, refused, temporary): retry, whichever attempt said it;
// - a name-resolution failure: no verdict. Only the hostname attempt resolves DNS, and
// at boot behind a captive portal or before the router's forwarder is up it fails
// this way while the network is merely not ready yet. Consult the next attempt;
// - anything else, notably a locally denied socket (WSAEACCES from a firewall blocking
// ctrld): definitive. Stop, because retrying cannot clear it - the Firewall Mode
// incident spent 256 retry cycles against filters that were never going to clear.
func errUrlNetworkError(err error) bool {
var urlErr *url.Error
if errors.As(err, &urlErr) {
return errNetworkError(urlErr.Err)
for _, attempt := range attemptErrors(err) {
var urlErr *url.Error
if !errors.As(attempt, &urlErr) {
continue
}
switch {
case errNetworkError(urlErr.Err):
return true
case errDNSResolutionFailure(urlErr.Err):
// Neutral; let a later attempt decide.
default:
return false
}
}
return false
}
// attemptErrors returns the per-attempt errors recorded in err, in the order they were
// tried. A composed fallback error wraps one per attempt; anything else is a single
// attempt.
func attemptErrors(err error) []error {
if multi, ok := err.(interface{ Unwrap() []error }); ok {
return multi.Unwrap()
}
return []error{err}
}
// errDNSResolutionFailure reports whether err is a name-resolution failure. Go marks a
// *net.DNSError as temporary only for socket failures that reached the server, so a
// SERVFAIL or "no such host" answer is not temporary - but it is also not evidence that
// retrying is pointless, which is why callers treat it as no verdict.
func errDNSResolutionFailure(err error) bool {
var dnsErr *net.DNSError
return errors.As(err, &dnsErr)
}
func errNetworkError(err error) bool {
var opErr *net.OpError
if errors.As(err, &opErr) {
+29 -8
View File
@@ -390,17 +390,31 @@ func addrsFromPort(ips []string, port string) []string {
return addrs
}
// doWithFallback sends req, retrying against apiIp directly if the first attempt
// fails (typically because DNS is not usable yet).
//
// Both failures are reported. The first attempt carries the diagnosis - on Windows
// a local firewall denying the socket surfaces there as WSAEACCES ("An attempt was
// made to access a socket in a way forbidden by its access permissions"), which
// says the host is blocking ctrld rather than that the network is down. Returning
// only the fallback error hid that behind a bare "no route to host" from the IPv6
// attempt and sent the Firewall Mode incident investigation after a routing
// problem that did not exist.
func doWithFallback(ctx context.Context, client *http.Client, req *http.Request, apiIp string) (*http.Response, error) {
resp, err := client.Do(req)
if err != nil {
logger := ctrld.LoggerFromCtx(ctx)
logger.Warn().Err(err).Msgf("Failed to send request, fallback to direct ip: %s", apiIp)
ipReq := req.Clone(req.Context())
ipReq.Host = apiIp
ipReq.URL.Host = apiIp
resp, err = client.Do(ipReq)
if err == nil {
return resp, nil
}
return resp, err
logger := ctrld.LoggerFromCtx(ctx)
logger.Warn().Err(err).Msgf("Failed to send request, fallback to direct ip: %s", apiIp)
ipReq := req.Clone(req.Context())
ipReq.Host = apiIp
ipReq.URL.Host = apiIp
resp, fallbackErr := client.Do(ipReq)
if fallbackErr != nil {
return nil, fmt.Errorf("request failed: %w; fallback to direct ip %s failed: %w", err, apiIp, fallbackErr)
}
return resp, nil
}
// apiServerIP returns the direct IP to connect to API server.
@@ -410,3 +424,10 @@ func apiServerIP(cdDev bool) string {
}
return apiDomainComIPv4
}
// DoWithFallbackForTest exposes doWithFallback so tests outside this package can drive
// the real two-attempt composition through the real retry predicate, rather than
// asserting a copy of this error shape against another copy of it.
func DoWithFallbackForTest(ctx context.Context, client *http.Client, req *http.Request, apiIp string) (*http.Response, error) {
return doWithFallback(ctx, client, req, apiIp)
}
+156
View File
@@ -0,0 +1,156 @@
package controld
import (
"context"
"errors"
"net"
"net/http"
"net/http/httptest"
"strings"
"syscall"
"testing"
)
// errRoundTripper fails the hostname attempt and the direct-ip attempt with
// different errors, mimicking the Firewall Mode incident: the hostname attempt is
// denied by a local firewall (WSAEACCES on Windows) while the direct-ip fallback
// reports an unreachable IPv6 route.
type errRoundTripper struct {
hostname string
firstErr error
fbErr error
fbCalled bool
}
func (rt *errRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
if req.URL.Host == rt.hostname {
return nil, &net.OpError{Op: "dial", Net: "tcp4", Err: rt.firstErr}
}
rt.fbCalled = true
if rt.fbErr == nil {
return &http.Response{
StatusCode: http.StatusOK,
Body: http.NoBody,
Request: req,
}, nil
}
return nil, &net.OpError{Op: "dial", Net: "tcp6", Err: rt.fbErr}
}
// wsaEACCES is WSAEACCES (10013): "An attempt was made to access a socket in a way
// forbidden by its access permissions." The value is what Windows reports when a
// WFP filter denies the connect; it is used here as a plain errno so the test runs
// on every platform.
const wsaEACCES = syscall.Errno(10013)
func TestDoWithFallbackPreservesFirstError(t *testing.T) {
const (
hostname = "api.controld.com"
apiIP = "147.185.34.1"
)
rt := &errRoundTripper{
hostname: hostname,
firstErr: wsaEACCES,
fbErr: syscall.EHOSTUNREACH,
}
req, err := http.NewRequest(http.MethodPost, "https://"+hostname+"/utility", nil)
if err != nil {
t.Fatal(err)
}
resp, err := doWithFallback(context.Background(), &http.Client{Transport: rt}, req, apiIP)
if err == nil {
t.Fatalf("expected an error, got response %v", resp)
}
if !rt.fbCalled {
t.Error("direct-ip fallback was not attempted")
}
// The actionable failure must survive: an operator reading this error has to be
// able to tell "the host is blocking us" from "the network is down".
if !errors.Is(err, wsaEACCES) {
t.Errorf("first-attempt error (WSAEACCES) was lost, got: %v", err)
}
if !errors.Is(err, syscall.EHOSTUNREACH) {
t.Errorf("fallback error was lost, got: %v", err)
}
if got := err.Error(); !strings.Contains(got, apiIP) {
t.Errorf("error does not mention the fallback ip %q: %v", apiIP, got)
}
}
func TestDoWithFallbackSucceedsOnFallback(t *testing.T) {
const hostname = "api.controld.com"
rt := &errRoundTripper{hostname: hostname, firstErr: wsaEACCES}
req, err := http.NewRequest(http.MethodPost, "https://"+hostname+"/utility", nil)
if err != nil {
t.Fatal(err)
}
resp, err := doWithFallback(context.Background(), &http.Client{Transport: rt}, req, "147.185.34.1")
if err != nil {
t.Fatalf("expected the fallback to succeed, got: %v", err)
}
defer resp.Body.Close()
if resp.StatusCode != http.StatusOK {
t.Errorf("StatusCode = %d, want %d", resp.StatusCode, http.StatusOK)
}
}
func TestDoWithFallbackNoFallbackOnSuccess(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
}))
defer srv.Close()
req, err := http.NewRequest(http.MethodPost, srv.URL, nil)
if err != nil {
t.Fatal(err)
}
resp, err := doWithFallback(context.Background(), srv.Client(), req, "127.0.0.2")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
defer resp.Body.Close()
if resp.StatusCode != http.StatusOK {
t.Errorf("StatusCode = %d, want %d", resp.StatusCode, http.StatusOK)
}
}
// TestDoWithFallbackComposesHostnameAttemptFirst pins the order of the composed error.
//
// The order is not cosmetic. cmd/cli's preflight retry predicate classifies this error
// with errors.As, which returns the first match in the tree, so whichever attempt is
// wrapped first decides whether processCDFlags keeps backing off or fails fast. That
// predicate lives in another package and cannot be called from here, so this test
// guards the property it depends on: the hostname attempt - the one that carries the
// diagnosis - must come first.
func TestDoWithFallbackComposesHostnameAttemptFirst(t *testing.T) {
const hostname = "api.controld.com"
rt := &errRoundTripper{
hostname: hostname,
firstErr: wsaEACCES,
fbErr: syscall.EHOSTUNREACH,
}
req, err := http.NewRequest(http.MethodPost, "https://"+hostname+"/utility", nil)
if err != nil {
t.Fatal(err)
}
_, gotErr := doWithFallback(context.Background(), &http.Client{Transport: rt}, req, "147.185.34.1")
if gotErr == nil {
t.Fatal("expected both attempts to fail")
}
var opErr *net.OpError
if !errors.As(gotErr, &opErr) {
t.Fatalf("no net.OpError in the chain: %v", gotErr)
}
if !errors.Is(opErr.Err, wsaEACCES) {
t.Errorf("first OpError in the chain is %v, want the hostname attempt (%v)", opErr.Err, wsaEACCES)
}
// The tcp4/tcp6 split distinguishes the two attempts in the fake transport.
if opErr.Net != "tcp4" {
t.Errorf("first OpError is from the %s attempt, want tcp4 (hostname)", opErr.Net)
}
}