diff --git a/pkg/registry/core/service/storage/rest.go b/pkg/registry/core/service/storage/rest.go index 95ea56b54bf..0faa738e597 100644 --- a/pkg/registry/core/service/storage/rest.go +++ b/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) diff --git a/pkg/registry/core/service/storage/rest_test.go b/pkg/registry/core/service/storage/rest_test.go index e06d5378600..cff1830607a 100644 --- a/pkg/registry/core/service/storage/rest_test.go +++ b/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) } diff --git a/test/integration/dualstack/dualstack_test.go b/test/integration/dualstack/dualstack_test.go index eeeba0219e6..4feacacf7fe 100644 --- a/test/integration/dualstack/dualstack_test.go +++ b/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) + } +}