fix(dataplane): fix setting ExternalTrafficPolicy on ingress service

Kong · Nov 14, 2024 · 71cdf75 · 71cdf75
1 parent f974562
commit 71cdf75
Show file tree

Hide file tree

Showing 4 changed files with 264 additions and 102 deletions.
diff --git a/controller/dataplane/owned_resources.go b/controller/dataplane/owned_resources.go
@@ -382,6 +382,7 @@ func ensureIngressServiceForDataPlane(
 	if count == 1 {
 		var updated bool
 		existingService := &services[0]
+		old := existingService.DeepCopy()
 		updated, existingService.ObjectMeta = k8sutils.EnsureObjectMetaIsUpdated(existingService.ObjectMeta, generatedService.ObjectMeta,
 			// enforce all the annotations provided through the dataplane API
 			func(existingMeta metav1.ObjectMeta, generatedMeta metav1.ObjectMeta) (bool, metav1.ObjectMeta) {
@@ -402,11 +403,19 @@ func ensureIngressServiceForDataPlane(
 			existingService.Spec.Type = generatedService.Spec.Type
 			updated = true
 		}
-		if generatedService.Spec.ExternalTrafficPolicy != "" &&
-			existingService.Spec.ExternalTrafficPolicy != generatedService.Spec.ExternalTrafficPolicy {
+
+		const (
+			defaultExternalTrafficPolicy = corev1.ServiceExternalTrafficPolicyCluster
+		)
+		// Do not update when
+		// - the existing service has the default value for ExternalTrafficPolicy
+		// - and the generated service has the default value for ExternalTrafficPolicy or is empty.
+		if !(existingService.Spec.ExternalTrafficPolicy == defaultExternalTrafficPolicy &&
+			(generatedService.Spec.ExternalTrafficPolicy == "" || generatedService.Spec.ExternalTrafficPolicy == defaultExternalTrafficPolicy)) {
 			existingService.Spec.ExternalTrafficPolicy = generatedService.Spec.ExternalTrafficPolicy
 			updated = true
 		}
+
 		if !cmp.Equal(existingService.Spec.Selector, generatedService.Spec.Selector) {
 			existingService.Spec.Selector = generatedService.Spec.Selector
 			updated = true
@@ -421,10 +430,11 @@ func ensureIngressServiceForDataPlane(
 		}
 
 		if updated {
-			if err := cl.Update(ctx, existingService); err != nil {
+			res, existingService, err := patch.ApplyPatchIfNotEmpty(ctx, cl, logger, existingService, old, updated)
+			if err != nil {
 				return op.Noop, existingService, fmt.Errorf("failed updating DataPlane Service %s: %w", existingService.Name, err)
 			}
-			return op.Updated, existingService, nil
+			return res, existingService, nil
 		}
 		return op.Noop, existingService, nil
 	}

diff --git a/controller/dataplane/owned_resources_test.go b/controller/dataplane/owned_resources_test.go
@@ -36,10 +36,14 @@ func TestEnsureIngressServiceForDataPlane(t *testing.T) {
 	}{
 		{
 			name: "should create a new service if service does not exist",
-			dataplane: builder.NewDataPlaneBuilder().WithObjectMeta(metav1.ObjectMeta{
-				Namespace: "default",
-				Name:      "dp-1",
-			}).WithIngressServiceType(corev1.ServiceTypeLoadBalancer).Build(),
+			dataplane: builder.
+				NewDataPlaneBuilder().
+				WithObjectMeta(metav1.ObjectMeta{
+					Namespace: "default",
+					Name:      "dp-1",
+				}).
+				WithIngressServiceType(corev1.ServiceTypeLoadBalancer).
+				Build(),
 			existingServiceModifier: func(t *testing.T, ctx context.Context, c client.Client, svc *corev1.Service) {
 				require.NoError(t, dataplane.OwnedObjectPreDeleteHook(ctx, c, svc))
 				require.NoError(t, c.Delete(ctx, svc))
@@ -50,21 +54,29 @@ func TestEnsureIngressServiceForDataPlane(t *testing.T) {
 		},
 		{
 			name: "should not update when a service exists",
-			dataplane: builder.NewDataPlaneBuilder().WithObjectMeta(metav1.ObjectMeta{
-				Namespace: "default",
-				Name:      "dp-1",
-			}).WithIngressServiceType(corev1.ServiceTypeLoadBalancer).Build(),
+			dataplane: builder.
+				NewDataPlaneBuilder().
+				WithObjectMeta(metav1.ObjectMeta{
+					Namespace: "default",
+					Name:      "dp-1",
+				}).
+				WithIngressServiceType(corev1.ServiceTypeLoadBalancer).
+				Build(),
 			expectedCreatedOrUpdated: op.Noop,
 			expectedServiceType:      corev1.ServiceTypeLoadBalancer,
 			expectedServicePorts:     k8sresources.DefaultDataPlaneIngressServicePorts,
 		},
 		{
 			name: "should add annotations to existing service",
-			dataplane: builder.NewDataPlaneBuilder().WithObjectMeta(metav1.ObjectMeta{
-				Namespace: "default",
-				Name:      "dp-1",
-			}).WithIngressServiceType(corev1.ServiceTypeLoadBalancer).
-				WithIngressServiceAnnotations(map[string]string{"foo": "bar"}).Build(),
+			dataplane: builder.
+				NewDataPlaneBuilder().
+				WithObjectMeta(metav1.ObjectMeta{
+					Namespace: "default",
+					Name:      "dp-1",
+				}).
+				WithIngressServiceType(corev1.ServiceTypeLoadBalancer).
+				WithIngressServiceAnnotations(map[string]string{"foo": "bar"}).
+				Build(),
 			existingServiceModifier: func(t *testing.T, ctx context.Context, c client.Client, svc *corev1.Service) {
 				svc.Annotations = nil
 				require.NoError(t, c.Update(ctx, svc))
@@ -89,11 +101,15 @@ func TestEnsureIngressServiceForDataPlane(t *testing.T) {
 				}
 				require.NoError(t, c.Update(ctx, svc))
 			},
-			dataplane: builder.NewDataPlaneBuilder().WithObjectMeta(metav1.ObjectMeta{
-				Namespace: "default",
-				Name:      "dp-1",
-			}).WithIngressServiceType(corev1.ServiceTypeLoadBalancer).
-				WithIngressServiceAnnotations(map[string]string{"foo": "bar"}).Build(),
+			dataplane: builder.
+				NewDataPlaneBuilder().
+				WithObjectMeta(metav1.ObjectMeta{
+					Namespace: "default",
+					Name:      "dp-1",
+				}).
+				WithIngressServiceType(corev1.ServiceTypeLoadBalancer).
+				WithIngressServiceAnnotations(map[string]string{"foo": "bar"}).
+				Build(),
 			expectedCreatedOrUpdated: op.Updated,
 			expectedServiceType:      corev1.ServiceTypeLoadBalancer,
 			expectedServicePorts:     k8sresources.DefaultDataPlaneIngressServicePorts,
@@ -108,10 +124,14 @@ func TestEnsureIngressServiceForDataPlane(t *testing.T) {
 		{
 			name:             "should create service when service does not contain additional labels",
 			additionalLabels: map[string]string{"foo": "bar"},
-			dataplane: builder.NewDataPlaneBuilder().WithObjectMeta(metav1.ObjectMeta{
-				Namespace: "default",
-				Name:      "dp-1",
-			}).WithIngressServiceType(corev1.ServiceTypeLoadBalancer).Build(),
+			dataplane: builder.
+				NewDataPlaneBuilder().
+				WithObjectMeta(metav1.ObjectMeta{
+					Namespace: "default",
+					Name:      "dp-1",
+				}).
+				WithIngressServiceType(corev1.ServiceTypeLoadBalancer).
+				Build(),
 			existingServiceModifier: func(t *testing.T, ctx context.Context, c client.Client, svc *corev1.Service) {
 				if svc.Labels != nil {
 					delete(svc.Labels, "foo")
@@ -125,16 +145,21 @@ func TestEnsureIngressServiceForDataPlane(t *testing.T) {
 		},
 		{
 			name: "should update ports",
-			dataplane: builder.NewDataPlaneBuilder().WithObjectMeta(metav1.ObjectMeta{
-				Namespace: "default",
-				Name:      "dp-1",
-			}).WithIngressServiceType(corev1.ServiceTypeLoadBalancer).WithIngressServicePorts([]operatorv1beta1.DataPlaneServicePort{
-				{
-					Name:       "http",
-					Port:       8080,
-					TargetPort: intstr.FromInt(8000),
-				},
-			}).Build(),
+			dataplane: builder.
+				NewDataPlaneBuilder().
+				WithObjectMeta(metav1.ObjectMeta{
+					Namespace: "default",
+					Name:      "dp-1",
+				}).
+				WithIngressServiceType(corev1.ServiceTypeLoadBalancer).
+				WithIngressServicePorts([]operatorv1beta1.DataPlaneServicePort{
+					{
+						Name:       "http",
+						Port:       8080,
+						TargetPort: intstr.FromInt(8000),
+					},
+				}).
+				Build(),
 			expectedCreatedOrUpdated: op.Updated,
 			expectedServiceType:      corev1.ServiceTypeLoadBalancer,
 			expectedServicePorts: []corev1.ServicePort{
@@ -146,6 +171,99 @@ func TestEnsureIngressServiceForDataPlane(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "should not need to update the service (LB) when it already has the cluster external traffic policy",
+			dataplane: builder.
+				NewDataPlaneBuilder().
+				WithObjectMeta(metav1.ObjectMeta{
+					Namespace: "default",
+					Name:      "dp-1",
+				}).
+				WithIngressServiceType(corev1.ServiceTypeLoadBalancer).
+				Build(),
+			existingServiceModifier: func(t *testing.T, ctx context.Context, c client.Client, svc *corev1.Service) {
+				svc.Spec.ExternalTrafficPolicy = corev1.ServiceExternalTrafficPolicyTypeCluster
+				require.NoError(t, c.Update(ctx, svc))
+			},
+			expectedCreatedOrUpdated: op.Noop,
+			expectedServiceType:      corev1.ServiceTypeLoadBalancer,
+			expectedServicePorts:     k8sresources.DefaultDataPlaneIngressServicePorts,
+		},
+		{
+			name: "should not need to update the service (LB) when it already has the cluster external traffic policy and dp spec has the same",
+			dataplane: builder.
+				NewDataPlaneBuilder().
+				WithObjectMeta(metav1.ObjectMeta{
+					Namespace: "default",
+					Name:      "dp-1",
+				}).
+				WithIngressServiceType(corev1.ServiceTypeLoadBalancer).
+				WithIngressServiceExternalTrafficPolicy(corev1.ServiceExternalTrafficPolicyCluster).
+				Build(),
+			existingServiceModifier: func(t *testing.T, ctx context.Context, c client.Client, svc *corev1.Service) {
+				svc.Spec.ExternalTrafficPolicy = corev1.ServiceExternalTrafficPolicyCluster
+				require.NoError(t, c.Update(ctx, svc))
+			},
+			expectedCreatedOrUpdated: op.Noop,
+			expectedServiceType:      corev1.ServiceTypeLoadBalancer,
+			expectedServicePorts:     k8sresources.DefaultDataPlaneIngressServicePorts,
+		},
+		{
+			name: "should update the service (LB) when it has the cluster external traffic policy and dp spec has local",
+			dataplane: builder.
+				NewDataPlaneBuilder().
+				WithObjectMeta(metav1.ObjectMeta{
+					Namespace: "default",
+					Name:      "dp-1",
+				}).
+				WithIngressServiceType(corev1.ServiceTypeLoadBalancer).
+				WithIngressServiceExternalTrafficPolicy(corev1.ServiceExternalTrafficPolicyLocal).
+				Build(),
+			existingServiceModifier: func(t *testing.T, ctx context.Context, c client.Client, svc *corev1.Service) {
+				svc.Spec.ExternalTrafficPolicy = corev1.ServiceExternalTrafficPolicyCluster
+				require.NoError(t, c.Update(ctx, svc))
+			},
+			expectedCreatedOrUpdated: op.Updated,
+			expectedServiceType:      corev1.ServiceTypeLoadBalancer,
+			expectedServicePorts:     k8sresources.DefaultDataPlaneIngressServicePorts,
+		},
+		{
+			name: "should update the service (LB) when it has the local external traffic policy and dp spec not specified it",
+			dataplane: builder.
+				NewDataPlaneBuilder().
+				WithObjectMeta(metav1.ObjectMeta{
+					Namespace: "default",
+					Name:      "dp-1",
+				}).
+				WithIngressServiceType(corev1.ServiceTypeLoadBalancer).
+				Build(),
+			existingServiceModifier: func(t *testing.T, ctx context.Context, c client.Client, svc *corev1.Service) {
+				svc.Spec.ExternalTrafficPolicy = corev1.ServiceExternalTrafficPolicyLocal
+				require.NoError(t, c.Update(ctx, svc))
+			},
+			expectedCreatedOrUpdated: op.Updated,
+			expectedServiceType:      corev1.ServiceTypeLoadBalancer,
+			expectedServicePorts:     k8sresources.DefaultDataPlaneIngressServicePorts,
+		},
+		{
+			name: "should not need to update the service (LB) when it has the local external traffic policy and dp spec has also local",
+			dataplane: builder.
+				NewDataPlaneBuilder().
+				WithObjectMeta(metav1.ObjectMeta{
+					Namespace: "default",
+					Name:      "dp-1",
+				}).
+				WithIngressServiceType(corev1.ServiceTypeLoadBalancer).
+				WithIngressServiceExternalTrafficPolicy(corev1.ServiceExternalTrafficPolicyLocal).
+				Build(),
+			existingServiceModifier: func(t *testing.T, ctx context.Context, c client.Client, svc *corev1.Service) {
+				svc.Spec.ExternalTrafficPolicy = corev1.ServiceExternalTrafficPolicyLocal
+				require.NoError(t, c.Update(ctx, svc))
+			},
+			expectedCreatedOrUpdated: op.Noop,
+			expectedServiceType:      corev1.ServiceTypeLoadBalancer,
+			expectedServicePorts:     k8sresources.DefaultDataPlaneIngressServicePorts,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -159,14 +277,12 @@ func TestEnsureIngressServiceForDataPlane(t *testing.T) {
 			existingSvc, err := k8sresources.GenerateNewIngressServiceForDataPlane(tc.dataplane)
 			require.NoError(t, err)
 			k8sutils.SetOwnerForObject(existingSvc, tc.dataplane)
-			err = fakeClient.Create(ctx, existingSvc)
-			require.NoError(t, err)
+			require.NoError(t, fakeClient.Create(ctx, existingSvc))
 			if tc.existingServiceModifier != nil {
 				tc.existingServiceModifier(t, ctx, fakeClient, existingSvc)
 			}
 			// create dataplane resource.
-			err = fakeClient.Create(ctx, tc.dataplane)
-			require.NoError(t, err, "should create dataplane successfully")
+			require.NoError(t, fakeClient.Create(ctx, tc.dataplane), "should create dataplane successfully")
 			res, svc, err := ensureIngressServiceForDataPlane(
 				ctx,
 				logr.Discard(),

diff --git a/controller/pkg/builder/builder.go b/controller/pkg/builder/builder.go
@@ -57,6 +57,13 @@ func (b *testDataPlaneBuilder) WithIngressServiceType(typ corev1.ServiceType) *t
 	return b
 }
 
+// WithIngressServiceExternalTrafficPolicy sets the ExternalTrafficPolicy of the Ingress service.
+func (b *testDataPlaneBuilder) WithIngressServiceExternalTrafficPolicy(typ corev1.ServiceExternalTrafficPolicyType) *testDataPlaneBuilder {
+	b.initIngressServiceOptions()
+	b.dataplane.Spec.DataPlaneOptions.Network.Services.Ingress.ExternalTrafficPolicy = typ
+	return b
+}
+
 // WithIngressServicePorts sets the Ports of the Ingress service.
 func (b *testDataPlaneBuilder) WithIngressServicePorts(ports []operatorv1beta1.DataPlaneServicePort) *testDataPlaneBuilder {
 	b.initIngressServiceOptions()