From 1e39f6ccf9f70bd38fa3382d85d3a5a6fd121d7d Mon Sep 17 00:00:00 2001
From: Tim Hockin <thockin@google.com>
Date: Wed, 25 Nov 2020 00:07:46 -0800
Subject: [PATCH] Two small bugs in dual-stack init

Imporved testing turned these up:

1) Headless+Selectorless, on a single-stack cluster, policy=PreferDual

Prior to this commit, the result was a single IPFamiliy (because we
checked that the 2nd allocator was present).  This changes that case to
populate both families (we don't care if the allocator exists), which is
the same as RequireDual.

2) ClusterIP, user specifies 2 families but no IPs

Prior to this commit, the policy was inferred to be SingleStack.  This
changes that case to correctly default to RequireDual when 2 families
are present but no IPs.
---
 pkg/registry/core/service/storage/rest.go     | 27 ++++++++++++-------
 .../core/service/storage/rest_test.go         |  4 +--
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/pkg/registry/core/service/storage/rest.go b/pkg/registry/core/service/storage/rest.go
index 06663d947ff..b080326e963 100644
--- a/pkg/registry/core/service/storage/rest.go
+++ b/pkg/registry/core/service/storage/rest.go
@@ -872,7 +872,7 @@ func (rs *REST) tryDefaultValidateServiceClusterIPFields(service *api.Service) e
 		}
 	}
 
-	// default families according to cluster IPs
+	// Infer IPFamilies[] from ClusterIPs[].
 	for i, ip := range service.Spec.ClusterIPs {
 		if ip == api.ClusterIPNone {
 			break
@@ -882,8 +882,8 @@ func (rs *REST) tryDefaultValidateServiceClusterIPFields(service *api.Service) e
 		// so the following is safe to do
 		isIPv6 := netutil.IsIPv6String(ip)
 
-		// family is not there.
-		if i > len(service.Spec.IPFamilies)-1 {
+		// Family is not specified yet.
+		if i >= len(service.Spec.IPFamilies) {
 			if isIPv6 {
 				// first make sure that family(ip) is configured
 				if _, found := rs.serviceIPAllocatorsByFamily[api.IPv6Protocol]; !found {
@@ -902,15 +902,23 @@ func (rs *REST) tryDefaultValidateServiceClusterIPFields(service *api.Service) e
 		}
 	}
 
-	// default headless+selectorless
-	if len(service.Spec.ClusterIPs) > 0 && service.Spec.ClusterIPs[0] == api.ClusterIPNone && len(service.Spec.Selector) == 0 {
+	// Infer IPFamilyPolicy from IPFamilies[].  This block does not handle the
+	// final defaulting - that happens a bit later, after special-cases.
+	if service.Spec.IPFamilyPolicy == nil && len(service.Spec.IPFamilies) == 2 {
+		requireDualStack := api.IPFamilyPolicyRequireDualStack
+		service.Spec.IPFamilyPolicy = &requireDualStack
+	}
 
+	// Special-case: headless+selectorless
+	if len(service.Spec.ClusterIPs) > 0 && service.Spec.ClusterIPs[0] == api.ClusterIPNone && len(service.Spec.Selector) == 0 {
+		// If the use said nothing about policy and we can't infer it, they get dual-stack
 		if service.Spec.IPFamilyPolicy == nil {
 			requireDualStack := api.IPFamilyPolicyRequireDualStack
 			service.Spec.IPFamilyPolicy = &requireDualStack
 		}
 
-		// if not set by user
+		// If IPFamilies was not set by the user, start with the default
+		// family.
 		if len(service.Spec.IPFamilies) == 0 {
 			service.Spec.IPFamilies = []api.IPFamily{rs.defaultServiceIPFamily}
 		}
@@ -919,8 +927,7 @@ func (rs *REST) tryDefaultValidateServiceClusterIPFields(service *api.Service) e
 		// cluster the user is allowed to create headless services that has multi families
 		// the validation allows it
 		if len(service.Spec.IPFamilies) < 2 {
-			if *(service.Spec.IPFamilyPolicy) == api.IPFamilyPolicyRequireDualStack ||
-				(*(service.Spec.IPFamilyPolicy) == api.IPFamilyPolicyPreferDualStack && len(rs.serviceIPAllocatorsByFamily) == 2) {
+			if *(service.Spec.IPFamilyPolicy) != api.IPFamilyPolicySingleStack {
 				// add the alt ipfamily
 				if service.Spec.IPFamilies[0] == api.IPv4Protocol {
 					service.Spec.IPFamilies = append(service.Spec.IPFamilies, api.IPv6Protocol)
@@ -956,8 +963,8 @@ func (rs *REST) tryDefaultValidateServiceClusterIPFields(service *api.Service) e
 		return errors.NewInvalid(api.Kind("Service"), service.Name, el)
 	}
 
-	// default ipFamilyPolicy to SingleStack. if there are
-	// web hooks, they must have already ran by now
+	// Finally, if IPFamilyPolicy is *still* not set, we can default it to
+	// SingleStack. If there are any webhooks, they have already run.
 	if service.Spec.IPFamilyPolicy == nil {
 		singleStack := api.IPFamilyPolicySingleStack
 		service.Spec.IPFamilyPolicy = &singleStack
diff --git a/pkg/registry/core/service/storage/rest_test.go b/pkg/registry/core/service/storage/rest_test.go
index 4721988b9b7..bee9080e6f1 100644
--- a/pkg/registry/core/service/storage/rest_test.go
+++ b/pkg/registry/core/service/storage/rest_test.go
@@ -3892,7 +3892,7 @@ func TestDefaultingValidation(t *testing.T) {
 				},
 			},
 			expectedIPFamilyPolicy: &preferDualStack,
-			expectedIPFamilies:     []api.IPFamily{api.IPv4Protocol},
+			expectedIPFamilies:     []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
 			expectError:            false,
 		},
 		// tests incorrect setting for IPFamilyPolicy
@@ -4173,7 +4173,7 @@ func TestDefaultingValidation(t *testing.T) {
 				},
 			},
 			expectedIPFamilyPolicy: &preferDualStack,
-			expectedIPFamilies:     []api.IPFamily{api.IPv6Protocol},
+			expectedIPFamilies:     []api.IPFamily{api.IPv6Protocol, api.IPv4Protocol},
 			expectError:            false,
 		},
 		// tests incorrect setting for IPFamilyPolicy