diff --git a/kube/certs/certs.go b/kube/certs/certs.go index e76d6e188..b8e6ccf14 100644 --- a/kube/certs/certs.go +++ b/kube/certs/certs.go @@ -7,12 +7,15 @@ import ( "context" + "errors" "fmt" "net" "slices" "sync" + "syscall" "time" + "tailscale.com/client/local" "tailscale.com/ipn" "tailscale.com/kube/localclient" "tailscale.com/types/logger" @@ -94,6 +97,51 @@ func (cm *CertManager) EnsureCertLoops(ctx context.Context, sc *ipn.ServeConfig) return nil } +// isTransientCertErr reports whether err represents a failure that did not +// reach the CA (ctx timeout, LocalAPI socket unreachable). Such errors must +// not advance the loop's retryCount. +func isTransientCertErr(err error) bool { + switch { + case errors.Is(err, context.DeadlineExceeded), + errors.Is(err, context.Canceled), + errors.Is(err, syscall.ECONNREFUSED), + errors.Is(err, syscall.ECONNRESET), + errors.Is(err, syscall.EHOSTUNREACH), + errors.Is(err, syscall.EPIPE): + return true + } + var ne net.Error + if errors.As(err, &ne) && ne.Timeout() { + return true + } + return false +} + +// nextRetryInterval picks the wait until the next CertPair attempt: +// - success: reset retryCount, normalInterval. +// - transient (never reached the CA): retrySchedule[0], no retryCount advance. +// - retryAfter > 0: honour it, retryCount still advances. +// - other: advance retryCount, walk retrySchedule. +func nextRetryInterval(err error, retryCount *int, normalInterval, retryAfter time.Duration) time.Duration { + switch { + case err == nil: + *retryCount = 0 + return normalInterval + case isTransientCertErr(err): + return retrySchedule[0] + } + *retryCount++ + idx := *retryCount - 1 + if idx >= len(retrySchedule) { + idx = len(retrySchedule) - 1 + } + interval := retrySchedule[idx] + if retryAfter > 0 { + interval = retryAfter + } + return interval +} + // retrySchedule is the wait between successive failed issuance attempts, // following LE's recommended schedule. // https://letsencrypt.org/docs/integration-guide/#retrying-failures @@ -108,8 +156,8 @@ func (cm *CertManager) EnsureCertLoops(ctx context.Context, sc *ipn.ServeConfig) // - calls localAPI certificate endpoint to ensure that certs are issued for the // given domain name // - calls localAPI certificate endpoint daily to ensure that certs are renewed -// - if certificate issuance failed retries after an exponential backoff period -// starting at 1 minute and capped at 24 hours. Reset the backoff once issuance succeeds. +// - if certificate issuance failed, retries on the schedule defined by +// [retrySchedule]; resets to the start once issuance succeeds. // Note that renewal check also happens when the node receives an HTTPS request and it is possible that certs get // renewed at that point. Renewal here is needed to prevent the shared certs from expiry in edge cases where the 'write' // replica does not get any HTTPS requests. @@ -147,22 +195,9 @@ func (cm *CertManager) runCertLoop(ctx context.Context, domain string) { ctxT, cancel := context.WithTimeout(ctx, 30*time.Minute) _, _, err := cm.lc.CertPair(ctxT, domain) cancel() + retryAfter, _ := local.RateLimitRetryAfter(err) + nextInterval := nextRetryInterval(err, &retryCount, normalInterval, retryAfter) if err != nil { - cm.logf("error refreshing certificate for %s: %v", domain, err) - } - var nextInterval time.Duration - // TODO(irbekrm): distinguish between LE rate limit errors and other - // error types like transient network errors. - if err == nil { - retryCount = 0 - nextInterval = normalInterval - } else { - retryCount++ - idx := retryCount - 1 - if idx >= len(retrySchedule) { - idx = len(retrySchedule) - 1 - } - nextInterval = retrySchedule[idx] cm.logf("Error refreshing certificate for %s (retry %d): %v. Will retry in %v\n", domain, retryCount, err, nextInterval) } diff --git a/kube/certs/certs_test.go b/kube/certs/certs_test.go index 27fe12752..c48fa3647 100644 --- a/kube/certs/certs_test.go +++ b/kube/certs/certs_test.go @@ -5,7 +5,10 @@ import ( "context" + "errors" + "fmt" "log" + "syscall" "testing" "time" @@ -284,3 +287,57 @@ func TestEnsureCertLoops(t *testing.T) { }) } } + +func TestIsTransientCertErr(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + {"deadline", context.DeadlineExceeded, true}, + {"canceled", context.Canceled, true}, + {"wrapped_deadline", fmt.Errorf("wrap: %w", context.DeadlineExceeded), true}, + {"connrefused", fmt.Errorf("dial: %w", syscall.ECONNREFUSED), true}, + {"connreset", fmt.Errorf("read: %w", syscall.ECONNRESET), true}, + {"random", errors.New("badNonce"), false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isTransientCertErr(tt.err); got != tt.want { + t.Errorf("isTransientCertErr(%v) = %v, want %v", tt.err, got, tt.want) + } + }) + } +} + +func TestNextRetryInterval(t *testing.T) { + const normal = 24 * time.Hour + tests := []struct { + name string + err error + retryAfter time.Duration + startCount int + wantCount int + wantInterval time.Duration + }{ + {"success", nil, 0, 5, 0, normal}, + {"transient_no_advance", context.DeadlineExceeded, 0, 3, 3, retrySchedule[0]}, + {"rate_limit_with_hint", errors.New("rate limited"), 17 * time.Minute, 0, 1, 17 * time.Minute}, + {"rate_limit_no_hint", errors.New("rate limited"), 0, 0, 1, retrySchedule[0]}, + {"other_advances", errors.New("badNonce"), 0, 0, 1, retrySchedule[0]}, + {"other_clamps", errors.New("badNonce"), 0, len(retrySchedule) + 3, len(retrySchedule) + 4, retrySchedule[len(retrySchedule)-1]}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := tt.startCount + got := nextRetryInterval(tt.err, &c, normal, tt.retryAfter) + if c != tt.wantCount { + t.Errorf("retryCount = %d, want %d", c, tt.wantCount) + } + if got != tt.wantInterval { + t.Errorf("interval = %v, want %v", got, tt.wantInterval) + } + }) + } +}