diff --git a/pkg/apis/core/validation/conditional_validation.go b/pkg/apis/core/validation/conditional_validation.go deleted file mode 100644 index 43266c16ccb..00000000000 --- a/pkg/apis/core/validation/conditional_validation.go +++ /dev/null @@ -1,61 +0,0 @@ -/* -Copyright 2019 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package validation - -import ( - "k8s.io/apimachinery/pkg/util/validation/field" - utilfeature "k8s.io/apiserver/pkg/util/feature" - api "k8s.io/kubernetes/pkg/apis/core" - "k8s.io/kubernetes/pkg/features" -) - -// ValidateConditionalService validates conditionally valid fields. -func ValidateConditionalService(service, oldService *api.Service) field.ErrorList { - var errs field.ErrorList - - errs = append(errs, validateMixedProtocolLBService(service, oldService)...) - - return errs -} - -// validateMixedProtocolLBService checks if the old Service has type=LoadBalancer and whether the Service has different Protocols -// on its ports. If the MixedProtocolLBService feature flag is disabled the usage of different Protocols in the new Service is -// valid only if the old Service has different Protocols, too. -func validateMixedProtocolLBService(service, oldService *api.Service) (errs field.ErrorList) { - if service.Spec.Type != api.ServiceTypeLoadBalancer { - return - } - if utilfeature.DefaultFeatureGate.Enabled(features.MixedProtocolLBService) { - return - } - - if serviceHasMixedProtocols(service) && !serviceHasMixedProtocols(oldService) { - errs = append(errs, field.Invalid(field.NewPath("spec", "ports"), service.Spec.Ports, "may not contain more than 1 protocol when type is 'LoadBalancer'")) - } - return -} - -func serviceHasMixedProtocols(service *api.Service) bool { - if service == nil { - return false - } - protos := map[string]bool{} - for _, port := range service.Spec.Ports { - protos[string(port.Protocol)] = true - } - return len(protos) > 1 -} diff --git a/pkg/apis/core/validation/conditional_validation_test.go b/pkg/apis/core/validation/conditional_validation_test.go deleted file mode 100644 index 12d0b2edb94..00000000000 --- a/pkg/apis/core/validation/conditional_validation_test.go +++ /dev/null @@ -1,277 +0,0 @@ -/* -Copyright 2019 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package validation - -import ( - "strings" - "testing" - - utilfeature "k8s.io/apiserver/pkg/util/feature" - featuregatetesting "k8s.io/component-base/featuregate/testing" - api "k8s.io/kubernetes/pkg/apis/core" - "k8s.io/kubernetes/pkg/features" -) - -func TestValidateMixedProtocolLBService(t *testing.T) { - newLBServiceDifferentProtocols := &api.Service{ - Spec: api.ServiceSpec{ - Ports: []api.ServicePort{ - { - Protocol: api.ProtocolTCP, - }, - { - Protocol: api.ProtocolUDP, - }, - }, - Type: api.ServiceTypeLoadBalancer, - }, - } - newLBServiceSameProtocols := &api.Service{ - Spec: api.ServiceSpec{ - Ports: []api.ServicePort{ - { - Protocol: api.ProtocolTCP, - }, - { - Protocol: api.ProtocolTCP, - }, - }, - Type: api.ServiceTypeLoadBalancer, - }, - } - newNonLBServiceDifferentProtocols := &api.Service{ - Spec: api.ServiceSpec{ - Ports: []api.ServicePort{ - { - Protocol: api.ProtocolTCP, - }, - { - Protocol: api.ProtocolUDP, - }, - }, - Type: api.ServiceTypeNodePort, - }, - } - newNonLBServiceSameProtocols := &api.Service{ - Spec: api.ServiceSpec{ - Ports: []api.ServicePort{ - { - Protocol: api.ProtocolUDP, - }, - { - Protocol: api.ProtocolUDP, - }, - }, - Type: api.ServiceTypeNodePort, - }, - } - oldLBServiceDifferentProtocols := &api.Service{ - Spec: api.ServiceSpec{ - Ports: []api.ServicePort{ - { - Protocol: api.ProtocolTCP, - }, - { - Protocol: api.ProtocolUDP, - }, - }, - Type: api.ServiceTypeLoadBalancer, - }, - } - oldLBServiceSameProtocols := &api.Service{ - Spec: api.ServiceSpec{ - Ports: []api.ServicePort{ - { - Protocol: api.ProtocolTCP, - }, - { - Protocol: api.ProtocolTCP, - }, - }, - Type: api.ServiceTypeLoadBalancer, - }, - } - oldNonLBServiceDifferentProtocols := &api.Service{ - Spec: api.ServiceSpec{ - Ports: []api.ServicePort{ - { - Protocol: api.ProtocolTCP, - }, - { - Protocol: api.ProtocolUDP, - }, - }, - Type: api.ServiceTypeNodePort, - }, - } - oldNonLBServiceSameProtocols := &api.Service{ - Spec: api.ServiceSpec{ - Ports: []api.ServicePort{ - { - Protocol: api.ProtocolUDP, - }, - { - Protocol: api.ProtocolUDP, - }, - }, - Type: api.ServiceTypeNodePort, - }, - } - cases := map[string]struct { - oldService *api.Service - newService *api.Service - fgEnabled bool - expectedError []string - }{ - "Old service is nil, new service has different protocols, feature gate false": { - oldService: nil, - newService: newLBServiceDifferentProtocols, - fgEnabled: false, - expectedError: []string{`spec.ports: Invalid value: []core.ServicePort{core.ServicePort{Name:"", Protocol:"TCP", AppProtocol:(*string)(nil), Port:0, TargetPort:intstr.IntOrString{Type:0, IntVal:0, StrVal:""}, NodePort:0}, core.ServicePort{Name:"", Protocol:"UDP", AppProtocol:(*string)(nil), Port:0, TargetPort:intstr.IntOrString{Type:0, IntVal:0, StrVal:""}, NodePort:0}}: may not contain more than 1 protocol when type is 'LoadBalancer'`}, - }, - "Old service is nil, new service has different protocols, feature gate true": { - oldService: nil, - newService: newLBServiceDifferentProtocols, - fgEnabled: true, - }, - "Old service is nil, new service does not have different protocols, feature gate false": { - oldService: nil, - newService: newLBServiceSameProtocols, - fgEnabled: false, - }, - "Old service is nil, new service does not have different protocols, feature gate true": { - oldService: nil, - newService: newLBServiceSameProtocols, - fgEnabled: true, - }, - "Old service is nil, new non-LB service has different protocols, feature gate false": { - oldService: nil, - newService: newNonLBServiceDifferentProtocols, - fgEnabled: false, - }, - "Old service is nil, new non-LB service has different protocols, feature gate true": { - oldService: nil, - newService: newNonLBServiceDifferentProtocols, - fgEnabled: true, - }, - "Old service is nil, new non-LB service does not have different protocols, feature gate false": { - oldService: nil, - newService: newNonLBServiceSameProtocols, - fgEnabled: false, - }, - "Old service is nil, new non-LB service does not have different protocols, feature gate true": { - oldService: nil, - newService: newNonLBServiceSameProtocols, - fgEnabled: true, - }, - "Non-LB services, both services have different protocols, feature gate false": { - oldService: oldNonLBServiceDifferentProtocols, - newService: newNonLBServiceDifferentProtocols, - fgEnabled: false, - }, - "Non-LB services, old service has same protocols, new service has different protocols, feature gate false": { - oldService: oldNonLBServiceSameProtocols, - newService: newNonLBServiceDifferentProtocols, - fgEnabled: false, - }, - "Non-LB services, old service has different protocols, new service has identical protocols, feature gate false": { - oldService: oldNonLBServiceDifferentProtocols, - newService: newNonLBServiceSameProtocols, - fgEnabled: false, - }, - "Non-LB services, both services have same protocols, feature gate false": { - oldService: oldNonLBServiceSameProtocols, - newService: newNonLBServiceSameProtocols, - fgEnabled: false, - }, - "Non-LB services, both services have different protocols, feature gate true": { - oldService: oldNonLBServiceDifferentProtocols, - newService: newNonLBServiceDifferentProtocols, - fgEnabled: true, - }, - "Non-LB services, old service has same protocols, new service has different protocols, feature gate true": { - oldService: oldNonLBServiceSameProtocols, - newService: newNonLBServiceDifferentProtocols, - fgEnabled: true, - }, - "Non-LB services, old service has different protocols, new service has identical protocols, feature gate true": { - oldService: oldNonLBServiceDifferentProtocols, - newService: newNonLBServiceSameProtocols, - fgEnabled: true, - }, - "Non-LB services, both services have same protocols, feature gate true": { - oldService: oldNonLBServiceSameProtocols, - newService: newNonLBServiceSameProtocols, - fgEnabled: true, - }, - "LB service, neither service has different protocols, feature gate false": { - oldService: oldLBServiceSameProtocols, - newService: newLBServiceSameProtocols, - fgEnabled: false, - }, - "LB service, old service does not have different protocols, new service has different protocols, feature gate false": { - oldService: oldLBServiceSameProtocols, - newService: newLBServiceDifferentProtocols, - fgEnabled: false, - expectedError: []string{`spec.ports: Invalid value: []core.ServicePort{core.ServicePort{Name:"", Protocol:"TCP", AppProtocol:(*string)(nil), Port:0, TargetPort:intstr.IntOrString{Type:0, IntVal:0, StrVal:""}, NodePort:0}, core.ServicePort{Name:"", Protocol:"UDP", AppProtocol:(*string)(nil), Port:0, TargetPort:intstr.IntOrString{Type:0, IntVal:0, StrVal:""}, NodePort:0}}: may not contain more than 1 protocol when type is 'LoadBalancer'`}, - }, - "LB service, old service has different protocols, new service does not have different protocols, feature gate false": { - oldService: oldLBServiceDifferentProtocols, - newService: newLBServiceSameProtocols, - fgEnabled: false, - }, - "LB service, both services have different protocols, feature gate false": { - oldService: oldLBServiceDifferentProtocols, - newService: newLBServiceDifferentProtocols, - fgEnabled: false, - }, - "LB service, neither service has different protocols, feature gate true": { - oldService: oldLBServiceSameProtocols, - newService: newLBServiceSameProtocols, - fgEnabled: true, - }, - "LB service, old service has different protocols, new service does not have different protocols, feature gate true": { - oldService: oldLBServiceDifferentProtocols, - newService: newLBServiceSameProtocols, - fgEnabled: true, - }, - "LB service, old service does not have different protocols, new service has different protocols, feature gate true": { - oldService: oldLBServiceSameProtocols, - newService: newLBServiceDifferentProtocols, - fgEnabled: true, - }, - "LB service, both services have different protocols, feature gate true": { - oldService: oldLBServiceDifferentProtocols, - newService: newLBServiceDifferentProtocols, - fgEnabled: true, - }, - } - for name, tc := range cases { - t.Run(name, func(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.MixedProtocolLBService, tc.fgEnabled)() - errs := validateMixedProtocolLBService(tc.newService, tc.oldService) - if len(errs) != len(tc.expectedError) { - t.Fatalf("unexpected number of errors: %v", errs) - } - for i := range errs { - if !strings.Contains(errs[i].Error(), tc.expectedError[i]) { - t.Errorf("unexpected error %d: %v", i, errs[i]) - } - } - }) - } -} diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 389eccfcb56..789d3171d67 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -571,6 +571,7 @@ const ( // kep: https://kep.k8s.io/1435 // alpha: v1.20 // beta: v1.24 + // ga: v1.26 // // Enables the usage of different protocols in the same Service with type=LoadBalancer MixedProtocolLBService featuregate.Feature = "MixedProtocolLBService" @@ -973,7 +974,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS MinimizeIPTablesRestore: {Default: false, PreRelease: featuregate.Alpha}, - MixedProtocolLBService: {Default: true, PreRelease: featuregate.Beta}, + MixedProtocolLBService: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.28 MultiCIDRRangeAllocator: {Default: false, PreRelease: featuregate.Alpha}, diff --git a/pkg/registry/core/service/storage/storage_test.go b/pkg/registry/core/service/storage/storage_test.go index bbff651b0fa..1345ed519fa 100644 --- a/pkg/registry/core/service/storage/storage_test.go +++ b/pkg/registry/core/service/storage/storage_test.go @@ -39,12 +39,9 @@ import ( genericregistrytest "k8s.io/apiserver/pkg/registry/generic/testing" "k8s.io/apiserver/pkg/registry/rest" etcd3testing "k8s.io/apiserver/pkg/storage/etcd3/testing" - utilfeature "k8s.io/apiserver/pkg/util/feature" - featuregatetesting "k8s.io/component-base/featuregate/testing" epstest "k8s.io/kubernetes/pkg/api/endpoints/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" @@ -6113,11 +6110,10 @@ func TestCreateDeleteReuse(t *testing.T) { func TestCreateInitNodePorts(t *testing.T) { testCases := []struct { - name string - svc *api.Service - expectError bool - expectNodePorts bool - gateMixedProtocolLBService bool + name string + svc *api.Service + expectError bool + expectNodePorts bool }{{ name: "type:ExternalName", svc: svctest.MakeService("foo"), @@ -6281,66 +6277,31 @@ func TestCreateInitNodePorts(t *testing.T) { svctest.SetNodePorts(30080, 30080)), expectError: true, }, { - // When the MixedProtocolLBService gate is locked, this can be removed. - name: "type:LoadBalancer_multiport_multiproto_unspecified_MixedProtocolLBService:off", + name: "type:LoadBalancer_multiport_multiproto_unspecified", svc: svctest.MakeService("foo", svctest.SetTypeLoadBalancer, svctest.SetPorts( svctest.MakeServicePort("p", 53, intstr.FromInt(53), api.ProtocolTCP), svctest.MakeServicePort("q", 53, intstr.FromInt(53), api.ProtocolUDP))), - gateMixedProtocolLBService: false, - expectError: true, + expectNodePorts: true, }, { - // When the MixedProtocolLBService gate is locked, this can be removed. - name: "type:LoadBalancer_multiport_multiproto_specified_MixedProtocolLBService:off", + name: "type:LoadBalancer_multiport_multiproto_specified", svc: svctest.MakeService("foo", svctest.SetTypeLoadBalancer, svctest.SetPorts( svctest.MakeServicePort("p", 53, intstr.FromInt(53), api.ProtocolTCP), svctest.MakeServicePort("q", 53, intstr.FromInt(53), api.ProtocolUDP)), svctest.SetUniqueNodePorts), - gateMixedProtocolLBService: false, - expectError: true, + expectNodePorts: true, }, { - // When the MixedProtocolLBService gate is locked, this can be removed. - name: "type:LoadBalancer_multiport_multiproto_same_MixedProtocolLBService:off", + name: "type:LoadBalancer_multiport_multiproto_same", svc: svctest.MakeService("foo", svctest.SetTypeLoadBalancer, svctest.SetPorts( svctest.MakeServicePort("p", 53, intstr.FromInt(53), api.ProtocolTCP), svctest.MakeServicePort("q", 53, intstr.FromInt(53), api.ProtocolUDP)), svctest.SetNodePorts(30053, 30053)), - gateMixedProtocolLBService: false, - expectError: true, - }, { - name: "type:LoadBalancer_multiport_multiproto_unspecified_MixedProtocolLBService:on", - svc: svctest.MakeService("foo", - svctest.SetTypeLoadBalancer, - svctest.SetPorts( - svctest.MakeServicePort("p", 53, intstr.FromInt(53), api.ProtocolTCP), - svctest.MakeServicePort("q", 53, intstr.FromInt(53), api.ProtocolUDP))), - gateMixedProtocolLBService: true, - expectNodePorts: true, - }, { - name: "type:LoadBalancer_multiport_multiproto_specified_MixedProtocolLBService:on", - svc: svctest.MakeService("foo", - svctest.SetTypeLoadBalancer, - svctest.SetPorts( - svctest.MakeServicePort("p", 53, intstr.FromInt(53), api.ProtocolTCP), - svctest.MakeServicePort("q", 53, intstr.FromInt(53), api.ProtocolUDP)), - svctest.SetUniqueNodePorts), - gateMixedProtocolLBService: true, - expectNodePorts: true, - }, { - name: "type:LoadBalancer_multiport_multiproto_same_MixedProtocolLBService:on", - svc: svctest.MakeService("foo", - svctest.SetTypeLoadBalancer, - svctest.SetPorts( - svctest.MakeServicePort("p", 53, intstr.FromInt(53), api.ProtocolTCP), - svctest.MakeServicePort("q", 53, intstr.FromInt(53), api.ProtocolUDP)), - svctest.SetNodePorts(30053, 30053)), - gateMixedProtocolLBService: true, - expectNodePorts: true, + expectNodePorts: true, }, { name: "type:LoadBalancer_multiport_multiproto_conflict", svc: svctest.MakeService("foo", @@ -6358,8 +6319,6 @@ func TestCreateInitNodePorts(t *testing.T) { defer storage.Store.DestroyFunc() for _, tc := range testCases { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.MixedProtocolLBService, tc.gateMixedProtocolLBService)() - t.Run(tc.name, func(t *testing.T) { ctx := genericapirequest.NewDefaultContext() createdObj, err := storage.Create(ctx, tc.svc, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) diff --git a/pkg/registry/core/service/strategy.go b/pkg/registry/core/service/strategy.go index e76855795f8..d2ab2d80c51 100644 --- a/pkg/registry/core/service/strategy.go +++ b/pkg/registry/core/service/strategy.go @@ -81,7 +81,6 @@ func (svcStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object func (svcStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { service := obj.(*api.Service) allErrs := validation.ValidateServiceCreate(service) - allErrs = append(allErrs, validation.ValidateConditionalService(service, nil)...) return allErrs } @@ -98,7 +97,6 @@ func (svcStrategy) AllowCreateOnUpdate() bool { func (strategy svcStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { allErrs := validation.ValidateServiceUpdate(obj.(*api.Service), old.(*api.Service)) - allErrs = append(allErrs, validation.ValidateConditionalService(obj.(*api.Service), old.(*api.Service))...) return allErrs } @@ -119,17 +117,6 @@ func (svcStrategy) AllowUnconditionalUpdate() bool { // } func dropServiceDisabledFields(newSvc *api.Service, oldSvc *api.Service) { - if !utilfeature.DefaultFeatureGate.Enabled(features.MixedProtocolLBService) { - if !serviceConditionsInUse(oldSvc) { - newSvc.Status.Conditions = nil - } - if !loadBalancerPortsInUse(oldSvc) { - for i := range newSvc.Status.LoadBalancer.Ingress { - newSvc.Status.LoadBalancer.Ingress[i].Ports = nil - } - } - } - // Clear InternalTrafficPolicy if not enabled if !utilfeature.DefaultFeatureGate.Enabled(features.ServiceInternalTrafficPolicy) { if !serviceInternalTrafficPolicyInUse(oldSvc) { @@ -138,27 +125,6 @@ func dropServiceDisabledFields(newSvc *api.Service, oldSvc *api.Service) { } } -// returns true when the svc.Status.Conditions field is in use. -func serviceConditionsInUse(svc *api.Service) bool { - if svc == nil { - return false - } - return svc.Status.Conditions != nil -} - -// returns true when the svc.Status.LoadBalancer.Ingress.Ports field is in use. -func loadBalancerPortsInUse(svc *api.Service) bool { - if svc == nil { - return false - } - for _, ing := range svc.Status.LoadBalancer.Ingress { - if ing.Ports != nil { - return true - } - } - return false -} - func serviceInternalTrafficPolicyInUse(svc *api.Service) bool { if svc == nil { return false diff --git a/pkg/registry/core/service/strategy_test.go b/pkg/registry/core/service/strategy_test.go index 7769a428b22..55d13d4adf2 100644 --- a/pkg/registry/core/service/strategy_test.go +++ b/pkg/registry/core/service/strategy_test.go @@ -166,7 +166,6 @@ func TestDropDisabledField(t *testing.T) { testCases := []struct { name string - enableMixedProtocol bool enableInternalTrafficPolicy bool svc *api.Service oldSvc *api.Service @@ -174,117 +173,53 @@ func TestDropDisabledField(t *testing.T) { }{ /* svc.Status.Conditions */ { - name: "mixed protocol not enabled, field not used in old, not used in new", - enableMixedProtocol: false, - svc: makeServiceWithConditions(nil), - oldSvc: makeServiceWithConditions(nil), - compareSvc: makeServiceWithConditions(nil), + name: "mixed protocol enabled, field not used in old, not used in new", + svc: makeServiceWithConditions(nil), + oldSvc: makeServiceWithConditions(nil), + compareSvc: makeServiceWithConditions(nil), }, { - name: "mixed protocol not enabled, field used in old and in new", - enableMixedProtocol: false, - svc: makeServiceWithConditions([]metav1.Condition{}), - oldSvc: makeServiceWithConditions([]metav1.Condition{}), - compareSvc: makeServiceWithConditions([]metav1.Condition{}), + name: "mixed protocol enabled, field used in old and in new", + svc: makeServiceWithConditions([]metav1.Condition{}), + oldSvc: makeServiceWithConditions([]metav1.Condition{}), + compareSvc: makeServiceWithConditions([]metav1.Condition{}), }, { - name: "mixed protocol not enabled, field not used in old, used in new", - enableMixedProtocol: false, - svc: makeServiceWithConditions([]metav1.Condition{}), - oldSvc: makeServiceWithConditions(nil), - compareSvc: makeServiceWithConditions(nil), + name: "mixed protocol enabled, field not used in old, used in new", + svc: makeServiceWithConditions([]metav1.Condition{}), + oldSvc: makeServiceWithConditions(nil), + compareSvc: makeServiceWithConditions([]metav1.Condition{}), }, { - name: "mixed protocol not enabled, field used in old, not used in new", - enableMixedProtocol: false, - svc: makeServiceWithConditions(nil), - oldSvc: makeServiceWithConditions([]metav1.Condition{}), - compareSvc: makeServiceWithConditions(nil), - }, - { - name: "mixed protocol enabled, field not used in old, not used in new", - enableMixedProtocol: true, - svc: makeServiceWithConditions(nil), - oldSvc: makeServiceWithConditions(nil), - compareSvc: makeServiceWithConditions(nil), - }, - { - name: "mixed protocol enabled, field used in old and in new", - enableMixedProtocol: true, - svc: makeServiceWithConditions([]metav1.Condition{}), - oldSvc: makeServiceWithConditions([]metav1.Condition{}), - compareSvc: makeServiceWithConditions([]metav1.Condition{}), - }, - { - name: "mixed protocol enabled, field not used in old, used in new", - enableMixedProtocol: true, - svc: makeServiceWithConditions([]metav1.Condition{}), - oldSvc: makeServiceWithConditions(nil), - compareSvc: makeServiceWithConditions([]metav1.Condition{}), - }, - { - name: "mixed protocol enabled, field used in old, not used in new", - enableMixedProtocol: true, - svc: makeServiceWithConditions(nil), - oldSvc: makeServiceWithConditions([]metav1.Condition{}), - compareSvc: makeServiceWithConditions(nil), + name: "mixed protocol enabled, field used in old, not used in new", + svc: makeServiceWithConditions(nil), + oldSvc: makeServiceWithConditions([]metav1.Condition{}), + compareSvc: makeServiceWithConditions(nil), }, /* svc.Status.LoadBalancer.Ingress.Ports */ { - name: "mixed protocol not enabled, field not used in old, not used in new", - enableMixedProtocol: false, - svc: makeServiceWithPorts(nil), - oldSvc: makeServiceWithPorts(nil), - compareSvc: makeServiceWithPorts(nil), + name: "mixed protocol enabled, field not used in old, not used in new", + svc: makeServiceWithPorts(nil), + oldSvc: makeServiceWithPorts(nil), + compareSvc: makeServiceWithPorts(nil), }, { - name: "mixed protocol not enabled, field used in old and in new", - enableMixedProtocol: false, - svc: makeServiceWithPorts([]api.PortStatus{}), - oldSvc: makeServiceWithPorts([]api.PortStatus{}), - compareSvc: makeServiceWithPorts([]api.PortStatus{}), + name: "mixed protocol enabled, field used in old and in new", + svc: makeServiceWithPorts([]api.PortStatus{}), + oldSvc: makeServiceWithPorts([]api.PortStatus{}), + compareSvc: makeServiceWithPorts([]api.PortStatus{}), }, { - name: "mixed protocol not enabled, field not used in old, used in new", - enableMixedProtocol: false, - svc: makeServiceWithPorts([]api.PortStatus{}), - oldSvc: makeServiceWithPorts(nil), - compareSvc: makeServiceWithPorts(nil), + name: "mixed protocol enabled, field not used in old, used in new", + svc: makeServiceWithPorts([]api.PortStatus{}), + oldSvc: makeServiceWithPorts(nil), + compareSvc: makeServiceWithPorts([]api.PortStatus{}), }, { - name: "mixed protocol not enabled, field used in old, not used in new", - enableMixedProtocol: false, - svc: makeServiceWithPorts(nil), - oldSvc: makeServiceWithPorts([]api.PortStatus{}), - compareSvc: makeServiceWithPorts(nil), - }, - { - name: "mixed protocol enabled, field not used in old, not used in new", - enableMixedProtocol: true, - svc: makeServiceWithPorts(nil), - oldSvc: makeServiceWithPorts(nil), - compareSvc: makeServiceWithPorts(nil), - }, - { - name: "mixed protocol enabled, field used in old and in new", - enableMixedProtocol: true, - svc: makeServiceWithPorts([]api.PortStatus{}), - oldSvc: makeServiceWithPorts([]api.PortStatus{}), - compareSvc: makeServiceWithPorts([]api.PortStatus{}), - }, - { - name: "mixed protocol enabled, field not used in old, used in new", - enableMixedProtocol: true, - svc: makeServiceWithPorts([]api.PortStatus{}), - oldSvc: makeServiceWithPorts(nil), - compareSvc: makeServiceWithPorts([]api.PortStatus{}), - }, - { - name: "mixed protocol enabled, field used in old, not used in new", - enableMixedProtocol: true, - svc: makeServiceWithPorts(nil), - oldSvc: makeServiceWithPorts([]api.PortStatus{}), - compareSvc: makeServiceWithPorts(nil), + name: "mixed protocol enabled, field used in old, not used in new", + svc: makeServiceWithPorts(nil), + oldSvc: makeServiceWithPorts([]api.PortStatus{}), + compareSvc: makeServiceWithPorts(nil), }, /* svc.spec.internalTrafficPolicy */ { @@ -312,7 +247,6 @@ func TestDropDisabledField(t *testing.T) { } for _, tc := range testCases { func() { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.MixedProtocolLBService, tc.enableMixedProtocol)() defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ServiceInternalTrafficPolicy, tc.enableInternalTrafficPolicy)() old := tc.oldSvc.DeepCopy() diff --git a/test/e2e/framework/endpoints/ports.go b/test/e2e/framework/endpoints/ports.go index efd960d93f4..8c116b031a5 100644 --- a/test/e2e/framework/endpoints/ports.go +++ b/test/e2e/framework/endpoints/ports.go @@ -24,6 +24,9 @@ import ( // PortsByPodUID is a map that maps pod UID to container ports. type PortsByPodUID map[types.UID][]int +// FullPortsByPodUID is a map that maps pod UID to container ports. +type FullPortsByPodUID map[types.UID][]v1.ContainerPort + // GetContainerPortsByPodUID returns a PortsByPodUID map on the given endpoints. func GetContainerPortsByPodUID(ep *v1.Endpoints) PortsByPodUID { m := PortsByPodUID{} @@ -40,3 +43,24 @@ func GetContainerPortsByPodUID(ep *v1.Endpoints) PortsByPodUID { } return m } + +// GetFullContainerPortsByPodUID returns a FullPortsByPodUID map on the given endpoints with all the port data. +func GetFullContainerPortsByPodUID(ep *v1.Endpoints) FullPortsByPodUID { + m := FullPortsByPodUID{} + for _, ss := range ep.Subsets { + for _, port := range ss.Ports { + containerPort := v1.ContainerPort{ + Name: port.Name, + ContainerPort: port.Port, + Protocol: port.Protocol, + } + for _, addr := range ss.Addresses { + if _, ok := m[addr.TargetRef.UID]; !ok { + m[addr.TargetRef.UID] = make([]v1.ContainerPort, 0) + } + m[addr.TargetRef.UID] = append(m[addr.TargetRef.UID], containerPort) + } + } + } + return m +} diff --git a/test/e2e/framework/endpointslice/ports.go b/test/e2e/framework/endpointslice/ports.go index c61230c7d2a..f51162b0083 100644 --- a/test/e2e/framework/endpointslice/ports.go +++ b/test/e2e/framework/endpointslice/ports.go @@ -17,6 +17,7 @@ limitations under the License. package endpointslice import ( + v1 "k8s.io/api/core/v1" discoveryv1 "k8s.io/api/discovery/v1" "k8s.io/apimachinery/pkg/types" ) @@ -24,6 +25,9 @@ import ( // PortsByPodUID is a map that maps pod UID to container ports. type PortsByPodUID map[types.UID][]int +// FullPortsByPodUID is a map that maps pod UID to container ports. +type FullPortsByPodUID map[types.UID][]v1.ContainerPort + // GetContainerPortsByPodUID returns a PortsByPodUID map on the given endpoints. func GetContainerPortsByPodUID(eps []discoveryv1.EndpointSlice) PortsByPodUID { m := PortsByPodUID{} @@ -44,3 +48,28 @@ func GetContainerPortsByPodUID(eps []discoveryv1.EndpointSlice) PortsByPodUID { } return m } + +// GetFullContainerPortsByPodUID returns a PortsByPodUID map on the given endpoints. +func GetFullContainerPortsByPodUID(eps []discoveryv1.EndpointSlice) FullPortsByPodUID { + m := FullPortsByPodUID{} + + for _, es := range eps { + for _, port := range es.Ports { + if port.Port == nil { + continue + } + containerPort := v1.ContainerPort{ + Name: *port.Name, + ContainerPort: *port.Port, + Protocol: *port.Protocol, + } + for _, ep := range es.Endpoints { + if _, ok := m[ep.TargetRef.UID]; !ok { + m[ep.TargetRef.UID] = make([]v1.ContainerPort, 0) + } + m[ep.TargetRef.UID] = append(m[ep.TargetRef.UID], containerPort) + } + } + } + return m +} diff --git a/test/e2e/framework/service/jig.go b/test/e2e/framework/service/jig.go index 6e8a39bfec1..202c872ae12 100644 --- a/test/e2e/framework/service/jig.go +++ b/test/e2e/framework/service/jig.go @@ -1030,6 +1030,8 @@ func (j *TestJig) CheckServiceReachability(svc *v1.Service, pod *v1.Pod) error { return j.checkNodePortServiceReachability(svc, pod) case v1.ServiceTypeExternalName: return j.checkExternalServiceReachability(svc, pod) + case v1.ServiceTypeLoadBalancer: + return j.checkClusterIPServiceReachability(svc, pod) default: return fmt.Errorf("unsupported service type \"%s\" to verify service reachability for \"%s\" service. This may due to diverse implementation of the service type", svcType, svc.Name) } @@ -1065,3 +1067,22 @@ func (j *TestJig) CreateSCTPServiceWithPort(tweak func(svc *v1.Service), port in } return j.sanityCheckService(result, svc.Spec.Type) } + +// CreateLoadBalancerServiceWaitForClusterIPOnly creates a loadbalancer service and waits +// for it to acquire a cluster IP +func (j *TestJig) CreateLoadBalancerServiceWaitForClusterIPOnly(tweak func(svc *v1.Service)) (*v1.Service, error) { + ginkgo.By("creating a service " + j.Namespace + "/" + j.Name + " with type=LoadBalancer") + svc := j.newServiceTemplate(v1.ProtocolTCP, 80) + svc.Spec.Type = v1.ServiceTypeLoadBalancer + // We need to turn affinity off for our LB distribution tests + svc.Spec.SessionAffinity = v1.ServiceAffinityNone + if tweak != nil { + tweak(svc) + } + result, err := j.Client.CoreV1().Services(j.Namespace).Create(context.TODO(), svc, metav1.CreateOptions{}) + if err != nil { + return nil, fmt.Errorf("failed to create LoadBalancer Service %q: %v", svc.Name, err) + } + + return j.sanityCheckService(result, v1.ServiceTypeLoadBalancer) +} diff --git a/test/e2e/network/service.go b/test/e2e/network/service.go index e196d805dcc..1a0ef2ff601 100644 --- a/test/e2e/network/service.go +++ b/test/e2e/network/service.go @@ -125,6 +125,12 @@ type portsByPodName map[string][]int // portsByPodUID is a map that maps pod name to container ports. type portsByPodUID map[types.UID][]int +// fullPortsByPodName is a map that maps pod name to container ports including their protocols. +type fullPortsByPodName map[string][]v1.ContainerPort + +// fullPortsByPodUID is a map that maps pod name to container ports. +type fullPortsByPodUID map[types.UID][]v1.ContainerPort + // affinityCheckFromPod returns interval, timeout and function pinging the service and // returning pinged hosts for pinging the service from execPod. func affinityCheckFromPod(execPod *v1.Pod, serviceIP string, servicePort int) (time.Duration, time.Duration, func() []string) { @@ -3731,6 +3737,77 @@ var _ = common.SIGDescribe("Services", func() { framework.Logf("Collection of services has been deleted") }) + /* + Release: v1.26 + Testname: Service, same ports with different protocols on a Load Balancer Service + Description: Create a LoadBalancer service with two ports that have the same value but use different protocols. Add a Pod that listens on both ports. The Pod must be reachable via the ClusterIP and both ports + */ + ginkgo.It("should serve endpoints on same port and different protocol for internal traffic on Type LoadBalancer ", func() { + serviceName := "multiprotocol-lb-test" + ns := f.Namespace.Name + jig := e2eservice.NewTestJig(cs, ns, serviceName) + + defer func() { + err := cs.CoreV1().Services(ns).Delete(context.TODO(), serviceName, metav1.DeleteOptions{}) + framework.ExpectNoError(err, "failed to delete service: %s in namespace: %s", serviceName, ns) + }() + + svc1port := "svc1" + svc2port := "svc2" + + ginkgo.By("creating service " + serviceName + " in namespace " + ns) + svc, err := jig.CreateLoadBalancerServiceWaitForClusterIPOnly(func(service *v1.Service) { + service.Spec.Ports = []v1.ServicePort{ + { + Name: "portname1", + Port: 80, + TargetPort: intstr.FromString(svc1port), + Protocol: v1.ProtocolTCP, + }, + { + Name: "portname2", + Port: 80, + TargetPort: intstr.FromString(svc2port), + Protocol: v1.ProtocolUDP, + }, + } + }) + framework.ExpectNoError(err) + + containerPort := 100 + + names := map[string]bool{} + defer func() { + for name := range names { + err := cs.CoreV1().Pods(ns).Delete(context.TODO(), name, metav1.DeleteOptions{}) + framework.ExpectNoError(err, "failed to delete pod: %s in namespace: %s", name, ns) + } + }() + + containerPorts := []v1.ContainerPort{ + { + Name: svc1port, + ContainerPort: int32(containerPort), + Protocol: v1.ProtocolTCP, + }, + { + Name: svc2port, + ContainerPort: int32(containerPort), + Protocol: v1.ProtocolUDP, + }, + } + + podname1 := "pod1" + + createPodOrFail(f, ns, podname1, jig.Labels, containerPorts, "netexec", "--http-port", strconv.Itoa(containerPort), "--udp-port", strconv.Itoa(containerPort)) + validateEndpointsPortsWithProtocolsOrFail(cs, ns, serviceName, fullPortsByPodName{podname1: containerPorts}) + + ginkgo.By("Checking if the Service forwards traffic to pods") + execPod := e2epod.CreateExecPodOrFail(cs, ns, "execpod", nil) + err = jig.CheckServiceReachability(svc, execPod) + framework.ExpectNoError(err) + e2epod.DeletePodOrFail(cs, ns, podname1) + }) }) @@ -4164,6 +4241,108 @@ func restartComponent(cs clientset.Interface, cName, ns string, matchLabels map[ return err } +// validateEndpointsPortsWithProtocolsOrFail validates that the given service exists and is served by the given expectedEndpoints. +func validateEndpointsPortsWithProtocolsOrFail(c clientset.Interface, namespace, serviceName string, expectedEndpoints fullPortsByPodName) { + ginkgo.By(fmt.Sprintf("waiting up to %v for service %s in namespace %s to expose endpoints %v", framework.ServiceStartTimeout, serviceName, namespace, expectedEndpoints)) + expectedPortsByPodUID, err := translatePortsByPodNameToPortsByPodUID(c, namespace, expectedEndpoints) + framework.ExpectNoError(err, "failed to translate pod name to UID, ns:%s, expectedEndpoints:%v", namespace, expectedEndpoints) + + var ( + pollErr error + i = 0 + ) + if pollErr = wait.PollImmediate(time.Second, framework.ServiceStartTimeout, func() (bool, error) { + i++ + + ep, err := c.CoreV1().Endpoints(namespace).Get(context.TODO(), serviceName, metav1.GetOptions{}) + if err != nil { + framework.Logf("Failed go get Endpoints object: %v", err) + // Retry the error + return false, nil + } + portsByUID := fullPortsByPodUID(e2eendpoints.GetFullContainerPortsByPodUID(ep)) + if err := validatePortsAndProtocols(portsByUID, expectedPortsByPodUID); err != nil { + if i%5 == 0 { + framework.Logf("Unexpected endpoints: found %v, expected %v, will retry", portsByUID, expectedEndpoints) + } + return false, nil + } + + // If EndpointSlice API is enabled, then validate if appropriate EndpointSlice objects + // were also create/updated/deleted. + if _, err := c.Discovery().ServerResourcesForGroupVersion(discoveryv1.SchemeGroupVersion.String()); err == nil { + opts := metav1.ListOptions{ + LabelSelector: "kubernetes.io/service-name=" + serviceName, + } + es, err := c.DiscoveryV1().EndpointSlices(namespace).List(context.TODO(), opts) + if err != nil { + framework.Logf("Failed go list EndpointSlice objects: %v", err) + // Retry the error + return false, nil + } + portsByUID = fullPortsByPodUID(e2eendpointslice.GetFullContainerPortsByPodUID(es.Items)) + if err := validatePortsAndProtocols(portsByUID, expectedPortsByPodUID); err != nil { + if i%5 == 0 { + framework.Logf("Unexpected endpoint slices: found %v, expected %v, will retry", portsByUID, expectedEndpoints) + } + return false, nil + } + } + framework.Logf("successfully validated that service %s in namespace %s exposes endpoints %v", + serviceName, namespace, expectedEndpoints) + return true, nil + }); pollErr != nil { + if pods, err := c.CoreV1().Pods(metav1.NamespaceAll).List(context.TODO(), metav1.ListOptions{}); err == nil { + for _, pod := range pods.Items { + framework.Logf("Pod %s\t%s\t%s\t%s", pod.Namespace, pod.Name, pod.Spec.NodeName, pod.DeletionTimestamp) + } + } else { + framework.Logf("Can't list pod debug info: %v", err) + } + } + framework.ExpectNoError(pollErr, "error waithing for service %s in namespace %s to expose endpoints %v: %v", serviceName, namespace, expectedEndpoints) +} + +func translatePortsByPodNameToPortsByPodUID(c clientset.Interface, ns string, expectedEndpoints fullPortsByPodName) (fullPortsByPodUID, error) { + portsByUID := make(fullPortsByPodUID) + for name, portList := range expectedEndpoints { + pod, err := c.CoreV1().Pods(ns).Get(context.TODO(), name, metav1.GetOptions{}) + if err != nil { + return nil, fmt.Errorf("failed to get pod %s, that's pretty weird. validation failed: %s", name, err) + } + portsByUID[pod.ObjectMeta.UID] = portList + } + return portsByUID, nil +} + +func validatePortsAndProtocols(ep, expectedEndpoints fullPortsByPodUID) error { + if len(ep) != len(expectedEndpoints) { + // should not happen because we check this condition before + return fmt.Errorf("invalid number of endpoints got %v, expected %v", ep, expectedEndpoints) + } + for podUID := range expectedEndpoints { + if _, ok := ep[podUID]; !ok { + return fmt.Errorf("endpoint %v not found", podUID) + } + if len(ep[podUID]) != len(expectedEndpoints[podUID]) { + return fmt.Errorf("invalid list of ports for uid %v. Got %v, expected %v", podUID, ep[podUID], expectedEndpoints[podUID]) + } + var match bool + for _, epPort := range ep[podUID] { + match = false + for _, expectedPort := range expectedEndpoints[podUID] { + if epPort.ContainerPort == expectedPort.ContainerPort && epPort.Protocol == expectedPort.Protocol { + match = true + } + } + if !match { + return fmt.Errorf("invalid list of ports for uid %v. Got %v, expected %v", podUID, ep[podUID], expectedEndpoints[podUID]) + } + } + } + return nil +} + var _ = common.SIGDescribe("SCTP [LinuxOnly]", func() { f := framework.NewDefaultFramework("sctp") f.NamespacePodSecurityEnforceLevel = admissionapi.LevelPrivileged diff --git a/test/integration/service/loadbalancer_test.go b/test/integration/service/loadbalancer_test.go index 63ce4d73f0f..04fecc25d98 100644 --- a/test/integration/service/loadbalancer_test.go +++ b/test/integration/service/loadbalancer_test.go @@ -381,6 +381,59 @@ func Test_UpdateLoadBalancerWithLoadBalancerClass(t *testing.T) { } } +// Test_ServiceLoadBalancerMixedProtocolSetup tests that a LoadBalancer Service with different protocol values +// can be created. +func Test_ServiceLoadBalancerMixedProtocolSetup(t *testing.T) { + server := kubeapiservertesting.StartTestServerOrDie(t, nil, nil, framework.SharedEtcd()) + defer server.TearDownFn() + + client, err := clientset.NewForConfig(server.ClientConfig) + if err != nil { + t.Fatalf("Error creating clientset: %v", err) + } + + ns := framework.CreateNamespaceOrDie(client, "test-service-mixed-protocols", t) + defer framework.DeleteNamespaceOrDie(client, ns, t) + + controller, cloud, informer := newServiceController(t, client) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + informer.Start(ctx.Done()) + go controller.Run(ctx, 1, controllersmetrics.NewControllerManagerMetrics("loadbalancer-test")) + + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-123", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + Ports: []corev1.ServicePort{ + { + Name: "tcpport", + Port: int32(53), + Protocol: corev1.ProtocolTCP, + }, + { + Name: "udpport", + Port: int32(53), + Protocol: corev1.ProtocolUDP, + }, + }, + }, + } + + _, err = client.CoreV1().Services(ns.Name).Create(context.TODO(), service, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Error creating test service: %v", err) + } + + time.Sleep(5 * time.Second) // sleep 5 second to wait for the service controller reconcile + if len(cloud.Calls) == 0 { + t.Errorf("expected cloud provider calls to create load balancer") + } +} + func newServiceController(t *testing.T, client *clientset.Clientset) (*servicecontroller.Controller, *fakecloud.Cloud, informers.SharedInformerFactory) { cloud := &fakecloud.Cloud{} informerFactory := informers.NewSharedInformerFactory(client, 0)