From a930892769177096c71da5f1cd622ffd9bd1dc66 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sun, 6 Aug 2023 16:53:10 -0700 Subject: [PATCH] Gate: disallow .status.loadBalancer on non-LB svc The fact that the .status.loadBalancer field can be set while .spec.type is not "LoadBalancer" is a flub. Any spec update will already clear .status.ingress, so it's hard to really rely on this. After this change, updates which try to set this combination will fail validation. Existing cases of this will not be broken. Any spec/metadata update will clear it (no error) and this is the only stanza of status. New gate "AllowServiceLBStatusOnNonLB" is off by default, but can be enabled if this change actually breaks someone, which seems exceeedingly unlikely. --- pkg/apis/core/validation/validation.go | 49 ++++++++++--------- pkg/apis/core/validation/validation_test.go | 38 ++++++++++++-- pkg/features/kube_features.go | 9 ++++ pkg/registry/core/service/strategy_test.go | 5 +- .../apiserver/apply/reset_fields_test.go | 2 +- test/integration/etcd/data.go | 2 +- 6 files changed, 75 insertions(+), 30 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 634b1b45bdb..29c92c41fbe 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5441,7 +5441,7 @@ func ValidateServiceUpdate(service, oldService *core.Service) field.ErrorList { // ValidateServiceStatusUpdate tests if required fields in the Service are set when updating status. func ValidateServiceStatusUpdate(service, oldService *core.Service) field.ErrorList { allErrs := ValidateObjectMetaUpdate(&service.ObjectMeta, &oldService.ObjectMeta, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateLoadBalancerStatus(&service.Status.LoadBalancer, field.NewPath("status", "loadBalancer"))...) + allErrs = append(allErrs, ValidateLoadBalancerStatus(&service.Status.LoadBalancer, field.NewPath("status", "loadBalancer"), &service.Spec)...) return allErrs } @@ -7053,32 +7053,37 @@ var ( ) // ValidateLoadBalancerStatus validates required fields on a LoadBalancerStatus -func ValidateLoadBalancerStatus(status *core.LoadBalancerStatus, fldPath *field.Path) field.ErrorList { +func ValidateLoadBalancerStatus(status *core.LoadBalancerStatus, fldPath *field.Path, spec *core.ServiceSpec) field.ErrorList { allErrs := field.ErrorList{} - for i, ingress := range status.Ingress { - idxPath := fldPath.Child("ingress").Index(i) - if len(ingress.IP) > 0 { - if isIP := (netutils.ParseIPSloppy(ingress.IP) != nil); !isIP { - allErrs = append(allErrs, field.Invalid(idxPath.Child("ip"), ingress.IP, "must be a valid IP address")) - } - } - - if utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) && ingress.IPMode == nil { + ingrPath := fldPath.Child("ingress") + if !utilfeature.DefaultFeatureGate.Enabled(features.AllowServiceLBStatusOnNonLB) && spec.Type != core.ServiceTypeLoadBalancer && len(status.Ingress) != 0 { + allErrs = append(allErrs, field.Forbidden(ingrPath, "may only be used when `spec.type` is 'LoadBalancer'")) + } else { + for i, ingress := range status.Ingress { + idxPath := ingrPath.Index(i) if len(ingress.IP) > 0 { - allErrs = append(allErrs, field.Required(idxPath.Child("ipMode"), "must be specified when `ip` is set")) + if isIP := (netutils.ParseIPSloppy(ingress.IP) != nil); !isIP { + allErrs = append(allErrs, field.Invalid(idxPath.Child("ip"), ingress.IP, "must be a valid IP address")) + } } - } else if ingress.IPMode != nil && len(ingress.IP) == 0 { - allErrs = append(allErrs, field.Forbidden(idxPath.Child("ipMode"), "may not be specified when `ip` is not set")) - } else if ingress.IPMode != nil && !supportedLoadBalancerIPMode.Has(string(*ingress.IPMode)) { - allErrs = append(allErrs, field.NotSupported(idxPath.Child("ipMode"), ingress.IPMode, supportedLoadBalancerIPMode.List())) - } - if len(ingress.Hostname) > 0 { - for _, msg := range validation.IsDNS1123Subdomain(ingress.Hostname) { - allErrs = append(allErrs, field.Invalid(idxPath.Child("hostname"), ingress.Hostname, msg)) + if utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) && ingress.IPMode == nil { + if len(ingress.IP) > 0 { + allErrs = append(allErrs, field.Required(idxPath.Child("ipMode"), "must be specified when `ip` is set")) + } + } else if ingress.IPMode != nil && len(ingress.IP) == 0 { + allErrs = append(allErrs, field.Forbidden(idxPath.Child("ipMode"), "may not be specified when `ip` is not set")) + } else if ingress.IPMode != nil && !supportedLoadBalancerIPMode.Has(string(*ingress.IPMode)) { + allErrs = append(allErrs, field.NotSupported(idxPath.Child("ipMode"), ingress.IPMode, supportedLoadBalancerIPMode.List())) } - if isIP := (netutils.ParseIPSloppy(ingress.Hostname) != nil); isIP { - allErrs = append(allErrs, field.Invalid(idxPath.Child("hostname"), ingress.Hostname, "must be a DNS name, not an IP address")) + + if len(ingress.Hostname) > 0 { + for _, msg := range validation.IsDNS1123Subdomain(ingress.Hostname) { + allErrs = append(allErrs, field.Invalid(idxPath.Child("hostname"), ingress.Hostname, msg)) + } + if isIP := (netutils.ParseIPSloppy(ingress.Hostname) != nil); isIP { + allErrs = append(allErrs, field.Invalid(idxPath.Child("hostname"), ingress.Hostname, "must be a DNS name, not an IP address")) + } } } } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 55a83faa8f2..dac9a4ba88b 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -23377,11 +23377,36 @@ func TestValidateLoadBalancerStatus(t *testing.T) { testCases := []struct { name string ipModeEnabled bool + nonLBAllowed bool tweakLBStatus func(s *core.LoadBalancerStatus) + tweakSvcSpec func(s *core.ServiceSpec) numErrs int }{ - /* LoadBalancerIPMode*/ { + name: "type is not LB", + nonLBAllowed: false, + tweakSvcSpec: func(s *core.ServiceSpec) { + s.Type = core.ServiceTypeClusterIP + }, + tweakLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.4", + }} + }, + numErrs: 1, + }, { + name: "type is not LB. back-compat", + nonLBAllowed: true, + tweakSvcSpec: func(s *core.ServiceSpec) { + s.Type = core.ServiceTypeClusterIP + }, + tweakLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.4", + }} + }, + numErrs: 0, + }, { name: "valid vip ipMode", ipModeEnabled: true, tweakLBStatus: func(s *core.LoadBalancerStatus) { @@ -23443,9 +23468,14 @@ func TestValidateLoadBalancerStatus(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.LoadBalancerIPMode, tc.ipModeEnabled)() - s := core.LoadBalancerStatus{} - tc.tweakLBStatus(&s) - errs := ValidateLoadBalancerStatus(&s, field.NewPath("status")) + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AllowServiceLBStatusOnNonLB, tc.nonLBAllowed)() + status := core.LoadBalancerStatus{} + tc.tweakLBStatus(&status) + spec := core.ServiceSpec{Type: core.ServiceTypeLoadBalancer} + if tc.tweakSvcSpec != nil { + tc.tweakSvcSpec(&spec) + } + errs := ValidateLoadBalancerStatus(&status, field.NewPath("status"), &spec) if len(errs) != tc.numErrs { t.Errorf("Unexpected error list for case %q(expected:%v got %v) - Errors:\n %v", tc.name, tc.numErrs, len(errs), errs.ToAggregate()) } diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 9b4d183f634..09f4f386526 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -44,6 +44,13 @@ const ( // Enable usage of Provision of PVCs from snapshots in other namespaces CrossNamespaceVolumeDataSource featuregate.Feature = "CrossNamespaceVolumeDataSource" + // owner: @thockin + // deprecated: v1.29 + // + // Enables Service.status.ingress.loadBanace to be set on + // services of types other than LoadBalancer. + AllowServiceLBStatusOnNonLB featuregate.Feature = "AllowServiceLBStatusOnNonLB" + // owner: @bswartz // alpha: v1.18 // beta: v1.24 @@ -955,6 +962,8 @@ func init() { var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ CrossNamespaceVolumeDataSource: {Default: false, PreRelease: featuregate.Alpha}, + AllowServiceLBStatusOnNonLB: {Default: false, PreRelease: featuregate.Deprecated}, // remove after 1.29 + AnyVolumeDataSource: {Default: true, PreRelease: featuregate.Beta}, // on by default in 1.24 APISelfSubjectReview: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // GA in 1.28; remove in 1.30 diff --git a/pkg/registry/core/service/strategy_test.go b/pkg/registry/core/service/strategy_test.go index 3a35b5860ba..40f4262f5be 100644 --- a/pkg/registry/core/service/strategy_test.go +++ b/pkg/registry/core/service/strategy_test.go @@ -107,9 +107,10 @@ func TestServiceStatusStrategy(t *testing.T) { t.Errorf("Service must be namespace scoped") } oldService := makeValidService() - newService := makeValidService() + oldService.Spec.Type = api.ServiceTypeLoadBalancer oldService.ResourceVersion = "4" - newService.ResourceVersion = "4" + oldService.Spec.SessionAffinity = "None" + newService := oldService.DeepCopy() newService.Spec.SessionAffinity = "ClientIP" newService.Status = api.ServiceStatus{ LoadBalancer: api.LoadBalancerStatus{ diff --git a/test/integration/apiserver/apply/reset_fields_test.go b/test/integration/apiserver/apply/reset_fields_test.go index 972acc192db..3869d99489c 100644 --- a/test/integration/apiserver/apply/reset_fields_test.go +++ b/test/integration/apiserver/apply/reset_fields_test.go @@ -107,7 +107,7 @@ var resetFieldsSpecData = map[schema.GroupVersionResource]string{ gvr("", "v1", "pods"): `{"metadata": {"deletionTimestamp": "2020-01-01T00:00:00Z", "ownerReferences":[]}, "spec": {"containers": [{"image": "` + image2 + `", "name": "container7"}]}}`, gvr("", "v1", "replicationcontrollers"): `{"spec": {"selector": {"new": "stuff2"}}}`, gvr("", "v1", "resourcequotas"): `{"spec": {"hard": {"cpu": "25M"}}}`, - gvr("", "v1", "services"): `{"spec": {"externalName": "service2name"}}`, + gvr("", "v1", "services"): `{"spec": {"type": "ClusterIP"}}`, gvr("apps", "v1", "daemonsets"): `{"spec": {"template": {"spec": {"containers": [{"image": "` + image2 + `", "name": "container6"}]}}}}`, gvr("apps", "v1", "deployments"): `{"metadata": {"labels": {"a":"c"}}, "spec": {"template": {"spec": {"containers": [{"image": "` + image2 + `", "name": "container6"}]}}}}`, gvr("apps", "v1", "replicasets"): `{"spec": {"template": {"spec": {"containers": [{"image": "` + image2 + `", "name": "container4"}]}}}}`, diff --git a/test/integration/etcd/data.go b/test/integration/etcd/data.go index 0e95c66de34..b0ba2f49787 100644 --- a/test/integration/etcd/data.go +++ b/test/integration/etcd/data.go @@ -45,7 +45,7 @@ func GetEtcdStorageDataForNamespace(namespace string) map[schema.GroupVersionRes ExpectedEtcdPath: "/registry/configmaps/" + namespace + "/cm1", }, gvr("", "v1", "services"): { - Stub: `{"metadata": {"name": "service1"}, "spec": {"externalName": "service1name", "ports": [{"port": 10000, "targetPort": 11000}], "selector": {"test": "data"}}}`, + Stub: `{"metadata": {"name": "service1"}, "spec": {"type": "LoadBalancer", "ports": [{"port": 10000, "targetPort": 11000}], "selector": {"test": "data"}}}`, ExpectedEtcdPath: "/registry/services/specs/" + namespace + "/service1", }, gvr("", "v1", "podtemplates"): {