Backport of enhance TLS certificate monitoring with retry logic and severity logging into release/2.0.x (#23577)

backport of commit 8b8f3c5

Co-authored-by: Sanika Chavan <sanika.vikaschavan@hashicorp.com>

Author: hc-github-team-consul-core

agent/agent.go

@@ -890,7 +890,12 @@ func (a *Agent) Start(ctx context.Context) error {
 		go a.retryJoinWAN()
 	}
 
-	if a.config.Telemetry.CertificateEnabled && a.tlsConfigurator.Cert() != nil {
+	shouldMonitorAgentTLS := a.config.Telemetry.CertificateEnabled &&
+		(a.tlsConfigurator.Cert() != nil ||
+			a.config.AutoEncryptTLS ||
+			a.config.TLS.InternalRPC.CertFile != "" ||
+			a.config.TLS.InternalRPC.KeyFile != "")
+	if shouldMonitorAgentTLS {
 		m := tlsCertExpirationMonitor(
 			a.tlsConfigurator,
 			a.config.Datacenter,

agent/consul/leader_metrics.go

@@ -26,6 +26,11 @@ var (
 
 	// TestCertExpirationMonitorInterval overrides the default emission cadence in tests.
 	TestCertExpirationMonitorInterval time.Duration
+
+	// certExpirationMonitorRetryInterval is used when metric emission fails (for example
+	// when cert state is not yet initialized during startup). In that case we retry faster
+	// than the normal hourly interval.
+	certExpirationMonitorRetryInterval = 10 * time.Second
 )
 
 var LeaderCertExpirationGauges = []prometheus.GaugeDefinition{
@@ -131,6 +136,27 @@ type CertExpirationMonitor struct {
 
 const certExpirationMonitorInterval = time.Hour
 
+func certDaysRemaining(untilAfter time.Duration) int {
+	return int(math.Floor(untilAfter.Hours() / 24))
+}
+
+func certLogSeverity(untilAfter time.Duration, criticalDays, warningDays int) string {
+	if untilAfter <= 0 {
+		return "expired"
+	}
+
+	criticalThreshold := time.Duration(criticalDays) * 24 * time.Hour
+	warningThreshold := time.Duration(warningDays) * 24 * time.Hour
+	switch {
+	case untilAfter <= criticalThreshold:
+		return "critical"
+	case untilAfter <= warningThreshold:
+		return "warning"
+	default:
+		return "ok"
+	}
+}
+
 func (m CertExpirationMonitor) Monitor(ctx context.Context) error {
 
 	// Check if certificate telemetry is enabled (only for server-based monitors)
@@ -142,20 +168,22 @@ func (m CertExpirationMonitor) Monitor(ctx context.Context) error {
 	if m.Interval > 0 {
 		interval = m.Interval
 	}
-
-	ticker := time.NewTicker(interval)
-	defer ticker.Stop()
+	retryInterval := certExpirationMonitorRetryInterval
+	if interval < retryInterval {
+		retryInterval = interval
+	}
 
 	logger := m.Logger.With("metric", strings.Join(m.Key, "."))
 
-	emitMetric := func() {
+	emitMetric := func() bool {
 		_, untilAfter, err := m.Query()
 		if err != nil {
 			logger.Warn("failed to emit certificate expiry metric", "error", err)
-			return
+			metrics.SetGaugeWithLabels(m.Key, float32(math.NaN()), m.Labels)
+			return false
 		}
 
-		daysRemaining := int(untilAfter.Hours() / 24)
+		daysRemaining := certDaysRemaining(untilAfter)
 
 		// Get thresholds from Server config or use provided values
 		var criticalDays, warningDays int
@@ -209,19 +237,29 @@ func (m CertExpirationMonitor) Monitor(ctx context.Context) error {
 		}
 
 		// Log based on threshold severity with detailed context
-		if daysRemaining <= criticalDays {
+		switch certLogSeverity(untilAfter, criticalDays, warningDays) {
+		case "expired":
+			logger.Error("certificate has expired", logFields...)
+		case "critical":
 			logger.Error("certificate expiring soon", logFields...)
-		} else if daysRemaining <= warningDays {
+		case "warning":
 			logger.Warn("certificate expiring soon", logFields...)
 		}
 
 		expiry := untilAfter / time.Second
 		metrics.SetGaugeWithLabels(m.Key, float32(expiry), m.Labels)
+		return true
 	}
 
 	// emit the metric immediately so that if a cert was just updated the
 	// new metric will be updated to the new expiration time.
-	emitMetric()
+	nextInterval := interval
+	if !emitMetric() {
+		nextInterval = retryInterval
+	}
+
+	timer := time.NewTimer(nextInterval)
+	defer timer.Stop()
 
 	for {
 		select {
@@ -230,8 +268,12 @@ func (m CertExpirationMonitor) Monitor(ctx context.Context) error {
 			// metric from a non-leader, it does not get a stale value.
 			metrics.SetGaugeWithLabels(m.Key, float32(math.NaN()), m.Labels)
 			return nil
-		case <-ticker.C:
-			emitMetric()
+		case <-timer.C:
+			nextInterval = interval
+			if !emitMetric() {
+				nextInterval = retryInterval
+			}
+			timer.Reset(nextInterval)
 		}
 	}
 }

agent/consul/leader_metrics_test.go

@@ -4,7 +4,15 @@
 package consul
 
 import (
+	"context"
+	"errors"
+	"sync/atomic"
 	"testing"
+	"time"
+
+	"github.com/armon/go-metrics"
+
+	"github.com/hashicorp/go-hclog"
 )
 
 // TestExpiresSoon was removed as we now use configurable thresholds
@@ -259,3 +267,121 @@ func TestCertificateTelemetry_Disabled(t *testing.T) {
 		t.Error("telemetry should be disabled")
 	}
 }
+
+func TestCertExpirationMonitor_RetriesQuicklyAfterQueryFailure(t *testing.T) {
+	oldRetry := certExpirationMonitorRetryInterval
+	certExpirationMonitorRetryInterval = 20 * time.Millisecond
+	t.Cleanup(func() {
+		certExpirationMonitorRetryInterval = oldRetry
+	})
+
+	var calls int32
+	success := make(chan struct{}, 1)
+	m := CertExpirationMonitor{
+		Key:      []string{"test", "cert", "expiry"},
+		Labels:   []metrics.Label{{Name: "datacenter", Value: "dc1"}},
+		Logger:   hclog.NewNullLogger(),
+		Interval: time.Hour,
+		Query: func() (time.Duration, time.Duration, error) {
+			if atomic.AddInt32(&calls, 1) == 1 {
+				return 0, 0, errors.New("not ready")
+			}
+			select {
+			case success <- struct{}{}:
+			default:
+			}
+			return 24 * time.Hour, 23 * time.Hour, nil
+		},
+	}
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	done := make(chan error, 1)
+	go func() {
+		done <- m.Monitor(ctx)
+	}()
+
+	select {
+	case <-success:
+		cancel()
+	case <-time.After(2 * time.Second):
+		t.Fatal("monitor did not retry quickly after initial query failure")
+	}
+
+	select {
+	case err := <-done:
+		if err != nil {
+			t.Fatalf("monitor returned unexpected error: %v", err)
+		}
+	case <-time.After(2 * time.Second):
+		t.Fatal("monitor did not exit after cancellation")
+	}
+
+	if got := atomic.LoadInt32(&calls); got < 2 {
+		t.Fatalf("expected at least 2 query attempts, got %d", got)
+	}
+}
+
+func TestCertLogSeverity(t *testing.T) {
+	tests := []struct {
+		name        string
+		untilAfter  time.Duration
+		critical    int
+		warning     int
+		wantLevel   string
+		wantMinDays int
+	}{
+		{
+			name:        "expired_subsecond",
+			untilAfter:  -565 * time.Millisecond,
+			critical:    7,
+			warning:     30,
+			wantLevel:   "expired",
+			wantMinDays: -1,
+		},
+		{
+			name:        "expired_one_hour",
+			untilAfter:  -1 * time.Hour,
+			critical:    7,
+			warning:     30,
+			wantLevel:   "expired",
+			wantMinDays: -1,
+		},
+		{
+			name:        "critical_threshold",
+			untilAfter:  6 * 24 * time.Hour,
+			critical:    7,
+			warning:     30,
+			wantLevel:   "critical",
+			wantMinDays: 6,
+		},
+		{
+			name:        "warning_threshold",
+			untilAfter:  10 * 24 * time.Hour,
+			critical:    7,
+			warning:     30,
+			wantLevel:   "warning",
+			wantMinDays: 10,
+		},
+		{
+			name:        "ok",
+			untilAfter:  60 * 24 * time.Hour,
+			critical:    7,
+			warning:     30,
+			wantLevel:   "ok",
+			wantMinDays: 60,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := certLogSeverity(tt.untilAfter, tt.critical, tt.warning); got != tt.wantLevel {
+				t.Fatalf("expected severity %q, got %q", tt.wantLevel, got)
+			}
+			if got := certDaysRemaining(tt.untilAfter); got < tt.wantMinDays {
+				t.Fatalf("expected days remaining >= %d, got %d", tt.wantMinDays, got)
+			}
+		})
+	}
+}