Svc REST: Fix single<->dual-stack updates

This removes the old rest_tests and adds significantly more coverage.
Maybe too much.  The v4,v6 and v6,v4 tables are identical but for the
order of families.

This exposed that `trimFieldsForDualStackDowngrade` is called too late
to do anything (since we don't run strategy twice any more).  I moved
similar logic to `PatchAllocatedValues` but I hit on some unclarity.

Specifically, consider a PATCH operation.

Assume I have a valid dual-stack service (with 2 IPs, 2 families, and
policy either require or prefer). What fields can I patch, on their own,
to trigger a downgrade to single-stack?

I think patching policy to "single" is pretty clear intent.

But what if I leave policy and only patch `ipFamilies` down to a single
value (without violating the "can't change first family" rule)?

Or what if I patch `clusterIPs` down to a single IP value?

After much discussion, we decided to make a small API change (OK since
we are beta).  When you want a dual-stack Service you MUST specify the
`ipFamilyPolicy`.  Now we can infer less and avoid ambiguity.

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

@@ -721,29 +721,14 @@ func isMatchingPreferDualStackClusterIPFields(oldService, service *api.Service)
 		return false
 	}
 
-	// compare ClusterIPs lengths.
+	if !sameClusterIPs(oldService, service) {
-	// due to validation.
-	if len(service.Spec.ClusterIPs) != len(oldService.Spec.ClusterIPs) {
 		return false
 	}
 
-	for i, ip := range service.Spec.ClusterIPs {
+	if !sameIPFamilies(oldService, service) {
-		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
@@ -865,6 +850,7 @@ func (al *RESTAllocStuff) initIPFamilyFields(oldService, service *api.Service) e
 			// Update: As long as ClusterIPs and IPFamilies have not changed,
 			// setting policy to single-stack is clear intent.
 			if *(service.Spec.IPFamilyPolicy) == api.IPFamilyPolicySingleStack {
+				// ClusterIPs[0] is immutable, so it is safe to keep.
 				if sameClusterIPs(oldService, service) && len(service.Spec.ClusterIPs) > 1 {
 					service.Spec.ClusterIPs = service.Spec.ClusterIPs[0:1]
 				}

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

@@ -26,7 +26,6 @@ import (
 	"sort"
 	"testing"
 
-	"k8s.io/apimachinery/pkg/api/errors"
 	metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -40,11 +39,8 @@ import (
 	etcd3testing "k8s.io/apiserver/pkg/storage/etcd3/testing"
 	"k8s.io/apiserver/pkg/storage/storagebackend"
 	"k8s.io/apiserver/pkg/util/dryrun"
-	utilfeature "k8s.io/apiserver/pkg/util/feature"
-	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	svctest "k8s.io/kubernetes/pkg/api/service/testing"
 	api "k8s.io/kubernetes/pkg/apis/core"
-	"k8s.io/kubernetes/pkg/features"
 	endpointstore "k8s.io/kubernetes/pkg/registry/core/endpoint/storage"
 	podstore "k8s.io/kubernetes/pkg/registry/core/pod/storage"
 	"k8s.io/kubernetes/pkg/registry/core/service/ipallocator"
@@ -676,56 +672,6 @@ func TestServiceRegistryList(t *testing.T) {
 	}
 }
 
-func TestServiceRegistryIPUpdate(t *testing.T) {
-	storage, server := NewTestREST(t, []api.IPFamily{api.IPv4Protocol})
-	defer server.Terminate(t)
-
-	svc := svctest.MakeService("foo")
-	ctx := genericapirequest.NewDefaultContext()
-	createdSvc, err := storage.Create(ctx, svc, rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	createdService := createdSvc.(*api.Service)
-	if createdService.Spec.Ports[0].Port != svc.Spec.Ports[0].Port {
-		t.Errorf("Expected port %d, but got %v", svc.Spec.Ports[0].Port, createdService.Spec.Ports[0].Port)
-	}
-	if !makeIPNet(t).Contains(netutils.ParseIPSloppy(createdService.Spec.ClusterIPs[0])) {
-		t.Errorf("Unexpected ClusterIP: %s", createdService.Spec.ClusterIPs[0])
-	}
-
-	update := createdService.DeepCopy()
-	update.Spec.Ports[0].Port = 6503
-
-	updatedSvc, _, errUpdate := storage.Update(ctx, update.Name, rest.DefaultUpdatedObjectInfo(update), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{})
-	if errUpdate != nil {
-		t.Fatalf("unexpected error during update %v", errUpdate)
-	}
-	updatedService := updatedSvc.(*api.Service)
-	if updatedService.Spec.Ports[0].Port != 6503 {
-		t.Errorf("Expected port 6503, but got %v", updatedService.Spec.Ports[0].Port)
-	}
-
-	testIPs := []string{"1.2.3.93", "1.2.3.94", "1.2.3.95", "1.2.3.96"}
-	testIP := ""
-	for _, ip := range testIPs {
-		if !ipIsAllocated(t, storage.alloc.serviceIPAllocatorsByFamily[storage.alloc.defaultServiceIPFamily].(*ipallocator.Range), ip) {
-			testIP = ip
-			break
-		}
-	}
-
-	update = updatedService.DeepCopy()
-	update.Spec.Ports[0].Port = 6503
-	update.Spec.ClusterIP = testIP
-	update.Spec.ClusterIPs[0] = testIP
-
-	_, _, err = storage.Update(ctx, update.Name, rest.DefaultUpdatedObjectInfo(update), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{})
-	if err == nil || !errors.IsInvalid(err) {
-		t.Errorf("Unexpected error type: %v", err)
-	}
-}
-
 // Validate the internalTrafficPolicy field when set to "Cluster" then updated to "Local"
 func TestServiceRegistryInternalTrafficPolicyClusterThenLocal(t *testing.T) {
 	ctx := genericapirequest.NewDefaultContext()
@@ -789,422 +735,3 @@ func TestServiceRegistryInternalTrafficPolicyLocalThenCluster(t *testing.T) {
 		t.Errorf("Expected internalTrafficPolicy to be Cluster, got: %s", *updatedService.Spec.InternalTrafficPolicy)
 	}
 }
-
-func TestServiceUpgrade(t *testing.T) {
-	requireDualStack := api.IPFamilyPolicyRequireDualStack
-
-	ctx := genericapirequest.NewDefaultContext()
-	testCases := []struct {
-		name                     string
-		updateFunc               func(svc *api.Service)
-		enableDualStackAllocator bool
-		enableDualStackGate      bool
-		allocateIPsBeforeUpdate  map[api.IPFamily]string
-		expectUpgradeError       bool
-		svc                      *api.Service
-	}{{
-		name:                     "normal, no upgrade needed",
-		enableDualStackAllocator: false,
-		enableDualStackGate:      true,
-		allocateIPsBeforeUpdate:  nil,
-		expectUpgradeError:       false,
-
-		updateFunc: func(s *api.Service) {
-			s.Spec.Selector = map[string]string{"bar": "baz2"}
-		},
-
-		svc: svctest.MakeService("foo"),
-	}, {
-		name:                     "error, no upgrade (has single allocator)",
-		enableDualStackAllocator: false,
-		enableDualStackGate:      true,
-		allocateIPsBeforeUpdate:  nil,
-		expectUpgradeError:       true,
-
-		updateFunc: func(s *api.Service) {
-			s.Spec.IPFamilyPolicy = &requireDualStack
-			s.Spec.IPFamilies = []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol}
-		},
-
-		svc: svctest.MakeService("foo", func(s *api.Service) {
-			s.Spec.IPFamilies = []api.IPFamily{api.IPv4Protocol}
-		}),
-	}, {
-		name:                     "upgrade to v4,6",
-		enableDualStackAllocator: true,
-		enableDualStackGate:      true,
-		allocateIPsBeforeUpdate:  nil,
-		expectUpgradeError:       false,
-
-		updateFunc: func(s *api.Service) {
-			s.Spec.IPFamilyPolicy = &requireDualStack
-			s.Spec.IPFamilies = []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol}
-		},
-
-		svc: svctest.MakeService("foo", func(s *api.Service) {
-			s.Spec.IPFamilies = []api.IPFamily{api.IPv4Protocol}
-		}),
-	}, {
-		name:                     "upgrade to v4,6 (specific ip)",
-		enableDualStackAllocator: true,
-		enableDualStackGate:      true,
-		allocateIPsBeforeUpdate:  nil,
-		expectUpgradeError:       false,
-
-		updateFunc: func(s *api.Service) {
-			s.Spec.IPFamilyPolicy = &requireDualStack
-			s.Spec.ClusterIPs = append(s.Spec.ClusterIPs, "2000:0:0:0:0:0:0:1")
-			s.Spec.IPFamilies = []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol}
-		},
-
-		svc: svctest.MakeService("foo", func(s *api.Service) {
-			s.Spec.IPFamilies = []api.IPFamily{api.IPv4Protocol}
-		}),
-	}, {
-		name:                     "upgrade to v4,6 (specific ip) - fail, ip is not available",
-		enableDualStackAllocator: true,
-		enableDualStackGate:      true,
-		allocateIPsBeforeUpdate:  map[api.IPFamily]string{api.IPv6Protocol: "2000:0:0:0:0:0:0:1"},
-		expectUpgradeError:       true,
-
-		updateFunc: func(s *api.Service) {
-			s.Spec.IPFamilyPolicy = &requireDualStack
-			s.Spec.ClusterIPs = append(s.Spec.ClusterIPs, "2000:0:0:0:0:0:0:1")
-			s.Spec.IPFamilies = []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol}
-		},
-
-		svc: svctest.MakeService("foo", func(s *api.Service) {
-			s.Spec.IPFamilies = []api.IPFamily{api.IPv4Protocol}
-		}),
-	}, {
-		name:                     "upgrade to v6,4",
-		enableDualStackAllocator: true,
-		enableDualStackGate:      true,
-		allocateIPsBeforeUpdate:  nil,
-		expectUpgradeError:       false,
-
-		updateFunc: func(s *api.Service) {
-			s.Spec.IPFamilyPolicy = &requireDualStack
-			s.Spec.IPFamilies = []api.IPFamily{api.IPv6Protocol, api.IPv4Protocol}
-		},
-
-		svc: svctest.MakeService("foo", func(s *api.Service) {
-			s.Spec.IPFamilies = []api.IPFamily{api.IPv6Protocol}
-		}),
-	}, {
-		name:                     "upgrade to v6,4 (specific ip)",
-		enableDualStackAllocator: true,
-		enableDualStackGate:      true,
-		allocateIPsBeforeUpdate:  nil,
-		expectUpgradeError:       false,
-
-		updateFunc: func(s *api.Service) {
-			s.Spec.IPFamilyPolicy = &requireDualStack
-			s.Spec.ClusterIPs = append(s.Spec.ClusterIPs, "1.2.3.4")
-			s.Spec.IPFamilies = []api.IPFamily{api.IPv6Protocol, api.IPv4Protocol}
-		},
-
-		svc: svctest.MakeService("foo", func(s *api.Service) {
-			s.Spec.IPFamilies = []api.IPFamily{api.IPv6Protocol}
-		}),
-	}, {
-		name:                     "upgrade to v6,4 (specific ip) - fail ip is already allocated",
-		enableDualStackAllocator: true,
-		enableDualStackGate:      true,
-		allocateIPsBeforeUpdate:  map[api.IPFamily]string{api.IPv4Protocol: "1.2.3.4"},
-		expectUpgradeError:       true,
-
-		updateFunc: func(s *api.Service) {
-			s.Spec.IPFamilyPolicy = &requireDualStack
-			s.Spec.ClusterIPs = append(s.Spec.ClusterIPs, "1.2.3.4")
-			s.Spec.IPFamilies = []api.IPFamily{api.IPv6Protocol, api.IPv4Protocol}
-		},
-
-		svc: svctest.MakeService("foo", func(s *api.Service) {
-			s.Spec.IPFamilies = []api.IPFamily{api.IPv6Protocol}
-		}),
-	}}
-
-	for _, testCase := range testCases {
-		t.Run(testCase.name, func(t *testing.T) {
-			families := []api.IPFamily{api.IPv4Protocol}
-			if testCase.enableDualStackAllocator {
-				families = append(families, api.IPv6Protocol)
-			}
-			storage, server := NewTestREST(t, families)
-			defer server.Terminate(t)
-			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, testCase.enableDualStackGate)()
-
-			obj, err := storage.Create(ctx, testCase.svc, rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
-			if err != nil {
-				t.Fatalf("error is unexpected: %v", err)
-			}
-
-			createdSvc := obj.(*api.Service)
-			// allocated IP
-			for family, ip := range testCase.allocateIPsBeforeUpdate {
-				alloc := storage.alloc.serviceIPAllocatorsByFamily[family]
-				if err := alloc.Allocate(netutils.ParseIPSloppy(ip)); err != nil {
-					t.Fatalf("test is incorrect, unable to preallocate ip:%v", ip)
-				}
-			}
-			// run the modifier
-			testCase.updateFunc(createdSvc)
-
-			// run the update
-			updated, _, err := storage.Update(ctx,
-				createdSvc.Name,
-				rest.DefaultUpdatedObjectInfo(createdSvc),
-				rest.ValidateAllObjectFunc,
-				rest.ValidateAllObjectUpdateFunc,
-				false,
-				&metav1.UpdateOptions{})
-
-			if err != nil && !testCase.expectUpgradeError {
-				t.Fatalf("an error was not expected during upgrade %v", err)
-			}
-
-			if err == nil && testCase.expectUpgradeError {
-				t.Fatalf("error was expected during upgrade")
-			}
-
-			if err != nil {
-				return
-			}
-
-			updatedSvc := updated.(*api.Service)
-			isValidClusterIPFields(t, storage, updatedSvc, updatedSvc)
-
-			shouldUpgrade := len(createdSvc.Spec.IPFamilies) == 2 && *(createdSvc.Spec.IPFamilyPolicy) != api.IPFamilyPolicySingleStack && len(storage.alloc.serviceIPAllocatorsByFamily) == 2
-			if shouldUpgrade && len(updatedSvc.Spec.ClusterIPs) < 2 {
-				t.Fatalf("Service should have been upgraded %+v", createdSvc)
-			}
-
-			if !shouldUpgrade && len(updatedSvc.Spec.ClusterIPs) > 1 {
-				t.Fatalf("Service should not have been upgraded %+v", createdSvc)
-			}
-
-			// make sure that ips were allocated, correctly
-			for i, family := range updatedSvc.Spec.IPFamilies {
-				ip := updatedSvc.Spec.ClusterIPs[i]
-				allocator := storage.alloc.serviceIPAllocatorsByFamily[family]
-				if !ipIsAllocated(t, allocator, ip) {
-					t.Fatalf("expected ip:%v to be allocated by %v allocator. it was not", ip, family)
-				}
-			}
-		})
-	}
-}
-
-func TestServiceDowngrade(t *testing.T) {
-	requiredDualStack := api.IPFamilyPolicyRequireDualStack
-	singleStack := api.IPFamilyPolicySingleStack
-	ctx := genericapirequest.NewDefaultContext()
-	testCases := []struct {
-		name                     string
-		updateFunc               func(svc *api.Service)
-		enableDualStackAllocator bool
-		enableDualStackGate      bool
-		expectDowngradeError     bool
-		svc                      *api.Service
-	}{{
-		name:                     "normal, no downgrade needed. single stack => single stack",
-		enableDualStackAllocator: true,
-		enableDualStackGate:      true,
-		expectDowngradeError:     false,
-
-		updateFunc: func(s *api.Service) { s.Spec.Selector = map[string]string{"bar": "baz2"} },
-
-		svc: svctest.MakeService("foo", func(s *api.Service) {
-			s.Spec.IPFamilyPolicy = &requiredDualStack
-			s.Spec.IPFamilies = []api.IPFamily{api.IPv4Protocol}
-		}),
-	}, {
-		name:                     "normal, no downgrade needed. dual stack => dual stack",
-		enableDualStackAllocator: true,
-		enableDualStackGate:      true,
-		expectDowngradeError:     false,
-
-		updateFunc: func(s *api.Service) { s.Spec.Selector = map[string]string{"bar": "baz2"} },
-
-		svc: svctest.MakeService("foo", func(s *api.Service) {
-			s.Spec.IPFamilyPolicy = &requiredDualStack
-			s.Spec.IPFamilies = []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol}
-		}),
-	}, {
-		name:                     "normal, downgrade v4,v6 => v4",
-		enableDualStackAllocator: true,
-		enableDualStackGate:      true,
-		expectDowngradeError:     false,
-
-		updateFunc: func(s *api.Service) {
-			s.Spec.IPFamilyPolicy = &singleStack
-			s.Spec.ClusterIPs = s.Spec.ClusterIPs[0:1]
-			s.Spec.IPFamilies = s.Spec.IPFamilies[0:1]
-		},
-
-		svc: svctest.MakeService("foo", func(s *api.Service) {
-			s.Spec.IPFamilyPolicy = &requiredDualStack
-			s.Spec.IPFamilies = []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol}
-		}),
-	}, {
-		name:                     "normal, downgrade v6,v4 => v6",
-		enableDualStackAllocator: true,
-		enableDualStackGate:      true,
-		expectDowngradeError:     false,
-
-		updateFunc: func(s *api.Service) {
-			s.Spec.IPFamilyPolicy = &singleStack
-			s.Spec.ClusterIPs = s.Spec.ClusterIPs[0:1]
-			s.Spec.IPFamilies = s.Spec.IPFamilies[0:1]
-		},
-
-		svc: svctest.MakeService("foo", func(s *api.Service) {
-			s.Spec.IPFamilyPolicy = &requiredDualStack
-			s.Spec.IPFamilies = []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol}
-		}),
-	}}
-
-	for _, testCase := range testCases {
-		t.Run(testCase.name, func(t *testing.T) {
-			storage, server := NewTestREST(t, []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol})
-			defer server.Terminate(t)
-			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, testCase.enableDualStackGate)()
-
-			obj, err := storage.Create(ctx, testCase.svc, rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
-			if err != nil {
-				t.Fatalf("error is unexpected: %v", err)
-			}
-
-			createdSvc := obj.(*api.Service)
-			copySvc := createdSvc.DeepCopy()
-
-			// run the modifier
-			testCase.updateFunc(createdSvc)
-
-			// run the update
-			updated, _, err := storage.Update(ctx,
-				createdSvc.Name,
-				rest.DefaultUpdatedObjectInfo(createdSvc),
-				rest.ValidateAllObjectFunc,
-				rest.ValidateAllObjectUpdateFunc,
-				false,
-				&metav1.UpdateOptions{})
-
-			if err != nil && !testCase.expectDowngradeError {
-				t.Fatalf("an error was not expected during upgrade %v", err)
-			}
-
-			if err == nil && testCase.expectDowngradeError {
-				t.Fatalf("error was expected during upgrade")
-			}
-
-			if err != nil {
-				return
-			}
-
-			updatedSvc := updated.(*api.Service)
-			isValidClusterIPFields(t, storage, createdSvc, updatedSvc)
-
-			shouldDowngrade := len(copySvc.Spec.ClusterIPs) == 2 && *(createdSvc.Spec.IPFamilyPolicy) == api.IPFamilyPolicySingleStack
-
-			if shouldDowngrade && len(updatedSvc.Spec.ClusterIPs) > 1 {
-				t.Fatalf("Service should have been downgraded %+v", createdSvc)
-			}
-
-			if !shouldDowngrade && len(updatedSvc.Spec.ClusterIPs) < 2 {
-				t.Fatalf("Service should not have been downgraded %+v", createdSvc)
-			}
-
-			if shouldDowngrade {
-				releasedIP := copySvc.Spec.ClusterIPs[1]
-				releasedIPFamily := copySvc.Spec.IPFamilies[1]
-				allocator := storage.alloc.serviceIPAllocatorsByFamily[releasedIPFamily]
-
-				if ipIsAllocated(t, allocator, releasedIP) {
-					t.Fatalf("expected ip:%v to be released by %v allocator. it was not", releasedIP, releasedIPFamily)
-				}
-			}
-		})
-	}
-}
-
-// validates that the service created, updated by REST
-// has correct ClusterIPs related fields
-func isValidClusterIPFields(t *testing.T, storage *REST, pre *api.Service, post *api.Service) {
-	t.Helper()
-
-	// valid for gate off/on scenarios
-	// ClusterIP
-	if len(post.Spec.ClusterIP) == 0 {
-		t.Fatalf("service must have clusterIP : %+v", post)
-	}
-	// cluster IPs
-	if len(post.Spec.ClusterIPs) == 0 {
-		t.Fatalf("new service must have at least one IP: %+v", post)
-	}
-
-	if post.Spec.ClusterIP != post.Spec.ClusterIPs[0] {
-		t.Fatalf("clusterIP does not match ClusterIPs[0]: %+v", post)
-	}
-
-	// if feature gate is not enabled then we need to ignore need fields
-	if !utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) {
-		if post.Spec.IPFamilyPolicy != nil {
-			t.Fatalf("service must be set to  nil for IPFamilyPolicy: %+v", post)
-		}
-
-		if len(post.Spec.IPFamilies) != 0 {
-			t.Fatalf("service must be set to nil for IPFamilies: %+v", post)
-		}
-
-		return
-	}
-
-	// for gate on scenarios
-	// prefer dual stack field
-	if post.Spec.IPFamilyPolicy == nil {
-		t.Fatalf("service must not have nil for IPFamilyPolicy: %+v", post)
-	}
-
-	if pre.Spec.IPFamilyPolicy != nil && *(pre.Spec.IPFamilyPolicy) != *(post.Spec.IPFamilyPolicy) {
-		t.Fatalf("new service must not change PreferDualStack if it was set by user pre: %v post: %v", *(pre.Spec.IPFamilyPolicy), *(post.Spec.IPFamilyPolicy))
-	}
-
-	if pre.Spec.IPFamilyPolicy == nil && *(post.Spec.IPFamilyPolicy) != api.IPFamilyPolicySingleStack {
-		t.Fatalf("new services with prefer dual stack nil must be set to false (prefer dual stack) %+v", post)
-	}
-
-	// external name or headless services offer no more ClusterIPs field validation
-	if post.Spec.ClusterIPs[0] == api.ClusterIPNone {
-		return
-	}
-
-	// len of ClusteIPs can not be more than Families
-	// and for providedIPs it needs to match
-
-	// if families are provided then it shouldn't be changed
-	// this applies on first entry on
-	if len(pre.Spec.IPFamilies) > 0 {
-		if len(post.Spec.IPFamilies) == 0 {
-			t.Fatalf("allocator shouldn't remove ipfamilies[0] pre:%+v, post:%+v", pre.Spec.IPFamilies, post.Spec.IPFamilies)
-		}
-
-		if pre.Spec.IPFamilies[0] != post.Spec.IPFamilies[0] {
-			t.Fatalf("allocator shouldn't change post.Spec.IPFamilies[0] pre:%+v post:%+v", pre.Spec.IPFamilies, post.Spec.IPFamilies)
-		}
-	}
-	// if two families are assigned, then they must be dual stack
-	if len(post.Spec.IPFamilies) == 2 {
-		if post.Spec.IPFamilies[0] == post.Spec.IPFamilies[1] {
-			t.Fatalf("allocator assigned two of the same family %+v", post)
-		}
-	}
-	// ips must match families
-	for i, ip := range post.Spec.ClusterIPs {
-		isIPv6 := netutils.IsIPv6String(ip)
-		if isIPv6 && post.Spec.IPFamilies[i] != api.IPv6Protocol {
-			t.Fatalf("ips does not match assigned families %+v %+v", post.Spec.ClusterIPs, post.Spec.IPFamilies)
-		}
-	}
-}

