diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 427339f9009..1e1ffc3850c 100644 --- a/pkg/apis/core/validation/validation.go +++ b/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 -func validateServiceClusterIPsRelatedFields(service *core.Service) field.ErrorList { +// ValidateServiceClusterIPsRelatedFields validates .spec.ClusterIPs,, +// .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")) } } diff --git a/pkg/registry/core/service/storage/rest.go b/pkg/registry/core/service/storage/rest.go index 45edb26b085..6e8ea1d3f33 100644 --- a/pkg/registry/core/service/storage/rest.go +++ b/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 - if service.Spec.IPFamilyPolicy != nil { - el := make(field.ErrorList, 0) - if *(service.Spec.IPFamilyPolicy) == 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 'clusterIPs' are specified")) + // Update-only prep work. + if oldService != nil { + np := service.Spec.IPFamilyPolicy + + // If they didn't specify policy, or specified anything but + // 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 { - el = append(el, field.Invalid(field.NewPath("spec", "ipFamilyPolicy"), service.Spec.IPFamilyPolicy, "must be RequireDualStack or PreferDualStack when multiple 'ipFamilies' are specified")) + if reducedIPFamilies(oldService, service) { + 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 { - return errors.NewInvalid(api.Kind("Service"), service.Name, el) + // Do some loose pre-validation of the input. This makes it easier in the + // 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 - // so the following is safe to do - isIPv6 := netutils.IsIPv6String(ip) + // We previously validated that IPs are well-formed and that if an + // ipFamilies[] entry exists it matches the IP. + fam := familyOf(ip) // If the corresponding family is not specified, add it. if i >= len(service.Spec.IPFamilies) { - if isIPv6 { - // first make sure that family(ip) is configured - if _, found := al.serviceIPAllocatorsByFamily[api.IPv6Protocol]; !found { - 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")} - return errors.NewInvalid(api.Kind("Service"), service.Name, el) - } - service.Spec.IPFamilies = append(service.Spec.IPFamilies, api.IPv6Protocol) + // Families are checked more later, but this is a better error in + // this specific case (indicating the user-provided IP, rather + // than than the auto-assigned family). + if _, found := al.serviceIPAllocatorsByFamily[fam]; !found { + el = append(el, field.Invalid(field.NewPath("spec", "clusterIPs").Index(i), service.Spec.ClusterIPs, + fmt.Sprintf("%s is not configured on this cluster", fam))) } else { - // first make sure that family(ip) is configured - if _, found := al.serviceIPAllocatorsByFamily[api.IPv4Protocol]; !found { - 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) + // OK to infer. + service.Spec.IPFamilies = append(service.Spec.IPFamilies, fam) } } } + // 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 { - el = append(el, field.Invalid(field.NewPath("spec", "ipFamilyPolicy"), service.Spec.IPFamilyPolicy, "Cluster is not configured for dual stack services")) + if getIPFamilyPolicy(service) == api.IPFamilyPolicyRequireDualStack { + 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) - } - - if service.Spec.IPFamilies[0] == api.IPv6Protocol { + } else if service.Spec.IPFamilies[0] == api.IPv6Protocol { service.Spec.IPFamilies = append(service.Spec.IPFamilies, api.IPv4Protocol) } } diff --git a/pkg/registry/core/service/storage/storage_test.go b/pkg/registry/core/service/storage/storage_test.go index 1b5247a7589..16e9d6593ec 100644 --- a/pkg/registry/core/service/storage/storage_test.go +++ b/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