Adds logic to fix panic if external controller removes all annotations

hashicorp · Feb 13, 2024 · 76acb71 · 76acb71
1 parent a3ac615
commit 76acb71
Show file tree

Hide file tree

Showing 2 changed files with 125 additions and 17 deletions.
diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go
@@ -45,13 +45,16 @@ var (
 	}
 
 	// These annotations are used for testing that annotations stay on the service after reconcile.
+	copyAnnotationKey = "copy-this-annotation"
+	copyAnnotations   = map[string]string{
+		copyAnnotationKey: "copy-this-annotation-value",
+	}
 	externalAnnotations = map[string]string{
 		"external-annotation": "external-annotation-value",
 	}
-	copyAnnotations            = []string{"copy-this-annotation"}
 	externalAndCopyAnnotations = map[string]string{
-		"external-annotation":  "external-annotation-value",
-		"copy-this-annotation": "copy-this-annotation-value",
+		"external-annotation": "external-annotation-value",
+		copyAnnotationKey:     "copy-this-annotation-value",
 	}
 
 	listeners = []gwv1beta1.Listener{
@@ -610,7 +613,7 @@ func TestUpsert(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{
 					Name:        name,
 					Namespace:   namespace,
-					Annotations: map[string]string{copyAnnotations[0]: "copy-this-annotation-value"},
+					Annotations: copyAnnotations,
 				},
 				Spec: gwv1beta1.GatewaySpec{
 					Listeners: listeners,
@@ -626,7 +629,7 @@ func TestUpsert(t *testing.T) {
 						MaxInstances:     common.PointerTo(int32(7)),
 						MinInstances:     common.PointerTo(int32(1)),
 					},
-					CopyAnnotations: v1alpha1.CopyAnnotationsSpec{Service: copyAnnotations},
+					CopyAnnotations: v1alpha1.CopyAnnotationsSpec{Service: []string{copyAnnotationKey}},
 					ServiceType:     (*corev1.ServiceType)(common.PointerTo("NodePort")),
 				},
 			},
@@ -674,6 +677,75 @@ func TestUpsert(t *testing.T) {
 			},
 			ignoreTimestampOnService: true,
 		},
