-
Notifications
You must be signed in to change notification settings - Fork 509
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Making GatewayClass an optional resource #614
Conversation
@robscott: GitHub didn't allow me to request PR reviews from the following users: howardjohn. 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. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
// Implementations may also support using this field to specify a controller | ||
// name directly without a corresponding GatewayClass resource. This is | ||
// particularly useful for namespace-scoped implementations that may not | ||
// have access to the cluster-scoped GatewayClass resource. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a controller meant to do if it is referred to by name? For example, a cluster has controller A with gateway classes A1 and A2 configured. When a gateway is specified with a classname of A, there's no configuration that matches that, so what is the right behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good question. From my perspective, referring to a controller name directly would result in using the controller's default behavior.
To take this a step further, I'd thought it might be worth supporting not just controller name, but controller name + suffix. Implementations could choose to ship with a set of preloaded "virtual" GatewayClasses that could be referenced by that suffix. For example, let's say that controller name is acme.io/gateway
and that controller ships with predefined/virtual xlb
and ilb
concepts. A new Gateway could be created with acme.io/gateway/xlb
as GatewayClassName
in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the critical part of the problem is
namespace-scoped implementations that may not have access to the cluster-scoped GatewayClass resource.
Maybe some more context around what kind of implementation you are thinking of here would make the change clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the upstream Ingress issue provides some compelling examples here. In that case, someone is trying to deploy either Traefik or ingress-nginx into a namespace but both fail because they don't have access to IngressClass. I think it's relatively common for users to want to be able to deploy an in-cluster proxy like this within a single namespace. In that case, they're not really concerned about custom behavior, just connecting a Gateway to a controller.
With that said, I think there are some interesting extensions of this model like the virtual/predefined GatewayClass concept that could enable further capabilities when scoped within a single namespace.
Although those 2 examples are from the upstream issue, I think the concept is broadly applicable. I'm hoping that whatever we try here could also make sense for Ingress. We could try to make namespace-scoped *Class resources here, but I think that would result in significantly more complexity than this approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess there's also a question of whether you can distinguish between a missing resource and a controller name/label?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess there's also a question of whether you can distinguish between a missing resource and a controller name/label?
I think overloading two different semantics here will be problematic.
If we have two fields - gatewayClassName
and controller
and only one of the two are required, that is a more clear API for the users and implementers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good point. That would be significantly clearer. Since this same problem applies to Ingress, I think I was getting confused by the terminology in that API where a class defined by an annotation does not refer to an IngressClass resource. Of course that was never really a formal part of the API and developed a bit more organically. If we went forward with controller
here, we could add validation to the webhook to ensure only one field was set at a time. Or if these really are mutually exclusive fields, maybe there's a more generic term we could use that would work for both use cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could add validation to the webhook to ensure only one field was set at a time
There is a way to do that validation using CRD itself but kubebuilder doesn't support it. -_-
But yes, that is something I was thinking too - two mutual exclusive fields.
Or if these really are mutually exclusive fields, maybe there's a more generic term we could use that would work for both use cases?
Using a single field for two (or more) constants or variable values would be okay but as @jpeach pointed out, the semantics here are different.
A solution that comes to mind is using a single field for value and then a union discriminator field to figure out the meaning of the value. That's more complexity for no gain, unless we plan to have a third field, which I feel is unlikely.
This has been a popular request for IngressClass as well and would make namespace-scoped implementations of the API possible.
70a72ab
to
f103c08
Compare
Can we also update documentation of GatewayClass resource? |
I think this requires a bit more thought. I'm working on a doc that goes through some of our options here. For now I'm going to close this, but may reopen later depending on the outcome of a broader discussion. /close |
@robscott: Closed this PR. 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. |
Hi @robscott, |
Hey @lukasrieger-dev, thanks for checking in on this. Unfortunately this lost steam. There were some concerns about the ambiguity of any possible optional GatewayClass solutions. Unfortunately I'm not sure how much time I'll have to push this forward in the next few months, but if anyone else has time to push this discussion forward, I'd be happy to provide feedback. |
What type of PR is this?
/kind feature
/kind api-change
What this PR does / why we need it:
This has been a popular request for IngressClass as well and would make namespace-scoped implementations of the API possible.
Which issue(s) this PR fixes:
Fixes #567
Does this PR introduce a user-facing change?:
/cc @howardjohn @hbagdi @bowei
/assign @thockin