Skip to content
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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
+13 −4
Closed

Making GatewayClass an optional resource #614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions apis/v1alpha1/gateway_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,13 @@ type GatewayList struct {
// webhook, but there are many cases that will require asynchronous
// signaling via the GatewayStatus block.
type GatewaySpec struct {
// GatewayClassName used for this Gateway. This is the name of a
// GatewayClass resource.
// GatewayClassName used for this Gateway. This may refer to a GatewayClass
// resource by name.
//
// 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.
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

@robscott robscott Apr 19, 2021 •
edited
Loading

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.

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Member Author

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?

Copy link
Contributor

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.

//
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
Expand Down
8 changes: 6 additions & 2 deletions config/crd/bases/networking.x-k8s.io_gateways.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.