From 0c7548f0204d901ccd0f6a0932a428b75904bde1 Mon Sep 17 00:00:00 2001 From: Rob Scott Date: Tue, 22 Oct 2019 15:39:49 -0700 Subject: [PATCH] Setting Hostname from Pods on EndpointSlice to match Endpoints behavior. This was an oversight in the initial EndpointSlice release. This update will ensure that Endpoints and EndpointSlices use the same logic to set the Hostname attribute. --- .../endpoint/endpoints_controller.go | 7 +- .../endpointslice_controller_test.go | 1 + pkg/controller/endpointslice/reconciler.go | 3 +- .../endpointslice/reconciler_test.go | 14 +-- pkg/controller/endpointslice/utils.go | 17 +++- pkg/controller/endpointslice/utils_test.go | 50 ++++++++-- .../util/endpoint/controller_utils.go | 6 ++ .../util/endpoint/controller_utils_test.go | 91 ++++++++++++++++--- 8 files changed, 151 insertions(+), 38 deletions(-) diff --git a/pkg/controller/endpoint/endpoints_controller.go b/pkg/controller/endpoint/endpoints_controller.go index 14996408cfa..0a7b592932e 100644 --- a/pkg/controller/endpoint/endpoints_controller.go +++ b/pkg/controller/endpoint/endpoints_controller.go @@ -427,11 +427,10 @@ func (e *EndpointController) syncService(key string) error { klog.V(2).Infof("failed to find endpoint for service:%v with ClusterIP:%v on pod:%v with error:%v", service.Name, service.Spec.ClusterIP, pod.Name, err) continue } - epa := *ep - hostname := pod.Spec.Hostname - if len(hostname) > 0 && pod.Spec.Subdomain == service.Name && service.Namespace == pod.Namespace { - epa.Hostname = hostname + epa := *ep + if endpointutil.ShouldSetHostname(pod, service) { + epa.Hostname = pod.Spec.Hostname } // Allow headless service not to have ports. diff --git a/pkg/controller/endpointslice/endpointslice_controller_test.go b/pkg/controller/endpointslice/endpointslice_controller_test.go index 7db25d847bd..752d2e6df81 100644 --- a/pkg/controller/endpointslice/endpointslice_controller_test.go +++ b/pkg/controller/endpointslice/endpointslice_controller_test.go @@ -280,6 +280,7 @@ func TestSyncServiceFull(t *testing.T) { Protocol: protoPtr(v1.ProtocolSCTP), Port: int32Ptr(int32(3456)), }}, slice.Ports) + assert.ElementsMatch(t, []discovery.Endpoint{{ Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, Addresses: []string{"1.2.3.4"}, diff --git a/pkg/controller/endpointslice/reconciler.go b/pkg/controller/endpointslice/reconciler.go index ae6ed299e9a..3f19493f1f4 100644 --- a/pkg/controller/endpointslice/reconciler.go +++ b/pkg/controller/endpointslice/reconciler.go @@ -90,7 +90,8 @@ func (r *reconciler) reconcile(service *corev1.Service, pods []*corev1.Pod, exis if err != nil { return err } - endpoint := podToEndpoint(pod, node, service.Spec.PublishNotReadyAddresses) + + endpoint := podToEndpoint(pod, node, service) desiredEndpointsByPortMap[epHash].Insert(&endpoint) numDesiredEndpoints++ } diff --git a/pkg/controller/endpointslice/reconciler_test.go b/pkg/controller/endpointslice/reconciler_test.go index bb94ba2be54..40c48a9238c 100644 --- a/pkg/controller/endpointslice/reconciler_test.go +++ b/pkg/controller/endpointslice/reconciler_test.go @@ -224,13 +224,13 @@ func TestReconcileEndpointSlicesSomePreexisting(t *testing.T) { // have approximately 1/4 in first slice endpointSlice1 := newEmptyEndpointSlice(1, namespace, endpointMeta, svc) for i := 1; i < len(pods)-4; i += 4 { - endpointSlice1.Endpoints = append(endpointSlice1.Endpoints, podToEndpoint(pods[i], &corev1.Node{}, false)) + endpointSlice1.Endpoints = append(endpointSlice1.Endpoints, podToEndpoint(pods[i], &corev1.Node{}, &svc)) } // have approximately 1/4 in second slice endpointSlice2 := newEmptyEndpointSlice(2, namespace, endpointMeta, svc) for i := 3; i < len(pods)-4; i += 4 { - endpointSlice2.Endpoints = append(endpointSlice2.Endpoints, podToEndpoint(pods[i], &corev1.Node{}, false)) + endpointSlice2.Endpoints = append(endpointSlice2.Endpoints, podToEndpoint(pods[i], &corev1.Node{}, &svc)) } existingSlices := []*discovery.EndpointSlice{endpointSlice1, endpointSlice2} @@ -276,13 +276,13 @@ func TestReconcileEndpointSlicesSomePreexistingWorseAllocation(t *testing.T) { // have approximately 1/4 in first slice endpointSlice1 := newEmptyEndpointSlice(1, namespace, endpointMeta, svc) for i := 1; i < len(pods)-4; i += 4 { - endpointSlice1.Endpoints = append(endpointSlice1.Endpoints, podToEndpoint(pods[i], &corev1.Node{}, false)) + endpointSlice1.Endpoints = append(endpointSlice1.Endpoints, podToEndpoint(pods[i], &corev1.Node{}, &svc)) } // have approximately 1/4 in second slice endpointSlice2 := newEmptyEndpointSlice(2, namespace, endpointMeta, svc) for i := 3; i < len(pods)-4; i += 4 { - endpointSlice2.Endpoints = append(endpointSlice2.Endpoints, podToEndpoint(pods[i], &corev1.Node{}, false)) + endpointSlice2.Endpoints = append(endpointSlice2.Endpoints, podToEndpoint(pods[i], &corev1.Node{}, &svc)) } existingSlices := []*discovery.EndpointSlice{endpointSlice1, endpointSlice2} @@ -358,7 +358,7 @@ func TestReconcileEndpointSlicesRecycling(t *testing.T) { if i%30 == 0 { existingSlices = append(existingSlices, newEmptyEndpointSlice(sliceNum, namespace, endpointMeta, svc)) } - existingSlices[sliceNum].Endpoints = append(existingSlices[sliceNum].Endpoints, podToEndpoint(pod, &corev1.Node{}, false)) + existingSlices[sliceNum].Endpoints = append(existingSlices[sliceNum].Endpoints, podToEndpoint(pod, &corev1.Node{}, &svc)) } createEndpointSlices(t, client, namespace, existingSlices) @@ -396,7 +396,7 @@ func TestReconcileEndpointSlicesUpdatePacking(t *testing.T) { slice1 := newEmptyEndpointSlice(1, namespace, endpointMeta, svc) for i := 0; i < 80; i++ { pod := newPod(i, namespace, true, 1) - slice1.Endpoints = append(slice1.Endpoints, podToEndpoint(pod, &corev1.Node{}, false)) + slice1.Endpoints = append(slice1.Endpoints, podToEndpoint(pod, &corev1.Node{}, &svc)) pods = append(pods, pod) } existingSlices = append(existingSlices, slice1) @@ -404,7 +404,7 @@ func TestReconcileEndpointSlicesUpdatePacking(t *testing.T) { slice2 := newEmptyEndpointSlice(2, namespace, endpointMeta, svc) for i := 100; i < 120; i++ { pod := newPod(i, namespace, true, 1) - slice2.Endpoints = append(slice2.Endpoints, podToEndpoint(pod, &corev1.Node{}, false)) + slice2.Endpoints = append(slice2.Endpoints, podToEndpoint(pod, &corev1.Node{}, &svc)) pods = append(pods, pod) } existingSlices = append(existingSlices, slice2) diff --git a/pkg/controller/endpointslice/utils.go b/pkg/controller/endpointslice/utils.go index 0ecb1dc94d2..aadfc6efdb1 100644 --- a/pkg/controller/endpointslice/utils.go +++ b/pkg/controller/endpointslice/utils.go @@ -30,13 +30,14 @@ import ( podutil "k8s.io/kubernetes/pkg/api/v1/pod" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/apis/discovery/validation" + endpointutil "k8s.io/kubernetes/pkg/controller/util/endpoint" ) // podEndpointChanged returns true if the results of podToEndpoint are different // for the pods passed to this function. func podEndpointChanged(pod1, pod2 *corev1.Pod) bool { - endpoint1 := podToEndpoint(pod1, &corev1.Node{}, false) - endpoint2 := podToEndpoint(pod2, &corev1.Node{}, false) + endpoint1 := podToEndpoint(pod1, &corev1.Node{}, &corev1.Service{Spec: corev1.ServiceSpec{}}) + endpoint2 := podToEndpoint(pod2, &corev1.Node{}, &corev1.Service{Spec: corev1.ServiceSpec{}}) endpoint1.TargetRef.ResourceVersion = "" endpoint2.TargetRef.ResourceVersion = "" @@ -45,7 +46,7 @@ func podEndpointChanged(pod1, pod2 *corev1.Pod) bool { } // podToEndpoint returns an Endpoint object generated from a Pod and Node. -func podToEndpoint(pod *corev1.Pod, node *corev1.Node, publishNotReadyAddresses bool) discovery.Endpoint { +func podToEndpoint(pod *corev1.Pod, node *corev1.Node, service *corev1.Service) discovery.Endpoint { // Build out topology information. This is currently limited to hostname, // zone, and region, but this will be expanded in the future. topology := map[string]string{} @@ -67,8 +68,8 @@ func podToEndpoint(pod *corev1.Pod, node *corev1.Node, publishNotReadyAddresses } } - ready := publishNotReadyAddresses || podutil.IsPodReady(pod) - return discovery.Endpoint{ + ready := service.Spec.PublishNotReadyAddresses || podutil.IsPodReady(pod) + ep := discovery.Endpoint{ Addresses: getEndpointAddresses(pod.Status), Conditions: discovery.EndpointConditions{ Ready: &ready, @@ -82,6 +83,12 @@ func podToEndpoint(pod *corev1.Pod, node *corev1.Node, publishNotReadyAddresses ResourceVersion: pod.ObjectMeta.ResourceVersion, }, } + + if endpointutil.ShouldSetHostname(pod, service) { + ep.Hostname = &pod.Spec.Hostname + } + + return ep } // getEndpointPorts returns a list of EndpointPorts generated from a Service diff --git a/pkg/controller/endpointslice/utils_test.go b/pkg/controller/endpointslice/utils_test.go index a0d5c31d2e1..610f83e8c18 100644 --- a/pkg/controller/endpointslice/utils_test.go +++ b/pkg/controller/endpointslice/utils_test.go @@ -74,11 +74,17 @@ func TestNewEndpointSlice(t *testing.T) { func TestPodToEndpoint(t *testing.T) { ns := "test" + svc, _ := newServiceAndEndpointMeta("foo", ns) + svcPublishNotReady, _ := newServiceAndEndpointMeta("publishnotready", ns) + svcPublishNotReady.Spec.PublishNotReadyAddresses = true readyPod := newPod(1, ns, true, 1) + readyPodHostname := newPod(1, ns, true, 1) + readyPodHostname.Spec.Subdomain = svc.Name + readyPodHostname.Spec.Hostname = "example-hostname" + unreadyPod := newPod(1, ns, false, 1) multiIPPod := newPod(1, ns, true, 1) - multiIPPod.Status.PodIPs = []v1.PodIP{{IP: "1.2.3.4"}, {IP: "1234::5678:0000:0000:9abc:def0"}} node1 := &v1.Node{ @@ -95,12 +101,14 @@ func TestPodToEndpoint(t *testing.T) { name string pod *v1.Pod node *v1.Node + svc *v1.Service expectedEndpoint discovery.Endpoint publishNotReadyAddresses bool }{ { name: "Ready pod", pod: readyPod, + svc: &svc, expectedEndpoint: discovery.Endpoint{ Addresses: []string{"1.2.3.5"}, Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, @@ -115,9 +123,9 @@ func TestPodToEndpoint(t *testing.T) { }, }, { - name: "Ready pod + publishNotReadyAddresses", - pod: readyPod, - publishNotReadyAddresses: true, + name: "Ready pod + publishNotReadyAddresses", + pod: readyPod, + svc: &svcPublishNotReady, expectedEndpoint: discovery.Endpoint{ Addresses: []string{"1.2.3.5"}, Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, @@ -134,6 +142,7 @@ func TestPodToEndpoint(t *testing.T) { { name: "Unready pod", pod: unreadyPod, + svc: &svc, expectedEndpoint: discovery.Endpoint{ Addresses: []string{"1.2.3.5"}, Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(false)}, @@ -148,9 +157,9 @@ func TestPodToEndpoint(t *testing.T) { }, }, { - name: "Unready pod + publishNotReadyAddresses", - pod: unreadyPod, - publishNotReadyAddresses: true, + name: "Unready pod + publishNotReadyAddresses", + pod: unreadyPod, + svc: &svcPublishNotReady, expectedEndpoint: discovery.Endpoint{ Addresses: []string{"1.2.3.5"}, Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, @@ -168,6 +177,7 @@ func TestPodToEndpoint(t *testing.T) { name: "Ready pod + node labels", pod: readyPod, node: node1, + svc: &svc, expectedEndpoint: discovery.Endpoint{ Addresses: []string{"1.2.3.5"}, Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, @@ -189,6 +199,7 @@ func TestPodToEndpoint(t *testing.T) { name: "Multi IP Ready pod + node labels", pod: multiIPPod, node: node1, + svc: &svc, expectedEndpoint: discovery.Endpoint{ Addresses: []string{"1.2.3.4", "1234::5678:0000:0000:9abc:def0"}, Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, @@ -206,11 +217,34 @@ func TestPodToEndpoint(t *testing.T) { }, }, }, + { + name: "Ready pod + hostname", + pod: readyPodHostname, + node: node1, + svc: &svc, + expectedEndpoint: discovery.Endpoint{ + Addresses: []string{"1.2.3.5"}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, + Hostname: &readyPodHostname.Spec.Hostname, + Topology: map[string]string{ + "kubernetes.io/hostname": "node-1", + "topology.kubernetes.io/zone": "us-central1-a", + "topology.kubernetes.io/region": "us-central1", + }, + TargetRef: &v1.ObjectReference{ + Kind: "Pod", + Namespace: ns, + Name: readyPodHostname.Name, + UID: readyPodHostname.UID, + ResourceVersion: readyPodHostname.ResourceVersion, + }, + }, + }, } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { - endpoint := podToEndpoint(testCase.pod, testCase.node, testCase.publishNotReadyAddresses) + endpoint := podToEndpoint(testCase.pod, testCase.node, testCase.svc) if !reflect.DeepEqual(testCase.expectedEndpoint, endpoint) { t.Errorf("Expected endpoint: %v, got: %v", testCase.expectedEndpoint, endpoint) } diff --git a/pkg/controller/util/endpoint/controller_utils.go b/pkg/controller/util/endpoint/controller_utils.go index 3c5383da842..cf07089e9b2 100644 --- a/pkg/controller/util/endpoint/controller_utils.go +++ b/pkg/controller/util/endpoint/controller_utils.go @@ -143,6 +143,12 @@ func ShouldPodBeInEndpoints(pod *v1.Pod) bool { return true } +// ShouldSetHostname returns true if the Hostname attribute should be set on an +// Endpoints Address or EndpointSlice Endpoint. +func ShouldSetHostname(pod *v1.Pod, svc *v1.Service) bool { + return len(pod.Spec.Hostname) > 0 && pod.Spec.Subdomain == svc.Name && svc.Namespace == pod.Namespace +} + // PodChanged returns two boolean values, the first returns true if the pod. // has changed, the second value returns true if the pod labels have changed. func PodChanged(oldPod, newPod *v1.Pod, endpointChanged EndpointsMatch) (bool, bool) { diff --git a/pkg/controller/util/endpoint/controller_utils_test.go b/pkg/controller/util/endpoint/controller_utils_test.go index bccfd66ae74..90ce9aec2ba 100644 --- a/pkg/controller/util/endpoint/controller_utils_test.go +++ b/pkg/controller/util/endpoint/controller_utils_test.go @@ -81,16 +81,19 @@ func TestDetermineNeededServiceUpdates(t *testing.T) { union: sets.NewString(), }, } - for _, testCase := range testCases { - retval := determineNeededServiceUpdates(testCase.a, testCase.b, false) - if !retval.Equal(testCase.xor) { - t.Errorf("%s (with podChanged=false): expected: %v got: %v", testCase.name, testCase.xor.List(), retval.List()) - } - retval = determineNeededServiceUpdates(testCase.a, testCase.b, true) - if !retval.Equal(testCase.union) { - t.Errorf("%s (with podChanged=true): expected: %v got: %v", testCase.name, testCase.union.List(), retval.List()) - } + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + retval := determineNeededServiceUpdates(testCase.a, testCase.b, false) + if !retval.Equal(testCase.xor) { + t.Errorf("%s (with podChanged=false): expected: %v got: %v", testCase.name, testCase.xor.List(), retval.List()) + } + + retval = determineNeededServiceUpdates(testCase.a, testCase.b, true) + if !retval.Equal(testCase.union) { + t.Errorf("%s (with podChanged=true): expected: %v got: %v", testCase.name, testCase.union.List(), retval.List()) + } + }) } } @@ -224,11 +227,73 @@ func TestShouldPodBeInEndpoints(t *testing.T) { expected: true, }, } + for _, test := range testCases { - result := ShouldPodBeInEndpoints(test.pod) - if result != test.expected { - t.Errorf("%s: expected : %t, got: %t", test.name, test.expected, result) - } + t.Run(test.name, func(t *testing.T) { + result := ShouldPodBeInEndpoints(test.pod) + if result != test.expected { + t.Errorf("expected: %t, got: %t", test.expected, result) + } + }) + } +} + +func TestShouldSetHostname(t *testing.T) { + testCases := map[string]struct { + pod *v1.Pod + service *v1.Service + expected bool + }{ + "all matching": { + pod: genSimplePod("ns", "foo", "svc-name"), + service: genSimpleSvc("ns", "svc-name"), + expected: true, + }, + "all matching, hostname not set": { + pod: genSimplePod("ns", "", "svc-name"), + service: genSimpleSvc("ns", "svc-name"), + expected: false, + }, + "all set, different name/subdomain": { + pod: genSimplePod("ns", "hostname", "subdomain"), + service: genSimpleSvc("ns", "name"), + expected: false, + }, + "all set, different namespace": { + pod: genSimplePod("ns1", "hostname", "svc-name"), + service: genSimpleSvc("ns2", "svc-name"), + expected: false, + }, + } + + for name, testCase := range testCases { + t.Run(name, func(t *testing.T) { + result := ShouldSetHostname(testCase.pod, testCase.service) + if result != testCase.expected { + t.Errorf("expected: %t, got: %t", testCase.expected, result) + } + }) + } +} + +func genSimplePod(namespace, hostname, subdomain string) *v1.Pod { + return &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + }, + Spec: v1.PodSpec{ + Hostname: hostname, + Subdomain: subdomain, + }, + } +} + +func genSimpleSvc(namespace, name string) *v1.Service { + return &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, } }