pkg/registry/core/service/strategy.go

@@ -124,7 +124,6 @@ func (strategy svcStrategy) PrepareForUpdate(ctx context.Context, obj, old runti
 	NormalizeClusterIPs(oldService, newService)
 	dropServiceDisabledFields(newService, oldService)
 	dropTypeDependentFields(newService, oldService)
-	trimFieldsForDualStackDowngrade(newService, oldService)
 }
 
 // Validate validates a new service.
@@ -314,7 +313,7 @@ func PatchAllocatedValues(newSvc, oldSvc *api.Service) {
 		if newSvc.Spec.ClusterIP == "" {
 			newSvc.Spec.ClusterIP = oldSvc.Spec.ClusterIP
 		}
-		if len(newSvc.Spec.ClusterIPs) == 0 {
+		if len(newSvc.Spec.ClusterIPs) == 0 && len(oldSvc.Spec.ClusterIPs) > 0 {
 			newSvc.Spec.ClusterIPs = oldSvc.Spec.ClusterIPs
 		}
 	}
@@ -620,33 +619,3 @@ func needsExternalTrafficPolicy(svc *api.Service) bool {
 func sameExternalTrafficPolicy(oldSvc, newSvc *api.Service) bool {
 	return oldSvc.Spec.ExternalTrafficPolicy == newSvc.Spec.ExternalTrafficPolicy
 }
-
-// this func allows user to downgrade a service by just changing
-// IPFamilyPolicy to SingleStack
-func trimFieldsForDualStackDowngrade(newService, oldService *api.Service) {
-	if !utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) {
-		return
-	}
-
-	// not an update
-	if oldService == nil {
-		return
-	}
-
-	oldIsDualStack := oldService.Spec.IPFamilyPolicy != nil &&
-		(*oldService.Spec.IPFamilyPolicy == api.IPFamilyPolicyRequireDualStack ||
-			*oldService.Spec.IPFamilyPolicy == api.IPFamilyPolicyPreferDualStack)
-
-	newIsNotDualStack := newService.Spec.IPFamilyPolicy != nil && *newService.Spec.IPFamilyPolicy == api.IPFamilyPolicySingleStack
-
-	// if user want to downgrade then we auto remove secondary ip and family
-	if oldIsDualStack && newIsNotDualStack {
-		if len(newService.Spec.ClusterIPs) > 1 {
-			newService.Spec.ClusterIPs = newService.Spec.ClusterIPs[0:1]
-		}
-
-		if len(newService.Spec.IPFamilies) > 1 {
-			newService.Spec.IPFamilies = newService.Spec.IPFamilies[0:1]
-		}
-	}
-}

pkg/registry/core/service/strategy_test.go

@@ -930,123 +930,15 @@ func TestDropTypeDependentFields(t *testing.T) {
 	}
 }
 
