Merge pull request #102898 from khenidak/fix-prefer-dualstack

fix auto upgraded preferDual-Stack services (in cluster upgrade)

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

@@ -211,7 +211,7 @@ func (rs *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 	// try set ip families (for missing ip families)
 	// we do it here, since we want this to be visible
 	// even when dryRun == true
-	if err := rs.tryDefaultValidateServiceClusterIPFields(service); err != nil {
+	if err := rs.tryDefaultValidateServiceClusterIPFields(nil, service); err != nil {
 		return nil, err
 	}
 
@@ -461,7 +461,7 @@ func (rs *REST) Update(ctx context.Context, name string, objInfo rest.UpdatedObj
 	defer nodePortOp.Finish()
 
 	// try set ip families (for missing ip families)
-	if err := rs.tryDefaultValidateServiceClusterIPFields(service); err != nil {
+	if err := rs.tryDefaultValidateServiceClusterIPFields(oldService, service); err != nil {
 		return nil, false, err
 	}
 
@@ -730,6 +730,19 @@ func (rs *REST) allocServiceClusterIPs(service *api.Service) (map[api.IPFamily]s
 // a map[family]ip for the caller to release when everything else has
 // executed successfully
 func (rs *REST) handleClusterIPsForUpdatedService(oldService *api.Service, service *api.Service) (allocated map[api.IPFamily]string, toRelease map[api.IPFamily]string, err error) {
+
+	// We don't want to upgrade (add an IP) or downgrade (remove an IP)
+	// following a cluster downgrade/upgrade to/from dual-stackness
+	// a PreferDualStack service following principle of least surprise
+	// That means: PreferDualStack service will only be upgraded
+	// if:
+	// - changes type to RequireDualStack
+	// - manually adding or removing ClusterIP (secondary)
+	// - manually adding or removing IPFamily (secondary)
+	if isMatchingPreferDualStackClusterIPFields(oldService, service) {
+		return allocated, toRelease, nil // nothing more to do.
+	}
+
 	// use cases:
 	// A: service changing types from ExternalName TO ClusterIP types ==> allocate all new
 	// B: service changing types from ClusterIP types TO ExternalName ==> release all allocated
@@ -846,9 +859,77 @@ func (rs *REST) releaseServiceClusterIPs(service *api.Service) (released map[api
 	return rs.releaseClusterIPs(toRelease)
 }
 
+// tests if two preferred dual-stack service have matching ClusterIPFields
+// assumption: old service is a valid, default service (e.g., loaded from store)
+func isMatchingPreferDualStackClusterIPFields(oldService, service *api.Service) bool {
+	if oldService == nil {
+		return false
+	}
+
+	if service.Spec.IPFamilyPolicy == nil {
+		return false
+	}
+
+	// if type mutated then it is an update
+	// that needs to run through the entire process.
+	if oldService.Spec.Type != service.Spec.Type {
+		return false
+	}
+	// both must be type that gets an IP assigned
+	if service.Spec.Type != api.ServiceTypeClusterIP &&
+		service.Spec.Type != api.ServiceTypeNodePort &&
+		service.Spec.Type != api.ServiceTypeLoadBalancer {
+		return false
+	}
+
+	if oldService.Spec.Type != api.ServiceTypeClusterIP &&
+		oldService.Spec.Type != api.ServiceTypeNodePort &&
+		oldService.Spec.Type != api.ServiceTypeLoadBalancer {
+		return false
+	}
+
+	// both must be of IPFamilyPolicy==PreferDualStack
+	if service.Spec.IPFamilyPolicy != nil && *(service.Spec.IPFamilyPolicy) != api.IPFamilyPolicyPreferDualStack {
+		return false
+	}
+
+	if oldService.Spec.IPFamilyPolicy != nil && *(oldService.Spec.IPFamilyPolicy) != api.IPFamilyPolicyPreferDualStack {
+		return false
+	}
+
+	// compare ClusterIPs lengths.
+	// due to validation.
+	if len(service.Spec.ClusterIPs) != len(oldService.Spec.ClusterIPs) {
+		return false
+	}
+
+	for i, ip := range service.Spec.ClusterIPs {
+		if oldService.Spec.ClusterIPs[i] != ip {
+			return false
+		}
+	}
+
+	// compare IPFamilies
+	if len(service.Spec.IPFamilies) != len(oldService.Spec.IPFamilies) {
+		return false
+	}
+
+	for i, family := range service.Spec.IPFamilies {
+		if oldService.Spec.IPFamilies[i] != family {
+			return false
+		}
+	}
+
+	// they match on
+	// Policy: preferDualStack
+	// ClusterIPs
+	// IPFamilies
+	return true
+}
+
 // attempts to default service ip families according to cluster configuration
 // while ensuring that provided families are configured on cluster.
-func (rs *REST) tryDefaultValidateServiceClusterIPFields(service *api.Service) error {
+func (rs *REST) tryDefaultValidateServiceClusterIPFields(oldService, service *api.Service) error {
 	// can not do anything here
 	if service.Spec.Type == api.ServiceTypeExternalName {
 		return nil
@@ -860,6 +941,18 @@ func (rs *REST) tryDefaultValidateServiceClusterIPFields(service *api.Service) e
 		return nil
 	}
 
+	// We don't want to upgrade (add an IP) or downgrade (remove an IP)
+	// following a cluster downgrade/upgrade to/from dual-stackness
+	// a PreferDualStack service following principle of least surprise
+	// That means: PreferDualStack service will only be upgraded
+	// if:
+	// - changes type to RequireDualStack
+	// - manually adding or removing ClusterIP (secondary)
+	// - manually adding or removing IPFamily (secondary)
+	if isMatchingPreferDualStackClusterIPFields(oldService, service) {
+		return nil // nothing more to do.
+	}
+
 	// two families or two IPs with SingleStack
 	if service.Spec.IPFamilyPolicy != nil {
 		el := make(field.ErrorList, 0)

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

@@ -3684,6 +3684,7 @@ func TestDefaultingValidation(t *testing.T) {
 	testCases := []struct {
 		name       string
 		modifyRest func(rest *REST)
+		oldSvc     *api.Service
 		svc        api.Service
 
 		expectedIPFamilyPolicy *api.IPFamilyPolicyType
@@ -5094,6 +5095,109 @@ func TestDefaultingValidation(t *testing.T) {
 			expectedIPFamilies:     nil,
 			expectError:            true,
 		},
+
+		// preferDualStack services should not be updated
+		// to match cluster config if the user didn't change any
+		// ClusterIPs related fields
+		{
+			name:       "unchanged preferDualStack-1-ClusterUpgraded",
+			modifyRest: fnMakeDualStackStackIPv4IPv6Allocator,
+			oldSvc: &api.Service{
+				Spec: api.ServiceSpec{
+					Type:           api.ServiceTypeClusterIP,
+					ClusterIPs:     []string{"1.1.1.1"},
+					IPFamilies:     []api.IPFamily{api.IPv4Protocol},
+					IPFamilyPolicy: &preferDualStack,
+				},
+			},
+
+			svc: api.Service{
+				Spec: api.ServiceSpec{
+					Type:           api.ServiceTypeClusterIP,
+					ClusterIPs:     []string{"1.1.1.1"},
+					IPFamilies:     []api.IPFamily{api.IPv4Protocol},
+					IPFamilyPolicy: &preferDualStack,
+				},
+			},
+			expectedIPFamilyPolicy: &preferDualStack,
+			expectedIPFamilies:     []api.IPFamily{api.IPv4Protocol},
+			expectError:            false,
+		},
+
+		{
+			name:       "unchanged preferDualStack-2-ClusterDowngraded",
+			modifyRest: fnMakeSingleStackIPv4Allocator,
+			oldSvc: &api.Service{
+				Spec: api.ServiceSpec{
+					Type:           api.ServiceTypeClusterIP,
+					ClusterIPs:     []string{"1.1.1.1", "2001::1"},
+					IPFamilies:     []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
+					IPFamilyPolicy: &preferDualStack,
+				},
+			},
+
+			svc: api.Service{
+				Spec: api.ServiceSpec{
+					Type:           api.ServiceTypeClusterIP,
+					ClusterIPs:     []string{"1.1.1.1", "2001::1"},
+					IPFamilies:     []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
+					IPFamilyPolicy: &preferDualStack,
+				},
+			},
+			expectedIPFamilyPolicy: &preferDualStack,
+			expectedIPFamilies:     []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
+			expectError:            false,
+		},
+
+		{
+			name:       "changed preferDualStack-1 (cluster upgraded)",
+			modifyRest: fnMakeDualStackStackIPv4IPv6Allocator,
+			oldSvc: &api.Service{
+				Spec: api.ServiceSpec{
+					Type:           api.ServiceTypeClusterIP,
+					ClusterIPs:     []string{"1.1.1.1"},
+					IPFamilies:     []api.IPFamily{api.IPv4Protocol},
+					IPFamilyPolicy: &preferDualStack,
+				},
+			},
+
+			svc: api.Service{
+				Spec: api.ServiceSpec{
+					Type:           api.ServiceTypeClusterIP,
+					ClusterIPs:     nil,
+					IPFamilies:     []api.IPFamily{api.IPv4Protocol},
+					IPFamilyPolicy: &requireDualStack,
+				},
+			},
+			expectedIPFamilyPolicy: &requireDualStack,
+			expectedIPFamilies:     []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
+			expectError:            false,
+		},
+
+		{
+			name:       "changed preferDualStack-2-ClusterDowngraded",
+			modifyRest: fnMakeSingleStackIPv4Allocator,
+			oldSvc: &api.Service{
+				Spec: api.ServiceSpec{
+					Type:           api.ServiceTypeClusterIP,
+					ClusterIPs:     []string{"1.1.1.1", "2001::1"},
+					IPFamilies:     []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
+					IPFamilyPolicy: &preferDualStack,
+				},
+			},
+
+			svc: api.Service{
+				Spec: api.ServiceSpec{
+					Type:           api.ServiceTypeClusterIP,
+					ClusterIPs:     []string{"1.1.1.1"},
+					IPFamilies:     []api.IPFamily{api.IPv4Protocol},
+					IPFamilyPolicy: &singleStack,
+				},
+			},
+			expectedIPFamilyPolicy: &singleStack,
+			expectedIPFamilies:     []api.IPFamily{api.IPv4Protocol},
+			expectError:            false,
+		},
 	}
 
 	// This func only runs when feature gate is on
@@ -5112,7 +5216,7 @@ func TestDefaultingValidation(t *testing.T) {
 				testCase.modifyRest(storage)
 			}
 
-			err := storage.tryDefaultValidateServiceClusterIPFields(&testCase.svc)
+			err := storage.tryDefaultValidateServiceClusterIPFields(testCase.oldSvc, &testCase.svc)
 			if err != nil && !testCase.expectError {
 				t.Fatalf("error %v was not expected", err)
 			}

test/integration/dualstack/dualstack_test.go

@@ -1479,3 +1479,190 @@ func validateServiceAndClusterIPFamily(svc *v1.Service, expectedIPFamilies []v1.
 
 	return nil
 }
+
+func TestUpgradeServicePreferToDualStack(t *testing.T) {
+	// Create an IPv4 only dual stack control-plane
+	serviceCIDR := "192.168.0.0/24"
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, true)()
+
+	cfg := framework.NewIntegrationTestControlPlaneConfig()
+	_, cidr, err := net.ParseCIDR(serviceCIDR)
+	if err != nil {
+		t.Fatalf("bad cidr: %v", err)
+	}
+	cfg.ExtraConfig.ServiceIPRange = *cidr
+	_, s, closeFn := framework.RunAnAPIServer(cfg)
+
+	client := clientset.NewForConfigOrDie(&restclient.Config{Host: s.URL})
+
+	// Wait until the default "kubernetes" service is created.
+	if err = wait.Poll(250*time.Millisecond, time.Minute, func() (bool, error) {
+		_, err := client.CoreV1().Services(metav1.NamespaceDefault).Get(context.TODO(), "kubernetes", metav1.GetOptions{})
+		if err != nil && !apierrors.IsNotFound(err) {
+			return false, err
+		}
+		return !apierrors.IsNotFound(err), nil
+	}); err != nil {
+		t.Fatalf("creating kubernetes service timed out")
+	}
+
+	preferDualStack := v1.IPFamilyPolicyPreferDualStack
+	svc := &v1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "svc-prefer-dual",
+		},
+		Spec: v1.ServiceSpec{
+			Type:           v1.ServiceTypeClusterIP,
+			ClusterIPs:     nil,
+			IPFamilies:     nil,
+			IPFamilyPolicy: &preferDualStack,
+			Ports: []v1.ServicePort{
+				{
+					Name:       "svc-port-1",
+					Port:       443,
+					TargetPort: intstr.IntOrString{IntVal: 443},
+					Protocol:   "TCP",
+				},
+			},
+		},
+	}
+
+	// create the service
+	_, err = client.CoreV1().Services(metav1.NamespaceDefault).Create(context.TODO(), svc, metav1.CreateOptions{})
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
+	// validate the service was created correctly if it was not expected to fail
+	svc, err = client.CoreV1().Services(metav1.NamespaceDefault).Get(context.TODO(), svc.Name, metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("Unexpected error to get the service %s %v", svc.Name, err)
+	}
+	if err := validateServiceAndClusterIPFamily(svc, []v1.IPFamily{v1.IPv4Protocol}); err != nil {
+		t.Fatalf("Unexpected error validating the service %s %v", svc.Name, err)
+	}
+
+	// reconfigure the apiserver to be dual-stack
+	closeFn()
+
+	secondaryServiceCIDR := "2001:db8:1::/48"
+	_, secCidr, err := net.ParseCIDR(secondaryServiceCIDR)
+	if err != nil {
+		t.Fatalf("bad cidr: %v", err)
+	}
+	cfg.ExtraConfig.SecondaryServiceIPRange = *secCidr
+	_, s, closeFn = framework.RunAnAPIServer(cfg)
+	defer closeFn()
+
+	client = clientset.NewForConfigOrDie(&restclient.Config{Host: s.URL})
+
+	// Wait until the default "kubernetes" service is created.
+	if err = wait.Poll(250*time.Millisecond, time.Minute, func() (bool, error) {
+		_, err := client.CoreV1().Services(metav1.NamespaceDefault).Get(context.TODO(), "kubernetes", metav1.GetOptions{})
+		if err != nil && !apierrors.IsNotFound(err) {
+			return false, err
+		}
+		return !apierrors.IsNotFound(err), nil
+	}); err != nil {
+		t.Fatalf("creating kubernetes service timed out")
+	}
+	// validate the service was created correctly if it was not expected to fail
+	svc, err = client.CoreV1().Services(metav1.NamespaceDefault).Get(context.TODO(), svc.Name, metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("Unexpected error to get the service %s %v", svc.Name, err)
+	}
+	// service should remain single stack
+	if err = validateServiceAndClusterIPFamily(svc, []v1.IPFamily{v1.IPv4Protocol}); err != nil {
+		t.Fatalf("Unexpected error validating the service %s %v", svc.Name, err)
+	}
+}
+
+func TestDowngradeServicePreferToDualStack(t *testing.T) {
+	// Create a dual stack control-plane
+	serviceCIDR := "192.168.0.0/24"
+	secondaryServiceCIDR := "2001:db8:1::/48"
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, true)()
+	dualStackCfg := framework.NewIntegrationTestControlPlaneConfig()
+	_, cidr, err := net.ParseCIDR(serviceCIDR)
+	if err != nil {
+		t.Fatalf("bad cidr: %v", err)
+	}
+	dualStackCfg.ExtraConfig.ServiceIPRange = *cidr
+	_, secCidr, err := net.ParseCIDR(secondaryServiceCIDR)
+	if err != nil {
+		t.Fatalf("bad cidr: %v", err)
+	}
+	dualStackCfg.ExtraConfig.SecondaryServiceIPRange = *secCidr
+	_, s, closeFn := framework.RunAnAPIServer(dualStackCfg)
+	client := clientset.NewForConfigOrDie(&restclient.Config{Host: s.URL})
+	// Wait until the default "kubernetes" service is created.
+	if err = wait.Poll(250*time.Millisecond, time.Minute, func() (bool, error) {
+		_, err := client.CoreV1().Services(metav1.NamespaceDefault).Get(context.TODO(), "kubernetes", metav1.GetOptions{})
+		if err != nil && !apierrors.IsNotFound(err) {
+			return false, err
+		}
+		return !apierrors.IsNotFound(err), nil
+	}); err != nil {
+		t.Fatalf("creating kubernetes service timed out")
+	}
+	preferDualStack := v1.IPFamilyPolicyPreferDualStack
+	svc := &v1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "svc-prefer-dual01",
+		},
+		Spec: v1.ServiceSpec{
+			Type:           v1.ServiceTypeClusterIP,
+			ClusterIPs:     nil,
+			IPFamilies:     nil,
+			IPFamilyPolicy: &preferDualStack,
+			Ports: []v1.ServicePort{
+				{
+					Name:       "svc-port-1",
+					Port:       443,
+					TargetPort: intstr.IntOrString{IntVal: 443},
+					Protocol:   "TCP",
+				},
+			},
+		},
+	}
+	// create the service
+	_, err = client.CoreV1().Services(metav1.NamespaceDefault).Create(context.TODO(), svc, metav1.CreateOptions{})
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
+	// validate the service was created correctly if it was not expected to fail
+	svc, err = client.CoreV1().Services(metav1.NamespaceDefault).Get(context.TODO(), svc.Name, metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("Unexpected error to get the service %s %v", svc.Name, err)
+	}
+	if err := validateServiceAndClusterIPFamily(svc, []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol}); err != nil {
+		t.Fatalf("Unexpected error validating the service %s %v", svc.Name, err)
+	}
+	// reconfigure the apiserver to be sinlge stack
+	closeFn()
+	// reset secondary
+	var emptyCidr net.IPNet
+	dualStackCfg.ExtraConfig.SecondaryServiceIPRange = emptyCidr
+
+	_, s, closeFn = framework.RunAnAPIServer(dualStackCfg)
+	defer closeFn()
+	client = clientset.NewForConfigOrDie(&restclient.Config{Host: s.URL})
+	// Wait until the default "kubernetes" service is created.
+	if err = wait.Poll(250*time.Millisecond, time.Minute, func() (bool, error) {
+		_, err := client.CoreV1().Services(metav1.NamespaceDefault).Get(context.TODO(), "kubernetes", metav1.GetOptions{})
+		if err != nil && !apierrors.IsNotFound(err) {
+			return false, err
+		}
+		return !apierrors.IsNotFound(err), nil
+	}); err != nil {
+		t.Fatalf("creating kubernetes service timed out")
+	}
+	// validate the service is still there.
+	svc, err = client.CoreV1().Services(metav1.NamespaceDefault).Get(context.TODO(), svc.Name, metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("Unexpected error to get the service %s %v", svc.Name, err)
+	}
+	// service should be single stack
+	if err = validateServiceAndClusterIPFamily(svc, []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol}); err != nil {
+		t.Fatalf("Unexpected error validating the service %s %v", svc.Name, err)
+	}
+}