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

Splitting IP address type into IPv4 and IPv6 for EndpointSlices #84971

Merged
merged 1 commit into from Nov 13, 2019

Conversation

@robscott
Copy link
Member

robscott commented Nov 8, 2019

What type of PR is this?
/kind api-change

What this PR does / why we need it:
This splits the IP address type into IPv4 and IPv6 for EndpointSlices. The background for this change is outlined in more detail in the corresponding enhancements PR: kubernetes/enhancements#1360. This PR also updates the EndpointSlice controller to work with the new address types.

I believe this will be a backwards compatible change since kube-proxy in 1.16 does not actually validate/filter on EndpointSlice address types, so as long as the controller is upgraded to 1.17 before kube-proxy, everything should work well.

Does this PR introduce a user-facing change?:

Splitting IP address type into IPv4 and IPv6 for EndpointSlices

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
Enhancement Issue: kubernetes/enhancements#752

/sig network
/priority important-soon
/cc @thockin @freehan @khenidak @andrewsykim

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 8, 2019

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.

@robscott robscott force-pushed the robscott:endpointslice-iptypes branch 3 times, most recently from 0c82240 to 6dc71b8 Nov 8, 2019
Copy link
Contributor

khenidak left a comment

Did one round, will come back to it first thing next week.

@@ -23,7 +23,7 @@ import (
)

