From e46572ef4b0f0a6b095c7dcdceb5bbca2ec0e9ff Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Sun, 24 May 2020 17:48:59 -0400 Subject: [PATCH] Improve EndpointController's handling of headless services under dual-stack EndpointController was accidentally requiring all headless services to be IPv4-only in clusters with IPv6DualStack enabled. This still leaves "legacy" (ie, IPFamily-less) headless services as always IPv4-only because the controller doesn't currently have easy access to the information that would allow it to fix that. (EndpointSliceController had the same problem already, and still does.) This can be fixed, if needed, by manually setting IPFamily, and the proposed API for 1.20 will handle this situation better. --- .../endpoint/endpoints_controller.go | 12 ++------ .../endpoint/endpoints_controller_test.go | 30 +++++++++---------- pkg/controller/endpointslice/reconciler.go | 2 +- pkg/controller/endpointslice/utils.go | 8 +---- pkg/controller/util/endpoint/BUILD | 2 ++ .../util/endpoint/controller_utils.go | 17 +++++++++++ 6 files changed, 39 insertions(+), 32 deletions(-) diff --git a/pkg/controller/endpoint/endpoints_controller.go b/pkg/controller/endpoint/endpoints_controller.go index d68b01fce2b..610ec5b34fa 100644 --- a/pkg/controller/endpoint/endpoints_controller.go +++ b/pkg/controller/endpoint/endpoints_controller.go @@ -215,19 +215,13 @@ func podToEndpointAddressForService(svc *v1.Service, pod *v1.Pod) (*v1.EndpointA var endpointIP string if !utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) { + // In a legacy cluster, the pod IP is guaranteed to be usable endpointIP = pod.Status.PodIP } else { - // api-server service controller ensured that the service got the correct IP Family - // according to user setup, here we only need to match EndPoint IPs' family to service - // actual IP family. as in, we don't need to check service.IPFamily - - ipv6ClusterIP := utilnet.IsIPv6String(svc.Spec.ClusterIP) + ipv6Service := endpointutil.IsIPv6Service(svc) for _, podIP := range pod.Status.PodIPs { ipv6PodIP := utilnet.IsIPv6String(podIP.IP) - // same family? - // TODO (khenidak) when we remove the max of 2 PodIP limit from pods - // we will have to return multiple endpoint addresses - if ipv6ClusterIP == ipv6PodIP { + if ipv6Service == ipv6PodIP { endpointIP = podIP.IP break } diff --git a/pkg/controller/endpoint/endpoints_controller_test.go b/pkg/controller/endpoint/endpoints_controller_test.go index c82a1d0ec0b..e5f6d9d2323 100644 --- a/pkg/controller/endpoint/endpoints_controller_test.go +++ b/pkg/controller/endpoint/endpoints_controller_test.go @@ -1249,21 +1249,21 @@ func TestPodToEndpointAddressForService(t *testing.T) { expectedEndpointFamily: ipv6, }, - // { - // name: "v6 headless service, in a dual stack cluster", - // - // enableDualStack: true, - // ipFamilies: ipv4ipv6, - // - // service: v1.Service{ - // Spec: v1.ServiceSpec{ - // ClusterIP: v1.ClusterIPNone, - // IPFamily: &ipv6, - // }, - // }, - // - // expectedEndpointFamily: ipv6, - // }, + { + name: "v6 headless service, in a dual stack cluster", + + enableDualStack: true, + ipFamilies: ipv4ipv6, + + service: v1.Service{ + Spec: v1.ServiceSpec{ + ClusterIP: v1.ClusterIPNone, + IPFamily: &ipv6, + }, + }, + + expectedEndpointFamily: ipv6, + }, { name: "v6 legacy headless service, in a dual stack cluster", diff --git a/pkg/controller/endpointslice/reconciler.go b/pkg/controller/endpointslice/reconciler.go index ef44809aea1..1f97e3294d1 100644 --- a/pkg/controller/endpointslice/reconciler.go +++ b/pkg/controller/endpointslice/reconciler.go @@ -59,7 +59,7 @@ type endpointMeta struct { func (r *reconciler) reconcile(service *corev1.Service, pods []*corev1.Pod, existingSlices []*discovery.EndpointSlice, triggerTime time.Time) error { addressType := discovery.AddressTypeIPv4 - if isIPv6Service(service) { + if endpointutil.IsIPv6Service(service) { addressType = discovery.AddressTypeIPv6 } diff --git a/pkg/controller/endpointslice/utils.go b/pkg/controller/endpointslice/utils.go index a3589e53907..316c26b856e 100644 --- a/pkg/controller/endpointslice/utils.go +++ b/pkg/controller/endpointslice/utils.go @@ -120,7 +120,7 @@ func getEndpointAddresses(podStatus corev1.PodStatus, service *corev1.Service) [ for _, podIP := range podStatus.PodIPs { isIPv6PodIP := utilnet.IsIPv6String(podIP.IP) - if isIPv6PodIP == isIPv6Service(service) { + if isIPv6PodIP == endpointutil.IsIPv6Service(service) { addresses = append(addresses, podIP.IP) } } @@ -128,12 +128,6 @@ func getEndpointAddresses(podStatus corev1.PodStatus, service *corev1.Service) [ return addresses } -// isIPv6Service returns true if the Service uses IPv6 addresses. -func isIPv6Service(service *corev1.Service) bool { - // IPFamily is not guaranteed to be set, even in an IPv6 only cluster. - return (service.Spec.IPFamily != nil && *service.Spec.IPFamily == corev1.IPv6Protocol) || utilnet.IsIPv6String(service.Spec.ClusterIP) -} - // endpointsEqualBeyondHash returns true if endpoints have equal attributes // but excludes equality checks that would have already been covered with // endpoint hashing (see hashEndpoint func for more info). diff --git a/pkg/controller/util/endpoint/BUILD b/pkg/controller/util/endpoint/BUILD index 3a17c9c71f9..81f4068279b 100644 --- a/pkg/controller/util/endpoint/BUILD +++ b/pkg/controller/util/endpoint/BUILD @@ -10,6 +10,7 @@ go_library( visibility = ["//visibility:public"], deps = [ "//pkg/api/v1/pod:go_default_library", + "//pkg/apis/core/v1/helper:go_default_library", "//pkg/controller:go_default_library", "//pkg/util/hash:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", @@ -19,6 +20,7 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//staging/src/k8s.io/client-go/listers/core/v1:go_default_library", "//staging/src/k8s.io/client-go/tools/cache:go_default_library", + "//vendor/k8s.io/utils/net:go_default_library", ], ) diff --git a/pkg/controller/util/endpoint/controller_utils.go b/pkg/controller/util/endpoint/controller_utils.go index d1744441bec..9c2ab1eedf9 100644 --- a/pkg/controller/util/endpoint/controller_utils.go +++ b/pkg/controller/util/endpoint/controller_utils.go @@ -32,8 +32,10 @@ import ( v1listers "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/tools/cache" podutil "k8s.io/kubernetes/pkg/api/v1/pod" + "k8s.io/kubernetes/pkg/apis/core/v1/helper" "k8s.io/kubernetes/pkg/controller" "k8s.io/kubernetes/pkg/util/hash" + utilnet "k8s.io/utils/net" ) // ServiceSelectorCache is a cache of service selectors to avoid high CPU consumption caused by frequent calls to AsSelectorPreValidated (see #73527) @@ -275,3 +277,18 @@ func (sl portsInOrder) Less(i, j int) bool { h2 := DeepHashObjectToString(sl[j]) return h1 < h2 } + +// IsIPv6Service checks if svc should have IPv6 endpoints +func IsIPv6Service(svc *v1.Service) bool { + if helper.IsServiceIPSet(svc) { + return utilnet.IsIPv6String(svc.Spec.ClusterIP) + } else if svc.Spec.IPFamily != nil { + return *svc.Spec.IPFamily == v1.IPv6Protocol + } else { + // FIXME: for legacy headless Services with no IPFamily, the current + // thinking is that we should use the cluster default. Unfortunately + // the endpoint controller doesn't know the cluster default. For now, + // assume it's IPv4. + return false + } +}