Correct Kubernetes Ingress and IngressRoute port heuristic for choosing HTTPS

[seh]

What does this PR do?

When applying the port value heuristic to decide whether to contact a Kubernetes Service's Endpoints using HTTP or HTTPS, inspect the Service's front-end port instead of the upstream servers' port values. That is, for example, when Traefik services an Ingress or IngressRoute object pointing at a Service that forwards from port 443 to servers listening on port 8443, it should use HTTPS.

Motivation

Fixes #5164 (though not yet honoring the "ingress.kubernetes.io/protocol" annotation yet like in Traefik version 1.7)

More

• Added/updated tests
• Added/updated documentation

Additional Notes

Should we include the "ingress.kubernetes.io/protocol" annotation here? At present, this patch addresses only the Service port value.

[dtomcej]

This PR exposes how the new providers do not support multiport services.

😢

[seh]

I hadn’t thought of that, coming at it with a narrow focus on repair. Should we confront that gap here?

[dtomcej]

Going to discuss with the team tomorrow. May require a refactor.

[seh]

While we do collect only one port per reference to a Service, an an IngressRoute can define multiple Routes, each of which can point at any number of Services. Even though each reference includes only one port of the Service, it looks like it's still possible, though, to include multiple references to the same Service, each referring to a different port.

Studying this code again, it's surprising that we watch all of these object kinds, but if any one of them changes, we reload the universe by making requests of the Kubernetes API server. If instead we used Informers, we'd have an in-memory cache of these objects, and could react more specifically to what changed. For instance, if an IngressRoute changes, you don't have to reload every other IngressRoute too. If an Endpoints changes, you have to work backwards through the dependency graph to figure out whether any Services of the same name exist and are referenced by any existing IngressRoutes, and, if so, only update those.

[dtomcej]

@seh,

Discussed with the team, and we feel that it is acceptable to proceed with this PR as it is now.

We will work on the multi-port issue in a future PR.

[dtomcej]

LGTM

[seh]

Thank you. I'd like to hear more detail about the multi-port concern, as my cursory reading per my comment above saw that we do accommodate Services with more than one port. Perhaps you're referring to a different meaning of "service" here.

Is it worth filing an issue suggesting those improvements to the controller reconciliation procedure, watching and reacting to more fine-grained changes in the object graph?

[dtomcej]

You can definitely file an issue, but currently providers are stateless. To maintain an object dependency graph, the state would need to be passed between iterations of the provider cycle, and potentially shared with the core server to persist with provider restarts.

May be worth examining, but Traefik does not operate as a controller, but as a watcher. The informers are registered to trigger a configuration reload, but each reload is atomic, so there is no inconsistencies to manage, and no "broken states".

[seh]

Is there something else to be done here? We’d like to take advantage of this fix in the next container image release.

[ldez]

@seh FYI our review process https://docs.traefik.io/v2.0/contributing/maintainers/#pr-review-process

[seh]

I read that document several times, and the most sense I can make of it is that we need two more maintainers to approve this patch, given that @dtomcej already gave it the nod. If there are daily meetings to discuss open requests, can you tell me whether this one has come up for discussion?

[dduportal]

Hi @seh , the daily meeting is for triaging. The review work on the PRs heavily depends on the maintainer availability. With the context here (assumption's week, summer's week), it's not realistic to commit on a deadline.

We hear your concern and we try our best, but right now there is nothing else outside waiting, as rushing a review is never a good idea.

Thanks for this contribution and for the help proposal, it's always really appreciated!

[ldez]

Thanks 👍

[ldez]

FYI We don't know who is @reddy-s but it's not a maintainer so his approval does not count.

[nmengin]

LGTM

[ldez]

The error is not related to the PR, I will fix that.
Fixed by #5207