diff --git a/cmd/cli/cli.go b/cmd/cli/cli.go index 5cd9d1d..610dfc4 100644 --- a/cmd/cli/cli.go +++ b/cmd/cli/cli.go @@ -318,23 +318,56 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { } if cdUID != "" { validateCdUpstreamProtocol() - if rc, err := processCDFlags(&cfg); err != nil { + // Bound API preflight by the service lifetime. Without this, a stop request + // arriving while the API is unreachable leaves this retry/backoff loop running + // after "service stopped" was logged, so the process keeps working on behalf of + // a service the OS considers stopped. + pf := runAPIPreflight(p.stopCh, &cfg) + switch { + case pf.stopRequested: + // Stop requested during preflight, whether or not the fetch itself + // succeeded. A successful fetch does not entitle startup to continue: the + // operator asked for a stop, and carrying on would set up listeners and + // interception for a service the OS already considers stopping. + // + // Exit the way a normal stop does: no Fatal, so the OS service manager does + // not see a failed start and apply its restart policy to a service the + // operator just asked to stop. + mainLog.Load().Notice().Msg("stop requested while fetching resolver config, shutting down") + notifyExitToLogServer() + return + case pf.err != nil: if isMobile() { - appCallback.Exit(err.Error()) + appCallback.Exit(pf.err.Error()) return } cdLogger := mainLog.Load().With().Str("mode", "cd").Logger() // Performs self-uninstallation if the ControlD device does not exist. var uer *controld.ErrorResponse - if errors.As(err, &uer) && uer.ErrorField.Code == controld.InvalidConfigCode { + if errors.As(pf.err, &uer) && uer.ErrorField.Code == controld.InvalidConfigCode { _ = uninstallInvalidCdUID(p, cdLogger, false) } + if rejection, ok := permanentAPIRejection(pf.err); ok { + // The API answered and rejected this request permanently. Restarting + // cannot change that answer, so exit cleanly rather than through Fatal: + // an abnormal exit spends one of the service manager's restart actions, + // and on Windows those are what bring enforcement back after a real + // crash. Burning that budget on a config problem also buries the API's + // reason under repeated start failures. + cdLogger.Error().Err(pf.err).Int("status", rejection.StatusCode).Msg("failed to fetch resolver config, the API rejected this configuration") + notifyExitToLogServer() + return + } notifyExitToLogServer() - cdLogger.Fatal().Err(err).Msg("failed to fetch resolver config") - } else { + // Everything else - a denied socket, an unreachable API, a proxy in the way, + // an API that is having a bad day - is a condition a later start may not hit, + // so keep the abnormal exit and let the service manager's recovery policy + // retry. + cdLogger.Fatal().Err(pf.err).Msg("failed to fetch resolver config") + default: p.mu.Lock() - p.rc = rc + p.rc = pf.rc p.mu.Unlock() } } @@ -649,24 +682,148 @@ func deactivationPinSet() bool { return cdDeactivationPin.Load() != defaultDeactivationPin } -func processCDFlags(cfg *ctrld.Config) (*controld.ResolverConfig, error) { +// fetchResolverConfig is a test seam for the ControlD resolver-config API call. +var fetchResolverConfig = controld.FetchResolverConfig + +// apiPreflight is the outcome of the API preflight fetch: the resolver config, the +// error if any, and whether the service was asked to stop while it ran. +type apiPreflight struct { + rc *controld.ResolverConfig + err error + stopRequested bool +} + +// runAPIPreflight fetches the ControlD resolver config bounded by the service +// lifetime, and reports whether a stop was requested while it ran. +// +// The distinction matters because the caller does very different things with it: a stop +// exits quietly, while a failure self-uninstalls a deleted device, surfaces the error to +// a mobile app, and reports a failed start to the service manager. +// +// stopRequested must not be derived from the context once it has been cancelled. +// context.CancelFunc sets ctx.Err() unconditionally, so reading it after the cancel +// classifies *every* failure - a deleted device, an exhausted retry, a mobile caller +// with no stop channel - as an operator stop. Reading the stop channel directly is also +// independent of whether the context's watcher goroutine has been scheduled yet. +func runAPIPreflight(stopCh <-chan struct{}, cfg *ctrld.Config) apiPreflight { + rc, err := fetchCDConfigBoundedBy(stopCh, cfg) + return apiPreflight{rc: rc, err: err, stopRequested: stopRequested(stopCh)} +} + +// permanentAPIRejection reports whether err is the API refusing this request in a way +// that a restart cannot change, and returns the rejection when it is. +// +// The type alone does not answer this. controld builds an *ErrorResponse for *any* +// non-200 whose body decodes, so a 502 from a load balancer and a 404 for a deleted +// device arrive as the same Go type. Treating both as permanent would let a few minutes +// of API trouble stop ctrld on every host with no service-manager retry behind it, which +// is strictly worse than the abnormal exit it replaced. +// +// So the HTTP status decides, and only a client-error status counts: +// +// - 4xx: the API examined this request and refused it - a deleted device, a revoked +// token, a malformed UID. The same request will be refused again. +// - 408 and 429 are the exceptions: they are the API asking for another attempt later. +// - 5xx, or no recorded status, says nothing about this configuration. Retry. +func permanentAPIRejection(err error) (*controld.ErrorResponse, bool) { + var uer *controld.ErrorResponse + if !errors.As(err, &uer) { + return nil, false + } + switch uer.StatusCode { + case http.StatusRequestTimeout, http.StatusTooManyRequests: + return nil, false + } + if uer.StatusCode < 400 || uer.StatusCode >= 500 { + return nil, false + } + return uer, true +} + +// processCDFlagsFn is the API fetch, indirected so the lifetime binding around it can be +// tested without reaching the network. +var processCDFlagsFn = processCDFlags + +// fetchCDConfigBoundedBy runs the API fetch bounded by stopCh, so a fetch that cannot +// reach the API stops when the service is asked to stop instead of working on behalf of a +// service the OS already considers stopped. The derived context is always cancelled, which +// releases the goroutine watching stopCh. +func fetchCDConfigBoundedBy(stopCh <-chan struct{}, cfg *ctrld.Config) (*controld.ResolverConfig, error) { + ctx, cancel := contextFromStopCh(stopCh) + defer cancel() + return processCDFlagsFn(ctx, cfg) +} + +// fetchCDConfigBoundedByLifetime is the reload path's fetch. Reload binds the same stop +// primitives as startup - it used to wire them up itself, where a dropped cancel or the +// wrong channel would have failed nothing. +func (p *prog) fetchCDConfigBoundedByLifetime(cfg *ctrld.Config) (*controld.ResolverConfig, error) { + return fetchCDConfigBoundedBy(p.stopCh, cfg) +} + +// stopRequested reports whether stopCh has been closed. A nil channel - mobile passes +// none - blocks forever, so the default case is taken and it reads as "no stop". +func stopRequested(stopCh <-chan struct{}) bool { + select { + case <-stopCh: + return true + default: + return false + } +} + +// contextFromStopCh returns a context that is cancelled when stopCh closes, so +// long-running startup work stops as soon as the service is asked to stop. The +// returned cancel func must be called to release the watcher goroutine. +func contextFromStopCh(stopCh <-chan struct{}) (context.Context, context.CancelFunc) { + ctx, cancel := context.WithCancel(context.Background()) + if stopCh == nil { + return ctx, cancel + } + go func() { + select { + case <-stopCh: + cancel() + case <-ctx.Done(): + } + }() + return ctx, cancel +} + +// processCDFlags fetches the ControlD configuration for cdUID and applies it to cfg. +// +// ctx bounds the bootstrap-DNS retry loop below. That loop retries indefinitely by +// design (a device with no network yet must eventually come up), so it must be +// cancellable: otherwise a stop request during preflight is ignored and the process +// keeps retrying after the service reports itself stopped. +func processCDFlags(ctx context.Context, cfg *ctrld.Config) (*controld.ResolverConfig, error) { logger := mainLog.Load().With().Str("mode", "cd").Logger() logger.Info().Msgf("fetching Controld D configuration from API: %s", cdUID) bo := backoff.NewBackoff("processCDFlags", logf, 30*time.Second) bo.LogLongerThan = 30 * time.Second - ctx := context.Background() + if ctx == nil { + ctx = context.Background() + } req := &controld.ResolverConfigRequest{ RawUID: cdUID, Version: rootCmd.Version, - Metadata: ctrld.SystemMetadataRuntime(context.Background()), + Metadata: ctrld.SystemMetadataRuntime(ctx), } - resolverConfig, err := controld.FetchResolverConfig(req, cdDev) + resolverConfig, err := fetchResolverConfig(ctx, req, cdDev) for { + if ctxErr := ctx.Err(); ctxErr != nil { + logger.Debug().Msg("resolver config fetch cancelled") + return nil, ctxErr + } if errUrlNetworkError(err) { bo.BackOff(ctx, err) + if ctxErr := ctx.Err(); ctxErr != nil { + logger.Debug().Msg("resolver config fetch cancelled during backoff") + return nil, ctxErr + } logger.Warn().Msg("could not fetch resolver using bootstrap DNS, retrying...") - resolverConfig, err = controld.FetchResolverConfig(req, cdDev) + resolverConfig, err = fetchResolverConfig(ctx, req, cdDev) continue } break @@ -1644,7 +1801,7 @@ func cdUIDFromProvToken() string { Metadata: ctrld.SystemMetadata(context.Background()), } // Process provision token if provided. - resolverConfig, err := controld.FetchResolverUID(req, rootCmd.Version, cdDev) + resolverConfig, err := controld.FetchResolverUID(context.Background(), req, rootCmd.Version, cdDev) if err != nil { mainLog.Load().Fatal().Err(err).Msgf("failed to fetch resolver uid with provision token: %s", redactToken(cdOrg)) } @@ -1998,7 +2155,7 @@ func doValidateCdRemoteConfig(cdUID string, fatal bool) error { Version: rootCmd.Version, Metadata: ctrld.SystemMetadataRuntime(context.Background()), } - rc, err := controld.FetchResolverConfig(req, cdDev) + rc, err := controld.FetchResolverConfig(context.Background(), req, cdDev) if err != nil { logger := mainLog.Load().Fatal() if !fatal { diff --git a/cmd/cli/cli_preflight_test.go b/cmd/cli/cli_preflight_test.go new file mode 100644 index 0000000..2f1847b --- /dev/null +++ b/cmd/cli/cli_preflight_test.go @@ -0,0 +1,403 @@ +package cli + +import ( + "context" + "errors" + "fmt" + "net" + "net/http" + "net/url" + "sync/atomic" + "syscall" + "testing" + "time" + + "github.com/Control-D-Inc/ctrld" + "github.com/Control-D-Inc/ctrld/internal/controld" +) + +func TestContextFromStopCh(t *testing.T) { + t.Run("cancels when stopCh closes", func(t *testing.T) { + stopCh := make(chan struct{}) + ctx, cancel := contextFromStopCh(stopCh) + defer cancel() + + if ctx.Err() != nil { + t.Fatalf("context cancelled before the stop request: %v", ctx.Err()) + } + close(stopCh) + select { + case <-ctx.Done(): + case <-time.After(5 * time.Second): + t.Fatal("context was not cancelled after stopCh closed") + } + if !errors.Is(ctx.Err(), context.Canceled) { + t.Errorf("ctx.Err() = %v, want %v", ctx.Err(), context.Canceled) + } + }) + + t.Run("cancel releases the watcher", func(t *testing.T) { + // stopCh is never closed: cancel() must still end the goroutine watching it. + ctx, cancel := contextFromStopCh(make(chan struct{})) + cancel() + select { + case <-ctx.Done(): + case <-time.After(5 * time.Second): + t.Fatal("context was not cancelled by cancel()") + } + }) + + t.Run("nil stopCh is usable", func(t *testing.T) { + // Mobile callers have no stop channel; preflight must still run. + ctx, cancel := contextFromStopCh(nil) + defer cancel() + if ctx.Err() != nil { + t.Fatalf("context cancelled immediately: %v", ctx.Err()) + } + }) +} + +// retryableNetworkErr is the shape processCDFlags treats as "retry with bootstrap +// DNS": a url.Error wrapping a network failure. +func retryableNetworkErr() error { + return &url.Error{ + Op: "Post", + URL: "https://api.controld.com/utility", + Err: &net.OpError{Op: "dial", Net: "tcp", Err: syscall.ECONNREFUSED}, + } +} + +func TestProcessCDFlagsStopsWhenCancelled(t *testing.T) { + oldFetch := fetchResolverConfig + oldUID := cdUID + t.Cleanup(func() { + fetchResolverConfig = oldFetch + cdUID = oldUID + }) + cdUID = "testuid" + + var calls atomic.Int64 + fetchResolverConfig = func(ctx context.Context, req *controld.ResolverConfigRequest, dev bool) (*controld.ResolverConfig, error) { + calls.Add(1) + return nil, retryableNetworkErr() + } + + // A stop request arriving while the API is unreachable. Before this was + // cancellable, the retry loop kept running after the service reported itself + // stopped, which is what kept the incident's process alive and enforcing. + stopCh := make(chan struct{}) + ctx, cancel := contextFromStopCh(stopCh) + defer cancel() + + done := make(chan error, 1) + go func() { + cfg := ctrld.Config{} + _, err := processCDFlags(ctx, &cfg) + done <- err + }() + + // Let it fail at least once and settle into backoff before stopping. + deadline := time.After(10 * time.Second) + for calls.Load() == 0 { + select { + case <-deadline: + t.Fatal("resolver config was never fetched") + case err := <-done: + t.Fatalf("processCDFlags returned before any fetch: %v", err) + default: + time.Sleep(5 * time.Millisecond) + } + } + close(stopCh) + + select { + case err := <-done: + if !errors.Is(err, context.Canceled) { + t.Errorf("processCDFlags err = %v, want it to report %v", err, context.Canceled) + } + case <-time.After(30 * time.Second): + t.Fatal("processCDFlags did not return after the stop request") + } +} + +func TestProcessCDFlagsReturnsImmediatelyWhenAlreadyCancelled(t *testing.T) { + oldFetch := fetchResolverConfig + oldUID := cdUID + t.Cleanup(func() { + fetchResolverConfig = oldFetch + cdUID = oldUID + }) + cdUID = "testuid" + + var calls atomic.Int64 + fetchResolverConfig = func(ctx context.Context, req *controld.ResolverConfigRequest, dev bool) (*controld.ResolverConfig, error) { + calls.Add(1) + return nil, retryableNetworkErr() + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + cfg := ctrld.Config{} + _, err := processCDFlags(ctx, &cfg) + if !errors.Is(err, context.Canceled) { + t.Errorf("processCDFlags err = %v, want %v", err, context.Canceled) + } + // One attempt is made before the loop notices; it must not retry past that. + if got := calls.Load(); got > 1 { + t.Errorf("fetched %d times with a cancelled context, want at most 1", got) + } +} + +// TestRunAPIPreflightClassification is the regression guard for classifying a preflight +// failure as an operator stop. +// +// runAPIPreflight cancels the context it derived from stopCh. Sampling the stop state +// from that context afterwards reports "stopped" unconditionally, because +// context.CancelFunc sets ctx.Err() whether or not anyone asked to stop. run() then +// takes the stop branch for every failure, which skips self-uninstalling a deleted +// device, skips the mobile exit callback, and tells the service manager a failed start +// was a clean exit. +func TestRunAPIPreflightClassification(t *testing.T) { + oldFetch := fetchResolverConfig + oldUID := cdUID + t.Cleanup(func() { + fetchResolverConfig = oldFetch + cdUID = oldUID + }) + cdUID = "testuid" + + // A deleted ControlD device: non-retryable, so preflight returns promptly. + deletedDevice := func() error { + e := &controld.ErrorResponse{} + e.ErrorField.Code = controld.InvalidConfigCode + e.ErrorField.Message = "device does not exist" + return e + } + + openCh := make(chan struct{}) + closedCh := make(chan struct{}) + close(closedCh) + + tests := []struct { + name string + stopCh <-chan struct{} + fetchErr func() error + wantStop bool + }{ + { + // The P1: no stop was requested, so this must reach the failure branch. + name: "api error with no stop request", + stopCh: openCh, + fetchErr: deletedDevice, + }, + { + // Mobile passes no stop channel at all, so it could never have stopped. + name: "api error with a nil stop channel", + stopCh: nil, + fetchErr: deletedDevice, + }, + { + name: "stop requested during preflight", + stopCh: closedCh, + fetchErr: func() error { return retryableNetworkErr() }, + wantStop: true, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + fetchResolverConfig = func(context.Context, *controld.ResolverConfigRequest, bool) (*controld.ResolverConfig, error) { + return nil, tc.fetchErr() + } + cfg := ctrld.Config{} + pf := runAPIPreflight(tc.stopCh, &cfg) + + if pf.err == nil { + t.Fatal("expected preflight to fail") + } + if pf.stopRequested != tc.wantStop { + t.Errorf("stopRequested = %v, want %v", pf.stopRequested, tc.wantStop) + } + }) + } +} + +// TestRunAPIPreflightPreservesAPIError verifies the error reaches the caller in a form +// the failure branch can still act on: self-uninstall keys off an *ErrorResponse with +// InvalidConfigCode, and it only runs if that error is both classified as a failure and +// still unwrappable. +func TestRunAPIPreflightPreservesAPIError(t *testing.T) { + oldFetch := fetchResolverConfig + oldUID := cdUID + t.Cleanup(func() { + fetchResolverConfig = oldFetch + cdUID = oldUID + }) + cdUID = "testuid" + + want := &controld.ErrorResponse{} + want.ErrorField.Code = controld.InvalidConfigCode + fetchResolverConfig = func(context.Context, *controld.ResolverConfigRequest, bool) (*controld.ResolverConfig, error) { + return nil, want + } + + cfg := ctrld.Config{} + pf := runAPIPreflight(make(chan struct{}), &cfg) + + if pf.stopRequested { + t.Error("a device-deleted failure must not be reported as an operator stop") + } + var got *controld.ErrorResponse + if !errors.As(pf.err, &got) { + t.Fatalf("error no longer unwraps to *controld.ErrorResponse: %v", pf.err) + } + if got.ErrorField.Code != controld.InvalidConfigCode { + t.Errorf("code = %d, want %d (self-uninstall would not trigger)", got.ErrorField.Code, controld.InvalidConfigCode) + } +} + +// TestPermanentAPIRejectionNarrowsToClientErrors is the regression guard for the clean +// exit added above. +// +// controld builds an *ErrorResponse for any non-200 whose body decodes, so the Go type +// says nothing about whether the API's answer will change on a retry. Keying the clean +// exit off the type alone meant a 502 from a load balancer, or an API having a bad ten +// minutes, stopped ctrld on every affected host with no service-manager retry behind it - +// worse than the abnormal exit it replaced, because a Fatal at least gets restarted. +// +// Only a client-error status may take that path. +func TestPermanentAPIRejectionNarrowsToClientErrors(t *testing.T) { + rejection := func(status, code int) error { + e := &controld.ErrorResponse{StatusCode: status} + e.ErrorField.Code = code + e.ErrorField.Message = "api said no" + return e + } + + tests := []struct { + name string + err error + wantPermanent bool + }{ + { + // The case the clean exit exists for: the device is gone, and every restart + // will be told the same thing. + name: "deleted device", + err: rejection(http.StatusNotFound, controld.InvalidConfigCode), + wantPermanent: true, + }, + {"revoked credentials", rejection(http.StatusUnauthorized, 0), true}, + {"forbidden", rejection(http.StatusForbidden, 0), true}, + {"malformed request", rejection(http.StatusBadRequest, 0), true}, + + // Server-side trouble. These must keep the abnormal exit so the service + // manager's recovery policy retries. + {"bad gateway", rejection(http.StatusBadGateway, 0), false}, + {"internal error", rejection(http.StatusInternalServerError, 0), false}, + {"service unavailable", rejection(http.StatusServiceUnavailable, 0), false}, + + // 4xx, but both are the API asking for a later attempt rather than refusing + // this configuration. + {"request timeout", rejection(http.StatusRequestTimeout, 0), false}, + {"rate limited", rejection(http.StatusTooManyRequests, 0), false}, + + // An *ErrorResponse built without a recorded status carries no verdict. A + // hand-constructed one, or a decode path that forgets to record the status, + // must not silently gain the clean exit. + {"no recorded status", rejection(0, controld.InvalidConfigCode), false}, + + // Not an API answer at all: the incident's denied socket reaches Fatal. + {"network failure", retryableNetworkErr(), false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, ok := permanentAPIRejection(tc.err) + if ok != tc.wantPermanent { + t.Errorf("permanentAPIRejection() = %v, want %v", ok, tc.wantPermanent) + } + if ok && got == nil { + t.Error("a permanent rejection must return the rejection for reporting") + } + }) + } + + // The wrapped form matters too: preflight composes the fetch error, and errors.As has + // to reach through that for either branch to be chosen correctly. + wrapped := fmt.Errorf("processCDFlags: %w", rejection(http.StatusNotFound, controld.InvalidConfigCode)) + if _, ok := permanentAPIRejection(wrapped); !ok { + t.Error("a wrapped API rejection must still be recognised") + } + wrappedTransient := fmt.Errorf("processCDFlags: %w", rejection(http.StatusBadGateway, 0)) + if _, ok := permanentAPIRejection(wrappedTransient); ok { + t.Error("a wrapped 502 must not be treated as a permanent rejection") + } +} + +func TestStopRequested(t *testing.T) { + closedCh := make(chan struct{}) + close(closedCh) + + if stopRequested(nil) { + t.Error("a nil stop channel must read as no stop (mobile passes none)") + } + if stopRequested(make(chan struct{})) { + t.Error("an open stop channel must read as no stop") + } + if !stopRequested(closedCh) { + t.Error("a closed stop channel must read as a stop") + } +} + +// TestReloadFetchIsBoundedByServiceLifetime covers the reload path's stop wiring. +// +// Reload fetches the ControlD config too, and it used to build the bounded context +// itself. Nothing tested that: the wrong channel, or a dropped cancel, would have left a +// reload retrying against an unreachable API after "service stopped" was logged, and no +// test would have failed. Both paths now go through one bounded fetch, so this pins it. +func TestReloadFetchIsBoundedByServiceLifetime(t *testing.T) { + original := processCDFlagsFn + t.Cleanup(func() { processCDFlagsFn = original }) + + t.Run("a stop request cancels the reload fetch", func(t *testing.T) { + stopCh := make(chan struct{}) + close(stopCh) + + var sawCancelled bool + processCDFlagsFn = func(ctx context.Context, _ *ctrld.Config) (*controld.ResolverConfig, error) { + select { + case <-ctx.Done(): + sawCancelled = true + case <-time.After(2 * time.Second): + } + return nil, ctx.Err() + } + + p := &prog{stopCh: stopCh} + if _, err := p.fetchCDConfigBoundedByLifetime(&ctrld.Config{}); !errors.Is(err, context.Canceled) { + t.Errorf("reload fetch err = %v, want %v", err, context.Canceled) + } + if !sawCancelled { + t.Error("the reload fetch did not observe the stop request: it is not bound to the service lifetime") + } + }) + + t.Run("the derived context is always released", func(t *testing.T) { + // stopCh stays open: the fetch's own cancel is what must end the watcher, or + // every reload leaks a goroutine. + var captured context.Context + processCDFlagsFn = func(ctx context.Context, _ *ctrld.Config) (*controld.ResolverConfig, error) { + captured = ctx + return nil, nil + } + + p := &prog{stopCh: make(chan struct{})} + if _, err := p.fetchCDConfigBoundedByLifetime(&ctrld.Config{}); err != nil { + t.Fatalf("unexpected error: %v", err) + } + select { + case <-captured.Done(): + case <-time.After(time.Second): + t.Error("the reload fetch left its context uncancelled") + } + }) +} diff --git a/cmd/cli/control_server.go b/cmd/cli/control_server.go index 976569b..5606521 100644 --- a/cmd/cli/control_server.go +++ b/cmd/cli/control_server.go @@ -237,7 +237,7 @@ func (p *prog) registerControlServerHandler() { Version: rootCmd.Version, Metadata: ctrld.SystemMetadataRuntime(context.Background()), } - if rc, err := controld.FetchResolverConfig(rcReq, cdDev); rc != nil { + if rc, err := controld.FetchResolverConfig(context.Background(), rcReq, cdDev); rc != nil { if rc.DeactivationPin != nil { cdDeactivationPin.Store(*rc.DeactivationPin) } else { @@ -351,7 +351,7 @@ func (p *prog) registerControlServerHandler() { } mainLog.Load().Debug().Msg("sending log file to ControlD server") resp := logSentResponse{Size: r.size} - if err := controld.SendLogs(req, cdDev); err != nil { + if err := controld.SendLogs(context.Background(), req, cdDev); err != nil { mainLog.Load().Error().Msgf("could not send log file to ControlD server: %v", err) resp.Error = err.Error() w.WriteHeader(http.StatusInternalServerError) diff --git a/cmd/cli/dns_proxy.go b/cmd/cli/dns_proxy.go index b34013c..b7d2038 100644 --- a/cmd/cli/dns_proxy.go +++ b/cmd/cli/dns_proxy.go @@ -1190,7 +1190,7 @@ func (p *prog) doSelfUninstall(answer *dns.Msg) { Version: rootCmd.Version, Metadata: ctrld.SystemMetadataRuntime(context.Background()), } - _, err := controld.FetchResolverConfig(req, cdDev) + _, err := controld.FetchResolverConfig(context.Background(), req, cdDev) logger.Debug().Msg("maximum number of refused queries reached, checking device status") selfUninstallCheck(err, p, logger) diff --git a/cmd/cli/prog.go b/cmd/cli/prog.go index 6f13349..acb12e1 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -324,7 +324,7 @@ func (p *prog) runWait() { continue } if cdUID != "" { - rc, err := processCDFlags(newCfg) + rc, err := p.fetchCDConfigBoundedByLifetime(newCfg) if err != nil { logger.Err(err).Msg("could not fetch ControlD config") waitOldRunDone() @@ -491,7 +491,7 @@ func (p *prog) apiConfigReload() { Version: rootCmd.Version, Metadata: ctrld.SystemMetadataRuntime(context.Background()), } - resolverConfig, err := controld.FetchResolverConfig(req, cdDev) + resolverConfig, err := controld.FetchResolverConfig(context.Background(), req, cdDev) selfUninstallCheck(err, p, logger) if err != nil { logger.Warn().Err(err).Msg("could not fetch resolver config") @@ -549,7 +549,7 @@ func (p *prog) apiConfigReload() { } if cfgErr != nil { logger.Warn().Err(err).Msg("skipping invalid custom config") - if _, err := controld.UpdateCustomLastFailed(cdUID, rootCmd.Version, cdDev, true); err != nil { + if _, err := controld.UpdateCustomLastFailed(context.Background(), cdUID, rootCmd.Version, cdDev, true); err != nil { logger.Error().Err(err).Msg("could not mark custom last update failed") } return diff --git a/internal/controld/config.go b/internal/controld/config.go index 181358c..05edc68 100644 --- a/internal/controld/config.go +++ b/internal/controld/config.go @@ -63,12 +63,35 @@ type ErrorResponse struct { Message string `json:"message"` Code int `json:"code"` } `json:"error"` + // StatusCode is the HTTP status the API answered with. It is not part of the JSON + // body: this type is built for *any* non-200 whose body decodes, so the body alone + // cannot tell a permanent rejection of the request from a transient server-side + // failure, and callers that act differently on the two need the status to tell them + // apart. Zero means the status was not recorded. + StatusCode int `json:"-"` } func (u ErrorResponse) Error() string { return u.ErrorField.Message } +// apiErrorFromResponse builds the error for a non-200 API answer, recording the HTTP +// status alongside the decoded body. +// +// The status is what tells a caller whether the answer will change on a retry: this type +// is built for every non-200 whose body decodes, so a 502 from a load balancer and a 404 +// for a deleted device are otherwise indistinguishable. Both response paths go through +// here so neither can decode a body and forget to record it. +func apiErrorFromResponse(statusCode int, d *json.Decoder) (*ErrorResponse, error) { + errResp := &ErrorResponse{StatusCode: statusCode} + if err := d.Decode(errResp); err != nil { + return nil, err + } + // Decode fills exported fields from the body; StatusCode is json:"-", so it survives. + errResp.StatusCode = statusCode + return errResp, nil +} + type utilityRequest struct { UID string `json:"uid"` ClientID string `json:"client_id,omitempty"` @@ -96,7 +119,7 @@ type LogsRequest struct { } // FetchResolverConfig fetch Control D config for given uid. -func FetchResolverConfig(req *ResolverConfigRequest, cdDev bool) (*ResolverConfig, error) { +func FetchResolverConfig(ctx context.Context, req *ResolverConfigRequest, cdDev bool) (*ResolverConfig, error) { uid, clientID := ParseRawUID(req.RawUID) uReq := utilityRequest{ UID: uid, @@ -106,11 +129,11 @@ func FetchResolverConfig(req *ResolverConfigRequest, cdDev bool) (*ResolverConfi uReq.ClientID = clientID } body, _ := json.Marshal(uReq) - return postUtilityAPI(req.Version, cdDev, false, bytes.NewReader(body)) + return postUtilityAPI(ctx, req.Version, cdDev, false, bytes.NewReader(body)) } // FetchResolverUID fetch resolver uid from a given request. -func FetchResolverUID(req *UtilityOrgRequest, version string, cdDev bool) (*ResolverConfig, error) { +func FetchResolverUID(ctx context.Context, req *UtilityOrgRequest, version string, cdDev bool) (*ResolverConfig, error) { if req == nil { return nil, errors.New("invalid request") } @@ -131,26 +154,29 @@ func FetchResolverUID(req *UtilityOrgRequest, version string, cdDev bool) (*Reso ctrld.ProxyLogger.Load().Debug().Msgf("Sending UID request to ControlD API") body, _ := json.Marshal(req) - return postUtilityAPI(version, cdDev, false, bytes.NewReader(body)) + return postUtilityAPI(ctx, version, cdDev, false, bytes.NewReader(body)) } // UpdateCustomLastFailed calls API to mark custom config is bad. -func UpdateCustomLastFailed(rawUID, version string, cdDev, lastUpdatedFailed bool) (*ResolverConfig, error) { +func UpdateCustomLastFailed(ctx context.Context, rawUID, version string, cdDev, lastUpdatedFailed bool) (*ResolverConfig, error) { uid, clientID := ParseRawUID(rawUID) req := utilityRequest{UID: uid} if clientID != "" { req.ClientID = clientID } body, _ := json.Marshal(req) - return postUtilityAPI(version, cdDev, true, bytes.NewReader(body)) + return postUtilityAPI(ctx, version, cdDev, true, bytes.NewReader(body)) } -func postUtilityAPI(version string, cdDev, lastUpdatedFailed bool, body io.Reader) (*ResolverConfig, error) { +func postUtilityAPI(ctx context.Context, version string, cdDev, lastUpdatedFailed bool, body io.Reader) (*ResolverConfig, error) { apiUrl := resolverDataURLCom if cdDev { apiUrl = resolverDataURLDev } - req, err := http.NewRequest("POST", apiUrl, body) + // Context-bound so an in-flight request is abandoned when the caller is + // cancelled - a service stop during API preflight must not wait out the + // request timeout, let alone keep retrying. + req, err := http.NewRequestWithContext(ctx, "POST", apiUrl, body) if err != nil { return nil, fmt.Errorf("http.NewRequest: %w", err) } @@ -174,8 +200,8 @@ func postUtilityAPI(version string, cdDev, lastUpdatedFailed bool, body io.Reade defer resp.Body.Close() d := json.NewDecoder(resp.Body) if resp.StatusCode != http.StatusOK { - errResp := &ErrorResponse{} - if err := d.Decode(errResp); err != nil { + errResp, err := apiErrorFromResponse(resp.StatusCode, d) + if err != nil { return nil, err } return nil, errResp @@ -189,13 +215,13 @@ func postUtilityAPI(version string, cdDev, lastUpdatedFailed bool, body io.Reade } // SendLogs sends runtime log to ControlD API. -func SendLogs(lr *LogsRequest, cdDev bool) error { +func SendLogs(ctx context.Context, lr *LogsRequest, cdDev bool) error { defer lr.Data.Close() apiUrl := logURLCom if cdDev { apiUrl = logURLDev } - req, err := http.NewRequest("POST", apiUrl, lr.Data) + req, err := http.NewRequestWithContext(ctx, "POST", apiUrl, lr.Data) if err != nil { return fmt.Errorf("http.NewRequest: %w", err) } @@ -215,8 +241,8 @@ func SendLogs(lr *LogsRequest, cdDev bool) error { defer resp.Body.Close() d := json.NewDecoder(resp.Body) if resp.StatusCode != http.StatusOK { - errResp := &ErrorResponse{} - if err := d.Decode(errResp); err != nil { + errResp, err := apiErrorFromResponse(resp.StatusCode, d) + if err != nil { return err } return errResp diff --git a/internal/controld/config_test.go b/internal/controld/config_test.go index b266142..5973de0 100644 --- a/internal/controld/config_test.go +++ b/internal/controld/config_test.go @@ -1,6 +1,9 @@ package controld import ( + "encoding/json" + "net/http" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -29,3 +32,67 @@ func Test_parseUID(t *testing.T) { }) } } + +// TestAPIErrorRecordsHTTPStatus pins the plumbing the caller's exit decision rests on. +// +// cmd/cli treats a 4xx as "this configuration is refused, restarting cannot help" and +// exits cleanly, while a 5xx keeps the abnormal exit so the service manager retries. Both +// readings need the status, and it is not in the JSON body - so a decode path that +// forgets to record it would quietly send every API error down the retry branch, +// including a deleted device that should self-uninstall and stop. +func TestAPIErrorRecordsHTTPStatus(t *testing.T) { + tests := []struct { + name string + statusCode int + body string + wantCode int + wantMsg string + }{ + { + name: "deleted device", + statusCode: http.StatusNotFound, + body: `{"error":{"message":"device does not exist","code":40402}}`, + wantCode: InvalidConfigCode, + wantMsg: "device does not exist", + }, + { + // A gateway error body carries no error object at all, which decodes + // cleanly into the zero value - so the status is the only thing that + // distinguishes it from a real rejection. + name: "gateway error with an empty body", + statusCode: http.StatusBadGateway, + body: `{}`, + }, + { + name: "service unavailable", + statusCode: http.StatusServiceUnavailable, + body: `{"error":{"message":"try again later","code":0}}`, + wantMsg: "try again later", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + d := json.NewDecoder(strings.NewReader(tc.body)) + errResp, err := apiErrorFromResponse(tc.statusCode, d) + if err != nil { + t.Fatalf("unexpected decode error: %v", err) + } + if errResp.StatusCode != tc.statusCode { + t.Errorf("StatusCode = %d, want %d: the caller cannot tell a permanent rejection from a transient failure without it", errResp.StatusCode, tc.statusCode) + } + if errResp.ErrorField.Code != tc.wantCode { + t.Errorf("code = %d, want %d", errResp.ErrorField.Code, tc.wantCode) + } + if errResp.Error() != tc.wantMsg { + t.Errorf("message = %q, want %q", errResp.Error(), tc.wantMsg) + } + }) + } + + t.Run("an undecodable body is reported as a decode failure", func(t *testing.T) { + d := json.NewDecoder(strings.NewReader("502 Bad Gateway")) + if _, err := apiErrorFromResponse(http.StatusBadGateway, d); err == nil { + t.Error("expected a decode error for a non-JSON body") + } + }) +}