From 1e39f6ccf9f70bd38fa3382d85d3a5a6fd121d7d Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Wed, 25 Nov 2020 00:07:46 -0800 Subject: [PATCH 1/2] 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 From 84856c7caedc239224870ce39da351b05a8d2e58 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Tue, 2 Mar 2021 21:03:24 -0800 Subject: [PATCH 2/2] Remove defaults for ipFamilyPolicy and ipFamilies These same values are set in the REST stack and it's easier to maintain that code all in one place. defaults.go is best for static values. --- pkg/apis/core/v1/defaults.go | 31 ---- pkg/apis/core/v1/defaults_test.go | 263 ------------------------------ 2 files changed, 294 deletions(-) diff --git a/pkg/apis/core/v1/defaults.go b/pkg/apis/core/v1/defaults.go index 28ceb24244e..96c8753ca8b 100644 --- a/pkg/apis/core/v1/defaults.go +++ b/pkg/apis/core/v1/defaults.go @@ -131,37 +131,6 @@ func SetDefaults_Service(obj *v1.Service) { obj.Spec.ExternalTrafficPolicy = v1.ServiceExternalTrafficPolicyTypeCluster } - if utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) { - // Default obj.Spec.IPFamilyPolicy if we *know* we can, otherwise it will - // be handled later in allocation. - if obj.Spec.Type != v1.ServiceTypeExternalName { - if obj.Spec.IPFamilyPolicy == nil { - if len(obj.Spec.ClusterIPs) == 2 || len(obj.Spec.IPFamilies) == 2 { - requireDualStack := v1.IPFamilyPolicyRequireDualStack - obj.Spec.IPFamilyPolicy = &requireDualStack - } - } - - // If the user demanded dual-stack, but only specified one family, we add - // the other. - if obj.Spec.IPFamilyPolicy != nil && *(obj.Spec.IPFamilyPolicy) == v1.IPFamilyPolicyRequireDualStack && len(obj.Spec.IPFamilies) == 1 { - if obj.Spec.IPFamilies[0] == v1.IPv4Protocol { - obj.Spec.IPFamilies = append(obj.Spec.IPFamilies, v1.IPv6Protocol) - } else { - obj.Spec.IPFamilies = append(obj.Spec.IPFamilies, v1.IPv4Protocol) - } - - // Any other dual-stack defaulting depends on cluster configuration. - // Further IPFamilies, IPFamilyPolicy defaulting is in ClusterIP alloc/reserve logic - // NOTE: strategy handles cases where ClusterIPs is used (but not ClusterIP). - } - } - - // any other defaulting depends on cluster configuration. - // further IPFamilies, IPFamilyPolicy defaulting is in ClusterIP alloc/reserve logic - // note: conversion logic handles cases where ClusterIPs is used (but not ClusterIP). - } - if utilfeature.DefaultFeatureGate.Enabled(features.ServiceLBNodePortControl) { if obj.Spec.Type == v1.ServiceTypeLoadBalancer { if obj.Spec.AllocateLoadBalancerNodePorts == nil { diff --git a/pkg/apis/core/v1/defaults_test.go b/pkg/apis/core/v1/defaults_test.go index dd7221be695..d0ea41f9af8 100644 --- a/pkg/apis/core/v1/defaults_test.go +++ b/pkg/apis/core/v1/defaults_test.go @@ -35,10 +35,6 @@ import ( // ensure types are installed _ "k8s.io/kubernetes/pkg/apis/core/install" - - utilfeature "k8s.io/apiserver/pkg/util/feature" - featuregatetesting "k8s.io/component-base/featuregate/testing" - "k8s.io/kubernetes/pkg/features" ) // TestWorkloadDefaults detects changes to defaults within PodTemplateSpec. @@ -1802,262 +1798,3 @@ func TestSetDefaultEnableServiceLinks(t *testing.T) { t.Errorf("Expected enableServiceLinks value: %+v\ngot: %+v\n", v1.DefaultEnableServiceLinks, *output.Spec.EnableServiceLinks) } } - -func TestSetDefaultIPFamilies(t *testing.T) { - preferDualStack := v1.IPFamilyPolicyPreferDualStack - requireDualStack := v1.IPFamilyPolicyRequireDualStack - testCases := []struct { - name string - expectedIPFamiliesWithGate []v1.IPFamily - svc v1.Service - }{ - { - name: "must not set for ExternalName", - expectedIPFamiliesWithGate: nil, - - svc: v1.Service{ - Spec: v1.ServiceSpec{ - Type: v1.ServiceTypeExternalName, - }, - }, - }, - { - name: "must not set for single stack", - expectedIPFamiliesWithGate: nil, - - svc: v1.Service{ - Spec: v1.ServiceSpec{ - Type: v1.ServiceTypeClusterIP, - }, - }, - }, - { - name: "must not set for single stack, even when a family is set", - expectedIPFamiliesWithGate: []v1.IPFamily{v1.IPv6Protocol}, - - svc: v1.Service{ - Spec: v1.ServiceSpec{ - Type: v1.ServiceTypeClusterIP, - IPFamilies: []v1.IPFamily{v1.IPv6Protocol}, - }, - }, - }, - { - name: "must not set for preferDualStack", - expectedIPFamiliesWithGate: []v1.IPFamily{v1.IPv6Protocol}, - - svc: v1.Service{ - Spec: v1.ServiceSpec{ - Type: v1.ServiceTypeClusterIP, - IPFamilyPolicy: &preferDualStack, - IPFamilies: []v1.IPFamily{v1.IPv6Protocol}, - }, - }, - }, - { - name: "must set for requireDualStack (6,4)", - expectedIPFamiliesWithGate: []v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol}, - - svc: v1.Service{ - Spec: v1.ServiceSpec{ - Type: v1.ServiceTypeClusterIP, - IPFamilyPolicy: &requireDualStack, - IPFamilies: []v1.IPFamily{v1.IPv6Protocol}, - }, - }, - }, - { - name: "must set for requireDualStack (4,6)", - expectedIPFamiliesWithGate: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol}, - - svc: v1.Service{ - Spec: v1.ServiceSpec{ - Type: v1.ServiceTypeClusterIP, - IPFamilyPolicy: &requireDualStack, - IPFamilies: []v1.IPFamily{v1.IPv4Protocol}, - }, - }, - }, - } - for _, test := range testCases { - // run with gate - t.Run(test.name, func(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, true)() - obj2 := roundTrip(t, runtime.Object(&test.svc)) - svc2 := obj2.(*v1.Service) - - if len(test.expectedIPFamiliesWithGate) != len(svc2.Spec.IPFamilies) { - t.Fatalf("expected .spec.IPFamilies len:%v got %v", len(test.expectedIPFamiliesWithGate), len(svc2.Spec.IPFamilies)) - } - - for i, family := range test.expectedIPFamiliesWithGate { - if svc2.Spec.IPFamilies[i] != family { - t.Fatalf("failed. expected family %v at %v got %v", family, i, svc2.Spec.IPFamilies[i]) - } - } - }) - - // TODO: @khenidak - // with BETA feature is always on. This ensure we test for gate on/off scenarios - // as we move to stable this entire test will need to be folded into one test - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, false)() - - // run without gate (families should not change) - t.Run(fmt.Sprintf("without-gate:%s", test.name), func(t *testing.T) { - obj2 := roundTrip(t, runtime.Object(&test.svc)) - svc2 := obj2.(*v1.Service) - - if len(test.svc.Spec.IPFamilies) != len(svc2.Spec.IPFamilies) { - t.Fatalf("expected .spec.IPFamilies len:%v got %v", len(test.expectedIPFamiliesWithGate), len(svc2.Spec.IPFamilies)) - } - - for i, family := range test.svc.Spec.IPFamilies { - if svc2.Spec.IPFamilies[i] != family { - t.Fatalf("failed. expected family %v at %v got %v", family, i, svc2.Spec.IPFamilies[i]) - } - } - }) - - } -} - -func TestSetDefaultServiceIPFamilyPolicy(t *testing.T) { - singleStack := v1.IPFamilyPolicySingleStack - preferDualStack := v1.IPFamilyPolicyPreferDualStack - requireDualStack := v1.IPFamilyPolicyRequireDualStack - - testCases := []struct { - name string - expectedIPfamilyPolicy *v1.IPFamilyPolicyType - svc v1.Service - }{ - { - name: "must not set for ExternalName", - expectedIPfamilyPolicy: nil, - - svc: v1.Service{ - Spec: v1.ServiceSpec{ - Type: v1.ServiceTypeExternalName, - }, - }, - }, - { - name: "must not set for ExternalName even with semantically valid data", - expectedIPfamilyPolicy: nil, - - svc: v1.Service{ - Spec: v1.ServiceSpec{ - Type: v1.ServiceTypeExternalName, - ClusterIPs: []string{"1.2.3.4", "2001::1"}, - IPFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol}, - }, - }, - }, - { - name: "must set if there are more than one ip", - expectedIPfamilyPolicy: &requireDualStack, - - svc: v1.Service{ - Spec: v1.ServiceSpec{ - ClusterIPs: []string{"1.2.3.4", "2001::1"}, - }, - }, - }, - { - name: "must set if there are more than one ip family", - expectedIPfamilyPolicy: &requireDualStack, - - svc: v1.Service{ - Spec: v1.ServiceSpec{ - IPFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol}, - }, - }, - }, - { - name: "must not set if there is one ip", - expectedIPfamilyPolicy: nil, - - svc: v1.Service{ - Spec: v1.ServiceSpec{ - ClusterIPs: []string{"1.2.3.4"}, - }, - }, - }, - { - name: "must not set if there is one ip family", - expectedIPfamilyPolicy: nil, - - svc: v1.Service{ - Spec: v1.ServiceSpec{ - IPFamilies: []v1.IPFamily{v1.IPv4Protocol}, - }, - }, - }, - { - name: "must not override user input", - expectedIPfamilyPolicy: &singleStack, - - svc: v1.Service{ - Spec: v1.ServiceSpec{ - IPFamilyPolicy: &singleStack, - }, - }, - }, - { - name: "must not override user input/ preferDualStack", - expectedIPfamilyPolicy: &preferDualStack, - - svc: v1.Service{ - Spec: v1.ServiceSpec{ - IPFamilyPolicy: &preferDualStack, - }, - }, - }, - - { - name: "must not override user input even when it is invalid", - expectedIPfamilyPolicy: &preferDualStack, - - svc: v1.Service{ - Spec: v1.ServiceSpec{ - IPFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol}, - IPFamilyPolicy: &preferDualStack, - }, - }, - }, - } - - for _, test := range testCases { - // with gate - t.Run(test.name, func(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, true)() - obj2 := roundTrip(t, runtime.Object(&test.svc)) - svc2 := obj2.(*v1.Service) - - if test.expectedIPfamilyPolicy == nil && svc2.Spec.IPFamilyPolicy != nil { - t.Fatalf("expected .spec.PreferDualStack to be unset (nil)") - } - if test.expectedIPfamilyPolicy != nil && (svc2.Spec.IPFamilyPolicy == nil || *(svc2.Spec.IPFamilyPolicy) != *(test.expectedIPfamilyPolicy)) { - t.Fatalf("expected .spec.PreferDualStack to be set to %v got %v", *(test.expectedIPfamilyPolicy), svc2.Spec.IPFamilyPolicy) - } - }) - - // without gate. IPFamilyPolicy should never change - t.Run(test.name, func(t *testing.T) { - //TODO: @khenidak - // BETA feature. gate is on. when we go stable fold the test into one scenario - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, false)() - - obj2 := roundTrip(t, runtime.Object(&test.svc)) - svc2 := obj2.(*v1.Service) - - if test.svc.Spec.IPFamilyPolicy == nil && svc2.Spec.IPFamilyPolicy != nil { - t.Fatalf("expected .spec.PreferDualStack to be unset (nil)") - } - if test.svc.Spec.IPFamilyPolicy != nil && (svc2.Spec.IPFamilyPolicy == nil || *(svc2.Spec.IPFamilyPolicy) != *(test.expectedIPfamilyPolicy)) { - t.Fatalf("expected .spec.PreferDualStack to be set to %v got %v", *(test.expectedIPfamilyPolicy), svc2.Spec.IPFamilyPolicy) - } - }) - - } -}