From 57548bc0ff7a9f1168cb2522c97b6dfd9d335ee3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Ma=C5=82ek?= Date: Thu, 17 Oct 2024 19:12:20 +0200 Subject: [PATCH] fix: set DataPlane's ExternalTrafficPolicy on updates and patches --- CHANGELOG.md | 2 + controller/dataplane/owned_resources.go | 4 + controller/konnect/ops/ops.go | 4 + pkg/utils/test/predicates.go | 20 ++++- test/integration/test_dataplane.go | 90 +++++++++++++++++++ .../zz_generated_registered_testcases.go | 1 + 6 files changed, 119 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d080edb1..e25441260 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -123,6 +123,8 @@ [#710](https://github.com/Kong/gateway-operator/pull/710) - Do not reconcile Gateways nor assign any finalizers when the referred GatewayClass is not supported. [#711](https://github.com/Kong/gateway-operator/pull/711) +- Fixed setting `ExternalTrafficPolicy` on `DataPlane`'s ingress `Service during update and patch operations. + [#750](https://github.com/Kong/gateway-operator/pull/750) ### Changes diff --git a/controller/dataplane/owned_resources.go b/controller/dataplane/owned_resources.go index 692d78a86..abc40db88 100644 --- a/controller/dataplane/owned_resources.go +++ b/controller/dataplane/owned_resources.go @@ -402,6 +402,10 @@ func ensureIngressServiceForDataPlane( existingService.Spec.Type = generatedService.Spec.Type updated = true } + if existingService.Spec.ExternalTrafficPolicy != generatedService.Spec.ExternalTrafficPolicy { + 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 diff --git a/controller/konnect/ops/ops.go b/controller/konnect/ops/ops.go index 457f269a8..ab12e0360 100644 --- a/controller/konnect/ops/ops.go +++ b/controller/konnect/ops/ops.go @@ -66,6 +66,10 @@ func Create[ case *configurationv1alpha1.KongRoute: err = createRoute(ctx, sdk.GetRoutesSDK(), ent) + + // TODO: modify the create* operation wrappers to not set Programmed conditions and return + // a KonnectEntityCreatedButRelationsFailedError if the entity was created but its relations assignment failed. + case *configurationv1.KongConsumer: err = createConsumer(ctx, sdk.GetConsumersSDK(), sdk.GetConsumerGroupsSDK(), cl, ent) case *configurationv1beta1.KongConsumerGroup: diff --git a/pkg/utils/test/predicates.go b/pkg/utils/test/predicates.go index 41cd7a8c5..dd8475f2c 100644 --- a/pkg/utils/test/predicates.go +++ b/pkg/utils/test/predicates.go @@ -431,10 +431,26 @@ func DataPlaneHasNReadyPods(t *testing.T, ctx context.Context, dataplaneName typ // DataPlaneHasService is a helper function for tests that returns a function // that can be used to check if a DataPlane has a service created. // Should be used in conjunction with require.Eventually or assert.Eventually. -func DataPlaneHasService(t *testing.T, ctx context.Context, dataplaneName types.NamespacedName, clients K8sClients, matchingLabels client.MatchingLabels) func() bool { +func DataPlaneHasService( + t *testing.T, + ctx context.Context, + dataplaneName types.NamespacedName, + clients K8sClients, + matchingLabels client.MatchingLabels, + asserts ...func(corev1.Service) bool, +) func() bool { return DataPlanePredicate(t, ctx, dataplaneName, func(dataplane *operatorv1beta1.DataPlane) bool { services := MustListDataPlaneServices(t, ctx, dataplane, clients.MgrClient, matchingLabels) - return len(services) == 1 + if len(services) != 1 { + return false + } + for _, a := range asserts { + if !a(services[0]) { + return false + } + } + + return true }, clients.OperatorClient) } diff --git a/test/integration/test_dataplane.go b/test/integration/test_dataplane.go index 9f9b9a209..a330eddfa 100644 --- a/test/integration/test_dataplane.go +++ b/test/integration/test_dataplane.go @@ -914,3 +914,93 @@ func TestDataPlanePodDisruptionBudget(t *testing.T) { assert.True(t, k8serrors.IsNotFound(err)) }, time.Minute, time.Second) } + +func TestDataPlaneServiceExternalTrafficPolicy(t *testing.T) { + t.Parallel() + namespace, cleaner := helpers.SetupTestEnv(t, GetCtx(), GetEnv()) + + t.Log("deploying DataPlane resource with 2 replicas") + dataplaneName := types.NamespacedName{ + Namespace: namespace.Name, + Name: uuid.NewString(), + } + dataplane := &operatorv1beta1.DataPlane{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: dataplaneName.Namespace, + Name: dataplaneName.Name, + }, + Spec: operatorv1beta1.DataPlaneSpec{ + DataPlaneOptions: operatorv1beta1.DataPlaneOptions{ + Deployment: operatorv1beta1.DataPlaneDeploymentOptions{ + DeploymentOptions: operatorv1beta1.DeploymentOptions{ + PodTemplateSpec: &corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: consts.DataPlaneProxyContainerName, + Image: helpers.GetDefaultDataPlaneImage(), + }, + }, + }, + }, + }, + }, + }, + }, + } + + dataplaneClient := GetClients().OperatorClient.ApisV1beta1().DataPlanes(namespace.Name) + + dataplane, err := dataplaneClient.Create(GetCtx(), dataplane, metav1.CreateOptions{}) + require.NoError(t, err) + cleaner.Add(dataplane) + + t.Log("verifying DataPlane gets marked provisioned") + require.Eventually(t, testutils.DataPlaneIsReady(t, GetCtx(), dataplaneName, GetClients().OperatorClient), time.Minute, time.Second) + + t.Log("verifying deployments managed by the DataPlane") + deployment := &appsv1.Deployment{} + require.Eventually(t, testutils.DataPlaneHasActiveDeployment(t, GetCtx(), dataplaneName, deployment, client.MatchingLabels{ + consts.GatewayOperatorManagedByLabel: consts.DataPlaneManagedLabelValue, + }, clients), time.Minute, time.Second) + + t.Log("changing in ExternalTrafficPolicy to Local") + require.Eventually(t, testutils.DataPlaneUpdateEventually(t, GetCtx(), dataplaneName, clients, func(dp *operatorv1beta1.DataPlane) { + dp.Spec.Network.Services = &operatorv1beta1.DataPlaneServices{ + Ingress: &operatorv1beta1.DataPlaneServiceOptions{ + ServiceOptions: operatorv1beta1.ServiceOptions{ + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, + }, + }, + } + }), time.Minute, time.Second) + + t.Log("verifying the DataPlane Service ExternalTrafficPolicy is updated to Local") + require.Eventually(t, testutils.DataPlaneHasService(t, GetCtx(), dataplaneName, clients, + client.MatchingLabels{ + consts.GatewayOperatorManagedByLabel: consts.DataPlaneManagedLabelValue, + consts.DataPlaneServiceTypeLabel: string(consts.DataPlaneIngressServiceLabelValue), + }, + func(svc corev1.Service) bool { + return svc.Spec.ExternalTrafficPolicy == corev1.ServiceExternalTrafficPolicyLocal + }, + ), time.Minute, time.Second) + + t.Log("setting DataPlane Service ExternalTrafficPolicy to Cluster") + require.Eventually(t, testutils.DataPlaneUpdateEventually(t, GetCtx(), dataplaneName, clients, + func(dp *operatorv1beta1.DataPlane) { + dp.Spec.Network.Services.Ingress.ExternalTrafficPolicy = corev1.ServiceExternalTrafficPolicyCluster + }, + ), time.Minute, time.Second) + + t.Log("verifying the DataPlane Service ExternalTrafficPolicy is updated to Cluster") + require.Eventually(t, testutils.DataPlaneHasService(t, GetCtx(), dataplaneName, clients, + client.MatchingLabels{ + consts.GatewayOperatorManagedByLabel: consts.DataPlaneManagedLabelValue, + consts.DataPlaneServiceTypeLabel: string(consts.DataPlaneIngressServiceLabelValue), + }, + func(svc corev1.Service) bool { + return svc.Spec.ExternalTrafficPolicy == corev1.ServiceExternalTrafficPolicyTypeCluster + }, + ), time.Minute, time.Second) +} diff --git a/test/integration/zz_generated_registered_testcases.go b/test/integration/zz_generated_registered_testcases.go index 8b700a7f3..8900affa5 100644 --- a/test/integration/zz_generated_registered_testcases.go +++ b/test/integration/zz_generated_registered_testcases.go @@ -13,6 +13,7 @@ func init() { TestDataPlaneEssentials, TestDataPlaneHorizontalScaling, TestDataPlanePodDisruptionBudget, + TestDataPlaneServiceExternalTrafficPolicy, TestDataPlaneUpdate, TestDataPlaneValidation, TestDataPlaneVolumeMounts,