diff --git a/pkg/registry/core/service/BUILD b/pkg/registry/core/service/BUILD index b4cab5ac82b..92d6c0555ed 100644 --- a/pkg/registry/core/service/BUILD +++ b/pkg/registry/core/service/BUILD @@ -49,6 +49,7 @@ go_test( "//staging/src/k8s.io/apiserver/pkg/registry/rest:go_default_library", "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library", + "//vendor/k8s.io/utils/pointer:go_default_library", ], ) diff --git a/pkg/registry/core/service/strategy.go b/pkg/registry/core/service/strategy.go index ed97a3045e7..79c8b7c7d8b 100644 --- a/pkg/registry/core/service/strategy.go +++ b/pkg/registry/core/service/strategy.go @@ -184,7 +184,9 @@ func dropServiceDisabledFields(newSvc *api.Service, oldSvc *api.Service) { // Clear AllocateLoadBalancerNodePorts if ServiceLBNodePortControl if not enabled if !utilfeature.DefaultFeatureGate.Enabled(features.ServiceLBNodePortControl) { - newSvc.Spec.AllocateLoadBalancerNodePorts = nil + if !allocateLoadBalancerNodePortsInUse(oldSvc) { + newSvc.Spec.AllocateLoadBalancerNodePorts = nil + } } if !utilfeature.DefaultFeatureGate.Enabled(features.MixedProtocolLBService) { @@ -199,6 +201,14 @@ func dropServiceDisabledFields(newSvc *api.Service, oldSvc *api.Service) { } } +// returns true if svc.Spec.AllocateLoadBalancerNodePorts field is in use +func allocateLoadBalancerNodePortsInUse(svc *api.Service) bool { + if svc == nil { + return false + } + return svc.Spec.AllocateLoadBalancerNodePorts != nil +} + // returns true if svc.Spec.ServiceIPFamily field is in use func serviceDualStackFieldsInUse(svc *api.Service) bool { if svc == nil { @@ -396,9 +406,14 @@ func dropTypeDependentFields(newSvc *api.Service, oldSvc *api.Service) { newSvc.Spec.HealthCheckNodePort = 0 } - // AllocateLoadBalancerNodePorts may only be set for type LoadBalancer - if newSvc.Spec.Type != api.ServiceTypeLoadBalancer { - newSvc.Spec.AllocateLoadBalancerNodePorts = nil + // If a user is switching to a type that doesn't need allocatedLoadBalancerNodePorts AND they did not change + // this field, it is safe to drop it. + if oldSvc.Spec.Type == api.ServiceTypeLoadBalancer && newSvc.Spec.Type != api.ServiceTypeLoadBalancer { + if newSvc.Spec.AllocateLoadBalancerNodePorts != nil && oldSvc.Spec.AllocateLoadBalancerNodePorts != nil { + if *oldSvc.Spec.AllocateLoadBalancerNodePorts == *newSvc.Spec.AllocateLoadBalancerNodePorts { + newSvc.Spec.AllocateLoadBalancerNodePorts = nil + } + } } // NOTE: there are other fields like `selector` which we could wipe. diff --git a/pkg/registry/core/service/strategy_test.go b/pkg/registry/core/service/strategy_test.go index ce097dbf05c..5f130afdd82 100644 --- a/pkg/registry/core/service/strategy_test.go +++ b/pkg/registry/core/service/strategy_test.go @@ -34,6 +34,7 @@ import ( utilfeature "k8s.io/apiserver/pkg/util/feature" featuregatetesting "k8s.io/component-base/featuregate/testing" "k8s.io/kubernetes/pkg/features" + utilpointer "k8s.io/utils/pointer" ) func newStrategy(cidr string, hasSecondary bool) (testStrategy Strategy, testStatusStrategy Strategy) { @@ -935,6 +936,15 @@ func TestDropTypeDependentFields(t *testing.T) { } } } + setAllocateLoadBalancerNodePortsTrue := func(svc *api.Service) { + svc.Spec.AllocateLoadBalancerNodePorts = utilpointer.BoolPtr(true) + } + setAllocateLoadBalancerNodePortsFalse := func(svc *api.Service) { + svc.Spec.AllocateLoadBalancerNodePorts = utilpointer.BoolPtr(false) + } + clearAllocateLoadBalancerNodePorts := func(svc *api.Service) { + svc.Spec.AllocateLoadBalancerNodePorts = nil + } testCases := []struct { name string @@ -1022,6 +1032,46 @@ func TestDropTypeDependentFields(t *testing.T) { svc: makeValidServiceCustom(setTypeLoadBalancer, setHCNodePort), patch: patches(setTypeClusterIP, changeHCNodePort), expect: makeValidServiceCustom(setHCNodePort, changeHCNodePort), + }, { // allocatedLoadBalancerNodePorts cases + name: "clear allocatedLoadBalancerNodePorts true -> true", + svc: makeValidServiceCustom(setTypeLoadBalancer, setAllocateLoadBalancerNodePortsTrue), + patch: setTypeNodePort, + expect: makeValidServiceCustom(setTypeNodePort), + }, { + name: "clear allocatedLoadBalancerNodePorts false -> false", + svc: makeValidServiceCustom(setTypeLoadBalancer, setAllocateLoadBalancerNodePortsFalse), + patch: setTypeNodePort, + expect: makeValidServiceCustom(setTypeNodePort), + }, { + name: "set allocatedLoadBalancerNodePorts nil -> true", + svc: makeValidServiceCustom(setTypeLoadBalancer), + patch: patches(setTypeNodePort, setAllocateLoadBalancerNodePortsTrue), + expect: makeValidServiceCustom(setTypeNodePort, setAllocateLoadBalancerNodePortsTrue), + }, { + name: "set allocatedLoadBalancerNodePorts nil -> false", + svc: makeValidServiceCustom(setTypeLoadBalancer), + patch: patches(setTypeNodePort, setAllocateLoadBalancerNodePortsFalse), + expect: makeValidServiceCustom(setTypeNodePort, setAllocateLoadBalancerNodePortsFalse), + }, { + name: "set allocatedLoadBalancerNodePorts true -> nil", + svc: makeValidServiceCustom(setTypeLoadBalancer, setAllocateLoadBalancerNodePortsTrue), + patch: patches(setTypeNodePort, clearAllocateLoadBalancerNodePorts), + expect: makeValidServiceCustom(setTypeNodePort), + }, { + name: "set allocatedLoadBalancerNodePorts false -> nil", + svc: makeValidServiceCustom(setTypeLoadBalancer, setAllocateLoadBalancerNodePortsFalse), + patch: patches(setTypeNodePort, clearAllocateLoadBalancerNodePorts), + expect: makeValidServiceCustom(setTypeNodePort), + }, { + name: "set allocatedLoadBalancerNodePorts true -> false", + svc: makeValidServiceCustom(setTypeLoadBalancer, setAllocateLoadBalancerNodePortsTrue), + patch: patches(setTypeNodePort, setAllocateLoadBalancerNodePortsFalse), + expect: makeValidServiceCustom(setTypeNodePort, setAllocateLoadBalancerNodePortsFalse), + }, { + name: "set allocatedLoadBalancerNodePorts false -> true", + svc: makeValidServiceCustom(setTypeLoadBalancer, setAllocateLoadBalancerNodePortsFalse), + patch: patches(setTypeNodePort, setAllocateLoadBalancerNodePortsTrue), + expect: makeValidServiceCustom(setTypeNodePort, setAllocateLoadBalancerNodePortsTrue), }} for _, tc := range testCases { @@ -1053,6 +1103,9 @@ func TestDropTypeDependentFields(t *testing.T) { if result.Spec.HealthCheckNodePort != tc.expect.Spec.HealthCheckNodePort { t.Errorf("failed %q: expected healthCheckNodePort %d, got %d", tc.name, tc.expect.Spec.HealthCheckNodePort, result.Spec.HealthCheckNodePort) } + if !reflect.DeepEqual(result.Spec.AllocateLoadBalancerNodePorts, tc.expect.Spec.AllocateLoadBalancerNodePorts) { + t.Errorf("failed %q: expected AllocateLoadBalancerNodePorts %v, got %v", tc.name, tc.expect.Spec.AllocateLoadBalancerNodePorts, result.Spec.AllocateLoadBalancerNodePorts) + } }) } }