+		"updating a gateway that has copy-annotations doesn't panic if another controller has removed them all": {
+			gateway: gwv1beta1.Gateway{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:        name,
+					Namespace:   namespace,
+					Annotations: copyAnnotations,
+				},
+				Spec: gwv1beta1.GatewaySpec{
+					Listeners: listeners,
+				},
+			},
+			gatewayClassConfig: v1alpha1.GatewayClassConfig{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "consul-gatewayclassconfig",
+				},
+				Spec: v1alpha1.GatewayClassConfigSpec{
+					DeploymentSpec: v1alpha1.DeploymentSpec{
+						DefaultInstances: common.PointerTo(int32(5)),
+						MaxInstances:     common.PointerTo(int32(7)),
+						MinInstances:     common.PointerTo(int32(1)),
+					},
+					CopyAnnotations: v1alpha1.CopyAnnotationsSpec{Service: []string{copyAnnotationKey}},
+					ServiceType:     (*corev1.ServiceType)(common.PointerTo("NodePort")),
+				},
+			},
+			helmConfig: common.HelmConfig{
+				ImageDataplane: dataplaneImage,
+			},
+			initialResources: resources{
+				services: []*corev1.Service{
+					configureService(name, namespace, labels, nil, (corev1.ServiceType)("NodePort"), []corev1.ServicePort{
+						{
+							Name:       "Listener 1",
+							Protocol:   "TCP",
+							Port:       8080,
+							TargetPort: intstr.FromInt(8080),
+						},
+						{
+							Name:       "Listener 2",
+							Protocol:   "TCP",
+							Port:       8081,
+							TargetPort: intstr.FromInt(8081),
+						},
+					}, "1", true, false),
+				},
+			},
+			finalResources: resources{
+				deployments: []*appsv1.Deployment{},
+				roles:       []*rbac.Role{},
+				services: []*corev1.Service{
+					configureService(name, namespace, labels, copyAnnotations, (corev1.ServiceType)("NodePort"), []corev1.ServicePort{
+						{
+							Name:       "Listener 1",
+							Protocol:   "TCP",
+							Port:       8080,
+							TargetPort: intstr.FromInt(8080),
+						},
+						{
+							Name:       "Listener 2",
+							Protocol:   "TCP",
+							Port:       8081,
+							TargetPort: intstr.FromInt(8081),
+						},
+					}, "2", false, false),
+				},
+				serviceAccounts: []*corev1.ServiceAccount{},
+			},
+			ignoreTimestampOnService: true,
+		},
 		"update a gateway deployment by scaling it when no min or max number of instances is defined on the GatewayClassConfig": {
 			gateway: gwv1beta1.Gateway{
 				ObjectMeta: metav1.ObjectMeta{

diff --git a/control-plane/api-gateway/gatekeeper/service.go b/control-plane/api-gateway/gatekeeper/service.go
@@ -113,30 +113,66 @@ func (g *Gatekeeper) service(gateway gwv1beta1.Gateway, gcc v1alpha1.GatewayClas
 // mergeService is used to keep annotations and ports from the `existing` Service
 // to the `desired` service. This prevents an infinite reconciliation loop when
 // Kubernetes adds this configuration back in.
-func mergeService(existing, desired *corev1.Service) {
+func mergeServiceInto(existing, desired *corev1.Service) {
+	duplicate := existing.DeepCopy()
 
-	// Only overwrite fields if the Service doesn't exist yet
-	if existing.ObjectMeta.CreationTimestamp.IsZero() {
-		existing.ObjectMeta.OwnerReferences = desired.ObjectMeta.OwnerReferences
+	// Reset the existing object in kubernetes to have the same base spec as
+	// our generated service.
+	existing.Spec = desired.Spec
+
+	// For NodePort services, kubernetes will internally set the ports[*].NodePort
+	// we don't want to override that, so reset it to what exists in the store.
+	if hasEqualPorts(duplicate, desired) {
+		existing.Spec.Ports = duplicate.Spec.Ports
+	}
+
+	// If the Service already exists, add any desired annotations + labels to existing set
+
+	// Note: the annotations could be empty if an external controller decided to remove them all
+	// do not want to panic in that case.
+	if existing.ObjectMeta.Annotations == nil {
 		existing.Annotations = desired.Annotations
+	} else {
+		for k, v := range desired.ObjectMeta.Annotations {
+			existing.ObjectMeta.Annotations[k] = v
+		}
+	}
+
+	// Note: the labels could be empty if an external controller decided to remove them all
+	// do not want to panic in that case.
+	if existing.ObjectMeta.Labels == nil {
 		existing.Labels = desired.Labels
-		return
+	} else {
+		for k, v := range desired.ObjectMeta.Labels {
+			existing.ObjectMeta.Labels[k] = v
+		}
 	}
 
-	existing.Spec.Ports = desired.Spec.Ports
+	return
+}
 
-	// If the Service already exists, add any desired annotations + labels to existing set
-	for k, v := range desired.ObjectMeta.Annotations {
-		existing.ObjectMeta.Annotations[k] = v
+// hasEqualPorts does a fuzzy comparison of the ports on a service spec
+// ignoring any fields set internally by Kubernetes.
+func hasEqualPorts(a, b *corev1.Service) bool {
+	if len(b.Spec.Ports) != len(a.Spec.Ports) {
+		return false
 	}
-	for k, v := range desired.ObjectMeta.Labels {
-		existing.ObjectMeta.Labels[k] = v
+
+	for i, port := range a.Spec.Ports {
+		otherPort := b.Spec.Ports[i]
+		if port.Port != otherPort.Port {
+			return false
+		}
+		if port.Protocol != otherPort.Protocol {
+			return false
+		}
 	}
+	return true
 }
 
 func newServiceMutator(existing, desired *corev1.Service, gateway gwv1beta1.Gateway, scheme *runtime.Scheme) resourceMutator {
 	return func() error {
-		mergeService(existing, desired)
+		mergeServiceInto(existing, desired)
 		return ctrl.SetControllerReference(&gateway, existing, scheme)
 	}
 }