diff --git a/pkg/certmanager/acme_errors.go b/pkg/certmanager/acme_errors.go index fb935821a..69611a5db 100644 --- a/pkg/certmanager/acme_errors.go +++ b/pkg/certmanager/acme_errors.go @@ -23,6 +23,8 @@ package certmanager import ( "errors" "fmt" + "net/http" + "strconv" "time" "golang.org/x/crypto/acme" @@ -37,12 +39,13 @@ var ( ) type ACMEError struct { - op string - err error - problemType string - detail string - rateLimited bool - retryAfter time.Duration + op string + err error + problemType string + detail string + rateLimited bool + retryAfter time.Duration + retryAfterSet bool } func (e *ACMEError) Error() string { @@ -70,14 +73,20 @@ func (e *ACMEError) Is(target error) bool { } // RetryAfter returns how long callers should wait before retrying. -// For rate-limited errors it prefers the ACME Retry-After value and falls back -// to defaultCooldown when the header is absent. Non-rate-limited errors return 0. +// For rate-limited errors it honors the ACME Retry-After value whenever the +// header is present and parseable — including a zero (or past) value, which the +// CA uses to permit an immediate retry. It falls back to defaultCooldown only +// when the header is absent or invalid. Non-rate-limited errors return 0. func (e *ACMEError) RetryAfter() time.Duration { if e == nil || !e.rateLimited { return 0 } - if e.retryAfter > 0 { + if e.retryAfterSet { + if e.retryAfter < 0 { + return 0 + } + return e.retryAfter } @@ -119,10 +128,43 @@ func newACMEError(op string, err error) *ACMEError { out.problemType = acmeErr.ProblemType out.detail = acmeErr.Detail - if retryAfter, ok := acme.RateLimit(acmeErr); ok { + if _, ok := acme.RateLimit(acmeErr); ok { out.rateLimited = true - out.retryAfter = retryAfter + + // acme.RateLimit collapses "Retry-After: 0", an invalid header, and a + // missing header all to a zero duration, so inspect the header directly + // to tell an explicit zero (immediate retry) apart from an absent one + // (fall back to defaultCooldown in RetryAfter). + if retryAfter, ok := parseRetryAfter(acmeErr.Header); ok { + out.retryAfter = retryAfter + out.retryAfterSet = true + } } return out } + +// parseRetryAfter reports the Retry-After delay and whether the header was +// present and parseable. It mirrors the delta-seconds and HTTP-date forms the +// ACME client understands. An absent or unparseable value returns ok=false so +// callers can apply their own fallback. +func parseRetryAfter(header http.Header) (time.Duration, bool) { + if header == nil { + return 0, false + } + + value := header.Get("Retry-After") + if value == "" { + return 0, false + } + + if seconds, err := strconv.Atoi(value); err == nil { + return time.Duration(seconds) * time.Second, true + } + + if date, err := http.ParseTime(value); err == nil { + return time.Until(date), true + } + + return 0, false +} diff --git a/pkg/certmanager/acme_test.go b/pkg/certmanager/acme_test.go index 5483a3e2c..1ab1825c1 100644 --- a/pkg/certmanager/acme_test.go +++ b/pkg/certmanager/acme_test.go @@ -26,11 +26,34 @@ import ( "testing" "time" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/crypto/acme" ) +func TestNewMetrics_SharedRegistererDoesNotPanic(t *testing.T) { + t.Parallel() + + registerer := prometheus.NewRegistry() + + first := newMetrics(registerer) + require.NotNil(t, first) + + // A second ACMEService sharing the registerer re-registers fixed-name + // collectors; this must reuse the existing ones instead of panicking. + var second *metrics + require.NotPanics(t, func() { + second = newMetrics(registerer) + }) + require.NotNil(t, second) + + assert.Same(t, first.provisionSteps, second.provisionSteps) + assert.Same(t, first.acmeErrors, second.acmeErrors) + assert.Same(t, first.stepDuration, second.stepDuration) + assert.Equal(t, first.acmeCooldown, second.acmeCooldown) +} + func TestNewACMEError_RateLimited(t *testing.T) { t.Parallel() @@ -63,6 +86,41 @@ func TestNewACMEError_RateLimitedDefaultCooldown(t *testing.T) { assert.Equal(t, defaultCooldown, err.RetryAfter()) } +func TestNewACMEError_RateLimitedRetryAfterZero(t *testing.T) { + t.Parallel() + + err := newACMEError( + "cannot create order", + &acme.Error{ + ProblemType: "urn:ietf:params:acme:error:rateLimited", + Header: http.Header{"Retry-After": []string{"0"}}, + }, + ) + + require.NotNil(t, err) + assert.ErrorIs(t, err, ErrACMERateLimited) + // An explicit Retry-After: 0 permits an immediate retry and must not be + // promoted to the one-hour default cooldown. + assert.Equal(t, time.Duration(0), err.RetryAfter()) +} + +func TestNewACMEError_RateLimitedRetryAfterInvalid(t *testing.T) { + t.Parallel() + + err := newACMEError( + "cannot create order", + &acme.Error{ + ProblemType: "urn:ietf:params:acme:error:rateLimited", + Header: http.Header{"Retry-After": []string{"not-a-date"}}, + }, + ) + + require.NotNil(t, err) + assert.ErrorIs(t, err, ErrACMERateLimited) + // An unparseable header falls back to the default cooldown. + assert.Equal(t, defaultCooldown, err.RetryAfter()) +} + func TestNewACMEError_NonRateLimited(t *testing.T) { t.Parallel() diff --git a/pkg/certmanager/metrics.go b/pkg/certmanager/metrics.go index 46b8e8418..3f1afb20c 100644 --- a/pkg/certmanager/metrics.go +++ b/pkg/certmanager/metrics.go @@ -21,6 +21,7 @@ package certmanager import ( + "errors" "time" "github.com/prometheus/client_golang/prometheus" @@ -56,48 +57,68 @@ func newMetrics(registerer prometheus.Registerer) *metrics { registerer = prometheus.DefaultRegisterer } - m := &metrics{ - provisionSteps: prometheus.NewCounterVec( - prometheus.CounterOpts{ - Subsystem: "certmanager", - Name: "certificate_provision_steps_total", - Help: "Certificate provisioning steps by phase and result.", - }, - []string{"phase", "result"}, + return &metrics{ + provisionSteps: registerCollector( + registerer, + prometheus.NewCounterVec( + prometheus.CounterOpts{ + Subsystem: "certmanager", + Name: "certificate_provision_steps_total", + Help: "Certificate provisioning steps by phase and result.", + }, + []string{"phase", "result"}, + ), ), - acmeErrors: prometheus.NewCounterVec( - prometheus.CounterOpts{ - Subsystem: "certmanager", - Name: "certificate_acme_errors_total", - Help: "ACME errors by problem type.", - }, - []string{"problem_type"}, + acmeErrors: registerCollector( + registerer, + prometheus.NewCounterVec( + prometheus.CounterOpts{ + Subsystem: "certmanager", + Name: "certificate_acme_errors_total", + Help: "ACME errors by problem type.", + }, + []string{"problem_type"}, + ), ), - acmeCooldown: prometheus.NewGauge( - prometheus.GaugeOpts{ - Subsystem: "certmanager", - Name: "certificate_acme_cooldown", - Help: "1 while the ACME client is in a global rate-limit cooldown.", - }, + acmeCooldown: registerCollector( + registerer, + prometheus.NewGauge( + prometheus.GaugeOpts{ + Subsystem: "certmanager", + Name: "certificate_acme_cooldown", + Help: "1 while the ACME client is in a global rate-limit cooldown.", + }, + ), ), - stepDuration: prometheus.NewHistogramVec( - prometheus.HistogramOpts{ - Subsystem: "certmanager", - Name: "certificate_provision_step_duration_seconds", - Help: "Duration of certificate provisioning steps in seconds.", - }, - []string{"phase"}, + stepDuration: registerCollector( + registerer, + prometheus.NewHistogramVec( + prometheus.HistogramOpts{ + Subsystem: "certmanager", + Name: "certificate_provision_step_duration_seconds", + Help: "Duration of certificate provisioning steps in seconds.", + }, + []string{"phase"}, + ), ), } +} - registerer.MustRegister( - m.provisionSteps, - m.acmeErrors, - m.acmeCooldown, - m.stepDuration, - ) +func registerCollector[T prometheus.Collector]( + registerer prometheus.Registerer, + collector T, +) T { + if err := registerer.Register(collector); err != nil { + if already, ok := errors.AsType[prometheus.AlreadyRegisteredError](err); ok { + if existing, ok := already.ExistingCollector.(T); ok { + return existing + } + } - return m + panic(err) + } + + return collector } func (m *metrics) observeStep(phase provisionPhase, result provisionResult, started time.Time) { diff --git a/pkg/certmanager/provision_worker.go b/pkg/certmanager/provision_worker.go index 5a40f61b3..01ba6dae5 100644 --- a/pkg/certmanager/provision_worker.go +++ b/pkg/certmanager/provision_worker.go @@ -43,6 +43,10 @@ const ( maxProvisioningRetries = 3 dnsExchangeTimeout = 10 * time.Second processTickTimeout = 90 * time.Second + // persistFailureTimeout bounds the retry-outcome write-back. It runs on a + // context detached from the process tick deadline so a timed-out attempt can + // still record its failure. + persistFailureTimeout = 15 * time.Second tracerName = "go.probo.inc/probo/pkg/certmanager" ) @@ -404,6 +408,15 @@ func (h *provisionHandler) persistFailure( ) error { errorCode := classifyProvisioningError(provisionErr) + // Process runs each tick under processTickTimeout. When that deadline fires + // mid-attempt, the same expired context reaches here, and the write-back + // silently fails — so the retry budget never advances and the certificate + // stays retriable forever. Detach from the tick deadline (and cancellation) + // and bound the write with its own timeout so repeated timeouts still make + // progress toward FAILED. + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), persistFailureTimeout) + defer cancel() + return h.pg.WithTx( ctx, func(ctx context.Context, tx pg.Tx) error { @@ -731,7 +744,8 @@ func (h *provisionHandler) checkCAARecords(ctx context.Context, hostname string) } return fmt.Errorf( - "caa records for domain %q do not permit issuance by %q", + "%w: domain %q by %q", + ErrCAANotPermitted, hostname, h.caaIssuerDomain, ) diff --git a/pkg/certmanager/provision_worker_test.go b/pkg/certmanager/provision_worker_test.go index acb95eee3..f38d9aec2 100644 --- a/pkg/certmanager/provision_worker_test.go +++ b/pkg/certmanager/provision_worker_test.go @@ -22,6 +22,7 @@ package certmanager import ( "errors" + "fmt" "testing" "github.com/stretchr/testify/assert" @@ -35,8 +36,11 @@ func TestClassifyProvisioningError(t *testing.T) { assert.Equal(t, ProvisioningErrorACMERateLimited, classifyProvisioningError(ErrACMERateLimited)) assert.Equal(t, ProvisioningErrorACMEInvalidOrder, classifyProvisioningError(ErrOrderInvalid)) assert.Equal(t, ProvisioningErrorDNSCNAME, classifyProvisioningError(errors.New("cname target mismatch"))) - assert.Equal(t, ProvisioningErrorDNSCAA, classifyProvisioningError(errors.New("caa records for domain"))) + assert.Equal(t, ProvisioningErrorDNSCAA, classifyProvisioningError(fmt.Errorf("%w: domain %q", ErrCAANotPermitted, "example.com"))) assert.Equal(t, ProvisioningErrorACMETemporary, classifyProvisioningError(errors.New("network timeout"))) + // A CAA resolver/transport failure shares the "caa records" wording with a + // real CAA misconfiguration but must consume the normal retry budget. + assert.Equal(t, ProvisioningErrorACMETemporary, classifyProvisioningError(errors.New("cannot exchange dns message for caa records: i/o timeout"))) } func TestDecideProvisioningOutcome_RateLimitKeepsRetryCountAndOrder(t *testing.T) { diff --git a/pkg/certmanager/provisioning_error.go b/pkg/certmanager/provisioning_error.go index dda6ae6cb..2e67ba4b2 100644 --- a/pkg/certmanager/provisioning_error.go +++ b/pkg/certmanager/provisioning_error.go @@ -34,6 +34,13 @@ const ( ProvisioningErrorACMEFailed = "ACME_FAILED" ) +// ErrCAANotPermitted marks a CAA policy that actively forbids issuance by our +// CA. This is a customer-side misconfiguration and is intentionally +// non-terminal. It must stay distinct from a CAA resolver/transport failure, +// which shares the "caa records" wording but is a transient error that has to +// consume the normal retry budget instead of retrying forever. +var ErrCAANotPermitted = errors.New("caa records do not permit issuance") + func classifyProvisioningError(err error) string { if err == nil { return "" @@ -47,12 +54,14 @@ func classifyProvisioningError(err error) string { return ProvisioningErrorACMEInvalidOrder } + if errors.Is(err, ErrCAANotPermitted) { + return ProvisioningErrorDNSCAA + } + msg := strings.ToLower(err.Error()) switch { case strings.Contains(msg, "cname"): return ProvisioningErrorDNSCNAME - case strings.Contains(msg, "caa record"): - return ProvisioningErrorDNSCAA case strings.Contains(msg, "status: invalid"), strings.Contains(msg, "order is in unexpected status \"invalid\""): return ProvisioningErrorACMEInvalidOrder default: diff --git a/pkg/coredata/certificate.go b/pkg/coredata/certificate.go index 07aca1e8c..57973e711 100644 --- a/pkg/coredata/certificate.go +++ b/pkg/coredata/certificate.go @@ -220,7 +220,7 @@ FOR UPDATE SKIP LOCKED q = fmt.Sprintf(q, scope.SQLFragment()) - args := pgx.NamedArgs{"id": certificateID} + args := pgx.StrictNamedArgs{"id": certificateID} maps.Copy(args, scope.SQLArguments()) rows, err := conn.Query(ctx, q, args) @@ -280,7 +280,7 @@ FOR UPDATE q = fmt.Sprintf(q, scope.SQLFragment()) - args := pgx.NamedArgs{"id": certificateID} + args := pgx.StrictNamedArgs{"id": certificateID} maps.Copy(args, scope.SQLArguments()) rows, err := conn.Query(ctx, q, args)