fix auto upgraded preferDualStack 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)
 			}