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

thockin · 2023-08-07T00:06:42Z

Fixes #119700

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.

/kind bug

Setting the `status.loadBalancer` of a Service whose `spec.type` is not `"LoadBalancer"` was previously allowed, but any update to the `metadata` or `spec` would wipe that field. Setting this field is no longer permitted unless `spec.type` is  `"LoadBalancer"`.  In the very unlikely event that this has unexpected impact, you can enable the `AllowServiceLBStatusOnNonLB` feature gate, which will restore the previous behavior.  If you do need to set this, please file an issue with the Kubernetes project to help contributors understand why you need it.

k8s-ci-robot · 2023-08-07T00:06:45Z

Please note that we're already in Test Freeze for the release-1.28 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.28.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Sun Aug 6 22:30:55 UTC 2023.

k8s-ci-robot · 2023-08-07T00:06:49Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

thockin · 2023-08-07T00:08:02Z

I'd like to make an argument for this to go into 1.28, but if it can't make the .0 then we should wait until 29 (and fix comments in this commit).

@kubernetes/release-managers

pacoxu · 2023-08-07T05:39:01Z

Unit test failed in TestServiceStatusStrategy for this change,
Othere2e failures can be fixed after #119454 is merged.

sftim · 2023-08-08T13:47:51Z

Changelog suggestion

-Setting the `status.loadBalancer` of a Service whose `spec.type` is not "LoadBalancer" was previously allowed, but any update to the `metadata` or `spec` would wipe that field.  Setting this field is no longer permitted unless `spec.type` is  "LoadBalancer".  In the very unlikely event that this has unexpected impact, the "AllowServiceLBStatusOnNonLB" feature gate may be set to true, which will restore the previous behavior.  If you need to set this, please file an issue with the Kubernetes project to help us understand why.
+Setting the `status.loadBalancer` of a Service whose `spec.type` is not `"LoadBalancer"` was previously allowed, but any update to the `metadata` or `spec` would wipe that field.
+Setting this field is no longer permitted unless `spec.type` is  `"LoadBalancer"`.  In the very unlikely event that this has unexpected impact, you can enable the `AllowServiceLBStatusOnNonLB` feature gate, which will restore the previous behavior.
+If you need to set this, please file an issue with the Kubernetes project to help contributors understand why you need it.

aojea · 2023-08-08T18:32:12Z

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)...)


why not check directly here?

Suggested change

allErrs = append(allErrs, ValidateLoadBalancerStatus(&service.Status.LoadBalancer, field.NewPath("status", "loadBalancer"), &service.Spec)...)

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 {

allErrs = append(allErrs, ValidateLoadBalancerStatus(&service.Status.LoadBalancer, field.NewPath("status", "loadBalancer"))...)

}

it felt more contained in the status-specific path. Other than avoiding one argument, is there a reason I am not seeing?

I was trying to avoid to plumb the spec on the status path

my point is that if we make service.Spec an argument of ValidateLoadBalancerStatus we may open a door to all kind of validations there , in this case have ValidateLoadBalancerStatus that is can be called from any serviceType, I think that this is our problem, we allow it from any service type.

with my suggesting once we remove the feature gate the code seems easy to read, as ValidateLoadBalancerStatus only will validate the status for types LoadBalancer that is what you expect from the name

pkg/features/kube_features.go

thockin · 2023-08-17T21:37:53Z

Rebased. Unit test updated.

thockin · 2023-08-18T22:58:25Z

I fixed the integration test, I think. The rest need the e2e fix in #119454

aojea · 2023-08-19T11:37:25Z

failed [FAILED] Could not patch service status%!(EXTRA *errors.StatusError=Service "test-service-7pblz" is invalid: status.loadBalancer.ingress: Forbidden: may only be used when spec.type is 'LoadBalancer'): Service "test-service-7pblz" is invalid: status.loadBalancer.ingress: Forbidden: may only be used when spec.type is 'LoadBalancer'
In [It] at: test/e2e/network/service.go:3406 @ 08/18/23 23:19:51.638

proof this fixes the problem

/retest

