Support multi-port services. #3121
Conversation
timoreimann
added
kind/enhancement
|
@dtomcej and I have been discussing about the design aspect of this PR on the issue already. For formality reasons and in case anyone else wants to chime in, however, I'll maintain status/1 for now. |
| @@ -287,8 +286,13 @@ func (p *Provider) loadIngresses(k8sClient Client) (*types.Configuration, error) | |||
| } | |||
|
|
|||
| for _, subset := range endpoints.Subsets { | |||
| endpointPort := endpointPortNumber(port, subset.Ports) | |||
| if endpointPort == 0 { | |||
There was a problem hiding this comment.
I don't think this is intuitive. I would lean towards a proper error being returned, and a message logged
There was a problem hiding this comment.
Logging wouldn't make too much sense here IMHO since it's sometimes totally legitimate to not find matching endpoints: If you have a multi-port service with ports A and B, there will also be endpoints ports A and B. So service port A will never match endpoints point B, and service port B will never match endpoints point A. I think it'd be quite noisy if we logged every such combo, even at DEBUG level.
| // endpointPortNumber returns the port to be used for this endpoint. It is zero | ||
| // if the endpoint does not match the given service port. | ||
| func endpointPortNumber(servicePort corev1.ServicePort, endpointPorts []corev1.EndpointPort) int32 { | ||
| // Is this reasonable to assume? |
There was a problem hiding this comment.
Note that I did not modify the logic here; I simply added the comment.
My gut feeling tells me that we shouldn't use the service port if no endpoints are available, but I didn't want to drive-by-change this part along an otherwise unrelated PR.
| // if the endpoint does not match the given service port. | ||
| func endpointPortNumber(servicePort corev1.ServicePort, endpointPorts []corev1.EndpointPort) int32 { | ||
| // Is this reasonable to assume? | ||
| if len(endpointPorts) == 0 { |
There was a problem hiding this comment.
Are you checking here to see if endpointPorts is defined at all? or just that the struct is empty?
There was a problem hiding this comment.
It'd be the same as both a nil slice and a non-nil, empty slice return true for a length check against zero.
Again, I didn't make any semantical changes to this part of the logic; I only improved readability so that we return right away if the length is zero.
| // For matching endpoints, the port names must correspond, either by | ||
| // being empty or non-empty. Multi-port services mandate non-empty | ||
| // names and allow us to filter for the right addresses. | ||
| if servicePort.Name == endpointPort.Name { |
There was a problem hiding this comment.
Looking at the docs: (https://godoc.org/k8s.io/api/core/v1#EndpointPort) It appears that endpointPort.Name is optional if only one port is defined. We may have to go deeper, to see if servicePort.Port == endpointPort.Port too. I can't think of a scenario of the top of my head where this would matter, but in theory it could (where an ingress uses a numeric port, and the endpoint is named?)
There was a problem hiding this comment.
You are right that the endpoints port is optional if and only if a single service port is defined. (Reference.)
I don't think that the associated ports would ever differ then. Regarding your example, is it really relevant? The ports that an Endpoints object exposes is defined by the Service, not the Ingress.
| } | ||
| return int(servicePort.Port) | ||
| return 0 |
There was a problem hiding this comment.
Functional, but man, does it suck debugging what 0 means. Can we provide/log a meaningful error?
There was a problem hiding this comment.
As explained above, it's not an error but completely legitimate if we can't find a match. For such cases, it's common to return a special value denoting a "not found" state. For instance, strings.Index from the stdlib does the same.
We could also use the "comma ok" idiom (i.e., return an integer and a boolean where the latter indicates if a value was found). Looking at the stdlib again, however, that's only really used when a special value cannot be employed (such as for map lookups).
Personally, I find the chosen approach simple and idiomatic. I also made sure to document the return value semantics in the function's GoDoc.
jbdoumenjou
added
status/2-needs-review
and removed
status/1-needs-design-review
labels
This enables the use case where multiple ports are defined on the same service object. Traefik now correctly maps the service ports to the corresponding endpoints by means of name-matching.
The updated logic reflects the actual behavior where port names between services and endpoints are always in sync. The tests needed to be updated accordingly.
timoreimann
force-pushed
the
kubernetes-support-multiport-services
branch
from
91a3c1c
to
2a7167d
Compare
What does this PR do?
Support Kubernetes services with multiple defined ports.
Motivation
More
[ ] Added/updated documentation(not needed as we simply match what upstream Kubernetes already does on Service objects)Fixes #3118.