Svc REST: clean up defaultOnRead to be consistent #104986

thockin · 2021-09-14T00:12:47Z

Headless+selectorless -> RequireDualStack

Headless+selector -> SingleStack

Add test cases to cover this and ExternalName and dual-stack init (which
I think can never trigger, but best to be safe).

/kind bug

Headless Services with no selector which were created without dual-stack enabled will be defaulted to RequireDualStack instead of PreferDualStack.  This is consistent with such Services which are created with dual-stack enabled.

k8s-ci-robot · 2021-09-14T00:12:55Z

@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-ci-robot · 2021-09-14T00:13:03Z

[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/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

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

aojea · 2021-09-14T07:42:42Z

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

+				if len(service.Spec.IPFamilies) == 1 {
+					service.Spec.IPFamilyPolicy = &singleStack
+				} else if len(service.Spec.IPFamilies) == 2 {
+					// It shouldn't be possible to get here, but just in case.
+					service.Spec.IPFamilyPolicy = &requireDualStack
+				}


why is this done inside the for loop?
I mean, it was this way before, but it can be set once we build the service.Spec.IPFamilies array, no?
it depends on len(service.Spec.ClusterIPs)

good catch!!

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

aojea · 2021-09-14T07:59:22Z

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

 			} else {
+				// Headless + selector - default to single.


why this stops following cluster configuration? it will be incoherent with the generated endpoints.

And now that I see this, we are not using the IPFamilyPolicy field in the endpointslice controller :/

kubernetes/pkg/controller/endpointslice/utils.go

Lines 359 to 363 in 623f9f3

// headless

// for now we assume two families. This should have minimal side effect

// if the service is headless with no selector, then this will remain the case

// if the service is headless with selector then chances are pods are still using single family

// since kubelet will need to restart in order to start patching pod status with multiple ips

If this is a service that doesn't have ipFamilies set, it MUST have been created while the cluster was in single-stack mode. We decided that ipFamilyPolicy was authoritative, and the user never set that.

If we make it follow the cluster, then such a service will auto-upgrade and we decided we never wanted that, right?

This is the main bug in this PR, I think.

aojea · 2021-09-14T08:06:44Z

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

+					service.Spec.IPFamilies[idx] = api.IPv4Protocol
+				}
+				if len(service.Spec.IPFamilies) == 1 {
+					service.Spec.IPFamilyPolicy = &singleStack


what if this was already set?

I don't know how IPFamilyPolicy would be set but ipFamilies would not?

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

thockin

I have pushed with all fixes except the "has policy, but not ipfamilies" ones. We could code those but I don't see how they can ever trigger?

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

thockin · 2021-09-14T15:35:22Z

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

 			} else {
+				// Headless + selector - default to single.


If this is a service that doesn't have ipFamilies set, it MUST have been created while the cluster was in single-stack mode. We decided that ipFamilyPolicy was authoritative, and the user never set that.

If we make it follow the cluster, then such a service will auto-upgrade and we decided we never wanted that, right?

This is the main bug in this PR, I think.

thockin · 2021-09-14T15:36:10Z

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

+					service.Spec.IPFamilies[idx] = api.IPv4Protocol
+				}
+				if len(service.Spec.IPFamilies) == 1 {
+					service.Spec.IPFamilyPolicy = &singleStack


I don't know how IPFamilyPolicy would be set but ipFamilies would not?

thockin · 2021-09-14T15:36:45Z

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

+				if len(service.Spec.IPFamilies) == 1 {
+					service.Spec.IPFamilyPolicy = &singleStack
+				} else if len(service.Spec.IPFamilies) == 2 {
+					// It shouldn't be possible to get here, but just in case.
+					service.Spec.IPFamilyPolicy = &requireDualStack
+				}


good catch!!

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

aojea · 2021-09-14T21:09:15Z

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

+		input: svctest.MakeService("foo", svctest.SetClusterIP("10.0.0.1")),
+		expect: svctest.MakeService("foo", svctest.SetClusterIPs("10.0.0.1"),
+			svctest.SetIPFamilyPolicy(api.IPFamilyPolicySingleStack),
+			svctest.SetIPFamilies(api.IPv4Protocol)),


this is a copy paste error, right? it is the same test that the below test "no change v4"

This one sets clusterIP (singlular) but not plural.

wow, so subtle, I swear I starred at the tests for 5 mins before commenting 🙃

aojea · 2021-09-14T21:10:57Z

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

+		name:  "missing clusterIPs v6",
+		input: svctest.MakeService("foo", svctest.SetClusterIP("2000::1")),
+		expect: svctest.MakeService("foo", svctest.SetClusterIPs("2000::1"),
+			svctest.SetIPFamilyPolicy(api.IPFamilyPolicySingleStack),
+			svctest.SetIPFamilies(api.IPv6Protocol)),


same issue as https://github.com/kubernetes/kubernetes/pull/104986/files#r708634421?

same answer

Headless+selectorless -> RequireDualStack Headless+selector -> SingleStack Add test cases to cover this and ExternalName and dual-stack init (which I think can never trigger, but best to be safe).

thockin

Rebased and squashed, too.

thockin · 2021-09-16T18:13:27Z

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

+		input: svctest.MakeService("foo", svctest.SetClusterIP("10.0.0.1")),
+		expect: svctest.MakeService("foo", svctest.SetClusterIPs("10.0.0.1"),
+			svctest.SetIPFamilyPolicy(api.IPFamilyPolicySingleStack),
+			svctest.SetIPFamilies(api.IPv4Protocol)),


This one sets clusterIP (singlular) but not plural.

thockin · 2021-09-16T18:16:46Z

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

+		name:  "missing clusterIPs v6",
+		input: svctest.MakeService("foo", svctest.SetClusterIP("2000::1")),
+		expect: svctest.MakeService("foo", svctest.SetClusterIPs("2000::1"),
+			svctest.SetIPFamilyPolicy(api.IPFamilyPolicySingleStack),
+			svctest.SetIPFamilies(api.IPv6Protocol)),


same answer

aojea · 2021-09-16T20:01:38Z

/lgtm

thockin assigned khenidak Sep 14, 2021

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Sep 14, 2021

k8s-ci-robot requested review from hongchaodeng and justinsb September 14, 2021 00:13

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2021

khenidak reviewed Sep 14, 2021

View reviewed changes

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

aojea reviewed Sep 14, 2021

View reviewed changes

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

aojea reviewed Sep 14, 2021

View reviewed changes

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

thockin force-pushed the svc-validation-cleanup branch from 204af75 to 7639ab1 Compare September 14, 2021 15:52

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2021

thockin commented Sep 14, 2021

View reviewed changes

aojea reviewed Sep 14, 2021

View reviewed changes

Svc REST: clean up defaultOnRead to be consistent

52f54ce

Headless+selectorless -> RequireDualStack Headless+selector -> SingleStack Add test cases to cover this and ExternalName and dual-stack init (which I think can never trigger, but best to be safe).

thockin force-pushed the svc-validation-cleanup branch from 7639ab1 to 52f54ce Compare September 16, 2021 18:18

thockin commented Sep 16, 2021

View reviewed changes

k8s-ci-robot assigned aojea Sep 16, 2021

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

k8s-ci-robot merged commit 924f196 into kubernetes:master Sep 16, 2021

k8s-ci-robot added this to the v1.23 milestone Sep 16, 2021

thockin deleted the svc-validation-cleanup branch July 6, 2022 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Svc REST: clean up defaultOnRead to be consistent #104986

Svc REST: clean up defaultOnRead to be consistent #104986

thockin commented Sep 14, 2021

k8s-ci-robot commented Sep 14, 2021

k8s-ci-robot commented Sep 14, 2021

aojea Sep 14, 2021 •

edited

thockin Sep 14, 2021

aojea Sep 14, 2021

thockin Sep 14, 2021

aojea Sep 14, 2021

thockin Sep 14, 2021

thockin left a comment

thockin Sep 14, 2021

thockin Sep 14, 2021

thockin Sep 14, 2021

aojea Sep 14, 2021

thockin Sep 16, 2021

aojea Sep 16, 2021

aojea Sep 14, 2021

thockin Sep 16, 2021

thockin left a comment

thockin Sep 16, 2021

thockin Sep 16, 2021

aojea commented Sep 16, 2021

	// headless
	// for now we assume two families. This should have minimal side effect
	// if the service is headless with no selector, then this will remain the case
	// if the service is headless with selector then chances are pods are still using single family
	// since kubelet will need to restart in order to start patching pod status with multiple ips

Svc REST: clean up defaultOnRead to be consistent #104986

Svc REST: clean up defaultOnRead to be consistent #104986

Conversation

thockin commented Sep 14, 2021

k8s-ci-robot commented Sep 14, 2021

k8s-ci-robot commented Sep 14, 2021

aojea Sep 14, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin left a comment

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

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

Choose a reason for hiding this comment

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Sep 16, 2021

aojea Sep 14, 2021 •

edited