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, + }, } }