Merge pull request #84207 from robscott/endpointslice-hostname

Setting Hostname from Pods on EndpointSlice to match Endpoints behavior.

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
+		epa := *ep
-		if len(hostname) > 0 && pod.Spec.Subdomain == service.Name && service.Namespace == pod.Namespace {
+		if endpointutil.ShouldSetHostname(pod, service) {
-			epa.Hostname = hostname
+			epa.Hostname = pod.Spec.Hostname
 		}
 
 		// Allow headless service not to have ports.

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

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

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)

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)
+	endpoint1 := podToEndpoint(pod1, &corev1.Node{}, &corev1.Service{Spec: corev1.ServiceSpec{}})
-	endpoint2 := podToEndpoint(pod2, &corev1.Node{}, false)
+	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)
+	ready := service.Spec.PublishNotReadyAddresses || podutil.IsPodReady(pod)
-	return discovery.Endpoint{
+	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

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",
+			name: "Ready pod + publishNotReadyAddresses",
-			pod:                      readyPod,
+			pod:  readyPod,
-			publishNotReadyAddresses: true,
+			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",
+			name: "Unready pod + publishNotReadyAddresses",
-			pod:                      unreadyPod,
+			pod:  unreadyPod,
-			publishNotReadyAddresses: true,
+			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)
 			}

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) {

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)
+	for _, testCase := range testCases {
-		if !retval.Equal(testCase.union) {
+		t.Run(testCase.name, func(t *testing.T) {
-			t.Errorf("%s (with podChanged=true): expected: %v  got: %v", testCase.name, testCase.union.List(), retval.List())
+			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)
+		t.Run(test.name, func(t *testing.T) {
-		if result != test.expected {
+			result := ShouldPodBeInEndpoints(test.pod)
-			t.Errorf("%s: expected : %t, got: %t", test.name, test.expected, result)
+			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,
+		},
 	}
 }