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.
This commit is contained in:
Rob Scott 2019-10-22 15:39:49 -07:00
parent f7c3fa8324
commit 0c7548f020
No known key found for this signature in database
GPG Key ID: 05B37CFC2CDE8B85
8 changed files with 151 additions and 38 deletions

View File

@ -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.

View File

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

View File

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

View File

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

View File

@ -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

View File

@ -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)
}

View File

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

View File

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