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 cc366cb57fa..262db4b4c4c 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"): {