From d8772e3a95bc9274462190313723dbeec0ca9c2a Mon Sep 17 00:00:00 2001 From: Zach Smith Date: Fri, 27 Feb 2026 11:08:21 -0800 Subject: [PATCH] fix: resolve gateway annotation phantom-blocking and httpproxy reconciliation storm Made-with: Cursor --- internal/controller/gateway_controller.go | 39 +++- .../controller/gateway_controller_test.go | 209 ++++++++++++++++++ internal/controller/httpproxy_controller.go | 28 +++ .../controller/httpproxy_controller_test.go | 130 +++++++++++ 4 files changed, 401 insertions(+), 5 deletions(-) diff --git a/internal/controller/gateway_controller.go b/internal/controller/gateway_controller.go index e73b7c8e..17a3619f 100644 --- a/internal/controller/gateway_controller.go +++ b/internal/controller/gateway_controller.go @@ -235,6 +235,7 @@ func (r *GatewayReconciler) ensureDownstreamGateway( if err := downstreamClient.Get(ctx, client.ObjectKeyFromObject(downstreamGateway), downstreamGateway); client.IgnoreNotFound(err) != nil { result.Err = fmt.Errorf("failed to get downstream gateway: %w", err) + return result, nil } verifiedHostnames, claimedHostnames, notClaimedHostnames, err := r.ensureHostnamesClaimed( @@ -270,6 +271,9 @@ func (r *GatewayReconciler) ensureDownstreamGateway( return result, nil } } else { + // Sync cert-manager annotations: add desired, remove stale + syncCertManagerAnnotations(downstreamGateway, desiredDownstreamGateway) + if !equality.Semantic.DeepEqual(downstreamGateway.Annotations, desiredDownstreamGateway.Annotations) || !equality.Semantic.DeepEqual(downstreamGateway.Spec, desiredDownstreamGateway.Spec) { // Take care not to clobber other annotations @@ -358,6 +362,14 @@ func (r *GatewayReconciler) getDesiredDownstreamGateway( var listeners []gatewayv1.Listener for listenerIndex, l := range upstreamGateway.Spec.Listeners { + // Only process listeners with verified custom hostnames; unclaimed + // hostnames must not influence cert-manager annotations or downstream + // listener configuration. + if l.Hostname != nil && !slices.Contains(claimedHostnames, string(*l.Hostname)) { + logger.Info("skipping downstream gateway listener with unclaimed hostname", "upstream_listener_index", listenerIndex, "hostname", *l.Hostname) + continue + } + if l.TLS != nil && l.TLS.Options[certificateIssuerTLSOption] != "" { if r.Config.Gateway.PerGatewayCertificateIssuer { if !metav1.HasAnnotation(downstreamGateway.ObjectMeta, "cert-manager.io/issuer") { @@ -388,12 +400,7 @@ func (r *GatewayReconciler) getDesiredDownstreamGateway( } } - // Add custom hostnames if they are verified if l.Hostname != nil { - if !slices.Contains(claimedHostnames, string(*l.Hostname)) { - logger.Info("skipping downstream gateway listener with unclaimed hostname", "upstream_listener_index", listenerIndex, "hostname", *l.Hostname) - continue - } listenerCopy := l.DeepCopy() if l.TLS != nil && l.TLS.Options[certificateIssuerTLSOption] != "" { // Translate upstream TLS settings to downstream TLS settings @@ -426,6 +433,28 @@ func (r *GatewayReconciler) getDesiredDownstreamGateway( return &downstreamGateway } +// certManagerAnnotationKeys are the annotations managed by the gateway controller +// for cert-manager integration. These are synced from the desired state and stale +// entries are removed when custom hostnames become unclaimed. +var certManagerAnnotationKeys = []string{ + "cert-manager.io/issuer", + "cert-manager.io/cluster-issuer", + "cert-manager.io/secret-template", +} + +// syncCertManagerAnnotations ensures the downstream gateway's cert-manager +// annotations match the desired state: desired keys are set, and keys that +// are no longer needed are removed. +func syncCertManagerAnnotations(downstream, desired *gatewayv1.Gateway) { + for _, key := range certManagerAnnotationKeys { + if v, ok := desired.Annotations[key]; ok { + metav1.SetMetaDataAnnotation(&downstream.ObjectMeta, key, v) + } else { + delete(downstream.Annotations, key) + } + } +} + func (r *GatewayReconciler) reconcileGatewayStatus( upstreamClient client.Client, upstreamGateway *gatewayv1.Gateway, diff --git a/internal/controller/gateway_controller_test.go b/internal/controller/gateway_controller_test.go index 58fcdecb..1e653acf 100644 --- a/internal/controller/gateway_controller_test.go +++ b/internal/controller/gateway_controller_test.go @@ -807,6 +807,215 @@ func newGateway( return gw } +func TestGetDesiredDownstreamGateway_UnclaimedHostnameNoAnnotations(t *testing.T) { + logger := zap.New(zap.UseFlagOptions(&zap.Options{Development: true})) + ctx := log.IntoContext(context.Background(), logger) + + tests := []struct { + name string + perGatewayCertIssuer bool + listeners []gatewayv1.Listener + claimedHostnames []string + expectIssuerAnnotation bool + expectClusterAnnotation bool + expectListeners int + }{ + { + name: "unclaimed hostname with TLS option does not produce annotations", + perGatewayCertIssuer: true, + listeners: []gatewayv1.Listener{ + { + Name: "custom-https", + Port: 443, + Protocol: gatewayv1.HTTPSProtocolType, + Hostname: ptr.To(gatewayv1.Hostname("unclaimed.example.com")), + TLS: &gatewayv1.GatewayTLSConfig{ + Options: map[gatewayv1.AnnotationKey]gatewayv1.AnnotationValue{ + certificateIssuerTLSOption: "letsencrypt", + }, + }, + }, + }, + claimedHostnames: nil, + expectIssuerAnnotation: false, + expectClusterAnnotation: false, + expectListeners: 0, + }, + { + name: "claimed hostname with TLS option produces issuer annotation (per-gateway mode)", + perGatewayCertIssuer: true, + listeners: []gatewayv1.Listener{ + { + Name: "custom-https", + Port: 443, + Protocol: gatewayv1.HTTPSProtocolType, + Hostname: ptr.To(gatewayv1.Hostname("claimed.example.com")), + TLS: &gatewayv1.GatewayTLSConfig{ + Options: map[gatewayv1.AnnotationKey]gatewayv1.AnnotationValue{ + certificateIssuerTLSOption: "letsencrypt", + }, + }, + }, + }, + claimedHostnames: []string{"claimed.example.com"}, + expectIssuerAnnotation: true, + expectClusterAnnotation: false, + expectListeners: 1, + }, + { + name: "claimed hostname with TLS option produces cluster-issuer annotation (cluster mode)", + perGatewayCertIssuer: false, + listeners: []gatewayv1.Listener{ + { + Name: "custom-https", + Port: 443, + Protocol: gatewayv1.HTTPSProtocolType, + Hostname: ptr.To(gatewayv1.Hostname("claimed.example.com")), + TLS: &gatewayv1.GatewayTLSConfig{ + Options: map[gatewayv1.AnnotationKey]gatewayv1.AnnotationValue{ + certificateIssuerTLSOption: "letsencrypt", + }, + }, + }, + }, + claimedHostnames: []string{"claimed.example.com"}, + expectIssuerAnnotation: false, + expectClusterAnnotation: true, + expectListeners: 1, + }, + { + name: "mix: only claimed hostname contributes annotations", + perGatewayCertIssuer: true, + listeners: []gatewayv1.Listener{ + { + Name: "unclaimed-https", + Port: 443, + Protocol: gatewayv1.HTTPSProtocolType, + Hostname: ptr.To(gatewayv1.Hostname("unclaimed.example.com")), + TLS: &gatewayv1.GatewayTLSConfig{ + Options: map[gatewayv1.AnnotationKey]gatewayv1.AnnotationValue{ + certificateIssuerTLSOption: "letsencrypt", + }, + }, + }, + { + Name: "claimed-https", + Port: 443, + Protocol: gatewayv1.HTTPSProtocolType, + Hostname: ptr.To(gatewayv1.Hostname("claimed.example.com")), + TLS: &gatewayv1.GatewayTLSConfig{ + Options: map[gatewayv1.AnnotationKey]gatewayv1.AnnotationValue{ + certificateIssuerTLSOption: "letsencrypt", + }, + }, + }, + }, + claimedHostnames: []string{"claimed.example.com"}, + expectIssuerAnnotation: true, + expectClusterAnnotation: false, + expectListeners: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reconciler := &GatewayReconciler{ + Config: config.NetworkServicesOperator{ + Gateway: config.GatewayConfig{ + DownstreamGatewayClassName: "envoy", + PerGatewayCertificateIssuer: tt.perGatewayCertIssuer, + }, + }, + } + + upstream := &gatewayv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{Name: "test-gw", Namespace: "default"}, + Spec: gatewayv1.GatewaySpec{Listeners: tt.listeners}, + } + + desired := reconciler.getDesiredDownstreamGateway(ctx, "test-cluster", upstream, tt.claimedHostnames) + + _, hasIssuer := desired.Annotations["cert-manager.io/issuer"] + _, hasClusterIssuer := desired.Annotations["cert-manager.io/cluster-issuer"] + + assert.Equal(t, tt.expectIssuerAnnotation, hasIssuer, "cert-manager.io/issuer annotation presence") + assert.Equal(t, tt.expectClusterAnnotation, hasClusterIssuer, "cert-manager.io/cluster-issuer annotation presence") + assert.Len(t, desired.Spec.Listeners, tt.expectListeners, "downstream listener count") + }) + } +} + +func TestSyncCertManagerAnnotations(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + existingAnnotations map[string]string + desiredAnnotations map[string]string + wantAnnotations map[string]string + }{ + { + name: "stale issuer annotation removed", + existingAnnotations: map[string]string{ + "cert-manager.io/issuer": "old-issuer", + "other-annotation": "keep-me", + }, + desiredAnnotations: map[string]string{}, + wantAnnotations: map[string]string{ + "other-annotation": "keep-me", + }, + }, + { + name: "issuer annotation updated to cluster-issuer", + existingAnnotations: map[string]string{ + "cert-manager.io/issuer": "old-issuer", + }, + desiredAnnotations: map[string]string{ + "cert-manager.io/cluster-issuer": "new-cluster-issuer", + }, + wantAnnotations: map[string]string{ + "cert-manager.io/cluster-issuer": "new-cluster-issuer", + }, + }, + { + name: "no-op when annotations match", + existingAnnotations: map[string]string{"cert-manager.io/issuer": "my-issuer"}, + desiredAnnotations: map[string]string{"cert-manager.io/issuer": "my-issuer"}, + wantAnnotations: map[string]string{"cert-manager.io/issuer": "my-issuer"}, + }, + { + name: "all three cert-manager annotations cleaned up", + existingAnnotations: map[string]string{ + "cert-manager.io/issuer": "old", + "cert-manager.io/cluster-issuer": "old", + "cert-manager.io/secret-template": "old", + "unrelated": "preserved", + }, + desiredAnnotations: map[string]string{}, + wantAnnotations: map[string]string{ + "unrelated": "preserved", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + existing := &gatewayv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{Annotations: tt.existingAnnotations}, + } + desired := &gatewayv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{Annotations: tt.desiredAnnotations}, + } + + syncCertManagerAnnotations(existing, desired) + + assert.Equal(t, tt.wantAnnotations, existing.Annotations) + }) + } +} + func newHTTPRoute(namespace, name string, opts ...func(*gatewayv1.HTTPRoute)) *gatewayv1.HTTPRoute { route := &gatewayv1.HTTPRoute{ ObjectMeta: metav1.ObjectMeta{ diff --git a/internal/controller/httpproxy_controller.go b/internal/controller/httpproxy_controller.go index 5e26b7b2..c2eaa08b 100644 --- a/internal/controller/httpproxy_controller.go +++ b/internal/controller/httpproxy_controller.go @@ -485,7 +485,9 @@ func (r *HTTPProxyReconciler) reconcileHTTPProxyHostnameStatus( availabilityStatuses := buildAvailabilityStatuses(acceptedHostnames, inUseHostnames, httpProxyCopy.Generation) dnsStatuses := r.buildDNSStatuses(ctx, cl, gateway, httpProxyCopy.Generation) certificateStatuses := r.buildCertificateStatuses(ctx, cl, clusterName, gateway, httpProxyCopy) + previousHostnameStatuses := httpProxyCopy.Status.HostnameStatuses httpProxyCopy.Status.HostnameStatuses = mergeHostnameStatuses(availabilityStatuses, dnsStatuses, certificateStatuses) + preserveHostnameConditionTransitions(httpProxyCopy.Status.HostnameStatuses, previousHostnameStatuses) r.setCertificatesReadyCondition(httpProxyCopy, certificateStatuses, gateway) } @@ -1273,6 +1275,32 @@ func mergeHostnameStatuses(statusSets ...[]networkingv1alpha.HostnameStatus) []n return result } +// preserveHostnameConditionTransitions retains the original LastTransitionTime +// for hostname conditions whose Status has not changed. Without this, freshly- +// built HostnameStatus objects always get LastTransitionTime=now, which causes +// a spurious status diff on every reconcile and drives an infinite loop. +func preserveHostnameConditionTransitions( + newStatuses, oldStatuses []networkingv1alpha.HostnameStatus, +) { + oldByHostname := make(map[string]networkingv1alpha.HostnameStatus, len(oldStatuses)) + for _, hs := range oldStatuses { + oldByHostname[hs.Hostname] = hs + } + + for i, newHS := range newStatuses { + oldHS, ok := oldByHostname[newHS.Hostname] + if !ok { + continue + } + for j, newCond := range newHS.Conditions { + oldCond := apimeta.FindStatusCondition(oldHS.Conditions, newCond.Type) + if oldCond != nil && oldCond.Status == newCond.Status { + newStatuses[i].Conditions[j].LastTransitionTime = oldCond.LastTransitionTime + } + } + } +} + func (r *HTTPProxyReconciler) reconcileConnectorEnvoyPatchPolicy( ctx context.Context, upstreamClient client.Client, diff --git a/internal/controller/httpproxy_controller_test.go b/internal/controller/httpproxy_controller_test.go index c930e4cf..a58c6122 100644 --- a/internal/controller/httpproxy_controller_test.go +++ b/internal/controller/httpproxy_controller_test.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "testing" + "time" envoygatewayv1alpha1 "github.com/envoyproxy/gateway/api/v1alpha1" "github.com/stretchr/testify/assert" @@ -2306,3 +2307,132 @@ func TestSetCertificatesReadyCondition(t *testing.T) { }) } } + +func TestPreserveHostnameConditionTransitions(t *testing.T) { + t.Parallel() + + oldTime := metav1.NewTime(metav1.Now().Add(-1 * time.Hour)) + freshTime := metav1.Now() + + tests := []struct { + name string + oldStatuses []networkingv1alpha.HostnameStatus + newStatuses []networkingv1alpha.HostnameStatus + wantTimes map[string]metav1.Time // hostname -> expected LastTransitionTime for Available condition + }{ + { + name: "no old statuses — fresh timestamps kept", + oldStatuses: nil, + newStatuses: []networkingv1alpha.HostnameStatus{ + makeHostnameStatus("a.example.com", metav1.ConditionTrue, freshTime), + }, + wantTimes: map[string]metav1.Time{ + "a.example.com": freshTime, + }, + }, + { + name: "status unchanged — old timestamp preserved", + oldStatuses: []networkingv1alpha.HostnameStatus{ + makeHostnameStatus("a.example.com", metav1.ConditionTrue, oldTime), + }, + newStatuses: []networkingv1alpha.HostnameStatus{ + makeHostnameStatus("a.example.com", metav1.ConditionTrue, freshTime), + }, + wantTimes: map[string]metav1.Time{ + "a.example.com": oldTime, + }, + }, + { + name: "status transitioned — fresh timestamp used", + oldStatuses: []networkingv1alpha.HostnameStatus{ + makeHostnameStatus("a.example.com", metav1.ConditionFalse, oldTime), + }, + newStatuses: []networkingv1alpha.HostnameStatus{ + makeHostnameStatus("a.example.com", metav1.ConditionTrue, freshTime), + }, + wantTimes: map[string]metav1.Time{ + "a.example.com": freshTime, + }, + }, + { + name: "new hostname not in old — fresh timestamp kept", + oldStatuses: []networkingv1alpha.HostnameStatus{ + makeHostnameStatus("a.example.com", metav1.ConditionTrue, oldTime), + }, + newStatuses: []networkingv1alpha.HostnameStatus{ + makeHostnameStatus("a.example.com", metav1.ConditionTrue, freshTime), + makeHostnameStatus("b.example.com", metav1.ConditionTrue, freshTime), + }, + wantTimes: map[string]metav1.Time{ + "a.example.com": oldTime, + "b.example.com": freshTime, + }, + }, + { + name: "multiple conditions — each preserved independently", + oldStatuses: []networkingv1alpha.HostnameStatus{ + { + Hostname: "a.example.com", + Conditions: []metav1.Condition{ + {Type: networkingv1alpha.HostnameConditionAvailable, Status: metav1.ConditionTrue, LastTransitionTime: oldTime}, + {Type: networkingv1alpha.HostnameConditionCertificateReady, Status: metav1.ConditionFalse, LastTransitionTime: oldTime}, + }, + }, + }, + newStatuses: []networkingv1alpha.HostnameStatus{ + { + Hostname: "a.example.com", + Conditions: []metav1.Condition{ + {Type: networkingv1alpha.HostnameConditionAvailable, Status: metav1.ConditionTrue, LastTransitionTime: freshTime}, + {Type: networkingv1alpha.HostnameConditionCertificateReady, Status: metav1.ConditionTrue, LastTransitionTime: freshTime}, + }, + }, + }, + wantTimes: map[string]metav1.Time{ + "a.example.com/Available": oldTime, + "a.example.com/CertificateReady": freshTime, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + preserveHostnameConditionTransitions(tt.newStatuses, tt.oldStatuses) + + for _, hs := range tt.newStatuses { + for _, cond := range hs.Conditions { + key := hs.Hostname + if len(tt.wantTimes) > 0 { + // Check for compound key first (hostname/condType) + if _, ok := tt.wantTimes[hs.Hostname+"/"+cond.Type]; ok { + key = hs.Hostname + "/" + cond.Type + } + } + wantTime, ok := tt.wantTimes[key] + if !ok { + continue + } + assert.Equal(t, wantTime, cond.LastTransitionTime, + "hostname %q condition %q: expected LastTransitionTime to be preserved/updated correctly", + hs.Hostname, cond.Type) + } + } + }) + } +} + +func makeHostnameStatus(hostname string, status metav1.ConditionStatus, transitionTime metav1.Time) networkingv1alpha.HostnameStatus { + return networkingv1alpha.HostnameStatus{ + Hostname: hostname, + Conditions: []metav1.Condition{ + { + Type: networkingv1alpha.HostnameConditionAvailable, + Status: status, + Reason: "TestReason", + LastTransitionTime: transitionTime, + }, + }, + } +}