diff --git a/services/protocols/cmd/lambdamail-protocols/main.go b/services/protocols/cmd/lambdamail-protocols/main.go index 1e61461..de4215f 100644 --- a/services/protocols/cmd/lambdamail-protocols/main.go +++ b/services/protocols/cmd/lambdamail-protocols/main.go @@ -226,6 +226,9 @@ func run(cfg config) { // like this: the auth service answered it by re-reading the stored status, // because the resolver and the record spec both live here. if cfg.CloudflareToken != "" || cfg.PublicIPv4 != "" { + // Verification writes down what it found, so the badge in the domain + // list stops disagreeing with the page next to it. + router.SetDnsStatusWriter(postgres.NewDomainRepository(pool)) router.SetAdminDnsAPI( &dnsSpecSource{cfg: cfg, dkim: dkimRepo}, netdns.NewPublicVerifier(), diff --git a/services/protocols/internal/infrastructure/postgres/domain_repository.go b/services/protocols/internal/infrastructure/postgres/domain_repository.go index b19e4f8..d9166c5 100644 --- a/services/protocols/internal/infrastructure/postgres/domain_repository.go +++ b/services/protocols/internal/infrastructure/postgres/domain_repository.go @@ -3,6 +3,7 @@ package postgres import ( "context" "fmt" + "strings" "github.com/jackc/pgx/v5/pgxpool" ) @@ -40,3 +41,39 @@ func (r *DomainRepository) ActiveDomainNames(ctx context.Context) ([]string, err } return out, rows.Err() } + +// SaveDnsStatus files the outcome of a verification against the domain. +// +// The console reads its badge from this column, and nothing ever wrote to it +// after onboarding: a domain whose records were all published and resolving +// was still listed as PENDING, last checked "never". +func (r *DomainRepository) SaveDnsStatus(ctx context.Context, domain, status string) error { + _, err := r.pool.Exec(ctx, ` + UPDATE domains + SET dns_status = $2, dns_last_checked_at = now() + WHERE lower(name) = lower($1) + `, domain, columnStatus(status)) + if err != nil { + return fmt.Errorf("save dns status for %s: %w", domain, err) + } + return nil +} + +// columnStatus maps the verifier's vocabulary onto the one the column accepts. +// +// dns_status is constrained to PENDING, VERIFIED, PARTIAL, DRIFT and ERROR, +// while verification also reports MISSING and UNKNOWN. Writing either of those +// violates the check constraint, so the update fails and the badge silently +// stays stale - the exact symptom this was meant to fix. +func columnStatus(status string) string { + switch strings.ToUpper(strings.TrimSpace(status)) { + case "VERIFIED", "PARTIAL", "DRIFT", "ERROR", "PENDING": + return strings.ToUpper(strings.TrimSpace(status)) + case "MISSING": + // Nothing resolved: the zone is not serving what it should. + return "ERROR" + default: + // UNKNOWN, and anything new the verifier learns to say. + return "PENDING" + } +} diff --git a/services/protocols/internal/infrastructure/postgres/domain_status_test.go b/services/protocols/internal/infrastructure/postgres/domain_status_test.go new file mode 100644 index 0000000..9957047 --- /dev/null +++ b/services/protocols/internal/infrastructure/postgres/domain_status_test.go @@ -0,0 +1,27 @@ +package postgres + +import "testing" + +// dns_status is constrained by the schema to a fixed set. The verifier answers +// with a wider vocabulary, and writing one of the extra words fails the check +// constraint - the update is rejected and the console goes on showing the +// stale badge this was written to fix. +func TestColumnStatus_OnlyEverProducesAcceptedValues(t *testing.T) { + allowed := map[string]bool{ + "PENDING": true, "VERIFIED": true, "PARTIAL": true, "DRIFT": true, "ERROR": true, + } + + for _, in := range []string{"VERIFIED", "PARTIAL", "MISSING", "UNKNOWN", "", "something new"} { + got := columnStatus(in) + if !allowed[got] { + t.Errorf("columnStatus(%q) = %q, which the check constraint rejects", in, got) + } + } + + if got := columnStatus("MISSING"); got != "ERROR" { + t.Errorf("MISSING became %q; nothing resolved, so it is an error, not a pending check", got) + } + if got := columnStatus("VERIFIED"); got != "VERIFIED" { + t.Errorf("VERIFIED became %q", got) + } +} diff --git a/services/protocols/internal/presentation/http/admin_dns_api.go b/services/protocols/internal/presentation/http/admin_dns_api.go index 8748e51..4cc5832 100644 --- a/services/protocols/internal/presentation/http/admin_dns_api.go +++ b/services/protocols/internal/presentation/http/admin_dns_api.go @@ -47,6 +47,15 @@ type DomainReconciler interface { ReconcileDomain(ctx context.Context, domain string) (ReconcileResult, error) } +// DnsStatusWriter records the outcome of a verification. +// +// Verification lives here, with the resolver; the badge the console shows is +// read from the domains table. Without this the two never met: a domain could +// verify 13 of 13 and still be listed as PENDING, never checked. +type DnsStatusWriter interface { + SaveDnsStatus(ctx context.Context, domain, status string) error +} + type adminDnsAPI struct { spec DnsSpecSource verifier DnsRecordVerifier @@ -54,6 +63,9 @@ type adminDnsAPI struct { // reconciler is optional: without it the route reports that the feature // is not configured rather than answering as though it had run. reconciler DomainReconciler + // status is optional: without it verification still answers, it just is + // not written down. + status DnsStatusWriter } // handleReconcile publishes the records a domain is missing. @@ -168,6 +180,15 @@ func (a *adminDnsAPI) handleVerify(w http.ResponseWriter, r *http.Request) { status = "PARTIAL" } + // Written down before answering, so the list badge and this page agree. + if a.status != nil { + if err := a.status.SaveDnsStatus(r.Context(), domain, status); err != nil { + // The check itself succeeded; failing to file it must not turn a + // good answer into an error for the operator. + log.Printf("dns: verified %s but could not record the status: %v", domain, err) + } + } + writeJSON(w, http.StatusOK, map[string]any{ "domain": domain, "status": status, @@ -189,6 +210,7 @@ func (r *Router) SetAdminDnsAPI(spec DnsSpecSource, verifier DnsRecordVerifier, // either order. They could not before, and main.go called them in the // order that lost the reconciler. reconciler: r.dnsReconciler, + status: r.dnsStatus, } } @@ -208,6 +230,14 @@ func (r *Router) handleAdminDnsReconcile(w http.ResponseWriter, req *http.Reques // SetDnsReconciler enables the reconcile route to actually publish records. // Without it the route reports that reconciliation is not configured. +// SetDnsStatusWriter lets verification record what it found. +func (r *Router) SetDnsStatusWriter(w DnsStatusWriter) { + r.dnsStatus = w + if r.dns != nil { + r.dns.status = w + } +} + func (r *Router) SetDnsReconciler(reconciler DomainReconciler) { // Kept on the router as well as on the API. Assigning only to r.dns meant // that calling this before SetAdminDnsAPI silently did nothing: the field diff --git a/services/protocols/internal/presentation/http/admin_dns_reconcile_test.go b/services/protocols/internal/presentation/http/admin_dns_reconcile_test.go index 7f3ffae..e6cdf94 100644 --- a/services/protocols/internal/presentation/http/admin_dns_reconcile_test.go +++ b/services/protocols/internal/presentation/http/admin_dns_reconcile_test.go @@ -204,3 +204,44 @@ func TestReconcilerSurvivesBeingWiredBeforeTheDnsAPI(t *testing.T) { }) } } + +type stubStatusWriter struct { + domain string + status string + calls int +} + +func (s *stubStatusWriter) SaveDnsStatus(_ context.Context, domain, status string) error { + s.calls++ + s.domain, s.status = domain, status + return nil +} + +// The console listed a fully verified domain as PENDING, "last checked: never". +// +// Verification ran in the protocols service, which owns the resolver, and its +// answer went straight to the browser. Nothing wrote it down, and the badge in +// the list is read from the domains table - so it stayed at the value set when +// the domain was first added, no matter how many times it verified 13 of 13. +func TestVerifyRecordsWhatItFound(t *testing.T) { + writer := &stubStatusWriter{} + api := &adminDnsAPI{ + spec: stubSpec{}, verifier: stubVerifier{}, + sessions: testVerifier(interopSecret), status: writer, + } + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/api/v1/admin/dns/verify?domain=example.test", nil) + req.Header.Set("Authorization", "Bearer "+adminSurfaceToken2()) + api.handleVerify(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("answered %d: %s", rec.Code, rec.Body) + } + if writer.calls != 1 { + t.Fatalf("wrote the status %d times, want 1: the console keeps showing a stale badge", writer.calls) + } + if writer.domain != "example.test" { + t.Errorf("recorded status for %q, want example.test", writer.domain) + } +} diff --git a/services/protocols/internal/presentation/http/router.go b/services/protocols/internal/presentation/http/router.go index 974b691..eb72a48 100644 --- a/services/protocols/internal/presentation/http/router.go +++ b/services/protocols/internal/presentation/http/router.go @@ -39,6 +39,7 @@ type Router struct { dns *adminDnsAPI // dnsReconciler is held here so it survives being set before dns exists. dnsReconciler DomainReconciler + dnsStatus DnsStatusWriter // degradedFunc reports a condition that leaves the service running but // not fit for production, the clearest case being a self-signed // certificate standing in for one Traefik never issued