Skip to content

Commit 5a64f09

Browse files
zachsmith1zachsmith1
authored
Merge pull request #120 from datum-cloud/fix/shared-tls-annotation-cleanup
fix: resolve gateway annotation phantom-blocking and httpproxy reconciliation storm
2 parents b2d02fb + d8772e3 commit 5a64f09

File tree

4 files changed

+401
-5
lines changed

4 files changed

+401
-5
lines changed

‎internal/controller/gateway_controller.go‎

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,7 @@ func (r *GatewayReconciler) ensureDownstreamGateway(
235235

236236
if err := downstreamClient.Get(ctx, client.ObjectKeyFromObject(downstreamGateway), downstreamGateway); client.IgnoreNotFound(err) != nil {
237237
result.Err = fmt.Errorf("failed to get downstream gateway: %w", err)
238+
return result, nil
238239
}
239240

240241
verifiedHostnames, claimedHostnames, notClaimedHostnames, err := r.ensureHostnamesClaimed(
@@ -270,6 +271,9 @@ func (r *GatewayReconciler) ensureDownstreamGateway(
270271
return result, nil
271272
}
272273
} else {
274+
// Sync cert-manager annotations: add desired, remove stale
275+
syncCertManagerAnnotations(downstreamGateway, desiredDownstreamGateway)
276+
273277
if !equality.Semantic.DeepEqual(downstreamGateway.Annotations, desiredDownstreamGateway.Annotations) ||
274278
!equality.Semantic.DeepEqual(downstreamGateway.Spec, desiredDownstreamGateway.Spec) {
275279
// Take care not to clobber other annotations
@@ -358,6 +362,14 @@ func (r *GatewayReconciler) getDesiredDownstreamGateway(
358362
var listeners []gatewayv1.Listener
359363

360364
for listenerIndex, l := range upstreamGateway.Spec.Listeners {
365+
// Only process listeners with verified custom hostnames; unclaimed
366+
// hostnames must not influence cert-manager annotations or downstream
367+
// listener configuration.
368+
if l.Hostname != nil && !slices.Contains(claimedHostnames, string(*l.Hostname)) {
369+
logger.Info("skipping downstream gateway listener with unclaimed hostname", "upstream_listener_index", listenerIndex, "hostname", *l.Hostname)
370+
continue
371+
}
372+
361373
if l.TLS != nil && l.TLS.Options[certificateIssuerTLSOption] != "" {
362374
if r.Config.Gateway.PerGatewayCertificateIssuer {
363375
if !metav1.HasAnnotation(downstreamGateway.ObjectMeta, "cert-manager.io/issuer") {
@@ -388,12 +400,7 @@ func (r *GatewayReconciler) getDesiredDownstreamGateway(
388400
}
389401
}
390402

391-
// Add custom hostnames if they are verified
392403
if l.Hostname != nil {
393-
if !slices.Contains(claimedHostnames, string(*l.Hostname)) {
394-
logger.Info("skipping downstream gateway listener with unclaimed hostname", "upstream_listener_index", listenerIndex, "hostname", *l.Hostname)
395-
continue
396-
}
397404
listenerCopy := l.DeepCopy()
398405
if l.TLS != nil && l.TLS.Options[certificateIssuerTLSOption] != "" {
399406
// Translate upstream TLS settings to downstream TLS settings
@@ -426,6 +433,28 @@ func (r *GatewayReconciler) getDesiredDownstreamGateway(
426433
return &downstreamGateway
427434
}
428435

436+
// certManagerAnnotationKeys are the annotations managed by the gateway controller
437+
// for cert-manager integration. These are synced from the desired state and stale
438+
// entries are removed when custom hostnames become unclaimed.
439+
var certManagerAnnotationKeys = []string{
440+
"cert-manager.io/issuer",
441+
"cert-manager.io/cluster-issuer",
442+
"cert-manager.io/secret-template",
443+
}
444+
445+
// syncCertManagerAnnotations ensures the downstream gateway's cert-manager
446+
// annotations match the desired state: desired keys are set, and keys that
447+
// are no longer needed are removed.
448+
func syncCertManagerAnnotations(downstream, desired *gatewayv1.Gateway) {
449+
for _, key := range certManagerAnnotationKeys {
450+
if v, ok := desired.Annotations[key]; ok {
451+
metav1.SetMetaDataAnnotation(&downstream.ObjectMeta, key, v)
452+
} else {
453+
delete(downstream.Annotations, key)
454+
}
455+
}
456+
}
457+
429458
func (r *GatewayReconciler) reconcileGatewayStatus(
430459
upstreamClient client.Client,
431460
upstreamGateway *gatewayv1.Gateway,

‎internal/controller/gateway_controller_test.go‎

Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -807,6 +807,215 @@ func newGateway(
807807
return gw
808808
}
809809

810+
func TestGetDesiredDownstreamGateway_UnclaimedHostnameNoAnnotations(t *testing.T) {
811+
logger := zap.New(zap.UseFlagOptions(&zap.Options{Development: true}))
812+
ctx := log.IntoContext(context.Background(), logger)
813+
814+
tests := []struct {
815+
name string
816+
perGatewayCertIssuer bool
817+
listeners []gatewayv1.Listener
818+
claimedHostnames []string
819+
expectIssuerAnnotation bool
820+
expectClusterAnnotation bool
821+
expectListeners int
822+
}{
823+
{
824+
name: "unclaimed hostname with TLS option does not produce annotations",
825+
perGatewayCertIssuer: true,
826+
listeners: []gatewayv1.Listener{
827+
{
828+
Name: "custom-https",
829+
Port: 443,
830+
Protocol: gatewayv1.HTTPSProtocolType,
831+
Hostname: ptr.To(gatewayv1.Hostname("unclaimed.example.com")),
832+
TLS: &gatewayv1.GatewayTLSConfig{
833+
Options: map[gatewayv1.AnnotationKey]gatewayv1.AnnotationValue{
834+
certificateIssuerTLSOption: "letsencrypt",
835+
},
836+
},
837+
},
838+
},
839+
claimedHostnames: nil,
840+
expectIssuerAnnotation: false,
841+
expectClusterAnnotation: false,
842+
expectListeners: 0,
843+
},
844+
{
845+
name: "claimed hostname with TLS option produces issuer annotation (per-gateway mode)",
846+
perGatewayCertIssuer: true,
847+
listeners: []gatewayv1.Listener{
848+
{
849+
Name: "custom-https",
850+
Port: 443,
851+
Protocol: gatewayv1.HTTPSProtocolType,
852+
Hostname: ptr.To(gatewayv1.Hostname("claimed.example.com")),
853+
TLS: &gatewayv1.GatewayTLSConfig{
854+
Options: map[gatewayv1.AnnotationKey]gatewayv1.AnnotationValue{
855+
certificateIssuerTLSOption: "letsencrypt",
856+
},
857+
},
858+
},
859+
},
860+
claimedHostnames: []string{"claimed.example.com"},
861+
expectIssuerAnnotation: true,
862+
expectClusterAnnotation: false,
863+
expectListeners: 1,
864+
},
865+
{
866+
name: "claimed hostname with TLS option produces cluster-issuer annotation (cluster mode)",
867+
perGatewayCertIssuer: false,
868+
listeners: []gatewayv1.Listener{
869+
{
870+
Name: "custom-https",
871+
Port: 443,
872+
Protocol: gatewayv1.HTTPSProtocolType,
873+
Hostname: ptr.To(gatewayv1.Hostname("claimed.example.com")),
874+
TLS: &gatewayv1.GatewayTLSConfig{
875+
Options: map[gatewayv1.AnnotationKey]gatewayv1.AnnotationValue{
876+
certificateIssuerTLSOption: "letsencrypt",
877+
},
878+
},
879+
},
880+
},
881+
claimedHostnames: []string{"claimed.example.com"},
882+
expectIssuerAnnotation: false,
883+
expectClusterAnnotation: true,
884+
expectListeners: 1,
885+
},
886+
{
887+
name: "mix: only claimed hostname contributes annotations",
888+
perGatewayCertIssuer: true,
889+
listeners: []gatewayv1.Listener{
890+
{
891+
Name: "unclaimed-https",
892+
Port: 443,
893+
Protocol: gatewayv1.HTTPSProtocolType,
894+
Hostname: ptr.To(gatewayv1.Hostname("unclaimed.example.com")),
895+
TLS: &gatewayv1.GatewayTLSConfig{
896+
Options: map[gatewayv1.AnnotationKey]gatewayv1.AnnotationValue{
897+
certificateIssuerTLSOption: "letsencrypt",
898+
},
899+
},
900+
},
901+
{
902+
Name: "claimed-https",
903+
Port: 443,
904+
Protocol: gatewayv1.HTTPSProtocolType,
905+
Hostname: ptr.To(gatewayv1.Hostname("claimed.example.com")),
906+
TLS: &gatewayv1.GatewayTLSConfig{
907+
Options: map[gatewayv1.AnnotationKey]gatewayv1.AnnotationValue{
908+
certificateIssuerTLSOption: "letsencrypt",
909+
},
910+
},
911+
},
912+
},
913+
claimedHostnames: []string{"claimed.example.com"},
914+
expectIssuerAnnotation: true,
915+
expectClusterAnnotation: false,
916+
expectListeners: 1,
917+
},
918+
}
919+
920+
for _, tt := range tests {
921+
t.Run(tt.name, func(t *testing.T) {
922+
reconciler := &GatewayReconciler{
923+
Config: config.NetworkServicesOperator{
924+
Gateway: config.GatewayConfig{
925+
DownstreamGatewayClassName: "envoy",
926+
PerGatewayCertificateIssuer: tt.perGatewayCertIssuer,
927+
},
928+
},
929+
}
930+
931+
upstream := &gatewayv1.Gateway{
932+
ObjectMeta: metav1.ObjectMeta{Name: "test-gw", Namespace: "default"},
933+
Spec: gatewayv1.GatewaySpec{Listeners: tt.listeners},
934+
}
935+
936+
desired := reconciler.getDesiredDownstreamGateway(ctx, "test-cluster", upstream, tt.claimedHostnames)
937+
938+
_, hasIssuer := desired.Annotations["cert-manager.io/issuer"]
939+
_, hasClusterIssuer := desired.Annotations["cert-manager.io/cluster-issuer"]
940+
941+
assert.Equal(t, tt.expectIssuerAnnotation, hasIssuer, "cert-manager.io/issuer annotation presence")
942+
assert.Equal(t, tt.expectClusterAnnotation, hasClusterIssuer, "cert-manager.io/cluster-issuer annotation presence")
943+
assert.Len(t, desired.Spec.Listeners, tt.expectListeners, "downstream listener count")
944+
})
945+
}
946+
}
947+
948+
func TestSyncCertManagerAnnotations(t *testing.T) {
949+
t.Parallel()
950+
951+
tests := []struct {
952+
name string
953+
existingAnnotations map[string]string
954+
desiredAnnotations map[string]string
955+
wantAnnotations map[string]string
956+
}{
957+
{
958+
name: "stale issuer annotation removed",
959+
existingAnnotations: map[string]string{
960+
"cert-manager.io/issuer": "old-issuer",
961+
"other-annotation": "keep-me",
962+
},
963+
desiredAnnotations: map[string]string{},
964+
wantAnnotations: map[string]string{
965+
"other-annotation": "keep-me",
966+
},
967+
},
968+
{
969+
name: "issuer annotation updated to cluster-issuer",
970+
existingAnnotations: map[string]string{
971+
"cert-manager.io/issuer": "old-issuer",
972+
},
973+
desiredAnnotations: map[string]string{
974+
"cert-manager.io/cluster-issuer": "new-cluster-issuer",
975+
},
976+
wantAnnotations: map[string]string{
977+
"cert-manager.io/cluster-issuer": "new-cluster-issuer",
978+
},
979+
},
980+
{
981+
name: "no-op when annotations match",
982+
existingAnnotations: map[string]string{"cert-manager.io/issuer": "my-issuer"},
983+
desiredAnnotations: map[string]string{"cert-manager.io/issuer": "my-issuer"},
984+
wantAnnotations: map[string]string{"cert-manager.io/issuer": "my-issuer"},
985+
},
986+
{
987+
name: "all three cert-manager annotations cleaned up",
988+
existingAnnotations: map[string]string{
989+
"cert-manager.io/issuer": "old",
990+
"cert-manager.io/cluster-issuer": "old",
991+
"cert-manager.io/secret-template": "old",
992+
"unrelated": "preserved",
993+
},
994+
desiredAnnotations: map[string]string{},
995+
wantAnnotations: map[string]string{
996+
"unrelated": "preserved",
997+
},
998+
},
999+
}
1000+
1001+
for _, tt := range tests {
1002+
t.Run(tt.name, func(t *testing.T) {
1003+
t.Parallel()
1004+
1005+
existing := &gatewayv1.Gateway{
1006+
ObjectMeta: metav1.ObjectMeta{Annotations: tt.existingAnnotations},
1007+
}
1008+
desired := &gatewayv1.Gateway{
1009+
ObjectMeta: metav1.ObjectMeta{Annotations: tt.desiredAnnotations},
1010+
}
1011+
1012+
syncCertManagerAnnotations(existing, desired)
1013+
1014+
assert.Equal(t, tt.wantAnnotations, existing.Annotations)
1015+
})
1016+
}
1017+
}
1018+
8101019
func newHTTPRoute(namespace, name string, opts ...func(*gatewayv1.HTTPRoute)) *gatewayv1.HTTPRoute {
8111020
route := &gatewayv1.HTTPRoute{
8121021
ObjectMeta: metav1.ObjectMeta{

‎internal/controller/httpproxy_controller.go‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -485,7 +485,9 @@ func (r *HTTPProxyReconciler) reconcileHTTPProxyHostnameStatus(
485485
availabilityStatuses := buildAvailabilityStatuses(acceptedHostnames, inUseHostnames, httpProxyCopy.Generation)
486486
dnsStatuses := r.buildDNSStatuses(ctx, cl, gateway, httpProxyCopy.Generation)
487487
certificateStatuses := r.buildCertificateStatuses(ctx, cl, clusterName, gateway, httpProxyCopy)
488+
previousHostnameStatuses := httpProxyCopy.Status.HostnameStatuses
488489
httpProxyCopy.Status.HostnameStatuses = mergeHostnameStatuses(availabilityStatuses, dnsStatuses, certificateStatuses)
490+
preserveHostnameConditionTransitions(httpProxyCopy.Status.HostnameStatuses, previousHostnameStatuses)
489491

490492
r.setCertificatesReadyCondition(httpProxyCopy, certificateStatuses, gateway)
491493
}
@@ -1273,6 +1275,32 @@ func mergeHostnameStatuses(statusSets ...[]networkingv1alpha.HostnameStatus) []n
12731275
return result
12741276
}
12751277

1278+
// preserveHostnameConditionTransitions retains the original LastTransitionTime
1279+
// for hostname conditions whose Status has not changed. Without this, freshly-
1280+
// built HostnameStatus objects always get LastTransitionTime=now, which causes
1281+
// a spurious status diff on every reconcile and drives an infinite loop.
1282+
func preserveHostnameConditionTransitions(
1283+
newStatuses, oldStatuses []networkingv1alpha.HostnameStatus,
1284+
) {
1285+
oldByHostname := make(map[string]networkingv1alpha.HostnameStatus, len(oldStatuses))
1286+
for _, hs := range oldStatuses {
1287+
oldByHostname[hs.Hostname] = hs
1288+
}
1289+
1290+
for i, newHS := range newStatuses {
1291+
oldHS, ok := oldByHostname[newHS.Hostname]
1292+
if !ok {
1293+
continue
1294+
}
1295+
for j, newCond := range newHS.Conditions {
1296+
oldCond := apimeta.FindStatusCondition(oldHS.Conditions, newCond.Type)
1297+
if oldCond != nil && oldCond.Status == newCond.Status {
1298+
newStatuses[i].Conditions[j].LastTransitionTime = oldCond.LastTransitionTime
1299+
}
1300+
}
1301+
}
1302+
}
1303+
12761304
func (r *HTTPProxyReconciler) reconcileConnectorEnvoyPatchPolicy(
12771305
ctx context.Context,
12781306
upstreamClient client.Client,

0 commit comments

Comments
 (0)