From 4254967b278ab249e85b52b3fc2f31517af74e1c Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Thu, 8 Feb 2024 11:07:00 -0500 Subject: [PATCH 01/10] First pass fix of merge services --- .../api-gateway/gatekeeper/gatekeeper_test.go | 120 +++++++++++++++--- .../api-gateway/gatekeeper/service.go | 58 ++++----- 2 files changed, 127 insertions(+), 51 deletions(-) diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go index 8e212344b5..b082bdc8c1 100644 --- a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go +++ b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go @@ -6,8 +6,6 @@ package gatekeeper import ( "context" "fmt" - "testing" - logrtest "github.com/go-logr/logr/testr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -23,6 +21,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" gwv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" + "testing" "github.com/hashicorp/consul-k8s/control-plane/api-gateway/common" "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" @@ -41,6 +40,10 @@ var ( "gateway.consul.hashicorp.com/namespace": namespace, createdAtLabelKey: createdAtLabelValue, "gateway.consul.hashicorp.com/managed": "true", + //"my-label": "keep-me-please", + } + annotations = map[string]string{ + "my-annotation": "keep-me-please", } listeners = []gwv1beta1.Listener{ { @@ -70,6 +73,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 +197,7 @@ func TestUpsert(t *testing.T) { Port: 8080, TargetPort: intstr.FromInt(8080), }, - }, "1"), + }, "1", false), }, serviceAccounts: []*corev1.ServiceAccount{}, }, @@ -242,7 +249,7 @@ func TestUpsert(t *testing.T) { Port: 8081, TargetPort: intstr.FromInt(8081), }, - }, "1"), + }, "1", false), }, serviceAccounts: []*corev1.ServiceAccount{}, }, @@ -300,7 +307,7 @@ func TestUpsert(t *testing.T) { Port: 8081, TargetPort: intstr.FromInt(8081), }, - }, "1"), + }, "1", false), }, serviceAccounts: []*corev1.ServiceAccount{ configureServiceAccount(name, namespace, labels, "1"), @@ -426,7 +433,7 @@ func TestUpsert(t *testing.T) { Protocol: "TCP", Port: 8080, }, - }, "1"), + }, "1", true), }, serviceAccounts: []*corev1.ServiceAccount{ configureServiceAccount(name, namespace, labels, "1"), @@ -456,12 +463,13 @@ func TestUpsert(t *testing.T) { Port: 8081, TargetPort: intstr.FromInt(8081), }, - }, "2"), + }, "2", 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 +523,7 @@ func TestUpsert(t *testing.T) { Protocol: "TCP", Port: 8081, }, - }, "1"), + }, "1", true), }, serviceAccounts: []*corev1.ServiceAccount{ configureServiceAccount(name, namespace, labels, "1"), @@ -539,12 +547,13 @@ func TestUpsert(t *testing.T) { Port: 8080, TargetPort: intstr.FromInt(8080), }, - }, "2"), + }, "2", 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 +596,74 @@ 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, + }, + 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{}, + ServiceType: (*corev1.ServiceType)(common.PointerTo("NodePort")), + }, + }, + helmConfig: common.HelmConfig{ + ImageDataplane: dataplaneImage, + }, + initialResources: resources{ + services: []*corev1.Service{ + configureService(name, namespace, labels, annotations, (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), + }, + }, + finalResources: resources{ + deployments: []*appsv1.Deployment{}, + roles: []*rbac.Role{}, + services: []*corev1.Service{ + configureService(name, namespace, labels, annotations, (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), + }, + 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 +852,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 +944,7 @@ func TestDelete(t *testing.T) { Protocol: "TCP", Port: 8081, }, - }, "1"), + }, "1", true), }, serviceAccounts: []*corev1.ServiceAccount{}, }, @@ -928,7 +1005,7 @@ func TestDelete(t *testing.T) { Protocol: "TCP", Port: 8081, }, - }, "1"), + }, "1", true), }, serviceAccounts: []*corev1.ServiceAccount{ configureServiceAccount(name, namespace, labels, "1"), @@ -964,7 +1041,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 +1071,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 +1178,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 +1403,9 @@ 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 bool) *corev1.Service { + + service := corev1.Service{ TypeMeta: metav1.TypeMeta{ APIVersion: "v1", Kind: "Service", @@ -1350,6 +1432,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..2c00bd8962 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,43 +110,33 @@ 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 mergeService(existing, desired *corev1.Service) { - to.Annotations = from.Annotations - to.Spec.Ports = from.Spec.Ports - - return to -} + existing.Spec = desired.Spec -func areServicesEqual(a, b *corev1.Service) bool { - if !equality.Semantic.DeepEqual(a.Annotations, b.Annotations) { - return false - } - if len(b.Spec.Ports) != len(a.Spec.Ports) { - return false + // Only overwrite fields if the Service doesn't exist yet + if existing.ObjectMeta.CreationTimestamp.IsZero() { + existing.ObjectMeta.OwnerReferences = desired.ObjectMeta.OwnerReferences + existing.Annotations = desired.Annotations + existing.Labels = desired.Labels + return } - 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 - } + // 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 + } + for k, v := range desired.ObjectMeta.Labels { + existing.ObjectMeta.Labels[k] = v } - 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) + mergeService(existing, desired) + return ctrl.SetControllerReference(&gateway, existing, scheme) } } From 3d9880af425ea9de2cb764ad1142b81fa0c56fe2 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Thu, 8 Feb 2024 15:31:00 -0500 Subject: [PATCH 02/10] Updates to add test with existing annotations --- .../api-gateway/gatekeeper/gatekeeper_test.go | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go index b082bdc8c1..e85402220e 100644 --- a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go +++ b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go @@ -42,9 +42,17 @@ var ( "gateway.consul.hashicorp.com/managed": "true", //"my-label": "keep-me-please", } - annotations = map[string]string{ + + // TODO: This could probably be handled better + externalAnnotations = map[string]string{ "my-annotation": "keep-me-please", } + copyAnnotations = []string{"copy-this-annotation"} + externalAndCopyAnnotations = map[string]string{ + "my-annotation": "keep-me-please", + "copy-this-annotation": "copy-this-annotation-value", + } + listeners = []gwv1beta1.Listener{ { Name: "Listener 1", @@ -599,8 +607,9 @@ func TestUpsert(t *testing.T) { "updating a gateway deployment respects the labels and annotations a user has set": { gateway: gwv1beta1.Gateway{ ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: namespace, + Name: name, + Namespace: namespace, + Annotations: map[string]string{copyAnnotations[0]: "copy-this-annotation-value"}, }, Spec: gwv1beta1.GatewaySpec{ Listeners: listeners, @@ -616,7 +625,7 @@ func TestUpsert(t *testing.T) { MaxInstances: common.PointerTo(int32(7)), MinInstances: common.PointerTo(int32(1)), }, - CopyAnnotations: v1alpha1.CopyAnnotationsSpec{}, + CopyAnnotations: v1alpha1.CopyAnnotationsSpec{Service: copyAnnotations}, ServiceType: (*corev1.ServiceType)(common.PointerTo("NodePort")), }, }, @@ -625,7 +634,7 @@ func TestUpsert(t *testing.T) { }, initialResources: resources{ services: []*corev1.Service{ - configureService(name, namespace, labels, annotations, (corev1.ServiceType)("NodePort"), []corev1.ServicePort{ + configureService(name, namespace, labels, externalAnnotations, (corev1.ServiceType)("NodePort"), []corev1.ServicePort{ { Name: "Listener 1", Protocol: "TCP", @@ -645,7 +654,7 @@ func TestUpsert(t *testing.T) { deployments: []*appsv1.Deployment{}, roles: []*rbac.Role{}, services: []*corev1.Service{ - configureService(name, namespace, labels, annotations, (corev1.ServiceType)("NodePort"), []corev1.ServicePort{ + configureService(name, namespace, labels, externalAndCopyAnnotations, (corev1.ServiceType)("NodePort"), []corev1.ServicePort{ { Name: "Listener 1", Protocol: "TCP", From d21117cb2d6cc5bcb42ecc1a375b797df9c70a02 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Fri, 9 Feb 2024 10:14:06 -0500 Subject: [PATCH 03/10] Update tests to handle external labels --- .../api-gateway/gatekeeper/gatekeeper_test.go | 42 +++++++++++-------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go index e85402220e..a35e423245 100644 --- a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go +++ b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go @@ -34,22 +34,23 @@ 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", - //"my-label": "keep-me-please", } // TODO: This could probably be handled better + // These annotations are used for testing that annotations stay on the service after reconcile externalAnnotations = map[string]string{ - "my-annotation": "keep-me-please", + "external-annotation": "external-annotation-value", } copyAnnotations = []string{"copy-this-annotation"} externalAndCopyAnnotations = map[string]string{ - "my-annotation": "keep-me-please", + "external-annotation": "external-annotation-value", "copy-this-annotation": "copy-this-annotation-value", } @@ -205,7 +206,7 @@ func TestUpsert(t *testing.T) { Port: 8080, TargetPort: intstr.FromInt(8080), }, - }, "1", false), + }, "1", false, false), }, serviceAccounts: []*corev1.ServiceAccount{}, }, @@ -257,7 +258,7 @@ func TestUpsert(t *testing.T) { Port: 8081, TargetPort: intstr.FromInt(8081), }, - }, "1", false), + }, "1", false, false), }, serviceAccounts: []*corev1.ServiceAccount{}, }, @@ -315,7 +316,7 @@ func TestUpsert(t *testing.T) { Port: 8081, TargetPort: intstr.FromInt(8081), }, - }, "1", false), + }, "1", false, false), }, serviceAccounts: []*corev1.ServiceAccount{ configureServiceAccount(name, namespace, labels, "1"), @@ -441,7 +442,7 @@ func TestUpsert(t *testing.T) { Protocol: "TCP", Port: 8080, }, - }, "1", true), + }, "1", true, false), }, serviceAccounts: []*corev1.ServiceAccount{ configureServiceAccount(name, namespace, labels, "1"), @@ -471,7 +472,7 @@ func TestUpsert(t *testing.T) { Port: 8081, TargetPort: intstr.FromInt(8081), }, - }, "2", false), + }, "2", false, false), }, serviceAccounts: []*corev1.ServiceAccount{ configureServiceAccount(name, namespace, labels, "1"), @@ -531,7 +532,7 @@ func TestUpsert(t *testing.T) { Protocol: "TCP", Port: 8081, }, - }, "1", true), + }, "1", true, false), }, serviceAccounts: []*corev1.ServiceAccount{ configureServiceAccount(name, namespace, labels, "1"), @@ -555,7 +556,7 @@ func TestUpsert(t *testing.T) { Port: 8080, TargetPort: intstr.FromInt(8080), }, - }, "2", false), + }, "2", false, false), }, serviceAccounts: []*corev1.ServiceAccount{ configureServiceAccount(name, namespace, labels, "1"), @@ -647,7 +648,7 @@ func TestUpsert(t *testing.T) { Port: 8081, TargetPort: intstr.FromInt(8081), }, - }, "1", true), + }, "1", true, false), }, }, finalResources: resources{ @@ -667,7 +668,7 @@ func TestUpsert(t *testing.T) { Port: 8081, TargetPort: intstr.FromInt(8081), }, - }, "2", false), + }, "2", false, false), }, serviceAccounts: []*corev1.ServiceAccount{}, }, @@ -953,7 +954,7 @@ func TestDelete(t *testing.T) { Protocol: "TCP", Port: 8081, }, - }, "1", true), + }, "1", true, false), }, serviceAccounts: []*corev1.ServiceAccount{}, }, @@ -1014,7 +1015,7 @@ func TestDelete(t *testing.T) { Protocol: "TCP", Port: 8081, }, - }, "1", true), + }, "1", true, false), }, serviceAccounts: []*corev1.ServiceAccount{ configureServiceAccount(name, namespace, labels, "1"), @@ -1412,7 +1413,14 @@ 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, isInitialResource bool) *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{ @@ -1422,7 +1430,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{ From f17369bd6a654d9d9c80fbb378ad3d0b6520d3e0 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Fri, 9 Feb 2024 10:30:23 -0500 Subject: [PATCH 04/10] Adds changelog --- .changelog/3597.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/3597.txt diff --git a/.changelog/3597.txt b/.changelog/3597.txt new file mode 100644 index 0000000000..8b55bd9c4c --- /dev/null +++ b/.changelog/3597.txt @@ -0,0 +1,3 @@ +```release-note:bug +api-gateway: fix issue where external annotations are being incorrectly deleted on services controlled by the API Gateway +``` \ No newline at end of file From ce75a10e315264eb469320eba00dc92c77182a25 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Fri, 9 Feb 2024 10:31:34 -0500 Subject: [PATCH 05/10] fix import reordering --- control-plane/api-gateway/gatekeeper/gatekeeper_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go index a35e423245..26ef5fffc4 100644 --- a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go +++ b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go @@ -6,6 +6,8 @@ package gatekeeper import ( "context" "fmt" + "testing" + logrtest "github.com/go-logr/logr/testr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -21,7 +23,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" gwv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" - "testing" "github.com/hashicorp/consul-k8s/control-plane/api-gateway/common" "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" From a0d0a4413d37104599a9d2d508528d2abd94471c Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Mon, 12 Feb 2024 13:33:01 -0500 Subject: [PATCH 06/10] Changed override of all spec to just be spec.ports --- control-plane/api-gateway/gatekeeper/service.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/control-plane/api-gateway/gatekeeper/service.go b/control-plane/api-gateway/gatekeeper/service.go index 2c00bd8962..732fd0f175 100644 --- a/control-plane/api-gateway/gatekeeper/service.go +++ b/control-plane/api-gateway/gatekeeper/service.go @@ -115,8 +115,6 @@ func (g *Gatekeeper) service(gateway gwv1beta1.Gateway, gcc v1alpha1.GatewayClas // Kubernetes adds this configuration back in. func mergeService(existing, desired *corev1.Service) { - existing.Spec = desired.Spec - // Only overwrite fields if the Service doesn't exist yet if existing.ObjectMeta.CreationTimestamp.IsZero() { existing.ObjectMeta.OwnerReferences = desired.ObjectMeta.OwnerReferences @@ -125,6 +123,8 @@ func mergeService(existing, desired *corev1.Service) { return } + existing.Spec.Ports = desired.Spec.Ports + // 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 From 2fb650e5ef9c04c69c8936fa3313bee5a1a36192 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Mon, 12 Feb 2024 13:40:37 -0500 Subject: [PATCH 07/10] Linter --- control-plane/api-gateway/gatekeeper/gatekeeper_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go index 26ef5fffc4..6c3bcefd49 100644 --- a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go +++ b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go @@ -44,8 +44,7 @@ var ( "gateway.consul.hashicorp.com/managed": "true", } - // TODO: This could probably be handled better - // These annotations are used for testing that annotations stay on the service after reconcile + // These annotations are used for testing that annotations stay on the service after reconcile. externalAnnotations = map[string]string{ "external-annotation": "external-annotation-value", } From 6e7d86724028b1e8ae8649603d769d3774213aa1 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Tue, 13 Feb 2024 12:24:33 -0500 Subject: [PATCH 08/10] Adds logic to fix panic if external controller removes all annotations --- .../api-gateway/gatekeeper/gatekeeper_test.go | 82 +++++++++++++++++-- .../api-gateway/gatekeeper/service.go | 60 +++++++++++--- 2 files changed, 125 insertions(+), 17 deletions(-) diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go index 6c3bcefd49..fd71278ef7 100644 --- 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 index 732fd0f175..4faa013f4a 100644 --- 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) } } From 15f01446fadc47d30b24d671a5452f430542506f Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Tue, 13 Feb 2024 12:29:27 -0500 Subject: [PATCH 09/10] Update test to have nil labels --- .changelog/3597.txt | 2 +- control-plane/api-gateway/gatekeeper/gatekeeper_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.changelog/3597.txt b/.changelog/3597.txt index 8b55bd9c4c..bcd0dafe8b 100644 --- a/.changelog/3597.txt +++ b/.changelog/3597.txt @@ -1,3 +1,3 @@ ```release-note:bug -api-gateway: fix issue where external annotations are being incorrectly deleted on services controlled by the API Gateway +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 fd71278ef7..8c0d842d49 100644 --- a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go +++ b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go @@ -677,7 +677,7 @@ func TestUpsert(t *testing.T) { }, ignoreTimestampOnService: true, }, - "updating a gateway that has copy-annotations doesn't panic if another controller has removed them all": { + "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, @@ -707,7 +707,7 @@ func TestUpsert(t *testing.T) { }, initialResources: resources{ services: []*corev1.Service{ - configureService(name, namespace, labels, nil, (corev1.ServiceType)("NodePort"), []corev1.ServicePort{ + configureService(name, namespace, nil, nil, (corev1.ServiceType)("NodePort"), []corev1.ServicePort{ { Name: "Listener 1", Protocol: "TCP", From 768ea38ffbec6c7ee66b7a891f0a1541fa16958d Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Tue, 13 Feb 2024 12:39:58 -0500 Subject: [PATCH 10/10] Lint --- control-plane/api-gateway/gatekeeper/service.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/control-plane/api-gateway/gatekeeper/service.go b/control-plane/api-gateway/gatekeeper/service.go index 4faa013f4a..e78afc239b 100644 --- a/control-plane/api-gateway/gatekeeper/service.go +++ b/control-plane/api-gateway/gatekeeper/service.go @@ -147,8 +147,6 @@ func mergeServiceInto(existing, desired *corev1.Service) { existing.ObjectMeta.Labels[k] = v } } - - return } // hasEqualPorts does a fuzzy comparison of the ports on a service spec