Merge pull request #99555 from thockin/dualstack-bugs-from-rest-overhaul

Two small bugs in dual-stack init

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 {

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)
-			}
-		})
-
-	}
-}

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.
+		// Family is not specified yet.
-		if i > len(service.Spec.IPFamilies)-1 {
+		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
+	// Infer IPFamilyPolicy from IPFamilies[].  This block does not handle the
-	if len(service.Spec.ClusterIPs) > 0 && service.Spec.ClusterIPs[0] == api.ClusterIPNone && len(service.Spec.Selector) == 0 {
+	// 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 ||
+			if *(service.Spec.IPFamilyPolicy) != api.IPFamilyPolicySingleStack {
-				(*(service.Spec.IPFamilyPolicy) == api.IPFamilyPolicyPreferDualStack && len(rs.serviceIPAllocatorsByFamily) == 2) {
 				// 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
+	// Finally, if IPFamilyPolicy is *still* not set, we can default it to
-	// web hooks, they must have already ran by now
+	// SingleStack. If there are any webhooks, they have already run.
 	if service.Spec.IPFamilyPolicy == nil {
 		singleStack := api.IPFamilyPolicySingleStack
 		service.Spec.IPFamilyPolicy = &singleStack

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