Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Svc REST: clean up defaultOnRead to be consistent #104986

Merged
merged 1 commit into from Sep 16, 2021

Conversation

@thockin
Copy link
Member

@thockin thockin commented Sep 14, 2021

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
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Sep 14, 2021

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

Loading

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Sep 14, 2021

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

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

Loading

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
}
Copy link
Member

@aojea aojea Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Loading

Copy link
Member Author

@thockin thockin Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!!

Loading

pkg/registry/core/service/storage/storage.go Outdated Show resolved Hide resolved
Loading
} else {
// Headless + selector - default to single.
Copy link
Member

@aojea aojea Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Loading

Copy link
Member Author

@thockin thockin Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

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

@aojea aojea Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if this was already set?

Loading

Copy link
Member Author

@thockin thockin Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

@thockin thockin force-pushed the svc-validation-cleanup branch from 204af75 to 7639ab1 Sep 14, 2021
Copy link
Member Author

@thockin thockin left a comment

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?

Loading

pkg/registry/core/service/storage/storage.go Show resolved Hide resolved
Loading
pkg/registry/core/service/storage/storage.go Outdated Show resolved Hide resolved
Loading
} else {
// Headless + selector - default to single.
Copy link
Member Author

@thockin thockin Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

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

@thockin thockin Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

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
}
Copy link
Member Author

@thockin thockin Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!!

Loading

Loading
Loading
Loading
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)),
Copy link
Member

@aojea aojea Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member Author

@thockin thockin Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one sets clusterIP (singlular) but not plural.

Loading

Copy link
Member

@aojea aojea Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

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)),
Copy link
Member

@aojea aojea Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loading

Copy link
Member Author

@thockin thockin Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same answer

Loading

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 thockin force-pushed the svc-validation-cleanup branch from 7639ab1 to 52f54ce Sep 16, 2021
Copy link
Member Author

@thockin thockin left a comment

Rebased and squashed, too.

Loading

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)),
Copy link
Member Author

@thockin thockin Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one sets clusterIP (singlular) but not plural.

Loading

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)),
Copy link
Member Author

@thockin thockin Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same answer

Loading

@aojea
Copy link
Member

@aojea aojea commented Sep 16, 2021

/lgtm

Loading

@k8s-ci-robot k8s-ci-robot merged commit 924f196 into kubernetes:master Sep 16, 2021
14 checks passed
Loading
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants