Adding GEP 746: Replace Cert Refs on HTTPRoute with Cross Namespace Refs from Gateway #749
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
@robscott: GitHub didn't allow me to request PR reviews from the following users: howardjohn, Miciah. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
k8s-ci-robot
added
approved
|
Adding a hold until we reach some consensus around this. /hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/lgtm |
|
/lgtm |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bowei, howardjohn, robscott The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
I agree with the point about confusion and the problems from the lack of a binding between route and certificate. "Implement a controller that attaches certificates to Gateway listeners" seems reasonable. One remaining concern is around scalability when a gateway has many routes with many certificates.
| Although the simplicity of this approach is nice, it ends up with many of the | ||
| same problems as certificates attached to Routes have and feels inconsistent | ||
| with how Routes attach to Gateways. |
There was a problem hiding this comment.
This may end up being necessary for scalability. Using direct references, if I have a gateway with 1000 httproutes, and each httproute has a certificate, adding a direct reference to each one would be cumbersome—or am I missing an alternative here?
There was a problem hiding this comment.
I think we're going to be limited to a cert per domain, or maybe a cert per domain per gateway. If you had 1000 httproutes with their own cert, would they also have unique hostnames? The end result in any case is the same, these certs need to be attached to a Gateway. The question is how we do that. It could be a direct ref from Route -> Cert, or as proposed here, a direct ref from Gateway Listener -> Cert.
I think automation could potentially help here. A controller could be configured to trust certs for *.example.com from foo namespace, and then could automatically attach them to a Gateway specified by some kind of label. That seems like it could be slightly easier than either Gateway or Route attachment when dealing with 1000+ domains. Though hopefully at that scale, cert generation can be automated with a tool like cert-manager.
Any way we do this, we'll need some kind of link between a Gateway and a cert. HTTPRoute provided one potential way to make that link, but I'm not sure it provided any significant scalability/usability improvements.
There was a problem hiding this comment.
I think we're going to be limited to a cert per domain, or maybe a cert per domain per gateway.
I might be missing some nuance. Should "domain" be read as "hostname" here?
If you had 1000 httproutes with their own cert, would they also have unique hostnames?
Yes.
I think automation could potentially help here. A controller could be configured to trust certs for *.example.com from foo namespace, and then could automatically attach them to a Gateway specified by some kind of label. That seems like it could be slightly easier than either Gateway or Route attachment when dealing with 1000+ domains.
I'm less concerned about automating the attachment and more concerned about fanout and the idea that the gateway would need 1000+ direct references to certificates. I suppose a controller could batch updates to mitigate contention issues, but it would be a bit inconvenient (especially for humans) to work with a gateway definition that had to list out explicitly every linked certificate.
There was a problem hiding this comment.
Oh, I just realized that gateway's spec.listener has // +kubebuilder:validation:MaxItems=64, and each listener can have only one certificate reference.
There was a problem hiding this comment.
I might be missing some nuance. Should "domain" be read as "hostname" here?
Yep, I should've just said "hostname"
I just realized that gateway's spec.listener has // +kubebuilder:validation:MaxItems=64, and each listener can have only one certificate reference.
There's some nuance here in that a single resource may be linked per listener, that resource could potentially contain more than one cert (not saying that's a good idea though).
I think one of the problems with certs being attached to Routes was that there was a bit too much magic going on. It's difficult/impossible to see which certs are actually attached to a given Gateway listener. Although certs appeared to be attached to Routes, they were being attached to the listeners attached to those Routes, and that was just being hidden. As an implementation you'd still need to have some way to represent that many certs being attached to your listeners. I'd rather increase the max number of listeners and/or turn certificate refs into a list because I think those changes would be clearer and easier for users to understand.
more concerned about fanout and the idea that the gateway would need 1000+ direct references to certificates
I agree that that's never going to be a good experience. Would Gateway merging help here? The idea that compatible Gateways can be merged together by implementations?
There was a problem hiding this comment.
There's some nuance here in that a single resource may be linked per listener, that resource could potentially contain more than one cert (not saying that's a good idea though).
The godoc implies that the resource can have exactly one certificate+key pair:
// CertificateRef is a reference to a Kubernetes object that contains a TLS
// certificate and private key. This certificate is used to establish a TLS
// handshake for requests that match the hostname of the associated listener.
I agree that that's never going to be a good experience. Would Gateway merging help here? The idea that compatible Gateways can be merged together by implementations?
I was thinking through this possibility. Gateway merging might be the best approach here. Either the cluster admin could give users permission to create gateways in their own namespaces, which would allow users to attach routes and certificates within their own respective namespaces (or others' namespaces, but only if permitted by some referencepolicy); or the controller that automated certificate management could do some sort of sharding with up to 64 certificates per shard/gateway. (I personally like the idea of allowing users to manage their own gateways, but I need to think through all the implications of that approach.)
There was a problem hiding this comment.
To be clear, I'm still on board with this GEP given how attaching a certificate to a route using the API as it stands without this GEP doesn't actually bind the certificate to the route, but the self-service use case still needs some clarification (not necessarily in this GEP though).
There was a problem hiding this comment.
Yeah I personally really like the idea of more smaller Gateways than a single massive one. I'm really hoping that pattern will work for most organizations. I think it's worth digging into this self service model a bit more, especially around the scale aspect. I'd primarily been thinking in terms of ~dozens of certs per Gateway, not 1000+.
To be clear, I'm still on board with this GEP given how attaching a certificate to a route using the API as it stands without this GEP doesn't actually bind the certificate to the route, but the self-service use case still needs some clarification (not necessarily in this GEP though).
Sounds good, and agree that we need a clearer understanding of how self-service could work. Maybe @maelvls or @jakexks would have some insights here around how we can build a more scalable API around this.
Given the v1alpha2 timeline, I think I'm going to create a follow up issue to continue the discussion around this so we can get this change in.
There was a problem hiding this comment.
Created #763 to track this.
k8s-ci-robot
removed
the
lgtm
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
I think we should get this one and keep discussing the self-service use-case on #763 . /lgtm |
k8s-ci-robot
added
the
lgtm
|
Related #3418 |
What type of PR is this?
/kind gep
What this PR does / why we need it:
This adds GEP #746, as a follow up to my original proposal doc.
Does this PR introduce a user-facing change?:
/cc @bowei @hbagdi @howardjohn @jpeach @Miciah @youngnick