diff --git a/pkg/api/service/testing/make.go b/pkg/api/service/testing/make.go index 87b86930cf4..4767448a67a 100644 --- a/pkg/api/service/testing/make.go +++ b/pkg/api/service/testing/make.go @@ -117,7 +117,7 @@ func MakeServicePort(name string, port int, tgtPort intstr.IntOrString, proto ap // SetHeadless sets the service as headless and clears other fields. func SetHeadless(svc *api.Service) { SetTypeClusterIP(svc) - svc.Spec.ClusterIP = api.ClusterIPNone + SetClusterIPs(api.ClusterIPNone)(svc) } // SetSelector sets the service selector. diff --git a/pkg/registry/core/service/storage/storage.go b/pkg/registry/core/service/storage/storage.go index 4250a334ab4..8c20b525a38 100644 --- a/pkg/registry/core/service/storage/storage.go +++ b/pkg/registry/core/service/storage/storage.go @@ -47,9 +47,8 @@ import ( svcreg "k8s.io/kubernetes/pkg/registry/core/service" "k8s.io/kubernetes/pkg/registry/core/service/ipallocator" "k8s.io/kubernetes/pkg/registry/core/service/portallocator" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - netutil "k8s.io/utils/net" + "sigs.k8s.io/structured-merge-diff/v4/fieldpath" ) type EndpointsStorage interface { @@ -248,65 +247,59 @@ func (r *REST) defaultOnReadService(service *api.Service) { return } + // Set ipFamilies and ipFamilyPolicy if needed. + r.defaultOnReadIPFamilies(service) +} + +func (r *REST) defaultOnReadIPFamilies(service *api.Service) { + // ExternalName does not need this. + if !needsClusterIP(service) { + return + } + + // If IPFamilies is set, we assume IPFamilyPolicy is also set (it should + // not be possible to have one and not the other), and therefore we don't + // need further defaulting. Likewise, if IPFamilies is *not* set, we + // assume IPFamilyPolicy can't be set either. if len(service.Spec.IPFamilies) > 0 { - return // already defaulted + return } - // set clusterIPs based on ClusterIP - if len(service.Spec.ClusterIPs) == 0 { - if len(service.Spec.ClusterIP) > 0 { - service.Spec.ClusterIPs = []string{service.Spec.ClusterIP} - } - } - - requireDualStack := api.IPFamilyPolicyRequireDualStack singleStack := api.IPFamilyPolicySingleStack - preferDualStack := api.IPFamilyPolicyPreferDualStack - // headless services - if len(service.Spec.ClusterIPs) == 1 && service.Spec.ClusterIPs[0] == api.ClusterIPNone { - service.Spec.IPFamilies = []api.IPFamily{r.primaryIPFamily} + requireDualStack := api.IPFamilyPolicyRequireDualStack - // headless+selectorless - // headless+selectorless takes both families. Why? - // at this stage we don't know what kind of endpoints (specifically their IPFamilies) the - // user has assigned to this selectorless service. We assume it has dualstack and we default - // it to PreferDualStack on any cluster (single or dualstack configured). + if service.Spec.ClusterIP == api.ClusterIPNone { + // Headless. if len(service.Spec.Selector) == 0 { - service.Spec.IPFamilyPolicy = &preferDualStack - if r.primaryIPFamily == api.IPv4Protocol { - service.Spec.IPFamilies = append(service.Spec.IPFamilies, api.IPv6Protocol) - } else { - service.Spec.IPFamilies = append(service.Spec.IPFamilies, api.IPv4Protocol) - } + // Headless + selectorless is a special-case. + // + // At this stage we don't know what kind of endpoints (specifically + // their IPFamilies) the user has assigned to this selectorless + // service. We assume it has dual-stack and we default it to + // RequireDualStack on any cluster (single- or dual-stack + // configured). + service.Spec.IPFamilyPolicy = &requireDualStack + service.Spec.IPFamilies = []api.IPFamily{r.primaryIPFamily, otherFamily(r.primaryIPFamily)} } else { - // headless w/ selector - // this service type follows cluster configuration. this service (selector based) uses a - // selector and will have to follow how the cluster is configured. If the cluster is - // configured to dual stack then the service defaults to PreferDualStack. Otherwise we - // default it to SingleStack. - if r.secondaryIPFamily != "" { - service.Spec.IPFamilies = append(service.Spec.IPFamilies, r.secondaryIPFamily) - service.Spec.IPFamilyPolicy = &preferDualStack - } else { - service.Spec.IPFamilyPolicy = &singleStack - } + // Headless + selector - default to single. + service.Spec.IPFamilyPolicy = &singleStack + service.Spec.IPFamilies = []api.IPFamily{r.primaryIPFamily} } } else { - // headful - // make sure a slice exists to receive the families - service.Spec.IPFamilies = make([]api.IPFamily, len(service.Spec.ClusterIPs), len(service.Spec.ClusterIPs)) + // Headful: init ipFamilies from clusterIPs. + service.Spec.IPFamilies = make([]api.IPFamily, len(service.Spec.ClusterIPs)) for idx, ip := range service.Spec.ClusterIPs { if netutil.IsIPv6String(ip) { service.Spec.IPFamilies[idx] = api.IPv6Protocol } else { service.Spec.IPFamilies[idx] = api.IPv4Protocol } - - if len(service.Spec.IPFamilies) == 1 { - service.Spec.IPFamilyPolicy = &singleStack - } else if len(service.Spec.IPFamilies) == 2 { - service.Spec.IPFamilyPolicy = &requireDualStack - } + } + if len(service.Spec.IPFamilies) == 1 { + service.Spec.IPFamilyPolicy = &singleStack + } else if len(service.Spec.IPFamilies) == 2 { + // It shouldn't be possible to get here, but just in case. + service.Spec.IPFamilyPolicy = &requireDualStack } } } diff --git a/pkg/registry/core/service/storage/storage_test.go b/pkg/registry/core/service/storage/storage_test.go index 74c61981aaa..93e603ba6df 100644 --- a/pkg/registry/core/service/storage/storage_test.go +++ b/pkg/registry/core/service/storage/storage_test.go @@ -561,29 +561,79 @@ func TestServiceDefaultOnRead(t *testing.T) { input runtime.Object expect runtime.Object }{{ - name: "no change v4", - input: svctest.MakeService("foo", svctest.SetClusterIPs("10.0.0.1")), - expect: svctest.MakeService("foo", svctest.SetClusterIPs("10.0.0.1")), + name: "single v4", + input: svctest.MakeService("foo", svctest.SetClusterIPs("10.0.0.1")), + expect: svctest.MakeService("foo", svctest.SetClusterIPs("10.0.0.1"), + svctest.SetIPFamilyPolicy(api.IPFamilyPolicySingleStack), + svctest.SetIPFamilies(api.IPv4Protocol)), }, { - name: "missing clusterIPs v4", - input: svctest.MakeService("foo", svctest.SetClusterIP("10.0.0.1")), - expect: svctest.MakeService("foo", svctest.SetClusterIPs("10.0.0.1")), + name: "single v6", + input: svctest.MakeService("foo", svctest.SetClusterIPs("2000::1")), + expect: svctest.MakeService("foo", svctest.SetClusterIPs("2000::1"), + svctest.SetIPFamilyPolicy(api.IPFamilyPolicySingleStack), + svctest.SetIPFamilies(api.IPv6Protocol)), }, { - name: "no change v6", - input: svctest.MakeService("foo", svctest.SetClusterIPs("2000::1")), - expect: svctest.MakeService("foo", svctest.SetClusterIPs("2000::1")), + name: "missing clusterIPs v4", + input: svctest.MakeService("foo", svctest.SetClusterIP("10.0.0.1")), + expect: svctest.MakeService("foo", svctest.SetClusterIPs("10.0.0.1"), + svctest.SetIPFamilyPolicy(api.IPFamilyPolicySingleStack), + svctest.SetIPFamilies(api.IPv4Protocol)), }, { - name: "missing clusterIPs v6", - input: svctest.MakeService("foo", svctest.SetClusterIP("2000::1")), - expect: svctest.MakeService("foo", svctest.SetClusterIPs("2000::1")), + name: "missing clusterIPs v6", + input: svctest.MakeService("foo", svctest.SetClusterIP("2000::1")), + expect: svctest.MakeService("foo", svctest.SetClusterIPs("2000::1"), + svctest.SetIPFamilyPolicy(api.IPFamilyPolicySingleStack), + svctest.SetIPFamilies(api.IPv6Protocol)), }, { - name: "list, no change v4", - input: makeServiceList(svctest.SetClusterIPs("10.0.0.1")), - expect: makeServiceList(svctest.SetClusterIPs("10.0.0.1")), + name: "list v4", + input: makeServiceList(svctest.SetClusterIPs("10.0.0.1")), + expect: makeServiceList(svctest.SetClusterIPs("10.0.0.1"), + svctest.SetIPFamilyPolicy(api.IPFamilyPolicySingleStack), + svctest.SetIPFamilies(api.IPv4Protocol)), }, { - name: "list, missing clusterIPs v4", - input: makeServiceList(svctest.SetClusterIP("10.0.0.1")), - expect: makeServiceList(svctest.SetClusterIPs("10.0.0.1")), + name: "list missing clusterIPs v4", + input: makeServiceList(svctest.SetClusterIP("10.0.0.1")), + expect: makeServiceList(svctest.SetClusterIPs("10.0.0.1"), + svctest.SetIPFamilyPolicy(api.IPFamilyPolicySingleStack), + svctest.SetIPFamilies(api.IPv4Protocol)), + }, { + name: "external name", + input: makeServiceList(svctest.SetTypeExternalName), + expect: makeServiceList(svctest.SetTypeExternalName), + }, { + name: "dual v4v6", + input: svctest.MakeService("foo", svctest.SetClusterIPs("10.0.0.1", "2000::1")), + expect: svctest.MakeService("foo", svctest.SetClusterIPs("10.0.0.1", "2000::1"), + svctest.SetIPFamilyPolicy(api.IPFamilyPolicyRequireDualStack), + svctest.SetIPFamilies(api.IPv4Protocol, api.IPv6Protocol)), + }, { + name: "dual v6v4", + input: svctest.MakeService("foo", svctest.SetClusterIPs("2000::1", "10.0.0.1")), + expect: svctest.MakeService("foo", svctest.SetClusterIPs("2000::1", "10.0.0.1"), + svctest.SetIPFamilyPolicy(api.IPFamilyPolicyRequireDualStack), + svctest.SetIPFamilies(api.IPv6Protocol, api.IPv4Protocol)), + }, { + name: "headless", + input: svctest.MakeService("foo", svctest.SetHeadless), + expect: svctest.MakeService("foo", svctest.SetHeadless, + svctest.SetIPFamilyPolicy(api.IPFamilyPolicySingleStack), + svctest.SetIPFamilies(api.IPv4Protocol)), + }, { + name: "headless selectorless", + input: svctest.MakeService("foo", svctest.SetHeadless, + svctest.SetSelector(map[string]string{})), + expect: svctest.MakeService("foo", svctest.SetHeadless, + svctest.SetIPFamilyPolicy(api.IPFamilyPolicyRequireDualStack), + svctest.SetIPFamilies(api.IPv4Protocol, api.IPv6Protocol)), + }, { + name: "headless selectorless pre-set", + input: svctest.MakeService("foo", svctest.SetHeadless, + svctest.SetSelector(map[string]string{}), + svctest.SetIPFamilyPolicy(api.IPFamilyPolicySingleStack), + svctest.SetIPFamilies(api.IPv6Protocol)), + expect: svctest.MakeService("foo", svctest.SetHeadless, + svctest.SetIPFamilyPolicy(api.IPFamilyPolicySingleStack), + svctest.SetIPFamilies(api.IPv6Protocol)), }, { name: "not Service or ServiceList", input: &api.Pod{}, @@ -593,7 +643,7 @@ func TestServiceDefaultOnRead(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - storage, _, server := newStorage(t, []api.IPFamily{api.IPv4Protocol}) + storage, _, server := newStorage(t, []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol}) defer server.Terminate(t) defer storage.Store.DestroyFunc() @@ -619,11 +669,17 @@ func TestServiceDefaultOnRead(t *testing.T) { } // Verify fields we know are affected - if svc.Spec.ClusterIP != exp.Spec.ClusterIP { - t.Errorf("clusterIP: expected %v, got %v", exp.Spec.ClusterIP, svc.Spec.ClusterIP) + if want, got := exp.Spec.ClusterIP, svc.Spec.ClusterIP; want != got { + t.Errorf("clusterIP: expected %v, got %v", want, got) } - if !reflect.DeepEqual(svc.Spec.ClusterIPs, exp.Spec.ClusterIPs) { - t.Errorf("clusterIPs: expected %v, got %v", exp.Spec.ClusterIPs, svc.Spec.ClusterIPs) + if want, got := exp.Spec.ClusterIPs, svc.Spec.ClusterIPs; !reflect.DeepEqual(want, got) { + t.Errorf("clusterIPs: expected %v, got %v", want, got) + } + if want, got := fmtIPFamilyPolicy(exp.Spec.IPFamilyPolicy), fmtIPFamilyPolicy(svc.Spec.IPFamilyPolicy); want != got { + t.Errorf("ipFamilyPolicy: expected %v, got %v", want, got) + } + if want, got := exp.Spec.IPFamilies, svc.Spec.IPFamilies; !reflect.DeepEqual(want, got) { + t.Errorf("ipFamilies: expected %v, got %v", want, got) } }) }