Svc REST: Validate input before IP allocation

This commit started as removing FIXME comments, but in doing so I
realized that the IP allocation process was using unvalidated user
input.  Before de-layering, validation was called twice - once before
init and once after, which the init code depended on.

Fortunately (or not?) we had duplicative checks that caught errors but
with less friendly messages.

This commit calls validation before initializing the rest of the
IP-related fields.

This also re-organizes that code a bit, cleans up error messages and
comments, and adds a test SPECIFICALLY for the errors in those cases.

pkg/apis/core/validation/validation.go

@@ -4358,7 +4358,7 @@ func ValidateService(service *core.Service) field.ErrorList {
 	}
 
 	// dualstack <-> ClusterIPs <-> ipfamilies
-	allErrs = append(allErrs, validateServiceClusterIPsRelatedFields(service)...)
+	allErrs = append(allErrs, ValidateServiceClusterIPsRelatedFields(service)...)
 
 	ipPath := specPath.Child("externalIPs")
 	for i, ip := range service.Spec.ExternalIPs {
@@ -6305,8 +6305,10 @@ func ValidateSpreadConstraintNotRepeat(fldPath *field.Path, constraint core.Topo
 	return nil
 }
 
-// validateServiceClusterIPsRelatedFields validates .spec.ClusterIPs,, .spec.IPFamilies, .spec.ipFamilyPolicy
+// ValidateServiceClusterIPsRelatedFields validates .spec.ClusterIPs,,
-func validateServiceClusterIPsRelatedFields(service *core.Service) field.ErrorList {
+// .spec.IPFamilies, .spec.ipFamilyPolicy.  This is exported because it is used
+// during IP init and allocation.
+func ValidateServiceClusterIPsRelatedFields(service *core.Service) field.ErrorList {
 	// ClusterIP, ClusterIPs, IPFamilyPolicy and IPFamilies are validated prior (all must be unset) for ExternalName service
 	if service.Spec.Type == core.ServiceTypeExternalName {
 		return field.ErrorList{}
@@ -6328,12 +6330,12 @@ func validateServiceClusterIPsRelatedFields(service *core.Service) field.ErrorLi
 		if len(service.Spec.ClusterIPs) == 0 {
 			allErrs = append(allErrs, field.Required(clusterIPsField, ""))
 		} else if service.Spec.ClusterIPs[0] != service.Spec.ClusterIP {
-			allErrs = append(allErrs, field.Invalid(clusterIPsField, service.Spec.ClusterIPs, "element [0] must match clusterIP"))
+			allErrs = append(allErrs, field.Invalid(clusterIPsField, service.Spec.ClusterIPs, "first value must match `clusterIP`"))
 		}
 	} else { // ClusterIP == ""
 		// If ClusterIP is not set, ClusterIPs must also be unset.
 		if len(service.Spec.ClusterIPs) != 0 {
-			allErrs = append(allErrs, field.Invalid(clusterIPsField, service.Spec.ClusterIPs, "must be empty when clusterIP is empty"))
+			allErrs = append(allErrs, field.Invalid(clusterIPsField, service.Spec.ClusterIPs, "must be empty when `clusterIP` is not specified"))
 		}
 	}
 

pkg/registry/core/service/storage/rest.go

@@ -19,7 +19,6 @@ package storage
 import (
 	"context"
 	"fmt"
-	"net"
 	"net/http"
 	"net/url"
 
@@ -36,6 +35,7 @@ import (
 	"k8s.io/klog/v2"
 	apiservice "k8s.io/kubernetes/pkg/api/service"
 	api "k8s.io/kubernetes/pkg/apis/core"
+	"k8s.io/kubernetes/pkg/apis/core/validation"
 	"k8s.io/kubernetes/pkg/features"
 	registry "k8s.io/kubernetes/pkg/registry/core/service"
 	"k8s.io/kubernetes/pkg/registry/core/service/ipallocator"
@@ -751,6 +751,68 @@ func isMatchingPreferDualStackClusterIPFields(oldService, service *api.Service)
 	return true
 }
 
+func sameClusterIPs(lhs, rhs *api.Service) bool {
+	if len(rhs.Spec.ClusterIPs) != len(lhs.Spec.ClusterIPs) {
+		return false
+	}
+
+	for i, ip := range rhs.Spec.ClusterIPs {
+		if lhs.Spec.ClusterIPs[i] != ip {
+			return false
+		}
+	}
+
+	return true
+}
+
+func reducedClusterIPs(before, after *api.Service) bool {
+	if len(after.Spec.ClusterIPs) == 0 { // Not specified
+		return false
+	}
+	return len(after.Spec.ClusterIPs) < len(before.Spec.ClusterIPs)
+}
+
+func sameIPFamilies(lhs, rhs *api.Service) bool {
+	if len(rhs.Spec.IPFamilies) != len(lhs.Spec.IPFamilies) {
+		return false
+	}
+
+	for i, family := range rhs.Spec.IPFamilies {
+		if lhs.Spec.IPFamilies[i] != family {
+			return false
+		}
+	}
+
+	return true
+}
+
+func reducedIPFamilies(before, after *api.Service) bool {
+	if len(after.Spec.IPFamilies) == 0 { // Not specified
+		return false
+	}
+	return len(after.Spec.IPFamilies) < len(before.Spec.IPFamilies)
+}
+
+// Helper to get the IP family of a given IP.
+func familyOf(ip string) api.IPFamily {
+	if netutils.IsIPv4String(ip) {
+		return api.IPv4Protocol
+	}
+	if netutils.IsIPv6String(ip) {
+		return api.IPv6Protocol
+	}
+	return api.IPFamily("unknown")
+}
+
+// Helper to avoid nil-checks all over.  Callers of this need to be checking
+// for an exact value.
+func getIPFamilyPolicy(svc *api.Service) api.IPFamilyPolicyType {
+	if svc.Spec.IPFamilyPolicy == nil {
+		return "" // callers need to handle this
+	}
+	return *svc.Spec.IPFamilyPolicy
+}
+
 // attempts to default service ip families according to cluster configuration
 // while ensuring that provided families are configured on cluster.
 func (al *RESTAllocStuff) initIPFamilyFields(oldService, service *api.Service) error {
@@ -777,20 +839,63 @@ func (al *RESTAllocStuff) initIPFamilyFields(oldService, service *api.Service) e
 		return nil // nothing more to do.
 	}
 
-	// two families or two IPs with SingleStack
+	// Update-only prep work.
-	if service.Spec.IPFamilyPolicy != nil {
+	if oldService != nil {
-		el := make(field.ErrorList, 0)
+		np := service.Spec.IPFamilyPolicy
-		if *(service.Spec.IPFamilyPolicy) == api.IPFamilyPolicySingleStack {
+
-			if len(service.Spec.ClusterIPs) == 2 {
+		// If they didn't specify policy, or specified anything but
-				el = append(el, field.Invalid(field.NewPath("spec", "ipFamilyPolicy"), service.Spec.IPFamilyPolicy, "must be RequireDualStack or PreferDualStack when multiple 'clusterIPs' are specified"))
+		// single-stack AND they reduced these fields, it's an error.  They
+		// need to specify policy.
+		if np == nil || *np != api.IPFamilyPolicySingleStack {
+			el := make(field.ErrorList, 0)
+
+			if reducedClusterIPs(oldService, service) {
+				el = append(el, field.Invalid(field.NewPath("spec", "ipFamilyPolicy"), service.Spec.IPFamilyPolicy,
+					"must be 'SingleStack' to release the secondary cluster IP"))
 			}
-			if len(service.Spec.IPFamilies) == 2 {
+			if reducedIPFamilies(oldService, service) {
-				el = append(el, field.Invalid(field.NewPath("spec", "ipFamilyPolicy"), service.Spec.IPFamilyPolicy, "must be RequireDualStack or PreferDualStack when multiple 'ipFamilies' are specified"))
+				el = append(el, field.Invalid(field.NewPath("spec", "ipFamilyPolicy"), service.Spec.IPFamilyPolicy,
+					"must be 'SingleStack' to release the secondary IP family"))
+			}
+
+			if len(el) > 0 {
+				return errors.NewInvalid(api.Kind("Service"), service.Name, el)
+			}
+		} else { // policy must be SingleStack
+			// Update: As long as ClusterIPs and IPFamilies have not changed,
+			// setting policy to single-stack is clear intent.
+			if *(service.Spec.IPFamilyPolicy) == api.IPFamilyPolicySingleStack {
+				if sameClusterIPs(oldService, service) && len(service.Spec.ClusterIPs) > 1 {
+					service.Spec.ClusterIPs = service.Spec.ClusterIPs[0:1]
+				}
+				if sameIPFamilies(oldService, service) && len(service.Spec.IPFamilies) > 1 {
+					service.Spec.IPFamilies = service.Spec.IPFamilies[0:1]
+				}
 			}
 		}
+	}
 
-		if len(el) > 0 {
+	// Do some loose pre-validation of the input.  This makes it easier in the
-			return errors.NewInvalid(api.Kind("Service"), service.Name, el)
+	// rest of allocation code to not have to consider corner cases.
+	// TODO(thockin): when we tighten validation (e.g. to require IPs) we will
+	// need a "strict" and a "loose" form of this.
+	if el := validation.ValidateServiceClusterIPsRelatedFields(service); len(el) != 0 {
+		return errors.NewInvalid(api.Kind("Service"), service.Name, el)
+	}
+
+	//TODO(thockin): Move this logic to validation?
+	el := make(field.ErrorList, 0)
+
+	// Make sure ipFamilyPolicy makes sense for the provided ipFamilies and
+	// clusterIPs.  Further checks happen below - after the special cases.
+	if getIPFamilyPolicy(service) == api.IPFamilyPolicySingleStack {
+		if len(service.Spec.ClusterIPs) == 2 {
+			el = append(el, field.Invalid(field.NewPath("spec", "ipFamilyPolicy"), service.Spec.IPFamilyPolicy,
+				"must be 'RequireDualStack' or 'PreferDualStack' when multiple cluster IPs are specified"))
+		}
+		if len(service.Spec.IPFamilies) == 2 {
+			el = append(el, field.Invalid(field.NewPath("spec", "ipFamilyPolicy"), service.Spec.IPFamilyPolicy,
+				"must be 'RequireDualStack' or 'PreferDualStack' when multiple IP families are specified"))
 		}
 	}
 
@@ -801,30 +906,30 @@ func (al *RESTAllocStuff) initIPFamilyFields(oldService, service *api.Service) e
 			break
 		}
 
-		// we have previously validated for ip correctness and if family exist it will match ip family
+		// We previously validated that IPs are well-formed and that if an
-		// so the following is safe to do
+		// ipFamilies[] entry exists it matches the IP.
-		isIPv6 := netutils.IsIPv6String(ip)
+		fam := familyOf(ip)
 
 		// If the corresponding family is not specified, add it.
 		if i >= len(service.Spec.IPFamilies) {
-			if isIPv6 {
+			// Families are checked more later, but this is a better error in
-				// first make sure that family(ip) is configured
+			// this specific case (indicating the user-provided IP, rather
-				if _, found := al.serviceIPAllocatorsByFamily[api.IPv6Protocol]; !found {
+			// than than the auto-assigned family).
-					el := field.ErrorList{field.Invalid(field.NewPath("spec", "clusterIPs").Index(i), service.Spec.ClusterIPs, "may not use IPv6 on a cluster which is not configured for it")}
+			if _, found := al.serviceIPAllocatorsByFamily[fam]; !found {
-					return errors.NewInvalid(api.Kind("Service"), service.Name, el)
+				el = append(el, field.Invalid(field.NewPath("spec", "clusterIPs").Index(i), service.Spec.ClusterIPs,
-				}
+					fmt.Sprintf("%s is not configured on this cluster", fam)))
-				service.Spec.IPFamilies = append(service.Spec.IPFamilies, api.IPv6Protocol)
 			} else {
-				// first make sure that family(ip) is configured
+				// OK to infer.
-				if _, found := al.serviceIPAllocatorsByFamily[api.IPv4Protocol]; !found {
+				service.Spec.IPFamilies = append(service.Spec.IPFamilies, fam)
-					el := field.ErrorList{field.Invalid(field.NewPath("spec", "clusterIPs").Index(i), service.Spec.ClusterIPs, "may not use IPv4 on a cluster which is not configured for it")}
-					return errors.NewInvalid(api.Kind("Service"), service.Name, el)
-				}
-				service.Spec.IPFamilies = append(service.Spec.IPFamilies, api.IPv4Protocol)
 			}
 		}
 	}
 
+	// If we have validation errors, bail out now so we don't make them worse.
+	if len(el) > 0 {
+		return errors.NewInvalid(api.Kind("Service"), service.Name, el)
+	}
+
 	// 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 {
@@ -870,19 +975,18 @@ func (al *RESTAllocStuff) initIPFamilyFields(oldService, service *api.Service) e
 	// Everything below this MUST happen *after* the above special cases.
 	//
 
-	// ipfamily check
-	// the following applies on all type of services including headless w/ selector
-	el := make(field.ErrorList, 0)
-
 	// Demanding dual-stack on a non dual-stack cluster.
-	if service.Spec.IPFamilyPolicy != nil && *(service.Spec.IPFamilyPolicy) == api.IPFamilyPolicyRequireDualStack && len(al.serviceIPAllocatorsByFamily) < 2 {
+	if getIPFamilyPolicy(service) == api.IPFamilyPolicyRequireDualStack {
-		el = append(el, field.Invalid(field.NewPath("spec", "ipFamilyPolicy"), service.Spec.IPFamilyPolicy, "Cluster is not configured for dual stack services"))
+		if len(al.serviceIPAllocatorsByFamily) < 2 {
+			el = append(el, field.Invalid(field.NewPath("spec", "ipFamilyPolicy"), service.Spec.IPFamilyPolicy,
+				"this cluster is not configured for dual-stack services"))
+		}
 	}
 
 	// If there is a family requested then it has to be configured on cluster.
 	for i, ipFamily := range service.Spec.IPFamilies {
 		if _, found := al.serviceIPAllocatorsByFamily[ipFamily]; !found {
-			el = append(el, field.Invalid(field.NewPath("spec", "ipFamilies").Index(i), service.Spec.ClusterIPs, fmt.Sprintf("ipfamily %v is not configured on cluster", ipFamily)))
+			el = append(el, field.Invalid(field.NewPath("spec", "ipFamilies").Index(i), ipFamily, "not configured on this cluster"))
 		}
 	}
 
@@ -911,9 +1015,7 @@ func (al *RESTAllocStuff) initIPFamilyFields(oldService, service *api.Service) e
 
 		if service.Spec.IPFamilies[0] == api.IPv4Protocol {
 			service.Spec.IPFamilies = append(service.Spec.IPFamilies, api.IPv6Protocol)
-		}
+		} else if service.Spec.IPFamilies[0] == api.IPv6Protocol {
-
-		if service.Spec.IPFamilies[0] == api.IPv6Protocol {
 			service.Spec.IPFamilies = append(service.Spec.IPFamilies, api.IPv4Protocol)
 		}
 	}

pkg/registry/core/service/storage/storage_test.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"net"
 	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -376,16 +377,6 @@ func fmtIPFamilies(fams []api.IPFamily) string {
 	return fmt.Sprintf("%v", fams)
 }
 
-func familyOf(ip string) api.IPFamily {
-	if netutils.IsIPv4String(ip) {
-		return api.IPv4Protocol
-	}
-	if netutils.IsIPv6String(ip) {
-		return api.IPv6Protocol
-	}
-	return api.IPFamily("unknown")
-}
-
 // Prove that create ignores IP and IPFamily stuff when type is ExternalName.
 func TestCreateIgnoresIPsForExternalName(t *testing.T) {
 	type testCase struct {
@@ -4765,6 +4756,141 @@ func TestCreateInitIPFields(t *testing.T) {
 	}
 }
 
+// There are enough corner-cases that it's useful to have a test that asserts
+// the errors.  Some of these are in other tests, but this is clearer.
+func TestCreateInvalidClusterIPInputs(t *testing.T) {
+	testCases := []struct {
+		name     string
+		families []api.IPFamily
+		svc      *api.Service
+		expect   []string
+	}{{
+		name:     "bad_ipFamilyPolicy",
+		families: []api.IPFamily{api.IPv4Protocol},
+		svc: svctest.MakeService("foo",
+			svctest.SetIPFamilyPolicy(api.IPFamilyPolicyType("garbage"))),
+		expect: []string{"Unsupported value"},
+	}, {
+		name:     "requiredual_ipFamilyPolicy_on_singlestack",
+		families: []api.IPFamily{api.IPv4Protocol},
+		svc: svctest.MakeService("foo",
+			svctest.SetIPFamilyPolicy(api.IPFamilyPolicyRequireDualStack)),
+		expect: []string{"cluster is not configured for dual-stack"},
+	}, {
+		name:     "bad_ipFamilies_0_value",
+		families: []api.IPFamily{api.IPv4Protocol},
+		svc: svctest.MakeService("foo",
+			svctest.SetIPFamilies(api.IPFamily("garbage"))),
+		expect: []string{"Unsupported value"},
+	}, {
+		name:     "bad_ipFamilies_1_value",
+		families: []api.IPFamily{api.IPv4Protocol},
+		svc: svctest.MakeService("foo",
+			svctest.SetIPFamilies(api.IPv4Protocol, api.IPFamily("garbage"))),
+		expect: []string{"Unsupported value"},
+	}, {
+		name:     "bad_ipFamilies_2_value",
+		families: []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
+		svc: svctest.MakeService("foo",
+			svctest.SetIPFamilies(api.IPv4Protocol, api.IPv6Protocol, api.IPFamily("garbage"))),
+		expect: []string{"Unsupported value"},
+	}, {
+		name:     "wrong_ipFamily",
+		families: []api.IPFamily{api.IPv4Protocol},
+		svc: svctest.MakeService("foo",
+			svctest.SetIPFamilies(api.IPv6Protocol)),
+		expect: []string{"not configured on this cluster"},
+	}, {
+		name:     "too_many_ipFamilies_on_singlestack",
+		families: []api.IPFamily{api.IPv4Protocol},
+		svc: svctest.MakeService("foo",
+			svctest.SetIPFamilies(api.IPv4Protocol, api.IPv6Protocol)),
+		expect: []string{"not configured on this cluster"},
+	}, {
+		name:     "dup_ipFamily_singlestack",
+		families: []api.IPFamily{api.IPv4Protocol},
+		svc: svctest.MakeService("foo",
+			svctest.SetIPFamilies(api.IPv4Protocol, api.IPv4Protocol)),
+		expect: []string{"Duplicate value"},
+	}, {
+		name:     "dup_ipFamily_dualstack",
+		families: []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
+		svc: svctest.MakeService("foo",
+			svctest.SetIPFamilies(api.IPv4Protocol, api.IPv6Protocol, api.IPv6Protocol)),
+		expect: []string{"Duplicate value"},
+	}, {
+		name:     "bad_IP",
+		families: []api.IPFamily{api.IPv4Protocol},
+		svc: svctest.MakeService("foo",
+			svctest.SetClusterIPs("garbage")),
+		expect: []string{"must be a valid IP"},
+	}, {
+		name:     "IP_wrong_family",
+		families: []api.IPFamily{api.IPv4Protocol},
+		svc: svctest.MakeService("foo",
+			svctest.SetClusterIPs("2000::1")),
+		expect: []string{"not configured on this cluster"},
+	}, {
+		name:     "IP_doesnt_match_family",
+		families: []api.IPFamily{api.IPv4Protocol},
+		svc: svctest.MakeService("foo",
+			svctest.SetIPFamilies(api.IPv4Protocol),
+			svctest.SetClusterIPs("2000::1")),
+		expect: []string{"expected an IPv4 value as indicated"},
+	}, {
+		name:     "too_many_IPs_singlestack",
+		families: []api.IPFamily{api.IPv4Protocol},
+		svc: svctest.MakeService("foo",
+			svctest.SetClusterIPs("10.0.0.1", "10.0.0.2")),
+		expect: []string{"no more than one IP for each IP family"},
+	}, {
+		name:     "too_many_IPs_dualstack",
+		families: []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
+		svc: svctest.MakeService("foo",
+			svctest.SetClusterIPs("10.0.0.1", "2000::1", "10.0.0.2")),
+		expect: []string{"only hold up to 2 values"},
+	}, {
+		name:     "dup_IPs",
+		families: []api.IPFamily{api.IPv4Protocol},
+		svc: svctest.MakeService("foo",
+			svctest.SetClusterIPs("10.0.0.1", "10.0.0.1")),
+		expect: []string{"no more than one IP for each IP family"},
+	}, {
+		name:     "empty_IP",
+		families: []api.IPFamily{api.IPv4Protocol},
+		svc: svctest.MakeService("foo",
+			svctest.SetClusterIPs("")),
+		expect: []string{"must be empty when", "must be a valid IP"},
+	}, {
+		name:     "None_IP_1",
+		families: []api.IPFamily{api.IPv4Protocol},
+		svc: svctest.MakeService("foo",
+			svctest.SetClusterIPs("10.0.0.1", "None")),
+		expect: []string{"must be a valid IP"},
+	}}
+
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, true)()
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			storage, _, server := newStorage(t, tc.families)
+			defer server.Terminate(t)
+			defer storage.Store.DestroyFunc()
+
+			ctx := genericapirequest.NewDefaultContext()
+			_, err := storage.Create(ctx, tc.svc, rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
+			if err == nil {
+				t.Fatalf("unexpected success creating service")
+			}
+			t.Logf("INFO: errstr:\n  %v", err)
+			for _, s := range tc.expect {
+				if !strings.Contains(err.Error(), s) {
+					t.Errorf("expected to find %q in the error:\n  %s", s, err.Error())
+				}
+			}
+		})
+	}
+}
 func TestCreateDeleteReuse(t *testing.T) {
 	testCases := []struct {
 		name string