Fix a small regression in Service updates #104601

thockin · 2021-08-26T05:43:49Z

Thanks to @liggitt for spotting this one ex post facto.

Prior to 1.22 a user could change NodePort values within a service during an update, and the apiserver would allocate values for any that were not specified.

Consider a YAML like:

apiVersion: v1
kind: Service
metadata:
  name: foo
spec:
  type: NodePort
  ports:
  - name: p
    port: 80
  - name: q
    port: 81
  selector:
    app: foo

When this is created, nodeport values will be allocated for each port. Something like:

apiVersion: v1
kind: Service
metadata:
  name: foo
spec:
  clusterIP: 10.0.149.11
  type: NodePort
  ports:
  - name: p
    nodePort: 30872
    port: 80
    protocol: TCP
    targetPort: 9376
  - name: q
    nodePort: 31310
    port: 81
    protocol: TCP
    targetPort: 81
  selector:
    app: foo

If the user PUTs (kubectl replace) the original YAML, we would see that .nodePort = 0, and allocate new ports. This was ugly at best.

In 1.22 we fixed this (#103532) to not allocate new values if we still had the old values, but instead re-assign them. Net new ports would still be seen as .nodePort = 0 and so new allocations would be made.

This broke a corner case as follows:

Prior to 1.22, the user could PUT this YAML:

apiVersion: v1
kind: Service
metadata:
  name: foo
spec:
  type: NodePort
  ports:
  - name: p
    nodePort: 31310 # note this is the `q` value
    port: 80
  - name: q
    # note this nodePort is not specified
    port: 81
  selector:
    app: foo

The p port would take the q port's value. The q port would be seen as .nodePort = 0 and a new value allocated. In 1.22 this results in an error (duplicate value in p and q).

This is VERY minor but it is an API regression, which we try to avoid, and the fix is not too horrible.

This commit adds more robust testing of this logic.

/kind bug
/kind api-change
/kind regression

Fix a small regression in 1.22 in Service updates. The circumstances are so unlikely that probably nobody would ever hit it.

xref https://github.com/kubernetes/kubernetes/pull/103532/files#r694831725

k8s-ci-robot · 2021-08-26T05:43:56Z

@thockin: 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.

k8s-triage-robot · 2021-08-26T06:19:02Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

aojea · 2021-08-26T11:08:43Z

pkg/registry/core/service/strategy.go

+
+		// Build a set of all the ports in oldSvc that are also in newSvc.  We know
+		// we can't patch these values.
+		used := nodePortsUsed(oldSvc).Intersection(nodePortsUsed(newSvc))


Jordan's example considers healthCheckNodePort too https://github.com/kubernetes/kubernetes/pull/103532/files#r694831725

If the user sets a new Nodeport value that matches the current allocated HealthCheckNodePort, should we change the HealthCheckNodePort?

the tests *swap-port-with-hcnp consider that should fail?

Yes, that has always failed, since the node port allocate step happens before the HCNP deallocate. I am fine with that continuing to fail, and honestly, letting nodeports fail isn't SO bad, but it is a breakage in 22 vs 21.

aojea · 2021-08-26T16:49:37Z

I have doubts this is a regression https://github.com/kubernetes/kubernetes/pull/103532/files#r696807794 , technically for nodeport it is, but for the user this update never worked.

quoting the comment here:

reproducer as integration test https://github.com/kubernetes/kubernetes/compare/master...aojea:svc_carry_node_ports_test?expand=1

in 1.22

/home/aojea/go/src/k8s.io/kubernetes/test/integration/service/update_test.go:81: Error updating test service: Service "test-123" is invalid: [metadata.resourceVersion: Invalid value: "": must be specified for an update, spec.ports[2].nodePort: Duplicate value: 30553]

in 1.21.2

nodeport_test.go:81: Error updating test service: Service "test-123" is invalid: [metadata.resourceVersion: Invalid value: "": must be specified for an update, spec.clusterIPs[0]: Invalid value: []string(nil): primary clusterIP can not be unset, spec.ipFamilies[0]: Invalid value: []core.IPFamily(nil): primary ipFamily can not be unset]
so, the nodeport behavior changed, no argument about that, however, the error was masked by the clusterIP.

If we can argument that there is no regression for the user (it fails one way or the other), can we go with the port conflict approach and protect users of making weird things (and remove more complex logic in the API) sweat_smile ?

liggitt · 2021-08-26T17:00:44Z

The failure based on the missing cluster IP with my example is orthogonal... a user successfully making use of Replace with services in 1.21 would have had to be setting/preserving the cluster IP somehow. I updated the example in my comment to make that clear.

I agree this is a small issue (the user steps to hit it are unlikely), but I am constantly amazed by the ... creativity of API use people do to make things work, and if we can avoid introducing a new failure, I think we should do so.

aojea · 2021-08-26T17:19:10Z

you are right , the patch was already broken but the replace worked as you are describing

$ kubectl apply -f svc_2.yml 
The Service "test" is invalid: spec.ports[2].nodePort: Duplicate value: 31289
$ kubectl replace -f svc_2.yml 
service/test replaced

pkg/registry/core/service/storage/rest_test.go

aojea · 2021-08-26T17:56:35Z

LGTM, just the tests questions
defer to @khenidak

khenidak

One comment. One question. But can merge as is.

khenidak · 2021-08-30T18:13:39Z

pkg/registry/core/service/storage/rest_test.go

@@ -622,48 +622,210 @@ func TestServiceRegistryUpdate(t *testing.T) {
 }

 func TestServiceRegistryUpdateUnspecifiedAllocations(t *testing.T) {
+	type proof func(t *testing.T, s *api.Service)
+	prove := func(proofs ...proof) []proof {
+		return proofs


Low pri

on all proves ( don't think we should fix but we should ack it). idx is never tested against len(<the thing we want to test>). This could yield into undesirable error, specially when i think these should be part of make.go

The good news is that they will panic if wrong.

I agree finding a way to re-use these will be nice - as a followup to the mega PR :)

pkg/registry/core/service/storage/rest_test.go

Prior to 1.22 a user could change NodePort values within a service during an update, and the apiserver would allocate values for any that were not specified. Consider a YAML like: ``` apiVersion: v1 kind: Service metadata: name: foo spec: type: NodePort ports: - name: p port: 80 - name: q port: 81 selector: app: foo ``` When this is created, nodeport values will be allocated for each port. Something like: ``` apiVersion: v1 kind: Service metadata: name: foo spec: clusterIP: 10.0.149.11 type: NodePort ports: - name: p nodePort: 30872 port: 80 protocol: TCP targetPort: 9376 - name: q nodePort: 31310 port: 81 protocol: TCP targetPort: 81 selector: app: foo ``` If the user PUTs (kubectl replace) the original YAML, we would see that `.nodePort = 0`, and allocate new ports. This was ugly at best. In 1.22 we fixed this to not allocate new values if we still had the old values, but instead re-assign them. Net new ports would still be seen as `.nodePort = 0` and so new allocations would be made. This broke a corner case as follows: Prior to 1.22, the user could PUT this YAML: ``` apiVersion: v1 kind: Service metadata: name: foo spec: type: NodePort ports: - name: p nodePort: 31310 # note this is the `q` value port: 80 - name: q # note this nodePort is not specified port: 81 selector: app: foo ``` The `p` port would take the `q` port's value. The `q` port would be seen as `.nodePort = 0` and a new value allocated. In 1.22 this results in an error (duplicate value in `p` and `q`). This is VERY minor but it is an API regression, which we try to avoid, and the fix is not too horrible. This commit adds more robust testing of this logic.

thockin · 2021-08-30T19:42:37Z

small comment cleanup and rebase

khenidak · 2021-08-30T19:45:25Z

/lgtm
/approve

k8s-ci-robot · 2021-08-30T19:45:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: khenidak, 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/api/OWNERS~~ [thockin]
~~pkg/registry/OWNERS~~ [thockin]

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

aojea · 2021-08-30T20:43:05Z

/retest

…532-#104601-upstream-release-1.22 Automated cherry pick of #103532: Service: Fix semantics for Update wrt allocations #104601: Fix a small regression in Service updates

…532-#104601-upstream-release-1.20 Automated cherry pick of #103532: Service: Fix semantics for Update wrt allocations #104601: Fix a small regression in Service updates

…532-#104601-upstream-release-1.21 Automated cherry pick of #103532: Service: Fix semantics for Update wrt allocations #104601: Fix a small regression in Service updates

thockin added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Aug 26, 2021

thockin assigned khenidak Aug 26, 2021

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 26, 2021

k8s-ci-robot requested review from caesarxuchao and freehan August 26, 2021 05:44

aojea reviewed Aug 26, 2021

View reviewed changes

pkg/registry/core/service/storage/rest_test.go Show resolved Hide resolved

aojea reviewed Aug 26, 2021

View reviewed changes

pkg/registry/core/service/storage/rest_test.go Outdated Show resolved Hide resolved

thockin force-pushed the patchAllocatedValues_port_reuse branch from f1099a0 to f8efea2 Compare August 26, 2021 19:35

khenidak reviewed Aug 30, 2021

View reviewed changes

thockin force-pushed the patchAllocatedValues_port_reuse branch from f8efea2 to 73503a4 Compare August 30, 2021 19:42

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

k8s-ci-robot merged commit bb9e89d into kubernetes:master Aug 30, 2021

k8s-ci-robot added this to the v1.23 milestone Aug 30, 2021

thockin deleted the patchAllocatedValues_port_reuse branch July 6, 2022 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a small regression in Service updates #104601

Fix a small regression in Service updates #104601

thockin commented Aug 26, 2021 •

edited by liggitt

k8s-ci-robot commented Aug 26, 2021

k8s-triage-robot commented Aug 26, 2021

aojea Aug 26, 2021

aojea Aug 26, 2021

thockin Aug 26, 2021

aojea commented Aug 26, 2021

liggitt commented Aug 26, 2021

aojea commented Aug 26, 2021

aojea commented Aug 26, 2021 •

edited

khenidak left a comment

khenidak Aug 30, 2021

thockin Aug 30, 2021

thockin commented Aug 30, 2021

khenidak commented Aug 30, 2021

k8s-ci-robot commented Aug 30, 2021

aojea commented Aug 30, 2021

Fix a small regression in Service updates #104601

Fix a small regression in Service updates #104601

Conversation

thockin commented Aug 26, 2021 • edited by liggitt

k8s-ci-robot commented Aug 26, 2021

k8s-triage-robot commented Aug 26, 2021

aojea Aug 26, 2021

Choose a reason for hiding this comment

aojea Aug 26, 2021

Choose a reason for hiding this comment

thockin Aug 26, 2021

Choose a reason for hiding this comment

aojea commented Aug 26, 2021

liggitt commented Aug 26, 2021

aojea commented Aug 26, 2021

aojea commented Aug 26, 2021 • edited

khenidak left a comment

Choose a reason for hiding this comment

khenidak Aug 30, 2021

Choose a reason for hiding this comment

thockin Aug 30, 2021

Choose a reason for hiding this comment

thockin commented Aug 30, 2021

khenidak commented Aug 30, 2021

k8s-ci-robot commented Aug 30, 2021

aojea commented Aug 30, 2021

thockin commented Aug 26, 2021 •

edited by liggitt

aojea commented Aug 26, 2021 •

edited