diff --git a/cmd/cli/library.go b/cmd/cli/library.go index 5247440..906e3ac 100644 --- a/cmd/cli/library.go +++ b/cmd/cli/library.go @@ -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 diff --git a/cmd/cli/library_retry_test.go b/cmd/cli/library_retry_test.go new file mode 100644 index 0000000..9252944 --- /dev/null +++ b/cmd/cli/library_retry_test.go @@ -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) + } +} diff --git a/cmd/cli/prog.go b/cmd/cli/prog.go index e8ed760..0255df8 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -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) { diff --git a/internal/controld/config.go b/internal/controld/config.go index f1bb861..712e2db 100644 --- a/internal/controld/config.go +++ b/internal/controld/config.go @@ -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) +} diff --git a/internal/controld/fallback_test.go b/internal/controld/fallback_test.go new file mode 100644 index 0000000..7ab0b33 --- /dev/null +++ b/internal/controld/fallback_test.go @@ -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) + } +}