Two small bugs in dual-stack init #99555

thockin · 2021-02-28T19:27:21Z

Working on the REST overhaul, adding testing turned these up:

Headless+Selectorless, on a single-stack cluster, policy=PreferDual

Prior to this commit, the result was a single IPFamiliy (because we
checked that the 2nd allocator was present). This changes that case to
populate both families (we don't care if the allocator exists), which is
the same as RequireDual.

ClusterIP, user specifies 2 families but no IPs

Prior to this commit, the policy was inferred to be SingleStack. This
changes that case to correctly default to RequireDual when 2 families
are present but no IPs.

On single-stack configured (IPv4 or IPv6, but not both) clusters, Services which are both headless (no clusterIP) and selectorless (empty or undefined selector) will report `ipFamilyPolicy RequireDualStack` and will have entries in `ipFamilies[]` for both IPv4 and IPv6.  This is a change from alpha, but does not have any impact on the manually-specified Endpoints and EndpointSlices for the Service.

k8s-ci-robot · 2021-02-28T19:27:27Z

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

aojea · 2021-03-02T11:18:07Z

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

@@ -3892,7 +3892,7 @@ func TestDefaultingValidation(t *testing.T) {
 				},
 			},
 			expectedIPFamilyPolicy: &preferDualStack,
-			expectedIPFamilies:     []api.IPFamily{api.IPv4Protocol},
+			expectedIPFamilies:     []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},


why if you "prefer" (not "require") and the cluster is single stack we have 2 ip families?

Headless, selector-less semantics.

Basically, this is a corner case we can't comprehend from Service alone. It could already be holding dual-stack endpoints (I know some users were doing this). So we acknowledge that and admit that this Service could have both. "require" vs "prefer" was what we specced - it felt more explicit.

aojea · 2021-03-02T11:20:20Z

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


+	// Special-case: headless+selectorless
+	if len(service.Spec.ClusterIPs) > 0 && service.Spec.ClusterIPs[0] == api.ClusterIPNone && len(service.Spec.Selector) == 0 {
+		// If the use said nothing about policy and we can't infer it, they get dual-stack


is this not against the least surprise principle?
previous behaviour was the same as before dualstack

a case could be made that it has zero effect on user. As a matter of fact users already use that to create dual stack endpoints for non dualstack clusters today (before dual stack became a thing on kubernetes)

Yeah, we don't enforce that they can't create Endpoints of any family. At this case we just need to pick a value for the policy. RequireDual was what we wrote in the KEP.

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

khenidak · 2021-03-02T18:10:35Z

nit: Please add release notes. Behavior is changing on single stack clusters.

re second part of the PR (2 ClusterIPs => RequireDualStack) -- we don't need to do that since it is already set in defaulting

kubernetes/pkg/apis/core/v1/defaults.go

Lines 139 to 142 in e4e9c31

    
           if len(obj.Spec.ClusterIPs) == 2 || len(obj.Spec.IPFamilies) == 2 { 
        
           	requireDualStack := v1.IPFamilyPolicyRequireDualStack 
        
           	obj.Spec.IPFamilyPolicy = &requireDualStack 
        
           }

.

thockin

Stray comments removed.

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

thockin · 2021-03-02T23:53:08Z

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


+	// Special-case: headless+selectorless
+	if len(service.Spec.ClusterIPs) > 0 && service.Spec.ClusterIPs[0] == api.ClusterIPNone && len(service.Spec.Selector) == 0 {
+		// If the use said nothing about policy and we can't infer it, they get dual-stack


Yeah, we don't enforce that they can't create Endpoints of any family. At this case we just need to pick a value for the policy. RequireDual was what we wrote in the KEP.

thockin · 2021-03-02T23:55:23Z

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

@@ -3892,7 +3892,7 @@ func TestDefaultingValidation(t *testing.T) {
 				},
 			},
 			expectedIPFamilyPolicy: &preferDualStack,
-			expectedIPFamilies:     []api.IPFamily{api.IPv4Protocol},
+			expectedIPFamilies:     []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},


Basically, this is a corner case we can't comprehend from Service alone. It could already be holding dual-stack endpoints (I know some users were doing this). So we acknowledge that and admit that this Service could have both. "require" vs "prefer" was what we specced - it felt more explicit.

thockin · 2021-03-03T00:09:54Z

we don't need to do that since it is already set in defaulting

That leaves a hole in the REST tests I am adding in the larger rework effort. Maybe it makes sense to just do it all here and remove that defaulting?

thockin · 2021-03-03T05:12:48Z

I added a second commit which removes the code in defaults.go - it is all handled in REST anyway.

pacoxu · 2021-03-03T05:24:32Z

/test pull-kubernetes-dependencies
for go build github.com/aws/aws-sdk-go/service/ec2: /usr/local/go/pkg/tool/linux_amd64/compile: signal: killed (oom?)

aojea · 2021-03-03T09:12:38Z

LGTM
defer to @khenidak

