Retrieve ports for rules that use ExternalName Services #985
Conversation
Add findPort() logic to return a port for an ExternalName Service definition. As these have no ports of their own, we simply return whichever port the Ingress rule requested. Fix #984
There was a problem hiding this comment.
LGTM, one nit attached (and considerations below).
I am concerned about introduction of TCP here - the information passed to findPort is not sufficient to determine the protocol; we should be cautious about such silent assumptions if we're willing to add UDP support (#925) soon (I know that the former implementation had TCP hardcoded here).
If you have an idea how to avoid TCP here, please consider changing the solution that way. Otherwise good to go.
Co-authored-by: Michał Flendrich <[email protected]>
|
I think we're kinda just stuck there--ExternalName doesn't specify protocol information either (honestly, it kinda feels like it should include port and protocol information, for exactly this kind of hinting). It may not matter, however, since we're using this to build Kong upstreams, which don't actually care about protocol: their targets are just host/IP+port combinations, with the actual protocol determined by the Kong service using the upstream. We still need protocol information for constructing the Kong service, and the existing implementation of that is a mixed bag: it's determined by the Kinda convoluted and messy (more evidence for why Service APIs are needed to radically revamp this ecosystem), but I think it's fine to leave the stub configuration here since we'll ignore it when creating the Kong configuration later anyway. |
| // We must assume that the user-requested port is valid and construct a ServicePort from it | ||
| if svc.Spec.Type == corev1.ServiceTypeExternalName { | ||
| return &corev1.ServicePort{ | ||
| Protocol: "TCP", |
There was a problem hiding this comment.
| Protocol: "TCP", |
There was a problem hiding this comment.
I think it's fine to leave the stub configuration here since we'll ignore it when creating the Kong configuration later anyway
In such case we should be able to remove the protocol field whatsoever, right?
There was a problem hiding this comment.
Maybe. Tests pass with it gone, but our coverage isn't great, and my concern is that there's something elsewhere that will try to make use of it and panic if it's nil. The original 0.8 code did include it, so IMO safest thing is to just let that persist. However, grep doesn't show anything in our code that looks like it could use it.
What this PR does / why we need it:
ExternalName Services have no ports of their own. They only receive ports when combined with an Ingress rule that specifies them.
findPort()currently returns an error if no port is available on the Service. ExternalName Services will thus always return the no port error.This change adds the 0.8 logic for ExternalName Services to
findPort(), which just builds a ServicePort from the requested port. It returns errors from the non-numeric port cases indicating that they are not valid for ExternalNames.Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #984