Svc REST: Set Cluster IPs during dry-run Update()

Dry-run should return valid results.

Also add a test.

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

@@ -287,17 +287,14 @@ func (al *RESTAllocStuff) allocateUpdate(service, oldService *api.Service, dryRu
 	}
 
 	// Allocate ClusterIPs
-	//FIXME: we need to put values in, even if dry run - else validation should
+	//TODO(thockin): validation should not pass with empty clusterIP, but it
-	//not pass.  It does but that should be fixed.  Plumb dryRun thru update
+	//does (and is tested!).  Fixing that all is a big PR and will have to
-	//logic.
+	//happen later.
-	// xref: https://groups.google.com/g/kubernetes-sig-api-machinery/c/_-5TKHXHcXE/m/RfKj7CtzAQAJ
+	if txn, err := al.allocUpdateServiceClusterIPsNew(service, oldService, dryRun); err != nil {
-	if !dryRun {
+		result.Revert()
-		if txn, err := al.allocUpdateServiceClusterIPsNew(service, oldService); err != nil {
+		return nil, err
-			result.Revert()
+	} else {
-			return nil, err
+		result = append(result, txn)
-		} else {
-			result = append(result, txn)
-		}
 	}
 
 	// Allocate ports
@@ -592,8 +589,8 @@ func (al *RESTAllocStuff) allocServiceClusterIPs(service *api.Service, dryRun bo
 }
 
 //FIXME: rename and merge with handleClusterIPsForUpdatedService
-func (al *RESTAllocStuff) allocUpdateServiceClusterIPsNew(service *api.Service, oldService *api.Service) (transaction, error) {
+func (al *RESTAllocStuff) allocUpdateServiceClusterIPsNew(service *api.Service, oldService *api.Service, dryRun bool) (transaction, error) {
-	allocated, released, err := al.handleClusterIPsForUpdatedService(oldService, service)
+	allocated, released, err := al.handleClusterIPsForUpdatedService(oldService, service, dryRun)
 	if err != nil {
 		return nil, err
 	}
@@ -605,12 +602,18 @@ func (al *RESTAllocStuff) allocUpdateServiceClusterIPsNew(service *api.Service,
 	//             released
 	txn := callbackTransaction{
 		commit: func() {
+			if dryRun {
+				return
+			}
 			if actuallyReleased, err := al.releaseClusterIPs(released); err != nil {
 				klog.V(4).Infof("service %v/%v failed to clean up after failed service update error:%v. ShouldRelease/Released:%v/%v",
 					service.Namespace, service.Name, err, released, actuallyReleased)
 			}
 		},
 		revert: func() {
+			if dryRun {
+				return
+			}
 			if actuallyReleased, err := al.releaseClusterIPs(allocated); err != nil {
 				klog.V(4).Infof("service %v/%v failed to clean up after failed service update error:%v. Allocated/Released:%v/%v",
 					service.Namespace, service.Name, err, allocated, actuallyReleased)
@@ -624,7 +627,7 @@ func (al *RESTAllocStuff) allocUpdateServiceClusterIPsNew(service *api.Service,
 // this func does not perform actual release of clusterIPs. it returns
 // a map[family]ip for the caller to release when everything else has
 // executed successfully
-func (al *RESTAllocStuff) handleClusterIPsForUpdatedService(oldService *api.Service, service *api.Service) (allocated map[api.IPFamily]string, toRelease map[api.IPFamily]string, err error) {
+func (al *RESTAllocStuff) handleClusterIPsForUpdatedService(oldService *api.Service, service *api.Service, dryRun bool) (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
@@ -647,8 +650,7 @@ func (al *RESTAllocStuff) handleClusterIPsForUpdatedService(oldService *api.Serv
 	// CASE A:
 	// Update service from ExternalName to non-ExternalName, should initialize ClusterIP.
 	if oldService.Spec.Type == api.ServiceTypeExternalName && service.Spec.Type != api.ServiceTypeExternalName {
-		//FIXME: plumb dryRun down here
+		allocated, err := al.allocServiceClusterIPs(service, dryRun)
-		allocated, err := al.allocServiceClusterIPs(service, false)
 		return allocated, nil, err
 	}
 
@@ -694,7 +696,7 @@ func (al *RESTAllocStuff) handleClusterIPsForUpdatedService(oldService *api.Serv
 		toAllocate[service.Spec.IPFamilies[1]] = service.Spec.ClusterIPs[1]
 
 		// allocate
-		allocated, err := al.allocClusterIPs(service, toAllocate, false) //FIXME: plumb dry-run down here
+		allocated, err := al.allocClusterIPs(service, toAllocate, dryRun)
 		// set if successful
 		if err == nil {
 			service.Spec.ClusterIPs[1] = allocated[service.Spec.IPFamilies[1]]

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

@@ -592,92 +592,6 @@ func TestServiceRegistryUpdateUnspecifiedAllocations(t *testing.T) {
 	}
 }
 
-func TestServiceRegistryUpdateDryRun(t *testing.T) {
-	ctx := genericapirequest.NewDefaultContext()
-	storage, server := NewTestREST(t, []api.IPFamily{api.IPv4Protocol})
-	defer server.Terminate(t)
-
-	obj, err := storage.Create(ctx, svctest.MakeService("foo", svctest.SetTypeExternalName), rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
-	if err != nil {
-		t.Fatalf("Expected no error: %v", err)
-	}
-	svc := obj.(*api.Service)
-
-	// Test dry run update request external name to node port
-	new1 := svc.DeepCopy()
-	svctest.SetTypeNodePort(new1)
-	svctest.SetNodePorts(30001)(new1) // DryRun does not set port values yet
-	obj, created, err := storage.Update(ctx, new1.Name, rest.DefaultUpdatedObjectInfo(new1),
-		rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}})
-	if err != nil {
-		t.Fatalf("Expected no error: %v", err)
-	}
-	if obj == nil {
-		t.Errorf("Expected non-nil object")
-	}
-	if created {
-		t.Errorf("expected not created")
-	}
-	if portIsAllocated(t, storage.alloc.serviceNodePorts, new1.Spec.Ports[0].NodePort) {
-		t.Errorf("unexpected side effect: NodePort allocated")
-	}
-
-	// Test dry run update request external name to cluster ip
-	new2 := svc.DeepCopy()
-	svctest.SetTypeClusterIP(new2)
-	svctest.SetClusterIPs("1.2.3.4")(new2) // DryRun does not set IP values yet
-	_, _, err = storage.Update(ctx, svc.Name, rest.DefaultUpdatedObjectInfo(new2),
-		rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}})
-	if err != nil {
-		t.Fatalf("Expected no error: %v", err)
-	}
-	if ipIsAllocated(t, storage.alloc.serviceIPAllocatorsByFamily[storage.alloc.defaultServiceIPFamily], new2.Spec.ClusterIP) {
-		t.Errorf("unexpected side effect: ip allocated")
-	}
-
-	// Test dry run update request remove node port
-	obj, err = storage.Create(ctx, svctest.MakeService("foo2", svctest.SetTypeNodePort, svctest.SetNodePorts(30001)), rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
-	if err != nil {
-		t.Fatalf("Expected no error: %v", err)
-	}
-	svc = obj.(*api.Service)
-	if !ipIsAllocated(t, storage.alloc.serviceIPAllocatorsByFamily[storage.alloc.defaultServiceIPFamily], svc.Spec.ClusterIP) {
-		t.Errorf("expected IP to be allocated")
-	}
-	if !portIsAllocated(t, storage.alloc.serviceNodePorts, svc.Spec.Ports[0].NodePort) {
-		t.Errorf("expected NodePort to be allocated")
-	}
-
-	new3 := svc.DeepCopy()
-	svctest.SetTypeExternalName(new3)
-	_, _, err = storage.Update(ctx, svc.Name, rest.DefaultUpdatedObjectInfo(new3),
-		rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}})
-	if err != nil {
-		t.Fatalf("Expected no error: %v", err)
-	}
-	if !portIsAllocated(t, storage.alloc.serviceNodePorts, svc.Spec.Ports[0].NodePort) {
-		t.Errorf("unexpected side effect: NodePort unallocated")
-	}
-
-	// Test dry run update request remove cluster ip
-	obj, err = storage.Create(ctx, svctest.MakeService("foo3", svctest.SetClusterIPs("1.2.3.4")), rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
-	if err != nil {
-		t.Fatalf("expected no error: %v", err)
-	}
-	svc = obj.(*api.Service)
-
-	new4 := svc.DeepCopy()
-	svctest.SetTypeExternalName(new4)
-	_, _, err = storage.Update(ctx, svc.Name, rest.DefaultUpdatedObjectInfo(new4),
-		rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}})
-	if err != nil {
-		t.Fatalf("expected no error: %v", err)
-	}
-	if !ipIsAllocated(t, storage.alloc.serviceIPAllocatorsByFamily[storage.alloc.defaultServiceIPFamily], svc.Spec.ClusterIP) {
-		t.Errorf("unexpected side effect: ip unallocated")
-	}
-}
-
 func TestServiceStorageValidatesUpdate(t *testing.T) {
 	ctx := genericapirequest.NewDefaultContext()
 	storage, server := NewTestREST(t, []api.IPFamily{api.IPv4Protocol})

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

@@ -5838,4 +5838,199 @@ func TestDeleteDryRun(t *testing.T) {
 	}
 }
 
+// Prove that a dry-run update doesn't actually allocate or deallocate IPs or ports.
+func TestUpdateDryRun(t *testing.T) {
+	testCases := []struct {
+		name            string
+		clusterFamilies []api.IPFamily
+		svc             *api.Service
+		update          *api.Service
+		verifyDryAllocs bool
+	}{{
+		name:            "singlestack:v4_NoAllocs-Allocs",
+		clusterFamilies: []api.IPFamily{api.IPv4Protocol},
+		svc:             svctest.MakeService("foo", svctest.SetTypeExternalName),
+		update: svctest.MakeService("foo", svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal)),
+		verifyDryAllocs: true, // make sure values were not allocated.
+	}, {
+		name:            "singlestack:v4_Allocs-NoAllocs",
+		clusterFamilies: []api.IPFamily{api.IPv4Protocol},
+		svc: svctest.MakeService("foo", svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal)),
+		update:          svctest.MakeService("foo", svctest.SetTypeExternalName),
+		verifyDryAllocs: false, // make sure values were not released.
+	}, {
+		name:            "singlestack:v6_NoAllocs-Allocs",
+		clusterFamilies: []api.IPFamily{api.IPv6Protocol},
+		svc:             svctest.MakeService("foo", svctest.SetTypeExternalName),
+		update: svctest.MakeService("foo", svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal)),
+		verifyDryAllocs: true, // make sure values were not allocated.
+	}, {
+		name:            "singlestack:v6_Allocs-NoAllocs",
+		clusterFamilies: []api.IPFamily{api.IPv6Protocol},
+		svc: svctest.MakeService("foo", svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal)),
+		update:          svctest.MakeService("foo", svctest.SetTypeExternalName),
+		verifyDryAllocs: false, // make sure values were not released.
+	}, {
+		name:            "dualstack:v4v6_NoAllocs-Allocs",
+		clusterFamilies: []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
+		svc:             svctest.MakeService("foo", svctest.SetTypeExternalName),
+		update: svctest.MakeService("foo", svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal)),
+		verifyDryAllocs: true, // make sure values were not allocated.
+	}, {
+		name:            "dualstack:v4v6_Allocs-NoAllocs",
+		clusterFamilies: []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
+		svc: svctest.MakeService("foo", svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal)),
+		update:          svctest.MakeService("foo", svctest.SetTypeExternalName),
+		verifyDryAllocs: false, // make sure values were not released.
+	}, {
+		name:            "dualstack:v6v4_NoAllocs-Allocs",
+		clusterFamilies: []api.IPFamily{api.IPv6Protocol, api.IPv4Protocol},
+		svc:             svctest.MakeService("foo", svctest.SetTypeExternalName),
+		update: svctest.MakeService("foo", svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal)),
+		verifyDryAllocs: true, // make sure values were not allocated.
+	}, {
+		name:            "dualstack:v6v4_Allocs-NoAllocs",
+		clusterFamilies: []api.IPFamily{api.IPv6Protocol, api.IPv4Protocol},
+		svc: svctest.MakeService("foo", svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal)),
+		update:          svctest.MakeService("foo", svctest.SetTypeExternalName),
+		verifyDryAllocs: false, // make sure values were not released.
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			storage, _, server := newStorage(t, tc.clusterFamilies)
+			defer server.Terminate(t)
+			defer storage.Store.DestroyFunc()
+
+			ctx := genericapirequest.NewDefaultContext()
+			obj, err := storage.Create(ctx, tc.svc, rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
+			if err != nil {
+				t.Fatalf("unexpected error creating service: %v", err)
+			}
+			createdSvc := obj.(*api.Service)
+
+			if tc.verifyDryAllocs {
+				// Dry allocs means no allocs on create.  Ensure values were
+				// NOT allocated.
+				for i, fam := range createdSvc.Spec.IPFamilies {
+					if ipIsAllocated(t, storage.alloc.serviceIPAllocatorsByFamily[fam], createdSvc.Spec.ClusterIPs[i]) {
+						t.Errorf("expected IP to not be allocated: %q", createdSvc.Spec.ClusterIPs[i])
+					}
+				}
+				for _, p := range createdSvc.Spec.Ports {
+					if p.NodePort != 0 {
+						t.Errorf("expected port to not be assigned: %d", p.NodePort)
+						if portIsAllocated(t, storage.alloc.serviceNodePorts, p.NodePort) {
+							t.Errorf("expected port to not be allocated: %d", p.NodePort)
+						}
+					}
+				}
+				if createdSvc.Spec.HealthCheckNodePort != 0 {
+					t.Errorf("expected HCNP to not be assigned: %d", createdSvc.Spec.HealthCheckNodePort)
+					if portIsAllocated(t, storage.alloc.serviceNodePorts, createdSvc.Spec.HealthCheckNodePort) {
+						t.Errorf("expected HCNP to not be allocated: %d", createdSvc.Spec.HealthCheckNodePort)
+					}
+				}
+			} else {
+				// Ensure IPs were allocated
+				for i, fam := range createdSvc.Spec.IPFamilies {
+					if !ipIsAllocated(t, storage.alloc.serviceIPAllocatorsByFamily[fam], createdSvc.Spec.ClusterIPs[i]) {
+						t.Errorf("expected IP to be allocated: %q", createdSvc.Spec.ClusterIPs[i])
+					}
+				}
+				for _, p := range createdSvc.Spec.Ports {
+					if !portIsAllocated(t, storage.alloc.serviceNodePorts, p.NodePort) {
+						t.Errorf("expected port to be allocated: %d", p.NodePort)
+					}
+				}
+				if !portIsAllocated(t, storage.alloc.serviceNodePorts, createdSvc.Spec.HealthCheckNodePort) {
+					t.Errorf("expected port to be allocated: %d", createdSvc.Spec.HealthCheckNodePort)
+				}
+			}
+
+			// Update the object to the new state and check the results.
+			obj, _, err = storage.Update(ctx, tc.update.Name,
+				rest.DefaultUpdatedObjectInfo(tc.update), rest.ValidateAllObjectFunc,
+				rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}})
+			if err != nil {
+				t.Fatalf("unexpected error updating service: %v", err)
+			}
+			updatedSvc := obj.(*api.Service)
+
+			if tc.verifyDryAllocs {
+				// Dry allocs means the values are assigned but not
+				// allocated.
+				if netutils.ParseIPSloppy(updatedSvc.Spec.ClusterIP) == nil {
+					t.Errorf("expected valid clusterIP: %q", updatedSvc.Spec.ClusterIP)
+				}
+				for _, ip := range updatedSvc.Spec.ClusterIPs {
+					if netutils.ParseIPSloppy(ip) == nil {
+						t.Errorf("expected valid clusterIP: %q", updatedSvc.Spec.ClusterIP)
+					}
+				}
+				for i, fam := range updatedSvc.Spec.IPFamilies {
+					if ipIsAllocated(t, storage.alloc.serviceIPAllocatorsByFamily[fam], updatedSvc.Spec.ClusterIPs[i]) {
+						t.Errorf("expected IP to not be allocated: %q", updatedSvc.Spec.ClusterIPs[i])
+					}
+				}
+
+				for _, p := range updatedSvc.Spec.Ports {
+					if p.NodePort == 0 {
+						t.Errorf("expected nodePort to be assigned: %d", p.NodePort)
+					}
+					if portIsAllocated(t, storage.alloc.serviceNodePorts, p.NodePort) {
+						t.Errorf("expected nodePort to not be allocated: %d", p.NodePort)
+					}
+				}
+
+				if updatedSvc.Spec.HealthCheckNodePort == 0 {
+					t.Errorf("expected HCNP to be assigned: %d", updatedSvc.Spec.HealthCheckNodePort)
+				}
+				if portIsAllocated(t, storage.alloc.serviceNodePorts, updatedSvc.Spec.HealthCheckNodePort) {
+					t.Errorf("expected HCNP to not be allocated: %d", updatedSvc.Spec.HealthCheckNodePort)
+				}
+			} else {
+				// Ensure IPs were unassigned but not deallocated.
+				if updatedSvc.Spec.ClusterIP != "" {
+					t.Errorf("expected clusterIP to be unset: %q", updatedSvc.Spec.ClusterIP)
+				}
+				if len(updatedSvc.Spec.ClusterIPs) != 0 {
+					t.Errorf("expected clusterIPs to be unset: %q", updatedSvc.Spec.ClusterIPs)
+				}
+				for i, fam := range createdSvc.Spec.IPFamilies {
+					if !ipIsAllocated(t, storage.alloc.serviceIPAllocatorsByFamily[fam], createdSvc.Spec.ClusterIPs[i]) {
+						t.Errorf("expected IP to still be allocated: %q", createdSvc.Spec.ClusterIPs[i])
+					}
+				}
+
+				for _, p := range updatedSvc.Spec.Ports {
+					if p.NodePort != 0 {
+						t.Errorf("expected nodePort to be unset: %d", p.NodePort)
+					}
+				}
+				for _, p := range createdSvc.Spec.Ports {
+					if !portIsAllocated(t, storage.alloc.serviceNodePorts, p.NodePort) {
+						t.Errorf("expected nodePort to still be allocated: %d", p.NodePort)
+					}
+				}
+
+				if updatedSvc.Spec.HealthCheckNodePort != 0 {
+					t.Errorf("expected HCNP to be unset: %d", updatedSvc.Spec.HealthCheckNodePort)
+				}
+				if !portIsAllocated(t, storage.alloc.serviceNodePorts, createdSvc.Spec.HealthCheckNodePort) {
+					t.Errorf("expected HCNP to still be allocated: %d", createdSvc.Spec.HealthCheckNodePort)
+				}
+			}
+		})
+	}
+}
+
 //FIXME: Tests for update()