From 2c464090f8bc0bc5962b945ee1a5eac90ae3df5c Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Tue, 13 Feb 2024 14:50:54 -0500 Subject: [PATCH] Fix logic on apigateway that ignores current annotations on services (#3597) --- .changelog/3597.txt | 3 + .../api-gateway/gatekeeper/gatekeeper_test.go | 209 ++++++++++++++++-- .../api-gateway/gatekeeper/service.go | 64 ++++-- 3 files changed, 239 insertions(+), 37 deletions(-) create mode 100644 .changelog/3597.txt diff --git a/.changelog/3597.txt b/.changelog/3597.txt new file mode 100644 index 0000000000..bcd0dafe8b --- /dev/null +++ b/.changelog/3597.txt @@ -0,0 +1,3 @@ +```release-note:bug +api-gateway: fix issue where external annotations and labels are being incorrectly deleted on services controlled by the API Gateway +``` \ No newline at end of file diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go index 8e212344b5..8c0d842d49 100644 --- a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go +++ b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go @@ -35,13 +35,28 @@ var ( dataplaneImage = "hashicorp/consul-dataplane" name = "test" namespace = "default" - labels = map[string]string{ + + labels = map[string]string{ "component": "api-gateway", "gateway.consul.hashicorp.com/name": name, "gateway.consul.hashicorp.com/namespace": namespace, createdAtLabelKey: createdAtLabelValue, "gateway.consul.hashicorp.com/managed": "true", } + + // 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", + } + externalAndCopyAnnotations = map[string]string{ + "external-annotation": "external-annotation-value", + copyAnnotationKey: "copy-this-annotation-value", + } + listeners = []gwv1beta1.Listener{ { Name: "Listener 1", @@ -70,6 +85,10 @@ type testCase struct { initialResources resources finalResources resources + + // This is used to ignore the timestamp on the service when comparing the final resources + // This is useful for testing an update on a service + ignoreTimestampOnService bool } type resources struct { @@ -190,7 +209,7 @@ func TestUpsert(t *testing.T) { Port: 8080, TargetPort: intstr.FromInt(8080), }, - }, "1"), + }, "1", false, false), }, serviceAccounts: []*corev1.ServiceAccount{}, }, @@ -242,7 +261,7 @@ func TestUpsert(t *testing.T) { Port: 8081, TargetPort: intstr.FromInt(8081), }, - }, "1"), + }, "1", false, false), }, serviceAccounts: []*corev1.ServiceAccount{}, }, @@ -300,7 +319,7 @@ func TestUpsert(t *testing.T) { Port: 8081, TargetPort: intstr.FromInt(8081), }, - }, "1"), + }, "1", false, false), }, serviceAccounts: []*corev1.ServiceAccount{ configureServiceAccount(name, namespace, labels, "1"), @@ -426,7 +445,7 @@ func TestUpsert(t *testing.T) { Protocol: "TCP", Port: 8080, }, - }, "1"), + }, "1", true, false), }, serviceAccounts: []*corev1.ServiceAccount{ configureServiceAccount(name, namespace, labels, "1"), @@ -456,12 +475,13 @@ func TestUpsert(t *testing.T) { Port: 8081, TargetPort: intstr.FromInt(8081), }, - }, "2"), + }, "2", false, false), }, serviceAccounts: []*corev1.ServiceAccount{ configureServiceAccount(name, namespace, labels, "1"), }, }, + ignoreTimestampOnService: true, }, "update a gateway, removing a listener from a service": { gateway: gwv1beta1.Gateway{ @@ -515,7 +535,7 @@ func TestUpsert(t *testing.T) { Protocol: "TCP", Port: 8081, }, - }, "1"), + }, "1", true, false), }, serviceAccounts: []*corev1.ServiceAccount{ configureServiceAccount(name, namespace, labels, "1"), @@ -539,12 +559,13 @@ func TestUpsert(t *testing.T) { Port: 8080, TargetPort: intstr.FromInt(8080), }, - }, "2"), + }, "2", false, false), }, serviceAccounts: []*corev1.ServiceAccount{ configureServiceAccount(name, namespace, labels, "1"), }, }, + ignoreTimestampOnService: true, }, "updating a gateway deployment respects the number of replicas a user has set": { gateway: gwv1beta1.Gateway{ @@ -587,6 +608,144 @@ func TestUpsert(t *testing.T) { serviceAccounts: []*corev1.ServiceAccount{}, }, }, + "updating a gateway deployment respects the labels and annotations a user has set": { + 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, externalAnnotations, (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, externalAndCopyAnnotations, (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, + }, + "updating a gateway that has copy-annotations and labels 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, nil, 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{ @@ -775,7 +934,7 @@ func TestUpsert(t *testing.T) { err := gatekeeper.Upsert(context.Background(), tc.gateway, tc.gatewayClassConfig, tc.helmConfig) require.NoError(t, err) - require.NoError(t, validateResourcesExist(t, client, tc.helmConfig, tc.finalResources)) + require.NoError(t, validateResourcesExist(t, client, tc.helmConfig, tc.finalResources, tc.ignoreTimestampOnService)) }) } } @@ -867,7 +1026,7 @@ func TestDelete(t *testing.T) { Protocol: "TCP", Port: 8081, }, - }, "1"), + }, "1", true, false), }, serviceAccounts: []*corev1.ServiceAccount{}, }, @@ -928,7 +1087,7 @@ func TestDelete(t *testing.T) { Protocol: "TCP", Port: 8081, }, - }, "1"), + }, "1", true, false), }, serviceAccounts: []*corev1.ServiceAccount{ configureServiceAccount(name, namespace, labels, "1"), @@ -964,7 +1123,7 @@ func TestDelete(t *testing.T) { Name: tc.gateway.Name, }) require.NoError(t, err) - require.NoError(t, validateResourcesExist(t, client, tc.helmConfig, tc.finalResources)) + require.NoError(t, validateResourcesExist(t, client, tc.helmConfig, tc.finalResources, false)) require.NoError(t, validateResourcesAreDeleted(t, client, tc.initialResources)) }) } @@ -994,7 +1153,7 @@ func joinResources(resources resources) (objs []client.Object) { return objs } -func validateResourcesExist(t *testing.T, client client.Client, helmConfig common.HelmConfig, resources resources) error { +func validateResourcesExist(t *testing.T, client client.Client, helmConfig common.HelmConfig, resources resources, ignoreTimestampOnService bool) error { t.Helper() for _, expected := range resources.deployments { @@ -1101,6 +1260,10 @@ func validateResourcesExist(t *testing.T, client client.Client, helmConfig commo actual.Labels[createdAtLabelKey] = createdAtLabelValue actual.Spec.Selector[createdAtLabelKey] = createdAtLabelValue + if ignoreTimestampOnService { + expected.CreationTimestamp = actual.CreationTimestamp + } + require.Equal(t, expected, actual) } @@ -1322,8 +1485,16 @@ func configureRoleBinding(name, namespace string, labels map[string]string, reso } } -func configureService(name, namespace string, labels, annotations map[string]string, serviceType corev1.ServiceType, ports []corev1.ServicePort, resourceVersion string) *corev1.Service { - return &corev1.Service{ +func configureService(name, namespace string, labels, annotations map[string]string, serviceType corev1.ServiceType, ports []corev1.ServicePort, resourceVersion string, isInitialResource, addExternalLabel bool) *corev1.Service { + + // This is used only to test that any external labels added to the service + // are not removed on reconcile + combinedLabels := labels + if addExternalLabel { + combinedLabels["extra-label"] = "extra-label-value" + } + + service := corev1.Service{ TypeMeta: metav1.TypeMeta{ APIVersion: "v1", Kind: "Service", @@ -1331,7 +1502,7 @@ func configureService(name, namespace string, labels, annotations map[string]str ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: namespace, - Labels: labels, + Labels: combinedLabels, Annotations: annotations, ResourceVersion: resourceVersion, OwnerReferences: []metav1.OwnerReference{ @@ -1350,6 +1521,12 @@ func configureService(name, namespace string, labels, annotations map[string]str Ports: ports, }, } + + if isInitialResource { + service.ObjectMeta.CreationTimestamp = metav1.Now() + } + + return &service } func configureServiceAccount(name, namespace string, labels map[string]string, resourceVersion string) *corev1.ServiceAccount { diff --git a/control-plane/api-gateway/gatekeeper/service.go b/control-plane/api-gateway/gatekeeper/service.go index a30a3df89f..e78afc239b 100644 --- a/control-plane/api-gateway/gatekeeper/service.go +++ b/control-plane/api-gateway/gatekeeper/service.go @@ -5,13 +5,11 @@ package gatekeeper import ( "context" - "github.com/hashicorp/consul-k8s/control-plane/api-gateway/common" "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" "k8s.io/apimachinery/pkg/types" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/equality" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -32,12 +30,12 @@ func (g *Gatekeeper) upsertService(ctx context.Context, gateway gwv1beta1.Gatewa return g.deleteService(ctx, types.NamespacedName{Namespace: gateway.Namespace, Name: gateway.Name}) } - service := g.service(gateway, gcc) + desiredService := g.service(gateway, gcc) - mutated := service.DeepCopy() - mutator := newServiceMutator(service, mutated, gateway, g.Client.Scheme()) + existingService := desiredService.DeepCopy() + mutator := newServiceMutator(existingService, desiredService, gateway, g.Client.Scheme()) - result, err := controllerutil.CreateOrUpdate(ctx, g.Client, mutated, mutator) + result, err := controllerutil.CreateOrUpdate(ctx, g.Client, existingService, mutator) if err != nil { return err } @@ -112,24 +110,48 @@ func (g *Gatekeeper) service(gateway gwv1beta1.Gateway, gcc v1alpha1.GatewayClas } } -// mergeService is used to keep annotations and ports from the `from` Service -// to the `to` service. This prevents an infinite reconciliation loop when +// 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(from, to *corev1.Service) *corev1.Service { - if areServicesEqual(from, to) { - return to +func mergeServiceInto(existing, desired *corev1.Service) { + duplicate := existing.DeepCopy() + + // 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 } - to.Annotations = from.Annotations - to.Spec.Ports = from.Spec.Ports + // If the Service already exists, add any desired annotations + labels to existing set - return to -} + // 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 + } + } -func areServicesEqual(a, b *corev1.Service) bool { - if !equality.Semantic.DeepEqual(a.Annotations, b.Annotations) { - return false + // 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 + } else { + for k, v := range desired.ObjectMeta.Labels { + existing.ObjectMeta.Labels[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 } @@ -146,9 +168,9 @@ func areServicesEqual(a, b *corev1.Service) bool { return true } -func newServiceMutator(service, mutated *corev1.Service, gateway gwv1beta1.Gateway, scheme *runtime.Scheme) resourceMutator { +func newServiceMutator(existing, desired *corev1.Service, gateway gwv1beta1.Gateway, scheme *runtime.Scheme) resourceMutator { return func() error { - mutated = mergeService(service, mutated) - return ctrl.SetControllerReference(&gateway, mutated, scheme) + mergeServiceInto(existing, desired) + return ctrl.SetControllerReference(&gateway, existing, scheme) } }