Svc REST: IP and port reallocation

Make sure the logic that was covered in rest_test is covered in
storage_test.

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

@@ -1043,97 +1043,6 @@ func TestServiceRegistryList(t *testing.T) {
 	}
 }
 
-func TestServiceRegistryCreateIPAllocation(t *testing.T) {
-	storage, server := NewTestREST(t, []api.IPFamily{api.IPv4Protocol})
-	defer server.Terminate(t)
-
-	svc1 := svctest.MakeService("foo")
-	ctx := genericapirequest.NewDefaultContext()
-	obj, err := storage.Create(ctx, svc1, rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
-	if err != nil {
-		t.Fatalf("error creating service: %v", err)
-	}
-	createdSvc1 := obj.(*api.Service)
-	if createdSvc1.Name != "foo" {
-		t.Errorf("Expected foo, but got %v", createdSvc1.Name)
-	}
-	if !makeIPNet(t).Contains(netutils.ParseIPSloppy(createdSvc1.Spec.ClusterIPs[0])) {
-		t.Errorf("Unexpected ClusterIP: %s", createdSvc1.Spec.ClusterIPs[0])
-	}
-
-	svc2 := svctest.MakeService("bar")
-	ctx = genericapirequest.NewDefaultContext()
-	obj, err = storage.Create(ctx, svc2, rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
-	if err != nil {
-		t.Fatalf("error creating service: %v", err)
-	}
-	createdSvc2 := obj.(*api.Service)
-	if createdSvc2.Name != "bar" {
-		t.Errorf("Expected bar, but got %v", createdSvc2.Name)
-	}
-	if !makeIPNet(t).Contains(netutils.ParseIPSloppy(createdSvc2.Spec.ClusterIPs[0])) {
-		t.Errorf("Unexpected ClusterIP: %s", createdSvc2.Spec.ClusterIPs[0])
-	}
-
-	testIPs := []string{"1.2.3.93", "1.2.3.94", "1.2.3.95", "1.2.3.96"}
-	testIP := "not-an-ip"
-	for _, ip := range testIPs {
-		if !ipIsAllocated(t, storage.alloc.serviceIPAllocatorsByFamily[storage.alloc.defaultServiceIPFamily].(*ipallocator.Range), ip) {
-			testIP = ip
-			break
-		}
-	}
-
-	svc3 := svctest.MakeService("qux", svctest.SetClusterIPs(testIP))
-	ctx = genericapirequest.NewDefaultContext()
-	obj, err = storage.Create(ctx, svc3, rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
-	if err != nil {
-		t.Fatal(err)
-	}
-	createdSvc3 := obj.(*api.Service)
-	if createdSvc3.Spec.ClusterIPs[0] != testIP { // specific IP
-		t.Errorf("Unexpected ClusterIP: %s", createdSvc3.Spec.ClusterIPs[0])
-	}
-}
-
-func TestServiceRegistryCreateIPReallocation(t *testing.T) {
-	storage, server := NewTestREST(t, []api.IPFamily{api.IPv4Protocol})
-	defer server.Terminate(t)
-
-	svc1 := svctest.MakeService("foo")
-	ctx := genericapirequest.NewDefaultContext()
-	obj, err := storage.Create(ctx, svc1, rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
-	if err != nil {
-		t.Fatalf("error creating service: %v", err)
-	}
-	createdSvc1 := obj.(*api.Service)
-	if createdSvc1.Name != "foo" {
-		t.Errorf("Expected foo, but got %v", createdSvc1.Name)
-	}
-	if !makeIPNet(t).Contains(netutils.ParseIPSloppy(createdSvc1.Spec.ClusterIPs[0])) {
-		t.Errorf("Unexpected ClusterIP: %s", createdSvc1.Spec.ClusterIPs[0])
-	}
-
-	_, _, err = storage.Delete(ctx, createdSvc1.Name, rest.ValidateAllObjectFunc, &metav1.DeleteOptions{})
-	if err != nil {
-		t.Errorf("Unexpected error deleting service: %v", err)
-	}
-
-	svc2 := svctest.MakeService("bar", svctest.SetClusterIPs(createdSvc1.Spec.ClusterIP))
-	ctx = genericapirequest.NewDefaultContext()
-	obj, err = storage.Create(ctx, svc2, rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
-	if err != nil {
-		t.Fatalf("error creating service: %v", err)
-	}
-	createdSvc2 := obj.(*api.Service)
-	if createdSvc2.Name != "bar" {
-		t.Errorf("Expected bar, but got %v", createdSvc2.Name)
-	}
-	if !makeIPNet(t).Contains(netutils.ParseIPSloppy(createdSvc2.Spec.ClusterIPs[0])) {
-		t.Errorf("Unexpected ClusterIP: %s", createdSvc2.Spec.ClusterIPs[0])
-	}
-}
-
 func TestServiceRegistryIPUpdate(t *testing.T) {
 	storage, server := NewTestREST(t, []api.IPFamily{api.IPv4Protocol})
 	defer server.Terminate(t)

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

@@ -4715,16 +4715,19 @@ func TestCreateInitIPFields(t *testing.T) {
 					}
 					if itc.expectHeadless {
 						if !reflect.DeepEqual(createdSvc.Spec.ClusterIPs, []string{"None"}) {
-							t.Errorf("wrong clusterIPs: want [\"None\"], got %v", createdSvc.Spec.ClusterIPs)
+							t.Fatalf("wrong clusterIPs: want [\"None\"], got %v", createdSvc.Spec.ClusterIPs)
 						}
-					} else {
+						return
-						if c, f := len(createdSvc.Spec.ClusterIPs), len(createdSvc.Spec.IPFamilies); c != f {
+					}
-							t.Errorf("clusterIPs and ipFamilies are not the same length: %d vs %d", c, f)
+					if c, f := len(createdSvc.Spec.ClusterIPs), len(createdSvc.Spec.IPFamilies); c != f {
+						t.Errorf("clusterIPs and ipFamilies are not the same length: %d vs %d", c, f)
+					}
+					for i, clip := range createdSvc.Spec.ClusterIPs {
+						if cf, ef := familyOf(clip), createdSvc.Spec.IPFamilies[i]; cf != ef {
+							t.Errorf("clusterIP is the wrong IP family: want %s, got %s", ef, cf)
 						}
-						for i, clip := range createdSvc.Spec.ClusterIPs {
+						if !ipIsAllocated(t, storage.alloc.serviceIPAllocatorsByFamily[familyOf(clip)], clip) {
-							if cf, ef := familyOf(clip), createdSvc.Spec.IPFamilies[i]; cf != ef {
+							t.Errorf("clusterIP is not allocated: %v", clip)
-								t.Errorf("clusterIP is the wrong IP family: want %s, got %s", ef, cf)
-							}
 						}
 					}
 				})
@@ -4733,6 +4736,66 @@ func TestCreateInitIPFields(t *testing.T) {
 	}
 }
 
+func TestCreateReallocation(t *testing.T) {
+	testCases := []struct {
+		name string
+		svc  *api.Service
+	}{{
+		name: "v4",
+		svc:  svctest.MakeService("foo", svctest.SetTypeNodePort, svctest.SetIPFamilies(api.IPv4Protocol)),
+	}, {
+		name: "v6",
+		svc:  svctest.MakeService("foo", svctest.SetTypeNodePort, svctest.SetIPFamilies(api.IPv6Protocol)),
+	}, {
+		name: "v4v6",
+		svc:  svctest.MakeService("foo", svctest.SetTypeNodePort, svctest.SetIPFamilies(api.IPv4Protocol, api.IPv6Protocol)),
+	}}
+
+	// This test is ONLY with the gate enabled.
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, true)()
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			storage, _, server := newStorage(t, []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol})
+			defer server.Terminate(t)
+			defer storage.Store.DestroyFunc()
+
+			ctx := genericapirequest.NewDefaultContext()
+			createdObj, err := storage.Create(ctx, tc.svc, rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
+			if err != nil {
+				t.Fatalf("unexpected error creating service: %v", err)
+			}
+			createdSvc := createdObj.(*api.Service)
+
+			_, _, err = storage.Delete(ctx, tc.svc.Name, rest.ValidateAllObjectFunc, &metav1.DeleteOptions{})
+			if err != nil {
+				t.Fatalf("unexpected error creating service: %v", err)
+			}
+			//FIXME: HACK!!  Delete above calls "inner" which doesn't
+			// yet call the allocators - no release = test errors!
+			for _, al := range storage.alloc.serviceIPAllocatorsByFamily {
+				for _, ip := range createdSvc.Spec.ClusterIPs {
+					al.Release(netutils.ParseIPSloppy(ip))
+				}
+			}
+			for _, p := range createdSvc.Spec.Ports {
+				storage.alloc.serviceNodePorts.Release(int(p.NodePort))
+			}
+
+			// Force the same IPs and ports
+			svc2 := tc.svc.DeepCopy()
+			svc2.Spec.ClusterIP = createdSvc.Spec.ClusterIP
+			svc2.Spec.ClusterIPs = createdSvc.Spec.ClusterIPs
+			svc2.Spec.Ports = createdSvc.Spec.Ports
+
+			_, err = storage.Create(ctx, svc2, rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
+			if err != nil {
+				t.Fatalf("unexpected error creating service: %v", err)
+			}
+		})
+	}
+}
+
 func TestCreateInitNodePorts(t *testing.T) {
 	testCases := []struct {
 		name                         string