-func TestTrimFieldsForDualStackDowngrade(t *testing.T) {
-	singleStack := api.IPFamilyPolicySingleStack
-	preferDualStack := api.IPFamilyPolicyPreferDualStack
-	requireDualStack := api.IPFamilyPolicyRequireDualStack
-	testCases := []struct {
-		name          string
-		oldPolicy     *api.IPFamilyPolicyType
-		oldClusterIPs []string
-		oldFamilies   []api.IPFamily
-
-		newPolicy          *api.IPFamilyPolicyType
-		expectedClusterIPs []string
-		expectedIPFamilies []api.IPFamily
-	}{
-
-		{
-			name:               "no change single to single",
-			oldPolicy:          &singleStack,
-			oldClusterIPs:      []string{"10.10.10.10"},
-			oldFamilies:        []api.IPFamily{api.IPv4Protocol},
-			newPolicy:          &singleStack,
-			expectedClusterIPs: []string{"10.10.10.10"},
-			expectedIPFamilies: []api.IPFamily{api.IPv4Protocol},
-		},
-
-		{
-			name:               "dualstack to dualstack (preferred)",
-			oldPolicy:          &preferDualStack,
-			oldClusterIPs:      []string{"10.10.10.10", "2000::1"},
-			oldFamilies:        []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
-			newPolicy:          &preferDualStack,
-			expectedClusterIPs: []string{"10.10.10.10", "2000::1"},
-			expectedIPFamilies: []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
-		},
-
-		{
-			name:               "dualstack to dualstack (required)",
-			oldPolicy:          &requireDualStack,
-			oldClusterIPs:      []string{"10.10.10.10", "2000::1"},
-			oldFamilies:        []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
-			newPolicy:          &preferDualStack,
-			expectedClusterIPs: []string{"10.10.10.10", "2000::1"},
-			expectedIPFamilies: []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
-		},
-
-		{
-			name:               "dualstack (preferred) to single",
-			oldPolicy:          &preferDualStack,
-			oldClusterIPs:      []string{"10.10.10.10", "2000::1"},
-			oldFamilies:        []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
-			newPolicy:          &singleStack,
-			expectedClusterIPs: []string{"10.10.10.10"},
-			expectedIPFamilies: []api.IPFamily{api.IPv4Protocol},
-		},
-
-		{
-			name:               "dualstack (require) to single",
-			oldPolicy:          &requireDualStack,
-			oldClusterIPs:      []string{"2000::1", "10.10.10.10"},
-			oldFamilies:        []api.IPFamily{api.IPv6Protocol, api.IPv4Protocol},
-			newPolicy:          &singleStack,
-			expectedClusterIPs: []string{"2000::1"},
-			expectedIPFamilies: []api.IPFamily{api.IPv6Protocol},
-		},
-	}
-	// only when gate is on
-	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, true)()
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			oldService := &api.Service{
-				Spec: api.ServiceSpec{
-					IPFamilyPolicy: tc.oldPolicy,
-					ClusterIPs:     tc.oldClusterIPs,
-					IPFamilies:     tc.oldFamilies,
-				},
-			}
-
-			newService := oldService.DeepCopy()
-			newService.Spec.IPFamilyPolicy = tc.newPolicy
-
-			trimFieldsForDualStackDowngrade(newService, oldService)
-
-			if len(newService.Spec.ClusterIPs) != len(tc.expectedClusterIPs) {
-				t.Fatalf("unexpected clusterIPs. expected %v and got %v", tc.expectedClusterIPs, newService.Spec.ClusterIPs)
-			}
-
-			// compare clusterIPS
-			for i, expectedIP := range tc.expectedClusterIPs {
-				if expectedIP != newService.Spec.ClusterIPs[i] {
-					t.Fatalf("unexpected clusterIPs. expected %v and got %v", tc.expectedClusterIPs, newService.Spec.ClusterIPs)
-				}
-			}
-
-			// families
-			if len(newService.Spec.IPFamilies) != len(tc.expectedIPFamilies) {
-				t.Fatalf("unexpected ipfamilies. expected %v and got %v", tc.expectedIPFamilies, newService.Spec.IPFamilies)
-			}
-
-			// compare clusterIPS
-			for i, expectedIPFamily := range tc.expectedIPFamilies {
-				if expectedIPFamily != newService.Spec.IPFamilies[i] {
-					t.Fatalf("unexpected ipfamilies. expected %v and got %v", tc.expectedIPFamilies, newService.Spec.IPFamilies)
-				}
-			}
-
-		})
-	}
-}
-
 func TestPatchAllocatedValues(t *testing.T) {
 	testCases := []struct {
-		name                 string
+		name                    string
-		before               *api.Service
+		before                  *api.Service
-		update               *api.Service
+		update                  *api.Service
-		expectSameClusterIPs bool
+		expectSameClusterIPs    bool
-		expectSameNodePort   bool
+		expectReducedClusterIPs bool
-		expectSameHCNP       bool
+		expectSameNodePort      bool
+		expectSameHCNP          bool
 	}{{
 		name: "all_patched",
 		before: svctest.MakeService("foo",
@@ -1120,16 +1012,28 @@ func TestPatchAllocatedValues(t *testing.T) {
 			update := tc.update.DeepCopy()
 			PatchAllocatedValues(update, tc.before)
 
-			if b, u := tc.before.Spec.ClusterIP, update.Spec.ClusterIP; tc.expectSameClusterIPs && b != u {
+			beforeIP := tc.before.Spec.ClusterIP
-				t.Errorf("expected clusterIP to be patched: %q != %q", b, u)
+			updateIP := update.Spec.ClusterIP
-			} else if !tc.expectSameClusterIPs && b == u {
+			if tc.expectSameClusterIPs || tc.expectReducedClusterIPs {
-				t.Errorf("expected clusterIP to not be patched: %q == %q", b, u)
+				if beforeIP != updateIP {
+					t.Errorf("expected clusterIP to be patched: %q != %q", beforeIP, updateIP)
+				}
+			} else if beforeIP == updateIP {
+				t.Errorf("expected clusterIP to not be patched: %q == %q", beforeIP, updateIP)
 			}
 
-			if b, u := tc.before.Spec.ClusterIPs, update.Spec.ClusterIPs; tc.expectSameClusterIPs && !cmp.Equal(b, u) {
+			beforeIPs := tc.before.Spec.ClusterIPs
-				t.Errorf("expected clusterIPs to be patched: %q != %q", b, u)
+			updateIPs := update.Spec.ClusterIPs
-			} else if !tc.expectSameClusterIPs && cmp.Equal(b, u) {
+			if tc.expectSameClusterIPs {
-				t.Errorf("expected clusterIPs to not be patched: %q == %q", b, u)
+				if !cmp.Equal(beforeIPs, updateIPs) {
+					t.Errorf("expected clusterIPs to be patched: %q != %q", beforeIPs, updateIPs)
+				}
+			} else if tc.expectReducedClusterIPs {
+				if len(updateIPs) != 1 || beforeIPs[0] != updateIPs[0] {
+					t.Errorf("expected clusterIPs to be trim-patched: %q -> %q", beforeIPs, updateIPs)
+				}
+			} else if cmp.Equal(beforeIPs, updateIPs) {
+				t.Errorf("expected clusterIPs to not be patched: %q == %q", beforeIPs, updateIPs)
 			}
 
 			if b, u := tc.before.Spec.Ports[0].NodePort, update.Spec.Ports[0].NodePort; tc.expectSameNodePort && b != u {