From 3ab0feae70b4875f2d0aa78de35e14f6c4a21493 Mon Sep 17 00:00:00 2001 From: Gang Liu Date: Wed, 19 Oct 2022 15:37:18 +0800 Subject: [PATCH 1/5] add externalTrafficPolicy for envoy service Signed-off-by: Gang Liu --- .../v1alpha1/contourdeployment.go | 9 +++++++++ examples/contour/01-crds.yaml | 6 ++++++ examples/render/contour-deployment.yaml | 6 ++++++ .../render/contour-gateway-provisioner.yaml | 6 ++++++ examples/render/contour-gateway.yaml | 6 ++++++ examples/render/contour.yaml | 6 ++++++ internal/provisioner/controller/gateway.go | 5 +++++ .../provisioner/controller/gateway_test.go | 3 ++- .../provisioner/controller/gatewayclass.go | 12 +++++++++++- internal/provisioner/model/model.go | 10 +++++++++- .../provisioner/objects/service/service.go | 2 +- .../docs/main/config/api-reference.html | 18 ++++++++++++++++++ 12 files changed, 85 insertions(+), 4 deletions(-) diff --git a/apis/projectcontour/v1alpha1/contourdeployment.go b/apis/projectcontour/v1alpha1/contourdeployment.go index a327122f283..ec164723d3a 100644 --- a/apis/projectcontour/v1alpha1/contourdeployment.go +++ b/apis/projectcontour/v1alpha1/contourdeployment.go @@ -199,6 +199,15 @@ type NetworkPublishing struct { // +optional Type NetworkPublishingType `json:"type,omitempty"` + // ExternalTrafficPolicy describes how nodes distribute service traffic they + // receive on one of the Service's "externally-facing" addresses (NodePorts, ExternalIPs, + // and LoadBalancer IPs). + // + // If unset, defaults to "Local". + // + // +optional + ExternalTrafficPolicy corev1.ServiceExternalTrafficPolicyType `json:"externalTrafficPolicy,omitempty"` + // ServiceAnnotations is the annotations to add to // the provisioned Envoy service. // diff --git a/examples/contour/01-crds.yaml b/examples/contour/01-crds.yaml index f23726a0930..76a67bcbfa5 100644 --- a/examples/contour/01-crds.yaml +++ b/examples/contour/01-crds.yaml @@ -2567,6 +2567,12 @@ spec: description: NetworkPublishing defines how to expose Envoy to a network. properties: + externalTrafficPolicy: + description: "ExternalTrafficPolicy describes how nodes distribute + service traffic they receive on one of the Service's \"externally-facing\" + addresses (NodePorts, ExternalIPs, and LoadBalancer IPs). + \n If unset, defaults to \"Local\"." + type: string serviceAnnotations: additionalProperties: type: string diff --git a/examples/render/contour-deployment.yaml b/examples/render/contour-deployment.yaml index ee7a25db53a..c81193e4d12 100644 --- a/examples/render/contour-deployment.yaml +++ b/examples/render/contour-deployment.yaml @@ -2776,6 +2776,12 @@ spec: description: NetworkPublishing defines how to expose Envoy to a network. properties: + externalTrafficPolicy: + description: "ExternalTrafficPolicy describes how nodes distribute + service traffic they receive on one of the Service's \"externally-facing\" + addresses (NodePorts, ExternalIPs, and LoadBalancer IPs). + \n If unset, defaults to \"Local\"." + type: string serviceAnnotations: additionalProperties: type: string diff --git a/examples/render/contour-gateway-provisioner.yaml b/examples/render/contour-gateway-provisioner.yaml index 13c563a9ab3..22c92a41033 100644 --- a/examples/render/contour-gateway-provisioner.yaml +++ b/examples/render/contour-gateway-provisioner.yaml @@ -2581,6 +2581,12 @@ spec: description: NetworkPublishing defines how to expose Envoy to a network. properties: + externalTrafficPolicy: + description: "ExternalTrafficPolicy describes how nodes distribute + service traffic they receive on one of the Service's \"externally-facing\" + addresses (NodePorts, ExternalIPs, and LoadBalancer IPs). + \n If unset, defaults to \"Local\"." + type: string serviceAnnotations: additionalProperties: type: string diff --git a/examples/render/contour-gateway.yaml b/examples/render/contour-gateway.yaml index b6d3cf497e0..ecc49d936d1 100644 --- a/examples/render/contour-gateway.yaml +++ b/examples/render/contour-gateway.yaml @@ -2782,6 +2782,12 @@ spec: description: NetworkPublishing defines how to expose Envoy to a network. properties: + externalTrafficPolicy: + description: "ExternalTrafficPolicy describes how nodes distribute + service traffic they receive on one of the Service's \"externally-facing\" + addresses (NodePorts, ExternalIPs, and LoadBalancer IPs). + \n If unset, defaults to \"Local\"." + type: string serviceAnnotations: additionalProperties: type: string diff --git a/examples/render/contour.yaml b/examples/render/contour.yaml index 08919f343e8..da5e884d5bf 100644 --- a/examples/render/contour.yaml +++ b/examples/render/contour.yaml @@ -2776,6 +2776,12 @@ spec: description: NetworkPublishing defines how to expose Envoy to a network. properties: + externalTrafficPolicy: + description: "ExternalTrafficPolicy describes how nodes distribute + service traffic they receive on one of the Service's \"externally-facing\" + addresses (NodePorts, ExternalIPs, and LoadBalancer IPs). + \n If unset, defaults to \"Local\"." + type: string serviceAnnotations: additionalProperties: type: string diff --git a/internal/provisioner/controller/gateway.go b/internal/provisioner/controller/gateway.go index ba43981d1e1..c99c678358b 100644 --- a/internal/provisioner/controller/gateway.go +++ b/internal/provisioner/controller/gateway.go @@ -284,6 +284,11 @@ func (r *gatewayReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct if networkPublishing.Type != "" { contourModel.Spec.NetworkPublishing.Envoy.Type = networkPublishing.Type } + + if networkPublishing.ExternalTrafficPolicy != "" { + contourModel.Spec.NetworkPublishing.Envoy.ExternalTrafficPolicy = networkPublishing.ExternalTrafficPolicy + } + contourModel.Spec.NetworkPublishing.Envoy.ServiceAnnotations = networkPublishing.ServiceAnnotations } diff --git a/internal/provisioner/controller/gateway_test.go b/internal/provisioner/controller/gateway_test.go index 825fd147cc0..b2e5c83f0a0 100644 --- a/internal/provisioner/controller/gateway_test.go +++ b/internal/provisioner/controller/gateway_test.go @@ -1015,7 +1015,8 @@ func TestGatewayReconcile(t *testing.T) { Spec: contourv1alpha1.ContourDeploymentSpec{ Envoy: &contourv1alpha1.EnvoySettings{ NetworkPublishing: &contourv1alpha1.NetworkPublishing{ - Type: contourv1alpha1.ClusterIPServicePublishingType, + Type: contourv1alpha1.ClusterIPServicePublishingType, + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeCluster, ServiceAnnotations: map[string]string{ "key-1": "val-1", "key-2": "val-2", diff --git a/internal/provisioner/controller/gatewayclass.go b/internal/provisioner/controller/gatewayclass.go index c1992a40e83..3f5061ba102 100644 --- a/internal/provisioner/controller/gatewayclass.go +++ b/internal/provisioner/controller/gatewayclass.go @@ -18,8 +18,10 @@ import ( "fmt" "strings" - "github.com/go-logr/logr" contour_api_v1alpha1 "github.com/projectcontour/contour/apis/projectcontour/v1alpha1" + + "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -180,6 +182,14 @@ func (r *gatewayClassReconciler) Reconcile(ctx context.Context, req ctrl.Request params.Spec.Envoy.NetworkPublishing.Type) invalidParamsMessages = append(invalidParamsMessages, msg) } + + switch params.Spec.Envoy.NetworkPublishing.ExternalTrafficPolicy { + case "", corev1.ServiceExternalTrafficPolicyTypeCluster, corev1.ServiceExternalTrafficPolicyTypeLocal: + default: + msg := fmt.Sprintf("invalid ContourDeployment spec.envoy.networkPublishing.externalTrafficPolicy %q, must be Local or Cluster", + params.Spec.Envoy.NetworkPublishing.ExternalTrafficPolicy) + invalidParamsMessages = append(invalidParamsMessages, msg) + } } if params.Spec.Envoy.ExtraVolumeMounts != nil { diff --git a/internal/provisioner/model/model.go b/internal/provisioner/model/model.go index c0225748263..28ee15e276a 100644 --- a/internal/provisioner/model/model.go +++ b/internal/provisioner/model/model.go @@ -40,7 +40,8 @@ func Default(namespace, name string) *Contour { EnvoyReplicas: 2, // ignored if not provisioning Envoy as a deployment. NetworkPublishing: NetworkPublishing{ Envoy: EnvoyNetworkPublishing{ - Type: LoadBalancerServicePublishingType, + Type: LoadBalancerServicePublishingType, + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeLocal, ContainerPorts: []ContainerPort{ { Name: "http", @@ -361,6 +362,13 @@ type EnvoyNetworkPublishing struct { // ServiceAnnotations is a set of annotations to add to the provisioned Envoy service. ServiceAnnotations map[string]string + + // ExternalTrafficPolicy describes how nodes distribute service traffic they + // receive on one of the Service's "externally-facing" addresses (NodePorts, ExternalIPs, + // and LoadBalancer IPs). + // + // If unset, defaults to "Local". + ExternalTrafficPolicy corev1.ServiceExternalTrafficPolicyType } type NetworkPublishingType = contourv1alpha1.NetworkPublishingType diff --git a/internal/provisioner/objects/service/service.go b/internal/provisioner/objects/service/service.go index c8cde6c4359..4220da529db 100644 --- a/internal/provisioner/objects/service/service.go +++ b/internal/provisioner/objects/service/service.go @@ -250,7 +250,7 @@ func DesiredEnvoyService(contour *model.Contour) *corev1.Service { epType := contour.Spec.NetworkPublishing.Envoy.Type if epType == model.LoadBalancerServicePublishingType || epType == model.NodePortServicePublishingType { - svc.Spec.ExternalTrafficPolicy = corev1.ServiceExternalTrafficPolicyTypeLocal + svc.Spec.ExternalTrafficPolicy = contour.Spec.NetworkPublishing.Envoy.ExternalTrafficPolicy } switch epType { case model.LoadBalancerServicePublishingType: diff --git a/site/content/docs/main/config/api-reference.html b/site/content/docs/main/config/api-reference.html index 9be7712ace6..ca6dbdf2cef 100644 --- a/site/content/docs/main/config/api-reference.html +++ b/site/content/docs/main/config/api-reference.html @@ -6835,6 +6835,24 @@

NetworkPublishing +externalTrafficPolicy +
+ + +Kubernetes core/v1.ServiceExternalTrafficPolicyType + + + + +(Optional) +

ExternalTrafficPolicy describes how nodes distribute service traffic they +receive on one of the Service’s “externally-facing” addresses (NodePorts, ExternalIPs, +and LoadBalancer IPs).

+

If unset, defaults to “Local”.

+ + + + serviceAnnotations
From 13b0eb631fa1e299bd8fdfcfa89db8500e10d6a0 Mon Sep 17 00:00:00 2001 From: Gang Liu Date: Wed, 19 Oct 2022 15:46:49 +0800 Subject: [PATCH 2/5] add change log Signed-off-by: Gang Liu --- changelogs/unreleased/4803-izturn-small.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelogs/unreleased/4803-izturn-small.md diff --git a/changelogs/unreleased/4803-izturn-small.md b/changelogs/unreleased/4803-izturn-small.md new file mode 100644 index 00000000000..1c27ed1237a --- /dev/null +++ b/changelogs/unreleased/4803-izturn-small.md @@ -0,0 +1,2 @@ +Add Service/Envoy's + ExternalTrafficPolicy configurability to ContourDeployment resource. From f89ff57fb6cb511236d342e0b936f15f9139fd21 Mon Sep 17 00:00:00 2001 From: Gang Liu Date: Wed, 19 Oct 2022 15:47:04 +0800 Subject: [PATCH 3/5] fix typo Signed-off-by: Gang Liu --- changelogs/unreleased/4803-izturn-small.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/changelogs/unreleased/4803-izturn-small.md b/changelogs/unreleased/4803-izturn-small.md index 1c27ed1237a..e0b830e20b0 100644 --- a/changelogs/unreleased/4803-izturn-small.md +++ b/changelogs/unreleased/4803-izturn-small.md @@ -1,2 +1 @@ -Add Service/Envoy's - ExternalTrafficPolicy configurability to ContourDeployment resource. +Add Service/Envoy's ExternalTrafficPolicy configurability to ContourDeployment resource. From 6e7cbccc668589941971de8c77c0d440b5226411 Mon Sep 17 00:00:00 2001 From: Gang Liu Date: Mon, 31 Oct 2022 12:55:37 +0800 Subject: [PATCH 4/5] add ut Signed-off-by: Gang Liu --- .../provisioner/controller/gateway_test.go | 2 +- .../controller/gatewayclass_test.go | 34 +++++++++++++++++++ .../provisioner/objects/service/service.go | 9 ++--- .../objects/service/service_test.go | 7 +++- 4 files changed, 46 insertions(+), 6 deletions(-) diff --git a/internal/provisioner/controller/gateway_test.go b/internal/provisioner/controller/gateway_test.go index b2e5c83f0a0..f28e6842b55 100644 --- a/internal/provisioner/controller/gateway_test.go +++ b/internal/provisioner/controller/gateway_test.go @@ -1051,7 +1051,7 @@ func TestGatewayReconcile(t *testing.T) { }, } require.NoError(t, r.client.Get(context.Background(), keyFor(svc), svc)) - + assert.Equal(t, corev1.ServiceExternalTrafficPolicyTypeLocal, svc.Spec.ExternalTrafficPolicy) assert.Equal(t, corev1.ServiceTypeClusterIP, svc.Spec.Type) require.Len(t, svc.Annotations, 2) assert.Equal(t, "val-1", svc.Annotations["key-1"]) diff --git a/internal/provisioner/controller/gatewayclass_test.go b/internal/provisioner/controller/gatewayclass_test.go index bfa55d2ea75..e014f903a5f 100644 --- a/internal/provisioner/controller/gatewayclass_test.go +++ b/internal/provisioner/controller/gatewayclass_test.go @@ -318,6 +318,40 @@ func TestGatewayClassReconcile(t *testing.T) { Reason: string(gatewayv1beta1.GatewayClassReasonInvalidParameters), }, }, + "gatewayclass controlled by us with a valid parametersRef but invalid parameter values for ExternalTrafficPolicy gets Accepted: false condition": { + gatewayClass: &gatewayv1beta1.GatewayClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gatewayclass-1", + }, + Spec: gatewayv1beta1.GatewayClassSpec{ + ControllerName: "projectcontour.io/gateway-controller", + ParametersRef: &gatewayv1beta1.ParametersReference{ + Group: "projectcontour.io", + Kind: "ContourDeployment", + Name: "gatewayclass-params", + Namespace: gatewayapi.NamespacePtr("projectcontour"), + }, + }, + }, + params: &contourv1alpha1.ContourDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "projectcontour", + Name: "gatewayclass-params", + }, + Spec: contourv1alpha1.ContourDeploymentSpec{ + Envoy: &contourv1alpha1.EnvoySettings{ + NetworkPublishing: &contourv1alpha1.NetworkPublishing{ + ExternalTrafficPolicy: "invalid-external-traffic-policy", + }, + }, + }, + }, + wantCondition: &metav1.Condition{ + Type: string(gatewayv1beta1.GatewayClassConditionStatusAccepted), + Status: metav1.ConditionFalse, + Reason: string(gatewayv1beta1.GatewayClassReasonInvalidParameters), + }, + }, } for name, tc := range tests { diff --git a/internal/provisioner/objects/service/service.go b/internal/provisioner/objects/service/service.go index 4220da529db..5605706d71c 100644 --- a/internal/provisioner/objects/service/service.go +++ b/internal/provisioner/objects/service/service.go @@ -196,10 +196,11 @@ func DesiredEnvoyService(contour *model.Contour) *corev1.Service { Labels: model.CommonLabels(contour), }, Spec: corev1.ServiceSpec{ - Ports: ports, - Selector: dataplane.EnvoyPodSelector(contour).MatchLabels, - SessionAffinity: corev1.ServiceAffinityNone, - LoadBalancerIP: contour.Spec.NetworkPublishing.Envoy.LoadBalancer.LoadBalancerIP, + Ports: ports, + Selector: dataplane.EnvoyPodSelector(contour).MatchLabels, + SessionAffinity: corev1.ServiceAffinityNone, + LoadBalancerIP: contour.Spec.NetworkPublishing.Envoy.LoadBalancer.LoadBalancerIP, + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeLocal, }, } diff --git a/internal/provisioner/objects/service/service_test.go b/internal/provisioner/objects/service/service_test.go index 5c72b3d84ae..1775c5f6019 100644 --- a/internal/provisioner/objects/service/service_test.go +++ b/internal/provisioner/objects/service/service_test.go @@ -177,14 +177,19 @@ func TestDesiredEnvoyService(t *testing.T) { checkServiceHasPortName(t, svc, "https") checkServiceHasPortProtocol(t, svc, corev1.ProtocolTCP) + cntr.Spec.NetworkPublishing.Envoy.Type = model.ClusterIPServicePublishingType + svc = DesiredEnvoyService(cntr) + checkServiceHasExternalTrafficPolicy(t, svc, corev1.ServiceExternalTrafficPolicyTypeLocal) + // Check LB annotations for the different provider types, starting with AWS ELB (the default // if AWS provider params are not passed). cntr.Spec.NetworkPublishing.Envoy.Type = model.LoadBalancerServicePublishingType + cntr.Spec.NetworkPublishing.Envoy.ExternalTrafficPolicy = corev1.ServiceExternalTrafficPolicyTypeCluster cntr.Spec.NetworkPublishing.Envoy.LoadBalancer.Scope = model.ExternalLoadBalancer cntr.Spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Type = model.AWSLoadBalancerProvider svc = DesiredEnvoyService(cntr) checkServiceHasType(t, svc, corev1.ServiceTypeLoadBalancer) - checkServiceHasExternalTrafficPolicy(t, svc, corev1.ServiceExternalTrafficPolicyTypeLocal) + checkServiceHasExternalTrafficPolicy(t, svc, corev1.ServiceExternalTrafficPolicyTypeCluster) checkServiceHasAnnotations(t, svc, awsLbBackendProtoAnnotation, awsLBProxyProtocolAnnotation) // Test proxy protocol for AWS Classic load balancer (when provider params are specified). From 021ccd1b0f1b37631bacad734e142d9a5ab6e8da Mon Sep 17 00:00:00 2001 From: Gang Liu Date: Tue, 1 Nov 2022 11:48:41 +0800 Subject: [PATCH 5/5] little refactor Signed-off-by: Gang Liu --- internal/provisioner/controller/gateway_test.go | 6 +++--- internal/provisioner/objects/service/service.go | 9 ++++----- internal/provisioner/objects/service/service_test.go | 9 ++++++++- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/internal/provisioner/controller/gateway_test.go b/internal/provisioner/controller/gateway_test.go index f28e6842b55..8610b9d38c6 100644 --- a/internal/provisioner/controller/gateway_test.go +++ b/internal/provisioner/controller/gateway_test.go @@ -1015,7 +1015,7 @@ func TestGatewayReconcile(t *testing.T) { Spec: contourv1alpha1.ContourDeploymentSpec{ Envoy: &contourv1alpha1.EnvoySettings{ NetworkPublishing: &contourv1alpha1.NetworkPublishing{ - Type: contourv1alpha1.ClusterIPServicePublishingType, + Type: contourv1alpha1.NodePortServicePublishingType, ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeCluster, ServiceAnnotations: map[string]string{ "key-1": "val-1", @@ -1051,8 +1051,8 @@ func TestGatewayReconcile(t *testing.T) { }, } require.NoError(t, r.client.Get(context.Background(), keyFor(svc), svc)) - assert.Equal(t, corev1.ServiceExternalTrafficPolicyTypeLocal, svc.Spec.ExternalTrafficPolicy) - assert.Equal(t, corev1.ServiceTypeClusterIP, svc.Spec.Type) + assert.Equal(t, corev1.ServiceExternalTrafficPolicyTypeCluster, svc.Spec.ExternalTrafficPolicy) + assert.Equal(t, corev1.ServiceTypeNodePort, svc.Spec.Type) require.Len(t, svc.Annotations, 2) assert.Equal(t, "val-1", svc.Annotations["key-1"]) assert.Equal(t, "val-2", svc.Annotations["key-2"]) diff --git a/internal/provisioner/objects/service/service.go b/internal/provisioner/objects/service/service.go index 5605706d71c..4220da529db 100644 --- a/internal/provisioner/objects/service/service.go +++ b/internal/provisioner/objects/service/service.go @@ -196,11 +196,10 @@ func DesiredEnvoyService(contour *model.Contour) *corev1.Service { Labels: model.CommonLabels(contour), }, Spec: corev1.ServiceSpec{ - Ports: ports, - Selector: dataplane.EnvoyPodSelector(contour).MatchLabels, - SessionAffinity: corev1.ServiceAffinityNone, - LoadBalancerIP: contour.Spec.NetworkPublishing.Envoy.LoadBalancer.LoadBalancerIP, - ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeLocal, + Ports: ports, + Selector: dataplane.EnvoyPodSelector(contour).MatchLabels, + SessionAffinity: corev1.ServiceAffinityNone, + LoadBalancerIP: contour.Spec.NetworkPublishing.Envoy.LoadBalancer.LoadBalancerIP, }, } diff --git a/internal/provisioner/objects/service/service_test.go b/internal/provisioner/objects/service/service_test.go index 1775c5f6019..1746cbd7c36 100644 --- a/internal/provisioner/objects/service/service_test.go +++ b/internal/provisioner/objects/service/service_test.go @@ -124,6 +124,13 @@ func checkServiceHasExternalTrafficPolicy(t *testing.T, svc *corev1.Service, pol } } +func checkServiceHasNoExternalTrafficPolicy(t *testing.T, svc *corev1.Service) { + t.Helper() + + if svc.Spec.ExternalTrafficPolicy != "" { + t.Errorf("service has invalid external traffic policy type %s", svc.Spec.ExternalTrafficPolicy) + } +} func checkServiceHasLoadBalancerAddress(t *testing.T, svc *corev1.Service, address string) { t.Helper() @@ -179,7 +186,7 @@ func TestDesiredEnvoyService(t *testing.T) { cntr.Spec.NetworkPublishing.Envoy.Type = model.ClusterIPServicePublishingType svc = DesiredEnvoyService(cntr) - checkServiceHasExternalTrafficPolicy(t, svc, corev1.ServiceExternalTrafficPolicyTypeLocal) + checkServiceHasNoExternalTrafficPolicy(t, svc) // Check LB annotations for the different provider types, starting with AWS ELB (the default // if AWS provider params are not passed).