From 0d20781a3df025f145b11fdd29354b233d6da36f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Ma=C5=82ek?= Date: Mon, 28 Oct 2024 14:45:35 +0100 Subject: [PATCH] fix(dataplane): do not set ExternalTrafficPolicy for ClusterIP services (#812) --- CHANGELOG.md | 3 + api/v1beta1/dataplane_types.go | 2 +- ...ateway-operator.konghq.com_dataplanes.yaml | 6 +- ...ator.konghq.com_gatewayconfigurations.yaml | 6 +- ...ateway-operator.konghq.com_dataplanes.yaml | 6 +- controller/dataplane/owned_resources.go | 3 +- pkg/utils/kubernetes/resources/services.go | 19 +++-- .../kubernetes/resources/services_test.go | 75 ++++++++++++++++++- 8 files changed, 108 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 54249b4f2..8c76616ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -126,6 +126,9 @@ [#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) +- Fixed setting `ExternalTrafficPolicy` on `DataPlane`'s ingress `Service`. + Remove the default value (`Cluster`). Prevent setting this field for `ClusterIP` `Service`s. + [#812](https://github.com/Kong/gateway-operator/pull/812) ### Changes diff --git a/api/v1beta1/dataplane_types.go b/api/v1beta1/dataplane_types.go index 81a810aca..7f7b1a494 100644 --- a/api/v1beta1/dataplane_types.go +++ b/api/v1beta1/dataplane_types.go @@ -239,6 +239,7 @@ type DataPlaneServicePort struct { // ServiceOptions is used to includes options to customize the ingress service, // such as the annotations. // +apireference:kgo:include +// +kubebuilder:validation:XValidation:message="Cannot set ExternalTrafficPolicy for ClusterIP service.", rule="has(self.type) && self.type == 'ClusterIP' ? !has(self.externalTrafficPolicy) : true" type ServiceOptions struct { // Type determines how the Service is exposed. // Defaults to `LoadBalancer`. @@ -285,7 +286,6 @@ type ServiceOptions struct { // More info: https://kubernetes.io/docs/tasks/access-application-cluster/create-external-load-balancer/#preserving-the-client-source-ip // // +optional - // +kubebuilder:default=Cluster // +kubebuilder:validation:Enum=Cluster;Local ExternalTrafficPolicy corev1.ServiceExternalTrafficPolicy `json:"externalTrafficPolicy,omitempty"` } diff --git a/config/crd/bases/gateway-operator.konghq.com_dataplanes.yaml b/config/crd/bases/gateway-operator.konghq.com_dataplanes.yaml index 0a50fb8ab..1ba05c70e 100644 --- a/config/crd/bases/gateway-operator.konghq.com_dataplanes.yaml +++ b/config/crd/bases/gateway-operator.konghq.com_dataplanes.yaml @@ -8858,7 +8858,6 @@ spec: More info: http://kubernetes.io/docs/user-guide/annotations type: object externalTrafficPolicy: - default: Cluster description: |- ExternalTrafficPolicy describes how nodes distribute service traffic they receive on one of the Service's "externally-facing" addresses (NodePorts, @@ -8941,6 +8940,11 @@ spec: - ClusterIP type: string type: object + x-kubernetes-validations: + - message: Cannot set ExternalTrafficPolicy for ClusterIP + service. + rule: 'has(self.type) && self.type == ''ClusterIP'' ? !has(self.externalTrafficPolicy) + : true' type: object type: object pluginsToInstall: diff --git a/config/crd/bases/gateway-operator.konghq.com_gatewayconfigurations.yaml b/config/crd/bases/gateway-operator.konghq.com_gatewayconfigurations.yaml index 4bf5d4e7d..9d08f4786 100644 --- a/config/crd/bases/gateway-operator.konghq.com_gatewayconfigurations.yaml +++ b/config/crd/bases/gateway-operator.konghq.com_gatewayconfigurations.yaml @@ -17130,7 +17130,6 @@ spec: More info: http://kubernetes.io/docs/user-guide/annotations type: object externalTrafficPolicy: - default: Cluster description: |- ExternalTrafficPolicy describes how nodes distribute service traffic they receive on one of the Service's "externally-facing" addresses (NodePorts, @@ -17172,6 +17171,11 @@ spec: - ClusterIP type: string type: object + x-kubernetes-validations: + - message: Cannot set ExternalTrafficPolicy for ClusterIP + service. + rule: 'has(self.type) && self.type == ''ClusterIP'' + ? !has(self.externalTrafficPolicy) : true' type: object type: object pluginsToInstall: diff --git a/config/crd/dataplane/gateway-operator.konghq.com_dataplanes.yaml b/config/crd/dataplane/gateway-operator.konghq.com_dataplanes.yaml index 0a50fb8ab..1ba05c70e 100644 --- a/config/crd/dataplane/gateway-operator.konghq.com_dataplanes.yaml +++ b/config/crd/dataplane/gateway-operator.konghq.com_dataplanes.yaml @@ -8858,7 +8858,6 @@ spec: More info: http://kubernetes.io/docs/user-guide/annotations type: object externalTrafficPolicy: - default: Cluster description: |- ExternalTrafficPolicy describes how nodes distribute service traffic they receive on one of the Service's "externally-facing" addresses (NodePorts, @@ -8941,6 +8940,11 @@ spec: - ClusterIP type: string type: object + x-kubernetes-validations: + - message: Cannot set ExternalTrafficPolicy for ClusterIP + service. + rule: 'has(self.type) && self.type == ''ClusterIP'' ? !has(self.externalTrafficPolicy) + : true' type: object type: object pluginsToInstall: diff --git a/controller/dataplane/owned_resources.go b/controller/dataplane/owned_resources.go index ebbee554a..17b201c83 100644 --- a/controller/dataplane/owned_resources.go +++ b/controller/dataplane/owned_resources.go @@ -402,7 +402,8 @@ func ensureIngressServiceForDataPlane( existingService.Spec.Type = generatedService.Spec.Type updated = true } - if existingService.Spec.ExternalTrafficPolicy != generatedService.Spec.ExternalTrafficPolicy { + if generatedService.Spec.ExternalTrafficPolicy != "" && + existingService.Spec.ExternalTrafficPolicy != generatedService.Spec.ExternalTrafficPolicy { existingService.Spec.ExternalTrafficPolicy = generatedService.Spec.ExternalTrafficPolicy updated = true } diff --git a/pkg/utils/kubernetes/resources/services.go b/pkg/utils/kubernetes/resources/services.go index 71064b5fe..143dd2c8a 100644 --- a/pkg/utils/kubernetes/resources/services.go +++ b/pkg/utils/kubernetes/resources/services.go @@ -61,10 +61,11 @@ func GenerateNewIngressServiceForDataPlane(dataplane *operatorv1beta1.DataPlane, Selector: map[string]string{ "app": dataplane.Name, }, - Ports: DefaultDataPlaneIngressServicePorts, - ExternalTrafficPolicy: getDataPlaneIngressServiceExternalTrafficPolicy(dataplane), + Ports: DefaultDataPlaneIngressServicePorts, }, } + + setDataPlaneIngressServiceExternalTrafficPolicy(dataplane, svc) LabelObjectAsDataPlaneManaged(svc) for _, opt := range opts { @@ -112,12 +113,18 @@ func getDataPlaneIngressServiceType(dataplane *operatorv1beta1.DataPlane) corev1 return dataplane.Spec.Network.Services.Ingress.Type } -func getDataPlaneIngressServiceExternalTrafficPolicy(dataplane *operatorv1beta1.DataPlane) corev1.ServiceExternalTrafficPolicy { - if dataplane == nil || dataplane.Spec.Network.Services == nil { - return corev1.ServiceExternalTrafficPolicyCluster +func setDataPlaneIngressServiceExternalTrafficPolicy( + dataplane *operatorv1beta1.DataPlane, + svc *corev1.Service, +) { + if dataplane == nil || + dataplane.Spec.Network.Services == nil || + dataplane.Spec.Network.Services.Ingress == nil || + dataplane.Spec.Network.Services.Ingress.ExternalTrafficPolicy == "" { + return } - return dataplane.Spec.Network.Services.Ingress.ExternalTrafficPolicy + svc.Spec.ExternalTrafficPolicy = dataplane.Spec.Network.Services.Ingress.ExternalTrafficPolicy } // ServiceOpt is an option function for a Service. diff --git a/pkg/utils/kubernetes/resources/services_test.go b/pkg/utils/kubernetes/resources/services_test.go index d34aae6fd..aaadafee9 100644 --- a/pkg/utils/kubernetes/resources/services_test.go +++ b/pkg/utils/kubernetes/resources/services_test.go @@ -138,7 +138,6 @@ func TestGenerateNewIngressServiceForDataPlane(t *testing.T) { Selector: map[string]string{ "app": "dp-1", }, - ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeCluster, }, }, expectedErr: nil, @@ -217,6 +216,80 @@ func TestGenerateNewIngressServiceForDataPlane(t *testing.T) { }, expectedErr: nil, }, + { + name: "setting ExternalTrafficPolicy to Cluster", + dataplane: &operatorv1beta1.DataPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dp-1", + Namespace: "default", + UID: types.UID("1234"), + }, + TypeMeta: metav1.TypeMeta{ + APIVersion: "gateway.konghq.com/v1beta1", + Kind: "DataPlane", + }, + Spec: operatorv1beta1.DataPlaneSpec{ + DataPlaneOptions: operatorv1beta1.DataPlaneOptions{ + Network: operatorv1beta1.DataPlaneNetworkOptions{ + Services: &operatorv1beta1.DataPlaneServices{ + Ingress: &operatorv1beta1.DataPlaneServiceOptions{ + ServiceOptions: operatorv1beta1.ServiceOptions{ + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeCluster, + Type: corev1.ServiceTypeLoadBalancer, + }, + }, + }, + }, + }, + }, + }, + expectedSvc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "dataplane-ingress-dp-1-", + Namespace: "default", + Labels: map[string]string{ + "app": "dp-1", + "gateway-operator.konghq.com/dataplane-service-type": "ingress", + "gateway-operator.konghq.com/managed-by": "dataplane", + "konghq.com/gateway-operator": "dataplane", + }, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "gateway.konghq.com/v1beta1", + Kind: "DataPlane", + Name: "dp-1", + UID: "1234", + Controller: lo.ToPtr(true), + }, + }, + Finalizers: []string{ + "gateway-operator.konghq.com/wait-for-owner", + }, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + Ports: []corev1.ServicePort{ + { + Name: "http", + Protocol: corev1.ProtocolTCP, + Port: 80, + TargetPort: intstr.FromInt(8000), + }, + { + Name: "https", + Protocol: corev1.ProtocolTCP, + Port: 443, + TargetPort: intstr.FromInt(8443), + }, + }, + Selector: map[string]string{ + "app": "dp-1", + }, + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeCluster, + }, + }, + expectedErr: nil, + }, } for _, tc := range testCases {