From 7aa20122bcf26c1a394cfd2c4575d15acc853baa Mon Sep 17 00:00:00 2001 From: Andrew Sy Kim Date: Tue, 4 Jan 2022 20:31:08 -0500 Subject: [PATCH 1/3] add integration test to check that Service internalTrafficPolicy is no longer defaulting when Type is ExternalName Signed-off-by: Andrew Sy Kim --- test/integration/service/service_test.go | 75 ++++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 test/integration/service/service_test.go diff --git a/test/integration/service/service_test.go b/test/integration/service/service_test.go new file mode 100644 index 00000000000..fea7658f346 --- /dev/null +++ b/test/integration/service/service_test.go @@ -0,0 +1,75 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package service + +import ( + "context" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + clientset "k8s.io/client-go/kubernetes" + restclient "k8s.io/client-go/rest" + "k8s.io/kubernetes/test/integration/framework" +) + +// Test_ExternalNameServiceStopsDefaultingInternalTrafficPolicy tests that Services no longer default +// the internalTrafficPolicy field when Type is ExternalName. This test exists due to historic reasons where +// the internalTrafficPolicy field was being defaulted in older versions. New versions stop defauting the +// field and drop on read, but for compatibility reasons we still accept the field. +func Test_ExternalNameServiceStopsDefaultingInternalTrafficPolicy(t *testing.T) { + controlPlaneConfig := framework.NewIntegrationTestControlPlaneConfig() + _, server, closeFn := framework.RunAnAPIServer(controlPlaneConfig) + defer closeFn() + + config := restclient.Config{Host: server.URL} + client, err := clientset.NewForConfig(&config) + if err != nil { + t.Fatalf("Error creating clientset: %v", err) + } + + ns := framework.CreateTestingNamespace("test-external-name-drops-internal-traffic-policy", server, t) + defer framework.DeleteTestingNamespace(ns, server, t) + + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-123", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeExternalName, + ExternalName: "foo.bar.com", + }, + } + + service, err = client.CoreV1().Services(ns.Name).Create(context.TODO(), service, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Error creating test service: %v", err) + } + + if service.Spec.InternalTrafficPolicy != nil { + t.Errorf("service internalTrafficPolicy should be droppped but is set: %v", service.Spec.InternalTrafficPolicy) + } + + service, err = client.CoreV1().Services(ns.Name).Get(context.TODO(), service.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("error getting service: %v", err) + } + + if service.Spec.InternalTrafficPolicy != nil { + t.Errorf("service internalTrafficPolicy should be droppped but is set: %v", service.Spec.InternalTrafficPolicy) + } +} From 9c3c3d8a3a9f3708102ad382850e5868c68a4380 Mon Sep 17 00:00:00 2001 From: Andrew Sy Kim Date: Tue, 4 Jan 2022 20:32:10 -0500 Subject: [PATCH 2/3] add integration test to validate that ExternalName Services can set internalTrafficPolicy, but the field is dropped on read Signed-off-by: Andrew Sy Kim --- test/integration/service/service_test.go | 48 ++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/test/integration/service/service_test.go b/test/integration/service/service_test.go index fea7658f346..a0a68583f19 100644 --- a/test/integration/service/service_test.go +++ b/test/integration/service/service_test.go @@ -73,3 +73,51 @@ func Test_ExternalNameServiceStopsDefaultingInternalTrafficPolicy(t *testing.T) t.Errorf("service internalTrafficPolicy should be droppped but is set: %v", service.Spec.InternalTrafficPolicy) } } + +// Test_ExternalNameServiceDropsInternalTrafficPolicy tests that Services accepts the internalTrafficPolicy field on Create, +// but drops the field on read. This test exists due to historic reasons where the internalTrafficPolicy field was being defaulted +// in older versions. New versions stop defauting the field and drop on read, but for compatibility reasons we still accept the field. +func Test_ExternalNameServiceDropsInternalTrafficPolicy(t *testing.T) { + controlPlaneConfig := framework.NewIntegrationTestControlPlaneConfig() + _, server, closeFn := framework.RunAnAPIServer(controlPlaneConfig) + defer closeFn() + + config := restclient.Config{Host: server.URL} + client, err := clientset.NewForConfig(&config) + if err != nil { + t.Fatalf("Error creating clientset: %v", err) + } + + ns := framework.CreateTestingNamespace("test-external-name-drops-internal-traffic-policy", server, t) + defer framework.DeleteTestingNamespace(ns, server, t) + + internalTrafficPolicy := corev1.ServiceInternalTrafficPolicyCluster + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-123", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeExternalName, + ExternalName: "foo.bar.com", + InternalTrafficPolicy: &internalTrafficPolicy, + }, + } + + service, err = client.CoreV1().Services(ns.Name).Create(context.TODO(), service, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Error creating test service: %v", err) + } + + if service.Spec.InternalTrafficPolicy != nil { + t.Errorf("service internalTrafficPolicy should be droppped but is set: %v", service.Spec.InternalTrafficPolicy) + } + + service, err = client.CoreV1().Services(ns.Name).Get(context.TODO(), service.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("error getting service: %v", err) + } + + if service.Spec.InternalTrafficPolicy != nil { + t.Errorf("service internalTrafficPolicy should be droppped but is set: %v", service.Spec.InternalTrafficPolicy) + } +} From aead636249eb9f400c0cc54118baa46126c01a26 Mon Sep 17 00:00:00 2001 From: Andrew Sy Kim Date: Tue, 4 Jan 2022 20:32:50 -0500 Subject: [PATCH 3/3] add integration test validating that converting a Service to ExternalName results in the field being dropped on read Signed-off-by: Andrew Sy Kim --- test/integration/service/service_test.go | 65 ++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/test/integration/service/service_test.go b/test/integration/service/service_test.go index a0a68583f19..fe6fe2a3d11 100644 --- a/test/integration/service/service_test.go +++ b/test/integration/service/service_test.go @@ -121,3 +121,68 @@ func Test_ExternalNameServiceDropsInternalTrafficPolicy(t *testing.T) { t.Errorf("service internalTrafficPolicy should be droppped but is set: %v", service.Spec.InternalTrafficPolicy) } } + +// Test_ConvertingToExternalNameServiceDropsInternalTrafficPolicy tests that converting a Service to Type=ExternalName +// results in the internalTrafficPolicy field being dropped.This test exists due to historic reasons where the internalTrafficPolicy +// field was being defaulted in older versions. New versions stop defauting the field and drop on read, but for compatibility reasons +// we still accept the field. +func Test_ConvertingToExternalNameServiceDropsInternalTrafficPolicy(t *testing.T) { + controlPlaneConfig := framework.NewIntegrationTestControlPlaneConfig() + _, server, closeFn := framework.RunAnAPIServer(controlPlaneConfig) + defer closeFn() + + config := restclient.Config{Host: server.URL} + client, err := clientset.NewForConfig(&config) + if err != nil { + t.Fatalf("Error creating clientset: %v", err) + } + + ns := framework.CreateTestingNamespace("test-external-name-drops-internal-traffic-policy", server, t) + defer framework.DeleteTestingNamespace(ns, server, t) + + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-123", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + Ports: []corev1.ServicePort{{ + Port: int32(80), + }}, + Selector: map[string]string{ + "foo": "bar", + }, + }, + } + + service, err = client.CoreV1().Services(ns.Name).Create(context.TODO(), service, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Error creating test service: %v", err) + } + + if *service.Spec.InternalTrafficPolicy != corev1.ServiceInternalTrafficPolicyCluster { + t.Error("service internalTrafficPolicy was not set for clusterIP Service") + } + + newService := service.DeepCopy() + newService.Spec.Type = corev1.ServiceTypeExternalName + newService.Spec.ExternalName = "foo.bar.com" + + service, err = client.CoreV1().Services(ns.Name).Update(context.TODO(), newService, metav1.UpdateOptions{}) + if err != nil { + t.Fatalf("error getting service: %v", err) + } + + if service.Spec.InternalTrafficPolicy != nil { + t.Errorf("service internalTrafficPolicy should be droppped but is set: %v", service.Spec.InternalTrafficPolicy) + } + + service, err = client.CoreV1().Services(ns.Name).Get(context.TODO(), service.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("error getting service: %v", err) + } + + if service.Spec.InternalTrafficPolicy != nil { + t.Errorf("service internalTrafficPolicy should be droppped but is set: %v", service.Spec.InternalTrafficPolicy) + } +}