var (
defaultAddressType = discoveryv1alpha1.AddressTypeIP
defaultAddressType = discoveryv1alpha1.AddressTypeIPv4

This comment has been minimized.

Copy link
@khenidak

khenidak Nov 8, 2019

Contributor

Why do we believe that we need a default?

This comment has been minimized.

Copy link
@khenidak

khenidak Nov 8, 2019

Contributor

hmm, here is a thought.

What we really care about is not to have mixed family/types in the slice, right? We don't need the user to tell us that this is an ipv4 or ipv6. We can set that. In other words, this field can be readonly. And in this case, it does not need to be defaulted or anything.

This comment has been minimized.

Copy link
@robscott

robscott Nov 11, 2019

Author Member

Unfortunately we can't do this automatically even if we wanted to since address type is immutable and an EndpointSlice with no addresses is valid. Additionally, it's likely that there are addresses that would be valid for multiple address types. I think having a default here is reasonable, and won't take much or any additional effort from EndpointSlice producers.

This comment has been minimized.

Copy link
@thockin

thockin Nov 11, 2019

Member

Does AddressType need to be immutable? If we're adding proper IP validation - we can reject any change that is inconsistent.

E.g., if a user changes all of the addresses from v4 to v6 and changes the addresstype at the same time, is that invalid? It's not particularly different than deleting one and adding another - does the fact that it has the same metadata.name represent a problem?

I don't see why the default here is needed? If it is immutable, the creator has to decide up front. Choosing not to decide, with a default, means you have chosen the default.

This comment has been minimized.

Copy link
@robscott

robscott Nov 11, 2019

Author Member

I think keeping AddressType immutable will help simplify logic for EndpointSlice consumers. As an example, kube-proxy watches for EndpointSlice creates, updates, and deletes, and will only process an EndpointSlice if it matches an AddressType it can handle. If we allowed AddressType to change, it could mean consumers would need to add special handling on updates to see if the AddressType had changed. If it was changing from an AddressType that it did support to one it didn't, it would then have to treat the action more like a delete.

It's worth noting that Service IPFamily is immutable, so this will not be an issue for the primary use case here. I can't think of many use cases where it would make sense to change an AddressType, and in those cases I think it's reasonable to expect EndpointSlices to be replaced (create+delete) instead of updated. That's at least how I think most consumers would prefer to handle the events.

As far as a default, I don't have a strong opinion here, but would lean towards keeping something. It doesn't cost us anything and is closer to backwards compatibility than not having a default value. We have a default for protocol (TCP) as well as port name (""), neither of which are necessary, but could be helpful in requiring a bit less yaml/config to use EndpointSlices. Defaults don't prevent consumers from being able to specify their own values, they just help simplify common configs.

This comment has been minimized.

Copy link
@robscott

robscott Nov 11, 2019

Author Member

Since key 1.17 PRs are dependent on this PR and code freeze is all too soon, I've gone ahead and removed the default value for address type here.

This comment has been minimized.

Copy link
@thockin

thockin Nov 12, 2019

Member

Can remove this variable now

staging/src/k8s.io/api/discovery/v1alpha1/types.go Outdated Show resolved Hide resolved
pkg/apis/discovery/types.go Show resolved Hide resolved
Copy link
Member

thockin left a comment

I focused on API. It seems the contention is really around whether AddressType needs to be immutable and whether a default is valuable or not.

I don't think a default is valuable any more - let's be explicit.

I am ambivalent about immutability. It doesn't feel particularly important, but also unlikely to matter either way. POLS seems like it should be mutable and validated.

pkg/apis/discovery/types.go Show resolved Hide resolved
@@ -23,7 +23,7 @@ import (
)

var (
defaultAddressType = discoveryv1alpha1.AddressTypeIP
defaultAddressType = discoveryv1alpha1.AddressTypeIPv4

This comment has been minimized.

Copy link
@thockin

thockin Nov 11, 2019

Member

Does AddressType need to be immutable? If we're adding proper IP validation - we can reject any change that is inconsistent.

E.g., if a user changes all of the addresses from v4 to v6 and changes the addresstype at the same time, is that invalid? It's not particularly different than deleting one and adding another - does the fact that it has the same metadata.name represent a problem?

I don't see why the default here is needed? If it is immutable, the creator has to decide up front. Choosing not to decide, with a default, means you have chosen the default.

pkg/apis/discovery/validation/validation.go Outdated Show resolved Hide resolved
@robscott robscott force-pushed the robscott:endpointslice-iptypes branch from 6dc71b8 to ccc4381 Nov 11, 2019
@robscott robscott force-pushed the robscott:endpointslice-iptypes branch from ccc4381 to 9a21a23 Nov 11, 2019
@robscott robscott force-pushed the robscott:endpointslice-iptypes branch from 9a21a23 to b5712b9 Nov 11, 2019
@robscott robscott force-pushed the robscott:endpointslice-iptypes branch from b5712b9 to a8d3264 Nov 11, 2019
@robscott robscott force-pushed the robscott:endpointslice-iptypes branch from a8d3264 to 3a49912 Nov 11, 2019
@robscott robscott force-pushed the robscott:endpointslice-iptypes branch from 3ea07af to 6c62aed Nov 12, 2019
@robscott robscott force-pushed the robscott:endpointslice-iptypes branch 2 times, most recently from 9f6108c to f994f4e Nov 12, 2019
Copy link
Member

thockin left a comment

I was going to approve, but I think the master change is just wrong. The other one could be a followup...


allErrs = append(allErrs, validateEndpoints(endpointSlice.Endpoints, addrType, field.NewPath("endpoints"))...)
allErrs = append(allErrs, validatePorts(endpointSlice.Ports, field.NewPath("ports"))...)
allErrs = append(allErrs, validateAddressType(endpointSlice.AddressType, supportedAddressTypes)...)

This comment has been minimized.

Copy link
@thockin

thockin Nov 12, 2019

Member

I would still make that change - I think it makes the code clearer, and when "IP" gets removed, we can remove that argument entirely.

I would also add a comment at the top of the switch along the lines of "This validates known address types, unknown types fall through and do not get validated", and at the bottom of the switch, indicating that fall-through scenario. So no well-meaning person adds a default block.

That way, invalid address types will flag the address type, but valid ones will flag invalid values.

endpointSlice.AddressType = &ipAddressType
// TODO: Add support for IPv6 addresses here (and in the rest of
// EndpointsAdapter).
endpointSlice.AddressType = discovery.AddressTypeIPv4

This comment has been minimized.

Copy link
@thockin

thockin Nov 12, 2019

Member

I'm not sold on this - I think IPv4 and IPv6 both "just work" today because we can carry wither on in an Endpoints. I think you need to iterate the Endpoints and create 1 slice for each family found therein.

@khenidak yes?

This comment has been minimized.

Copy link
@robscott

robscott Nov 12, 2019

Author Member

@thockin Since there's still only one kubernetes service created here, and that service does not specify an IPFamily, I think it'll default to IPv4. Ideally I think the EndpointSlice would match the Service IPFamily if/until we support multiple Services, one for v4 and one for v6. I think the long term solution involves passing the IPFamily through to the EndpointsAdapter, but that initially felt out of scope here. Would it be sufficient to filter Endpoint addresses here to only add IPv4 addresses similar to what I'm doing in the EndpointSlice controller here.

This comment has been minimized.

Copy link
@robscott

robscott Nov 12, 2019

Author Member

I've pushed an update that provides an easier path forward towards true EndpointSlice dual stack support here while also making the code safer for the short term. The current meta proxier kube-proxy implementation will break if an Endpoints resource has more than one IPFamily represented (https://github.com/kubernetes/kubernetes/blob/master/pkg/proxy/ipvs/meta_proxier.go#L177-L203). With that in mind, even if the Endpoints resource here were created with multiple IP families represented, it would just break kube-proxy. I'm assuming the long term solution here involves IP family specific Service, Endpoints, and EndpointSlice, but that feels out of scope here.

For now I've added some safety checks to ensure that if an Endpoints resource does end up with multiple IP families, only IPv4 addresses end up making it into the corresponding EndpointSlice. Additionally, I've added some logic to ensure that EndpointSlices with the deprecated IP address type are replaced with an IPv4 EndpointSlice. This should help for anyone that happens to be upgrading from 1.16 with EndpointSlice alpha enabled (similar functionality already exists in the EndpointSlice controller).

This comment has been minimized.

Copy link
@thockin

thockin Nov 12, 2019

Member

that service does not specify an IPFamily

@khenidak this sounds like a bug - if the master is on IPv6, the service needs to say so

@robscott robscott force-pushed the robscott:endpointslice-iptypes branch 2 times, most recently from 20e84cf to 73a73b6 Nov 12, 2019
@robscott robscott force-pushed the robscott:endpointslice-iptypes branch from 73a73b6 to 0fa9981 Nov 12, 2019
@robscott

This comment has been minimized.

Copy link
Member Author

robscott commented Nov 12, 2019

@thockin I think I've addressed everything, ready for another round of review when you have time.

Copy link
Member

thockin left a comment

Thanks!

/lgtm
/approve

}

allErrs = append(allErrs, validateEndpoints(endpointSlice.Endpoints, addrType, field.NewPath("endpoints"))...)
allErrs = append(allErrs, validateAddressType(endpointSlice.AddressType, validAddressTypes)...)

This comment has been minimized.

Copy link
@thockin

thockin Nov 12, 2019

Member

nit: This function could be in-lined here, now.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 12, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robscott, 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

@robscott

This comment has been minimized.

Copy link
Member Author

robscott commented Nov 12, 2019

/test pull-kubernetes-node-e2e-containerd

@robscott

This comment has been minimized.

Copy link
Member Author

robscott commented Nov 12, 2019

/retest

1 similar comment
@robscott

This comment has been minimized.

Copy link
Member Author

robscott commented Nov 12, 2019

/retest

@fedebongio

This comment has been minimized.

Copy link
Contributor

fedebongio commented Nov 12, 2019

/remove-sig api-machinery

@robscott

This comment has been minimized.

Copy link
Member Author

robscott commented Nov 12, 2019

/retest

@robscott robscott mentioned this pull request Nov 13, 2019
7 of 8 tasks complete
@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Nov 13, 2019

/test all

this is potentially failing pull-kubernetes-verify in the merge queue

https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/batch/pull-kubernetes-verify/1194358015214489600

@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Nov 13, 2019

/hold

@robscott

This comment has been minimized.

Copy link
Member Author

robscott commented Nov 13, 2019

@ahg-g The logs from the many failed verify jobs all point to #84771. Looking at the full build log the error starts something like this, and continues with a longer diff:

     "io.k8s.api.flowcontrol.v1alpha1.PriorityLevelConfigurationSpec": {
-      "description": "PriorityLevelConfigurationSpec specifies the configuration of a priority level.",
+      "description": "PriorityLevelConfigurationSpec is specification of a priority level",

Somehow the presubmit tests must have passed for that PR despite it making a change to types without actually updating swagger.json. I'd actually noticed this yesterday but thought it would eventually cycle out of the merge queue as different combinations of PRs were tried.

I think all of that combined with tests passing here and failing on the other PR make it reasonable to remove the hold on this one. Feel free to add it back if I missed something. Thanks for the help trying to get the merge queue moving again!

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit c560907 into kubernetes:master Nov 13, 2019
16 checks passed
16 checks passed
cla/linuxfoundation robscott authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-kind-ipv6 Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.