thockin · 2021-03-03T16:31:04Z

/retest

khenidak · 2021-03-03T17:03:18Z

/lgtm
/approve

Imporved testing turned these up: 1) Headless+Selectorless, on a single-stack cluster, policy=PreferDual Prior to this commit, the result was a single IPFamiliy (because we checked that the 2nd allocator was present). This changes that case to populate both families (we don't care if the allocator exists), which is the same as RequireDual. 2) ClusterIP, user specifies 2 families but no IPs Prior to this commit, the policy was inferred to be SingleStack. This changes that case to correctly default to RequireDual when 2 families are present but no IPs.

These same values are set in the REST stack and it's easier to maintain that code all in one place. defaults.go is best for static values.

thockin · 2021-03-03T17:49:15Z

It looks like the go 1.15 is being used in the test? When I run these locally they pass..

thockin · 2021-03-03T19:52:39Z

Rebased

khenidak · 2021-03-03T20:00:42Z

/lgtm
/approve
/retest

k8s-ci-robot · 2021-03-03T20:00:52Z

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

khenidak · 2021-03-03T20:30:29Z

/test pull-kubernetes-e2e-kind-ipv6

thockin assigned khenidak Feb 28, 2021

k8s-ci-robot requested review from deads2k and juanvallejo February 28, 2021 19:28

aojea reviewed Mar 2, 2021

View reviewed changes

khenidak reviewed Mar 2, 2021

View reviewed changes

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

thockin force-pushed the dualstack-bugs-from-rest-overhaul branch from 15e9642 to e27168d Compare March 2, 2021 23:55

thockin commented Mar 2, 2021

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Mar 3, 2021

thockin force-pushed the dualstack-bugs-from-rest-overhaul branch from e27168d to b21cf1a Compare March 3, 2021 05:11

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 3, 2021

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

thockin added 2 commits March 3, 2021 09:42

Remove defaults for ipFamilyPolicy and ipFamilies

84856c7

These same values are set in the REST stack and it's easier to maintain that code all in one place. defaults.go is best for static values.

thockin force-pushed the dualstack-bugs-from-rest-overhaul branch from b21cf1a to 84856c7 Compare March 3, 2021 19:51

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 3, 2021

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

k8s-ci-robot merged commit 4013bd1 into kubernetes:master Mar 3, 2021

k8s-ci-robot added this to the v1.21 milestone Mar 3, 2021

thockin deleted the dualstack-bugs-from-rest-overhaul branch August 2, 2021 04:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Two small bugs in dual-stack init #99555

Two small bugs in dual-stack init #99555

thockin commented Feb 28, 2021 •

edited

k8s-ci-robot commented Feb 28, 2021

aojea Mar 2, 2021

khenidak Mar 2, 2021

thockin Mar 2, 2021

aojea Mar 2, 2021 •

edited

khenidak Mar 2, 2021

thockin Mar 2, 2021

khenidak commented Mar 2, 2021

thockin left a comment

thockin Mar 2, 2021

thockin Mar 2, 2021

thockin commented Mar 3, 2021

thockin commented Mar 3, 2021

pacoxu commented Mar 3, 2021 •

edited

aojea commented Mar 3, 2021

thockin commented Mar 3, 2021

khenidak commented Mar 3, 2021

thockin commented Mar 3, 2021

thockin commented Mar 3, 2021

khenidak commented Mar 3, 2021

k8s-ci-robot commented Mar 3, 2021

khenidak commented Mar 3, 2021

Two small bugs in dual-stack init #99555

Two small bugs in dual-stack init #99555

Conversation

thockin commented Feb 28, 2021 • edited

k8s-ci-robot commented Feb 28, 2021

aojea Mar 2, 2021

Choose a reason for hiding this comment

khenidak Mar 2, 2021

Choose a reason for hiding this comment

thockin Mar 2, 2021

Choose a reason for hiding this comment

aojea Mar 2, 2021 • edited

Choose a reason for hiding this comment

khenidak Mar 2, 2021

Choose a reason for hiding this comment

thockin Mar 2, 2021

Choose a reason for hiding this comment

khenidak commented Mar 2, 2021

thockin left a comment

Choose a reason for hiding this comment

thockin Mar 2, 2021

Choose a reason for hiding this comment

thockin Mar 2, 2021

Choose a reason for hiding this comment

thockin commented Mar 3, 2021

thockin commented Mar 3, 2021

pacoxu commented Mar 3, 2021 • edited

aojea commented Mar 3, 2021

thockin commented Mar 3, 2021

khenidak commented Mar 3, 2021

thockin commented Mar 3, 2021

thockin commented Mar 3, 2021

khenidak commented Mar 3, 2021

k8s-ci-robot commented Mar 3, 2021

khenidak commented Mar 3, 2021

thockin commented Feb 28, 2021 •

edited

aojea Mar 2, 2021 •

edited

pacoxu commented Mar 3, 2021 •

edited