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

kube-proxy only looks at Addresses[0] in EndpointSlice #106267

Closed
danwinship opened this issue Nov 9, 2021 · 28 comments · Fixed by #106643
Closed

kube-proxy only looks at Addresses[0] in EndpointSlice #106267

danwinship opened this issue Nov 9, 2021 · 28 comments · Fixed by #106643
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. sig/network Categorizes an issue or PR as relevant to SIG Network.

Comments

@danwinship
Copy link
Contributor

danwinship commented Nov 9, 2021

What happened?

An EndpointSlice can contain up to 1000 Endpoints, which can each contain up to 100 Addresses. Or at least, that's what the docs say, but actually, kube-proxy (or more specifically, pkg/proxy/endpointslicecache.go) only looks at the first Address in each Endpoint:

		endpointInfo := newBaseEndpointInfo(endpoint.Addresses[0], nodeName, zone, portNum, isLocal,
			endpoint.Ready, endpoint.Serving, endpoint.Terminating, endpoint.ZoneHints)

(This is compatible with how EndpointSliceController works, since it would only generate multi-Address Endpoints if a pod had multiple PodIPs of the same address family, which is not allowed. But it's not compatible with manually-generated EndpointSlices.)

What did you expect to happen?

Either all Addresses are used, or else the docs state that only Addresses[0] is used and the field is an array solely for historical reasons.

@danwinship danwinship added the kind/bug Categorizes issue or PR as related to a bug. label Nov 9, 2021
@k8s-ci-robot k8s-ci-robot added needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 9, 2021
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 9, 2021

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

@danwinship
Copy link
Contributor Author

danwinship commented Nov 9, 2021

/sig network
/priority important-longterm

/cc @robscott

@k8s-ci-robot k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 9, 2021
@aojea
Copy link
Member

aojea commented Nov 9, 2021

All addresses in the slice must be of the same type, do you mean creating an Slice with different address types in the Addresses field?

// addressType specifies the type of address carried by this EndpointSlice.
// All addresses in this slice must be the same type. This field is
// immutable after creation. The following address types are currently
// supported:
// * IPv4: Represents an IPv4 Address.
// * IPv6: Represents an IPv6 Address.
// * FQDN: Represents a Fully Qualified Domain Name.
AddressType AddressType `json:"addressType" protobuf:"bytes,4,rep,name=addressType"`

type Endpoint struct {
// addresses of this endpoint. The contents of this field are interpreted
// according to the corresponding EndpointSlice addressType field. Consumers
// must handle different types of addresses in the context of their own
// capabilities. This must contain at least one address but no more than
// 100.
// +listType=set
Addresses []string `json:"addresses" protobuf:"bytes,1,rep,name=addresses"`

@robscott
Copy link
Member

robscott commented Nov 9, 2021

I'm not sure if/how kube-proxy should handle an endpoint with more than 1 address. I think the goal of kube-proxy is to give each endpoint an equivalent probability of being selected. To extend that logic here, I think we'd need to take the probability assigned to an endpoint and divide it by the number of addresses attached to that endpoint. So if an endpoint has a 50% probability of being selected, and it provides 50 addresses, each should end up with a 1% probability of being selected. Of course with the way we need to do descending probabilities, the logic may get a bit complicated.

This isn't as clear as it should be, but I don't think kube-proxy or any consumer of the EndpointSlice API needs to commit to supporting every value specified by that API. For example, many may filter by address type, Service, or some other means. I think it's also valid for a consumer to say they only support a single address per family per endpoint.

So all of that to say, I'd support either documenting that kube-proxy only selects the first address in an endpoint, or updating kube-proxy to support more than one address per endpoint. Given the number of changes currently in flight for kube-proxy, documentation seems like a safer initial approach to me.

@cyclinder
Copy link
Contributor

cyclinder commented Nov 15, 2021

/assign

@cyclinder
Copy link
Contributor

cyclinder commented Nov 15, 2021

can I have this?

editing only the first address(Address[0]) in Endpoint.Address to be used. The use of an array in this field is based on historical reasons. Isn't that right ?

@robscott
Copy link
Member

robscott commented Nov 19, 2021

@cyclinder thanks for volunteering to help with this one! I know @danwinship has some big kube-proxy changes/fixes in flight right now, so it would be good to be sure you don't conflict with him. Otherwise, as long as @danwinship isn't already working on this, I think you're free to have it.

@danwinship
Copy link
Contributor Author

danwinship commented Nov 19, 2021

editing only the first address(Address[0]) in Endpoint.Address to be used. The use of an array in this field is based on historical reasons. Isn't that right ?

I don't think we agreed that the array is for historical reasons only. We aren't currently using it, but we aren't ready to say that we won't use it in the future at this point.

So, the docs should warn that kube-proxy currently ignores additional addresses, but it shouldn't imply that it's actually wrong to have additional addresses

Otherwise, as long as @danwinship isn't already working on this, I think you're free to have it.

I'm not

@cyclinder
Copy link
Contributor

cyclinder commented Nov 22, 2021

thanks @danwinship @robscott

i will work on this

@thockin
Copy link
Member

thockin commented Nov 24, 2021

It was added for a reason, but I think that reason was mixed-family slices (before we decided to break it up into single-family slices).

What would it mean to have multiple addresses here? Are they assumed to be equivalent? Or do they have different context (like a split-horizon)? If the latter, there isn't any metadata here (i.e. it's not a struct like podIPs to allow new fields).

I am inclined to believe it was meant as equivalent, and as such it's probably OK to document that assumption clearly. That said, I don't see how anyone could use multiple addresses in a meaningful way - they are either assumed equivalent or they lack enough metadata to distinguish their differences.

??

If we do document the equivalence, it should be generically worded. E.g. "These addresses are assumed to be equivalent and some consumers may choose to ignore all but the first value."

@thockin thockin self-assigned this Nov 24, 2021
@cyclinder
Copy link
Contributor

cyclinder commented Nov 25, 2021

What would it mean to have multiple addresses here? Are they assumed to be equivalent?

if it was meant as equivalent, why put multiple same addresses into Addresses? I don't understand what the purpose of this is.

@thockin
Copy link
Member

thockin commented Nov 25, 2021

@cyclinder
Copy link
Contributor

cyclinder commented Nov 25, 2021

The first version of the API had both IP families in a single slice, so it made sense to have multiple addresses. We changed that, though we didn't immediately remove the old support. So my best guess is that we should have converted this to a singular value when we did that, but we missed it.

On Wed, Nov 24, 2021, 5:45 PM Cyclinder @.***> wrote: What would it mean to have multiple addresses here? Are they assumed to be equivalent? if it was meant as equivalent, why put multiple same addresses into Addresses? what is the purpose of this? I don't understand what the purpose of this is. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#106267 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABKWAVHO5EOMOKCAXCXHT3TUNWIMXANCNFSM5HVQ6CJQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

so we should change the API doc? Or It's just that documenting it will only use the first address in the pkg/proxy/endpointslicecache.go, like you said above

@thockin
Copy link
Member

thockin commented Nov 25, 2021

I'd like some of the others to comment, but it's this a US holiday, so it might be a few days..

@danwinship
Copy link
Contributor Author

danwinship commented Dec 4, 2021

I don't see how anyone could use multiple addresses in a meaningful way - they are either assumed equivalent or they lack enough metadata to distinguish their differences.

So maybe we should just declare that the array is essentially a bug, and only the first address is used? Maybe something should warn if you ever set additional addresses?

(FTR, the context that I noticed this in was that I was adding unit tests, and was creating EndpointSlices with a single Endpoint and multiple Addresses, which doesn't work; you need multiple Endpoints with a single Address each.)

@thockin
Copy link
Member

thockin commented Dec 10, 2021

I think the simplest and most compat approach is to say "these are all assumed to be fungible and clients may choose to only use the first element".

Thoughts?

@robscott
Copy link
Member

robscott commented Dec 10, 2021

I think the simplest and most compat approach is to say "these are all assumed to be fungible and clients may choose to only use the first element".

Big +1 to that approach. I don't think it's necessarily invalid to set multiple addresses here, there may actually be some use cases for it, but adding this kind of statement should help clear things up significantly.

@khenidak
Copy link
Contributor

khenidak commented Dec 13, 2021

hmm as we push for multi homed pod this will make a lot of sense. There might also be a chance for community to provide a drop-in replacement for proxy that does much more than we do now. I think we should update the docs and don't modify anything for now.

@thockin
Copy link
Member

thockin commented Dec 13, 2021

@aojea
Copy link
Member

aojea commented Dec 13, 2021

hmm as we push for multi homed pod this will make a lot of sense

🤔

@danwinship
Copy link
Contributor Author

danwinship commented Dec 14, 2021

I think the simplest and most compat approach is to say "these are all assumed to be fungible and clients may choose to only use the first element".

If they're fungible, then "clients may choose to use only one of the elements" (or "only a subset"?). No reason to call out the first one specifically.

@thockin
Copy link
Member

thockin commented Dec 14, 2021

@sspreitzer
Copy link

sspreitzer commented Jun 30, 2022

I would like to add to this issue that it would makes sense to have kube-proxy reduce the subset of endpoints in the endpointslice based on the topology.kubernetes.io/zone label of the node and the zone & conditions attributes of the endpoint. Taking only into consideration the first endpoint which is in the same zone of the node or has no zone dependency and is ready for serving.

@thockin & @cyclinder: Would it make sense to reopen this issue?

@aojea
Copy link
Member

aojea commented Jun 30, 2022

I would like to add to this issue that it would makes sense to have kube-proxy reduce the subset of endpoints in the endpointslice based on the topology.kubernetes.io/zone label of the node and the zone & conditions attributes of the endpoint. Taking only into consideration the first endpoint which is in the same zone of the node or has no zone dependency and is ready for serving.

@thockin & @cyclinder: Would it make sense to reopen this issue?

I don't follow entirely what you are suggesting, can you elaborate?

@sspreitzer
Copy link

sspreitzer commented Jun 30, 2022

@aojea

I don't follow entirely what you are suggesting, can you elaborate?

kube-proxy should understand which zone a node is in and prefer endpoints in the same zone.

@aojea
Copy link
Member

aojea commented Jun 30, 2022

@aojea

I don't follow entirely what you are suggesting, can you elaborate?

kube-proxy should understand which zone a node is in and prefer endpoints in the same zone.

but that is unrelated to this issue, topology is considered

// canUseTopology returns true if topology aware routing is enabled and properly configured
// in this cluster. That is, it checks that:
// * The TopologyAwareHints feature is enabled
// * The "service.kubernetes.io/topology-aware-hints" annotation on this Service is set to "Auto"
// * The node's labels include "topology.kubernetes.io/zone"
// * All of the endpoints for this Service have a topology hint
// * At least one endpoint for this Service is hinted for this node's zone.
func canUseTopology(endpoints []Endpoint, svcInfo ServicePort, nodeLabels map[string]string) bool {

See KEPs related, topology-aware-routing was replaced by topology-aware-hints

https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/536-topology-aware-routing
https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/2433-topology-aware-hints

@sspreitzer
Copy link

sspreitzer commented Jun 30, 2022

@aojea
thank you for clarifying. It seems ambiguous to me to have a zone attribute and a hint attribute.
I fail to implement the hinting on a service in v1.23.8, but this is urelated to this issue.

@aojea
Copy link
Member

aojea commented Jun 30, 2022

@aojea thank you for clarifying. It seems ambiguous to me to have a zone attribute and a hint attribute. I fail to implement the hinting on a service in v1.23.8, but this is urelated to this issue.

heh, don't worry, this topic is complex and we went through different solutions, you have more historical context on the KEPs I linked above and this thread in the mailing list https://groups.google.com/g/kubernetes-sig-network/c/wXd1D_fKjqU/m/SEwjsOfpAAAJ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. sig/network Categorizes an issue or PR as relevant to SIG Network.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants