github/kubernetes
1
0
Fork 0
mirror of https://github.com/k3s-io/kubernetes.git synced 2025-08-10 20:42:26 +00:00
Code Issues Projects Releases Wiki Activity

Gate: disallow .status.loadBalancer on non-LB svc

Browse Source 
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.
This commit is contained in:
Tim Hockin 2023-08-06 16:53:10 -07:00
parent 2979242743
commit a930892769
No known key found for this signature in database
6 changed files with 75 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

49
pkg/apis/core/validation/validation.go
View File

@ -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"))
}
}
}
}

38
pkg/apis/core/validation/validation_test.go
View File

@ -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())
}

9
pkg/features/kube_features.go
View File

@ -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

5
pkg/registry/core/service/strategy_test.go
View File

@ -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{

2
test/integration/apiserver/apply/reset_fields_test.go
View File

@ -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"}]}}}}`,

2
test/integration/etcd/data.go
View File

@ -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"): {