aojea · 2023-08-19T11:40:24Z

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


why this change, why do you have WIP in the commit?

There are a couple tests which interate all of the API types looking for /status subresources. There's a table of inputs for spec and another table for status, which it tries to apply to /status. This test tried to apply status.loadBalancer.ingress while the spec is type: ClusterIP. That fails now.

"WIP" in comment because I was not 100% confident in it.

thockin · 2023-08-19T19:35:34Z

I don't understand the remaining integration fail so I am rebasing and will see if it pops up again.

thockin · 2023-08-19T20:23:49Z

I think I got it now. These tests are WAY too brittle.

k8s-ci-robot · 2023-08-19T20:24:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/apis/OWNERS~~ [thockin]
~~pkg/features/OWNERS~~ [thockin]
~~pkg/registry/core/service/OWNERS~~ [thockin]
~~test/integration/apiserver/OWNERS~~ [thockin]
~~test/integration/etcd/OWNERS~~ [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

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.

thockin · 2023-08-21T15:31:32Z

/retest

thockin · 2023-08-21T22:38:30Z

is green now!

aojea · 2023-08-22T08:21:24Z

pkg/apis/core/validation/validation.go

+ 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"))


technically I think an IPv4 is a valid dns name, no?

git blame says this was this way since forever so 🤷

I think DNS says a hostname which is all numeric is not valid, precisely because it is ambiguous.

I do not why I ask this things, now I had to spend some time going through the RFCs just to know the truth 😄

I found this in https://www.ietf.org/rfc/rfc1123.txt

2. GENERAL ISSUES This section contains general requirements that may be applicable to all application-layer protocols. 2.1 Host Names and Numbers The syntax of a legal Internet host name was specified in RFC-952 [DNS:4]. One aspect of host name syntax is hereby changed: the restriction on the first character is relaxed to allow either a letter or a digit. Host software MUST support this more liberal syntax. Host software MUST handle host names of up to 63 characters and SHOULD handle host names of up to 255 characters. Whenever a user inputs the identity of an Internet host, it SHOULD be possible to enter either (1) a host domain name or (2) an IP address in dotted-decimal ("#.#.#.#") form. The host SHOULD check the string syntactically for a dotted-decimal number before looking it up in the Domain Name System. DISCUSSION: This last requirement is not intended to specify the complete syntactic form for entering a dotted-decimal host number; that is considered to be a user-interface issue. For example, a dotted-decimal number must be enclosed within "[ ]" brackets for SMTP mail (see Section 5.2.17). This notation could be made universal within a host system, simplifying the syntactic checking for a dotted-decimal number. If a dotted-decimal number can be entered without such identifying delimiters, then a full syntactic check must be made, because a segment of a host domain name is now allowed to begin with a digit and could legally be entirely numeric (see Section 6.1.2.4). However, a valid host name can never have the dotted-decimal form #.#.#.#, since at least the highest-level component label will be alphabetic.

aojea · 2023-08-22T08:26:55Z

3 greens on the last runs of integration job

https://prow.k8s.io/pr-history/?org=kubernetes&repo=kubernetes&pr=119789

/lgtm

k8s-ci-robot · 2023-08-22T08:27:01Z

LGTM label has been added.

Git tree hash: a997b249e6903a4f165466df50fbfba880c22373

thockin · 2023-08-22T15:48:35Z

/retest

k8s-ci-robot · 2023-08-22T16:47:31Z

@thockin: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-node-e2e-containerd	`a930892`	link	true	`/test pull-kubernetes-node-e2e-containerd`
pull-kubernetes-unit	`a930892`	link	unknown	`/test pull-kubernetes-unit`
pull-kubernetes-integration	`a930892`	link	unknown	`/test pull-kubernetes-integration`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

thockin · 2023-08-22T16:54:10Z

/retest

thockin assigned aojea Aug 7, 2023

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 7, 2023

k8s-ci-robot requested review from dchen1107 and smarterclayton August 7, 2023 00:06

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 7, 2023

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 7, 2023

aojea reviewed Aug 8, 2023

View reviewed changes

pkg/features/kube_features.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2023

thockin force-pushed the deprecate_svc_lb_ingress_with_clusterip branch from 43af7ca to 0c9c6d8 Compare August 17, 2023 21:37

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 17, 2023

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Aug 18, 2023

aojea reviewed Aug 19, 2023

View reviewed changes

thockin force-pushed the deprecate_svc_lb_ingress_with_clusterip branch 2 times, most recently from 9782e72 to 962495f Compare August 19, 2023 20:23

thockin force-pushed the deprecate_svc_lb_ingress_with_clusterip branch from 62267a0 to a930892 Compare August 20, 2023 23:41

aojea reviewed Aug 22, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2023

k8s-ci-robot merged commit c0691f3 into kubernetes:master Aug 22, 2023
11 of 14 checks passed

k8s-ci-robot added this to the v1.29 milestone Aug 22, 2023

HirazawaUi mentioned this pull request Mar 9, 2024

[kubelet]: Fix the bug where pod grace period will be overwritten #122890

Open

thockin deleted the deprecate_svc_lb_ingress_with_clusterip branch March 10, 2024 01:03

HirazawaUi mentioned this pull request Apr 4, 2024

Fix unexpected change of container ready state when kubelet restart #123982

Open

aojea mentioned this pull request Apr 19, 2024

Guidance for skipping Alpha for feature gated changes kubernetes/community#7828

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

thockin commented Aug 7, 2023 •

edited

k8s-ci-robot commented Aug 7, 2023

k8s-ci-robot commented Aug 7, 2023

thockin commented Aug 7, 2023

pacoxu commented Aug 7, 2023 •

edited

sftim commented Aug 8, 2023

aojea Aug 8, 2023 •

edited

thockin Aug 17, 2023

aojea Aug 19, 2023

aojea Aug 19, 2023

thockin commented Aug 17, 2023

thockin commented Aug 18, 2023 •

edited

aojea commented Aug 19, 2023

aojea Aug 19, 2023

thockin Aug 19, 2023

thockin commented Aug 19, 2023

thockin commented Aug 19, 2023

k8s-ci-robot commented Aug 19, 2023

thockin commented Aug 21, 2023

thockin commented Aug 21, 2023

aojea Aug 22, 2023 •

edited

aojea Aug 22, 2023

thockin Aug 22, 2023

aojea Aug 22, 2023

aojea commented Aug 22, 2023

k8s-ci-robot commented Aug 22, 2023

thockin commented Aug 22, 2023

k8s-ci-robot commented Aug 22, 2023

thockin commented Aug 22, 2023

- allErrs = append(allErrs, ValidateLoadBalancerStatus(&service.Status.LoadBalancer, field.NewPath("status", "loadBalancer"), &service.Spec)...)
+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 {
+ allErrs = append(allErrs, ValidateLoadBalancerStatus(&service.Status.LoadBalancer, field.NewPath("status", "loadBalancer"))...)
+ }

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

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

Conversation

thockin commented Aug 7, 2023 • edited

k8s-ci-robot commented Aug 7, 2023

k8s-ci-robot commented Aug 7, 2023

thockin commented Aug 7, 2023

pacoxu commented Aug 7, 2023 • edited

sftim commented Aug 8, 2023

aojea Aug 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Aug 17, 2023

thockin commented Aug 18, 2023 • edited

aojea commented Aug 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Aug 19, 2023

thockin commented Aug 19, 2023

k8s-ci-robot commented Aug 19, 2023

thockin commented Aug 21, 2023

thockin commented Aug 21, 2023

aojea Aug 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Aug 22, 2023

k8s-ci-robot commented Aug 22, 2023

thockin commented Aug 22, 2023

k8s-ci-robot commented Aug 22, 2023

thockin commented Aug 22, 2023

thockin commented Aug 7, 2023 •

edited

pacoxu commented Aug 7, 2023 •

edited

aojea Aug 8, 2023 •

edited

thockin commented Aug 18, 2023 •

edited

aojea Aug 22, 2023 •

edited