design: Add design for multiple Envoy listeners #3263
Conversation
As part of implementing Service APIs, it's necessary to reconsider Contour's insecure to secure redirect functionality. This document suggests keeping the functionality, while allowing additional listening ports to be defined. Updates projectcontour#1300 Updates projectcontour#2922 Updates projectcontour#3086 Signed-off-by: Nick Young <[email protected]>
youngnick
requested review from
jpeach and
sunjayBhatia
and removed request for
a team
Codecov Report
@@ Coverage Diff @@
## main #3263 +/- ##
==========================================
+ Coverage 75.41% 77.84% +2.43%
==========================================
Files 96 103 +7
Lines 5963 9024 +3061
==========================================
+ Hits 4497 7025 +2528
- Misses 1366 1865 +499
- Partials 100 134 +34
|
|
I've opened this one because I think that the Gateway object in Service-APIs implies that we should allow the creation of extra Listeners. We also have outstanding requests for similar functionality. I'm going to say again though, that although this allows you to forward some traffic on an arbitrary port at Layer 4, I don't believe this is "Adding layer 4 load balancing to Contour". It's missing large swaths of functionality for that (most importantly, the ability to choose your serving IP), and is heavily dependent on your Envoy being configured correctly (trying to bind to ports under 1024 will fail if your container is not running, and so on). I'm working on adding this detail to the |
youngnick
force-pushed
the
multiple-listeners-design
branch
from
2ffe6a9 to
371ff12
Compare
Signed-off-by: Nick Young <[email protected]>
youngnick
force-pushed
the
multiple-listeners-design
branch
from
371ff12 to
c12703b
Compare
| @@ -0,0 +1,111 @@ | |||
| # Automatic redirection with arbitrary listeners | |||
|
|
|||
| Status: Accepted | |||
There was a problem hiding this comment.
As we've done for other current designs, setting this to Accepted because it won't be merged unless it is accepted.
|
|
||
| ### HTTPProxy processing | ||
|
|
||
| For HTTPProxy resources, Contour will add an optional `port` field to the `virtualhost` YAML stanza, which matches the externally visible port for the Envoys. |
There was a problem hiding this comment.
I'm open to discussion on the name here, as it's the translated port. Not sure if that means we should give it a different name, or if we should just be very clear in the documentation what port we are talking about.
There was a problem hiding this comment.
Maybe just not add to HTTPProxy at first revisit the extra overhead later?
There was a problem hiding this comment.
It seemed better to me that if we're going to add the functionality, we should add it into HTTPProxy first, while we do the rest of the Service APIs design, then it's easier to just consume it.
| - port set | ||
| - TLS details set - any port other than the external version of the secure or insecure ports, allow. A new listener will be created if it doesn't exist. No redirection created for you. | ||
| - TLS details set - port is the external version of the secure or insecure ports - treat as though no port was set. Redirection will be created. | ||
| - no TLS details set - any port other than the insecure or secure ports, allow. A new listener will be created if one doesn't exist. |
There was a problem hiding this comment.
Should we add some detail around here or the detailed design section about what will happen if a port is set but it conflicts with an existing listener?
e.g.
- existing HTTPProxy that asks to listen on port 9000 with no TLS details set
- gets Envoy listener + filter chain with plaintext transport socket
- new HTTPProxy (different vhost) that asks to listen on port 9000 with TLS details set
- TLS transport socket would conflict with plaintext transport socket
With Service APIs this configuration would be "not mergeable", seems the port in addition to vhost could be a uniqueness constraint to add onto HTTPProxy resources if I'm understanding correctly?
There was a problem hiding this comment.
Yes, that's a great point, thanks Sunjay. Currently any two HTTPProxy objects that match on vhost name render both proxies invalid. We will need to extend that to ports as well, there are some cases around what to do with mixed port/no port configurations that match on vhosts as well. That is definitely something for the detailed design section.
There was a problem hiding this comment.
I've added a small note in here about the uniqueness constraint, but I think further detail is for the detailed design section.
There was a problem hiding this comment.
Related to this, you could have conflicts across different APIs, e.g. an HTTPProxy conflicts with a Gateway.
| Currently, Contour allows the configuration of the secure and insecure listening ports for Envoy using the `--envoy-service-https-port` and the `--envoy-service-http-port` parameters respectively. | ||
| (It's fair to say that these parameters are confusingly named). | ||
|
|
||
| These will also be exposed in the configuration file as `secureListenerPort` and `insecureListenerPort` respectively. |
There was a problem hiding this comment.
It feels like the service-api spec should be able to define this and not rely on the implementation.
There was a problem hiding this comment.
I don't disagree, but don't want to block this on waiting for upstream.
| Currently, Contour allows the configuration of the secure and insecure listening ports for Envoy using the `--envoy-service-https-port` and the `--envoy-service-http-port` parameters respectively. | ||
| (It's fair to say that these parameters are confusingly named). | ||
|
|
||
| These will also be exposed in the configuration file as `secureListenerPort` and `insecureListenerPort` respectively. |
There was a problem hiding this comment.
Could also use a convention that the service port named http will be the insecure port, the port named https will be the secure. This removes the need for the extra config options.
There was a problem hiding this comment.
In the service APIs though, the ports don't have names. So it would need to be "the first port with protocol: HTTP" or something. I thought it was clearer to put the behavior in Contour, which allows us to backport it to HTTPProxy.
There was a problem hiding this comment.
The k8s service has names and the design says we're going to watch them, so given that you can match up the port name in the service to a port on a listener.
|
|
||
| ### HTTPProxy processing | ||
|
|
||
| For HTTPProxy resources, Contour will add an optional `port` field to the `virtualhost` YAML stanza, which matches the externally visible port for the Envoys. |
There was a problem hiding this comment.
Maybe just not add to HTTPProxy at first revisit the extra overhead later?
|
xref: projectcontour/contour-operator#151 (operator network publishing API) |
Signed-off-by: Nick Young <[email protected]>
youngnick
force-pushed
the
multiple-listeners-design
branch
from
eda29ef to
080bce2
Compare
| with "externally visible secure port" meaning "whatever port an external user will go to to get to the secure port", | ||
| and similarly for the "externally visible insecure port". | ||
|
|
||
| Importantly, pecifying a port for Envoy's secure or insecure listening port that does not match the a port on the Service will be a fatal error for Contour. |
There was a problem hiding this comment.
nit: s/pecifying/specifying/
There was a problem hiding this comment.
I'm fine with this proposal, though I think I'd also be fine saying that for our initial service-apis implementation, we only support two listeners (one http, one https) just like Contour works today, and any additional ones would be errors; then we could tackle this later. IOW, I'm not seeing that this has to be a prereq for an initial service-apis implementation, especially given the alpha state of the API.
|
I'm going to leave this open for now, it will be easy to rebase later, and I think we should look at this again soon (once we have some basic Service-APIs support). |
|
@cten this was the listeners design doc if you had anything to add. =) |
|
Marking this PR stale since there has been no activity for 14 days. It will be closed if there is no activity for another 90 days. |
github-actions
bot
added
the
lifecycle/stale
|
The additional arbitrary port listeners makes it possible to select one envoy by 2 private ( |
|
I think this issue is still not solved so is it alright to re-open? |
|
We can leave it open; I suspect we will need it sooner than I thought. |
|
Is this available yet? |
|
No, this design has not been finished. As I said earlier, I think we still need it. |
As part of implementing Service APIs, it's necessary to reconsider Contour's insecure to secure redirect functionality.
This document suggests keeping the functionality, while allowing additional listening ports to be defined.
Updates #1300
Updates #2922
Updates #3086
Blocks #3213 until we decide one way or another.
Signed-off-by: Nick Young [email protected]