diff --git a/internal/dnsprovider/providers.go b/internal/dnsprovider/providers.go index 57b9b2ad..47f78ca4 100644 --- a/internal/dnsprovider/providers.go +++ b/internal/dnsprovider/providers.go @@ -467,13 +467,54 @@ func jsonHTTPProviderRequest(ctx context.Context, method, endpoint, authorizatio return request, nil } +// APIError is a provider API response outside the 2xx range. +type APIError struct { + Provider string + StatusCode int + Status string + Body string + // RetryAfter is the wait the provider asked for in a Retry-After header, + // or zero when it named none. + RetryAfter time.Duration +} + +func (apiError *APIError) Error() string { + return fmt.Sprintf("%s API returned %s: %s", apiError.Provider, apiError.Status, apiError.Body) +} + +// Temporary reports whether the provider is rate limiting or briefly failing +// on its own side, so the same request is worth sending again later. +func (apiError *APIError) Temporary() bool { + return apiError.StatusCode == http.StatusTooManyRequests || apiError.StatusCode >= http.StatusInternalServerError +} + func responseError(provider string, response *http.Response) error { contents, _ := io.ReadAll(io.LimitReader(response.Body, 4096)) - message := strings.TrimSpace(string(contents)) + // One line per body keeps a joined error to one line per failed record. + message := strings.Join(strings.Fields(string(contents)), " ") if message == "" { message = response.Status } - return fmt.Errorf("%s API returned %s: %s", provider, response.Status, message) + return &APIError{ + Provider: provider, StatusCode: response.StatusCode, Status: response.Status, + Body: message, RetryAfter: retryAfter(response.Header.Get("Retry-After"), time.Now()), + } +} + +// retryAfter reads a Retry-After header in either of its forms: a number of +// seconds or an HTTP date. +func retryAfter(value string, now time.Time) time.Duration { + value = strings.TrimSpace(value) + if value == "" { + return 0 + } + if seconds, err := strconv.Atoi(value); err == nil { + return max(time.Duration(seconds)*time.Second, 0) + } + if at, err := http.ParseTime(value); err == nil { + return max(at.Sub(now), 0) + } + return 0 } func relativeName(name, zone string) string { diff --git a/internal/dnsprovider/providers_test.go b/internal/dnsprovider/providers_test.go index cdc585a0..8f87ec06 100644 --- a/internal/dnsprovider/providers_test.go +++ b/internal/dnsprovider/providers_test.go @@ -503,3 +503,53 @@ func TestNamecheapFailuresDoNotNameTheAPIKey(t *testing.T) { t.Fatalf("error = %v", err) } } + +func TestProviderAPIErrorsSayWhetherToRetry(t *testing.T) { + for _, test := range []struct { + status int + retryAfter string + temporary bool + wait time.Duration + }{ + {status: http.StatusBadGateway, retryAfter: "60", temporary: true, wait: time.Minute}, + {status: http.StatusTooManyRequests, temporary: true}, + {status: http.StatusBadRequest}, + } { + client := &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + response := httpResponse(test.status, `{"title":"failure"}`) + if test.retryAfter != "" { + response.Header.Set("Retry-After", test.retryAfter) + } + return response, nil + })} + provider := &cloudflareProvider{ + credentials: Credentials{APIToken: "token", ZoneID: "zone-1"}, client: client, baseURL: "https://cloudflare.test/client/v4", + } + _, err := provider.EnsureRecord(context.Background(), Record{ + Zone: "example.com", Name: "home.example.com", Type: TypeA, Value: "198.51.100.1", TTL: 300, + }) + var apiError *APIError + if !errors.As(err, &apiError) { + t.Fatalf("status %d: error %v is not an APIError", test.status, err) + } + if apiError.StatusCode != test.status || apiError.Temporary() != test.temporary || apiError.RetryAfter != test.wait { + t.Errorf("status %d: APIError = %+v, temporary %v", test.status, apiError, apiError.Temporary()) + } + } +} + +func TestRetryAfterReadsSecondsAndDates(t *testing.T) { + now := time.Date(2026, time.October, 7, 14, 46, 37, 0, time.UTC) + for value, expected := range map[string]time.Duration{ + "": 0, + "120": 2 * time.Minute, + "-5": 0, + "soon": 0, + "Wed, 07 Oct 2026 14:47:37 GMT": time.Minute, + "Wed, 07 Oct 2026 14:40:00 GMT": 0, + } { + if actual := retryAfter(value, now); actual != expected { + t.Errorf("retryAfter(%q) = %s, want %s", value, actual, expected) + } + } +} diff --git a/internal/dynamicdns/manager.go b/internal/dynamicdns/manager.go index ee9489c0..fc439b6f 100644 --- a/internal/dynamicdns/manager.go +++ b/internal/dynamicdns/manager.go @@ -295,6 +295,9 @@ type reconcileResult struct { ipv6 string changed int unchanged int + // retryAfter is the longest wait a provider asked for before the next + // attempt. + retryAfter time.Duration } func (manager *Manager) reconcile(ctx context.Context, settings config.DynamicDNS) (reconcileResult, error) { @@ -323,26 +326,41 @@ func (manager *Manager) reconcile(ctx context.Context, settings config.DynamicDN publicationErrors = append(publicationErrors, fmt.Errorf("initialize %s provider: %w", configuredPublisher.Provider, providerErr)) continue } - for _, configured := range configuredPublisher.Records { - for _, recordType := range recordTypes(configured) { - address := addresses[recordType] - changed, ensureErr := publisher.EnsureRecord(ctx, dnsprovider.Record{ - Zone: configured.Zone, Name: configured.Name, Type: recordType, - Value: address.String(), TTL: configured.TTL, - }) - if ensureErr != nil { - publicationErrors = append(publicationErrors, fmt.Errorf("%s: publish %s %s: %w", configuredPublisher.Provider, configured.Name, recordType, ensureErr)) - continue - } + if err := publishRecords(ctx, configuredPublisher, publisher, addresses, &result); err != nil { + publicationErrors = append(publicationErrors, err) + } + } + return result, errors.Join(publicationErrors...) +} + +// publishRecords ensures every record of one publisher. A provider that is rate +// limiting or failing on its own side gets no more requests this attempt: +// the rest would fail the same way, and the retry backoff covers them all. +func publishRecords(ctx context.Context, configured config.DynamicDNSPublisher, publisher provider, addresses map[string]netip.Addr, result *reconcileResult) error { + var publicationErrors []error + for _, record := range configured.Records { + for _, recordType := range recordTypes(record) { + changed, err := publisher.EnsureRecord(ctx, dnsprovider.Record{ + Zone: record.Zone, Name: record.Name, Type: recordType, + Value: addresses[recordType].String(), TTL: record.TTL, + }) + if err == nil { if changed { result.changed++ } else { result.unchanged++ } + continue + } + publicationErrors = append(publicationErrors, fmt.Errorf("%s: publish %s %s: %w", configured.Provider, record.Name, recordType, err)) + var apiError *dnsprovider.APIError + if errors.As(err, &apiError) && apiError.Temporary() { + result.retryAfter = max(result.retryAfter, apiError.RetryAfter) + return errors.Join(publicationErrors...) } } } - return result, errors.Join(publicationErrors...) + return errors.Join(publicationErrors...) } func (manager *Manager) discoverAddresses(ctx context.Context, settings config.DynamicDNS) (reconcileResult, map[string]netip.Addr, error) { @@ -494,7 +512,11 @@ func (manager *Manager) finishAttempt(started time.Time, interval time.Duration, } manager.status.ConsecutiveFailures++ manager.status.LastError = err.Error() - manager.status.NextAttempt = started.Add(retryDelay(manager.status.ConsecutiveFailures, interval)) + delay := retryDelay(manager.status.ConsecutiveFailures, interval) + if result.retryAfter > delay { + delay = min(result.retryAfter, maximumRetry) + } + manager.status.NextAttempt = started.Add(delay) } func retryDelay(failures int, interval time.Duration) time.Duration { diff --git a/internal/dynamicdns/manager_test.go b/internal/dynamicdns/manager_test.go index 53d948f0..60740fbf 100644 --- a/internal/dynamicdns/manager_test.go +++ b/internal/dynamicdns/manager_test.go @@ -462,3 +462,54 @@ func testDynamicDNSSettings() config.DynamicDNS { }}, } } + +func TestReconcileStopsCallingAProviderThatIsFailingOnItsSide(t *testing.T) { + t.Parallel() + for _, test := range []struct { + name string + err *dnsprovider.APIError + attempts int + wantRetry time.Duration + }{ + {name: "a 502 stops after the first record", err: &dnsprovider.APIError{StatusCode: 502}, attempts: 1, wantRetry: 30 * time.Second}, + {name: "a 429 waits as long as the provider asks", err: &dnsprovider.APIError{StatusCode: 429, RetryAfter: 2 * time.Minute}, attempts: 1, wantRetry: 2 * time.Minute}, + {name: "a 400 still tries every record", err: &dnsprovider.APIError{StatusCode: 400, RetryAfter: time.Hour}, attempts: 3, wantRetry: 30 * time.Second}, + } { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + settings := config.DynamicDNS{ + Enabled: true, Interval: config.Duration{Duration: 5 * time.Minute}, + IPv4URL: "https://ipv4.test", IPv6URL: "https://ipv6.test", + Publishers: []config.DynamicDNSPublisher{ + {Provider: "cloudflare", Records: []config.DynamicDNSRecord{ + {Zone: "example.com", Name: "home.example.com", IPv4: true, IPv6: true, TTL: 300}, + {Zone: "example.com", Name: "vpn.example.com", IPv4: true, TTL: 300}, + }}, + {Provider: "route53", Records: []config.DynamicDNSRecord{{Zone: "example.net", Name: "edge.example.net", IPv4: true, TTL: 300}}}, + }, + } + providers := map[string]*testProvider{"cloudflare": {err: test.err}, "route53": {changed: true}} + manager := newTestManager(settings, providers["cloudflare"]) + manager.newProvider = func(name string, _ dnsprovider.Credentials) (provider, error) { return providers[name], nil } + manager.discover = func(_ context.Context, _ string, recordType string) (netip.Addr, error) { + if recordType == dnsprovider.TypeA { + return netip.MustParseAddr("8.8.8.8"), nil + } + return netip.MustParseAddr("2001:4860:4860::8888"), nil + } + + manager.runOnce(context.Background()) + + if attempts := len(providers["cloudflare"].records); attempts != test.attempts { + t.Fatalf("Cloudflare requests = %d, want %d", attempts, test.attempts) + } + if len(providers["route53"].records) != 1 { + t.Fatalf("Route 53 requests = %d, want 1", len(providers["route53"].records)) + } + status := manager.Status(context.Background()) + if retry := status.NextAttempt.Sub(status.LastAttempt); retry != test.wantRetry { + t.Fatalf("next attempt in %s, want %s", retry, test.wantRetry) + } + }) + } +} diff --git a/internal/web/assets/app.css b/internal/web/assets/app.css index 7ba03e98..8ab8bd96 100644 --- a/internal/web/assets/app.css +++ b/internal/web/assets/app.css @@ -3293,9 +3293,10 @@ button.top-stats-row { width: 100%; border-width: 0 0 1px; background: none; col .unifi-status.warning { border-color: color-mix(in srgb, var(--warning-strong) 45%, var(--border)); background: color-mix(in srgb, var(--warning-strong) 8%, transparent); } .unifi-status.warning > .nav-icon { color: var(--warning-strong); } .unifi-status > .nav-icon { width: 1rem; height: 1rem; flex: 0 0 1rem; margin-top: .1rem; } -.unifi-status > div { display: grid; gap: .2rem; } +.unifi-status > div { display: grid; min-width: 0; gap: .2rem; } .unifi-status strong { font-size: .75rem; } .unifi-status span { color: var(--muted-foreground); font-size: .7rem; line-height: 1.45; } +.integration-error-summary { white-space: pre-line; overflow-wrap: anywhere; } .integration-error-details { min-width: 0; margin-top: .15rem; color: var(--muted-foreground); font-size: .68rem; } .integration-error-details summary { width: max-content; cursor: pointer; color: var(--foreground); } .integration-error-details pre { max-width: 100%; max-height: 12rem; overflow: auto; margin: var(--space-2) 0 0; border: 1px solid var(--border); border-radius: .4rem; background: var(--background); padding: .65rem; color: var(--muted-foreground); font-size: .65rem; line-height: 1.45; white-space: pre-wrap; overflow-wrap: anywhere; } diff --git a/internal/web/dynamic_dns.go b/internal/web/dynamic_dns.go index b87a3894..d56ddb3b 100644 --- a/internal/web/dynamic_dns.go +++ b/internal/web/dynamic_dns.go @@ -1,12 +1,14 @@ package web import ( + "cmp" "context" "encoding/json" "errors" "fmt" "net/http" "regexp" + "slices" "sort" "strconv" "strings" @@ -125,42 +127,137 @@ func dynamicDNSStatusView(provider string, status dynamicdns.Status, display pag var providerSecretPattern = regexp.MustCompile(`(?i)((?:authorization|api[-_ ]?key|access[-_ ]?key|token|secret|password)\s*[:=]\s*)(?:bearer\s+)?[^\s,;]+`) +var ( + dynamicDNSPublishFailure = regexp.MustCompile(`^(?:([a-z0-9]+): )?publish (\S+) (A|AAAA): (.*)$`) + providerHTTPStatus = regexp.MustCompile(`API returned ((\d{3})[^:]*): (.*)$`) +) + +// maximumListedRecords is how many records one error line names before it +// counts the rest. +const maximumListedRecords = 3 + +// dynamicDNSErrorDisplay turns the publisher's last error into a readable +// summary and, when a provider sent a JSON body, that body with its secrets +// redacted. Each failed record is one line of the error, and records that +// failed the same way share one summary line. func dynamicDNSErrorDisplay(provider, raw string) (string, string) { raw = strings.TrimSpace(raw) if raw == "" { return "", "" } + type failureGroup struct { + records []string + summary, detail string + } + var groups []*failureGroup + bySummary := make(map[string]*failureGroup) + for line := range strings.Lines(raw) { + line = strings.TrimSpace(line) + if line == "" { + continue + } + lineProvider, record, message := provider, "", line + if match := dynamicDNSPublishFailure.FindStringSubmatch(line); match != nil { + lineProvider, record, message = cmp.Or(match[1], provider), match[2]+" "+match[3], match[4] + } + summary, detail := providerErrorDisplay(lineProvider, message) + group := bySummary[summary] + if group == nil { + group = &failureGroup{summary: summary, detail: detail} + bySummary[summary] = group + groups = append(groups, group) + } + if record != "" && !slices.Contains(group.records, record) { + group.records = append(group.records, record) + } + } + summaries := make([]string, 0, len(groups)) + var details []string + for _, group := range groups { + summaries = append(summaries, failedRecordsLabel(group.records)+group.summary) + if group.detail != "" { + details = append(details, group.detail) + } + } + return strings.Join(summaries, "\n"), strings.Join(details, "\n\n") +} - start := strings.IndexAny(raw, "{[") - if start < 0 { - return redactProviderSecrets(raw), "" +func failedRecordsLabel(records []string) string { + switch { + case len(records) == 0: + return "" + case len(records) > maximumListedRecords: + return fmt.Sprintf("%s and %d more: ", strings.Join(records[:maximumListedRecords], ", "), len(records)-maximumListedRecords) + default: + return strings.Join(records, ", ") + ": " } +} - var payload any - decoder := json.NewDecoder(strings.NewReader(raw[start:])) - decoder.UseNumber() - if err := decoder.Decode(&payload); err != nil { - return redactProviderSecrets(raw), "" +// providerErrorDisplay summarizes one provider failure. A rate limit or a +// failure on the provider's side is named as such, because Sable retries it +// and nothing needs fixing. A rejected request names the provider's own +// messages and codes. +func providerErrorDisplay(provider, message string) (string, string) { + name := dynamicDNSProviderName(provider) + status, statusCode, body := "", 0, message + if match := providerHTTPStatus.FindStringSubmatch(message); match != nil { + status, body = strings.TrimSpace(match[1]), strings.TrimSpace(match[3]) + statusCode, _ = strconv.Atoi(match[2]) + } + + payload, decoded := providerPayload(body) + detail := "" + if decoded { + if encoded, err := json.MarshalIndent(sanitizeProviderPayload(payload), "", " "); err == nil { + detail = string(encoded) + } } - messages, codes := providerDiagnostics(payload) - if len(messages) == 0 { - return redactProviderSecrets(raw), "" + switch { + case statusCode == http.StatusTooManyRequests: + return name + " is rate limiting requests (" + status + "). Sable will try again.", detail + case statusCode >= http.StatusInternalServerError: + return name + " had a temporary problem (" + status + "). Sable will try again.", detail + } + if decoded { + if messages, codes := providerDiagnostics(payload); len(messages) > 0 { + return name + " rejected the request: " + strings.Join(messages, ": ") + providerCodesLabel(codes) + ".", detail + } + } + if status == "" { + return redactProviderSecrets(message), detail } + if !decoded && body != status && !strings.HasPrefix(body, "<") { + return name + " returned " + status + ": " + redactProviderSecrets(body), "" + } + return name + " returned " + status + ".", detail +} - summary := dynamicDNSProviderName(provider) + " rejected the request: " + strings.Join(messages, ": ") - if len(codes) == 1 { - summary += " (code " + codes[0] + ")" - } else if len(codes) > 1 { - summary += " (codes " + strings.Join(codes, ", ") + ")" +// providerPayload decodes the JSON a provider sent, which starts at the first +// brace or bracket of the message. +func providerPayload(message string) (any, bool) { + start := strings.IndexAny(message, "{[") + if start < 0 { + return nil, false + } + var payload any + decoder := json.NewDecoder(strings.NewReader(message[start:])) + decoder.UseNumber() + if err := decoder.Decode(&payload); err != nil { + return nil, false } - summary += "." + return payload, true +} - detail, err := json.MarshalIndent(sanitizeProviderPayload(payload), "", " ") - if err != nil { - return summary, "" +func providerCodesLabel(codes []string) string { + switch len(codes) { + case 0: + return "" + case 1: + return " (code " + codes[0] + ")" + default: + return " (codes " + strings.Join(codes, ", ") + ")" } - return summary, string(detail) } func providerDiagnostics(payload any) ([]string, []string) { diff --git a/internal/web/integrations_test.go b/internal/web/integrations_test.go index 2e485268..d047759d 100644 --- a/internal/web/integrations_test.go +++ b/internal/web/integrations_test.go @@ -2,6 +2,7 @@ package web import ( "context" + "fmt" "io" "log/slog" "net/http" @@ -407,6 +408,43 @@ func TestDynamicDNSErrorDisplaySummarizesAndSanitizesProviderResponse(t *testing } } +func TestDynamicDNSErrorDisplayCollapsesRepeatedProviderOutages(t *testing.T) { + t.Parallel() + body := `{"type":"https://developers.cloudflare.com/support/troubleshooting/http-status-codes/cloudflare-5xx-errors/error-502/","title":"Error 502: Bad gateway","status":502,"detail":"The origin web server returned an invalid or incomplete response.","instance":"%s","zone":"api-gateway.api.cloudflare.com","retryable":true}` + var lines []string + for _, record := range []string{"home.example.test A", "home.example.test AAAA", "vpn.example.test A", "edge.example.test A"} { + lines = append(lines, "cloudflare: publish "+record+": DNS provider API returned 502 Bad Gateway: "+fmt.Sprintf(body, record)) + } + + summary, detail := dynamicDNSErrorDisplay("", strings.Join(lines, "\n")) + + expected := "home.example.test A, home.example.test AAAA, vpn.example.test A and 1 more: Cloudflare had a temporary problem (502 Bad Gateway). Sable will try again." + if summary != expected { + t.Fatalf("summary = %q, want %q", summary, expected) + } + if !strings.Contains(detail, `"title": "Error 502: Bad gateway"`) || strings.Count(detail, `"title"`) != 1 { + t.Fatalf("detail should hold the first provider response once: %s", detail) + } +} + +func TestDynamicDNSErrorDisplayNamesRateLimitsAndPlainResponses(t *testing.T) { + t.Parallel() + for raw, expected := range map[string]string{ + "route53: publish edge.example.test A: Route 53 API returned 429 Too Many Requests: Too Many Requests": "edge.example.test A: Amazon Route 53 is rate limiting requests (429 Too Many Requests). Sable will try again.", + "publish edge.example.test A: GoDaddy API returned 404 Not Found: Not Found": "edge.example.test A: GoDaddy returned 404 Not Found.", + "publish edge.example.test A: GoDaddy API returned 403 Forbidden: access denied for token=secret-value": "edge.example.test A: GoDaddy returned 403 Forbidden: access denied for token=[redacted]", + "cloudflare DNS credentials are not configured\nroute53: publish edge.example.test A: Route 53 API returned 503 Service Unavailable: busy": "cloudflare DNS credentials are not configured\nedge.example.test A: Amazon Route 53 had a temporary problem (503 Service Unavailable). Sable will try again.", + } { + provider := "godaddy" + if strings.Contains(raw, "Route 53") { + provider = "" + } + if summary, _ := dynamicDNSErrorDisplay(provider, raw); summary != expected { + t.Errorf("summary of %q = %q, want %q", raw, summary, expected) + } + } +} + func TestDynamicDNSErrorDisplayFallsBackToPlainText(t *testing.T) { t.Parallel() summary, detail := dynamicDNSErrorDisplay("cloudflare", "request failed with token=secret-value") diff --git a/internal/web/pages/integrations.templ b/internal/web/pages/integrations.templ index 65455b71..5d6cc6d9 100644 --- a/internal/web/pages/integrations.templ +++ b/internal/web/pages/integrations.templ @@ -439,7 +439,7 @@ templ DynamicDNSStatusPanel(view DynamicDNSAppView) { @Icon("alert-triangle")
Last publish failed - { view.Status.LastError } + { view.Status.LastError } if view.Status.LastErrorDetail != "" {
View provider details diff --git a/internal/web/pages/integrations_templ.go b/internal/web/pages/integrations_templ.go index 63d27214..ffb5a558 100644 --- a/internal/web/pages/integrations_templ.go +++ b/internal/web/pages/integrations_templ.go @@ -875,14 +875,14 @@ func DynamicDNSStatusPanel(view DynamicDNSAppView) templ.Component { if templ_7745c5c3_Err != nil { return templ_7745c5c3_Err } - templ_7745c5c3_Err = templruntime.WriteString(templ_7745c5c3_Buffer, 19, "
Last publish failed ") + templ_7745c5c3_Err = templruntime.WriteString(templ_7745c5c3_Buffer, 19, "
Last publish failed ") if templ_7745c5c3_Err != nil { return templ_7745c5c3_Err } var templ_7745c5c3_Var18 string templ_7745c5c3_Var18, templ_7745c5c3_Err = templ.JoinStringErrs(view.Status.LastError) if templ_7745c5c3_Err != nil { - return templ.Error{Err: templ_7745c5c3_Err, FileName: `integrations.templ`, Line: 442, Col: 34} + return templ.Error{Err: templ_7745c5c3_Err, FileName: `integrations.templ`, Line: 442, Col: 68} } _, templ_7745c5c3_Err = templ_7745c5c3_Buffer.WriteString(templ.EscapeString(templ_7745c5c3_Var18)) if templ_7745c5c3_Err != nil {