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.

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

gateway.listeners: make optional #1596

Closed

Conversation

howardjohn
Copy link
Contributor

What type of PR is this?
/kind api-change

What this PR does / why we need it:
This changes listeners field to be optional (i.e. allow 0 length list).

The motivation here is that a Gateway actually provisions underlying infrastructure. This can take a long time, too, depending on the set.
It may be useful to initially provision a Gateway with
zero listeners, and then later add them.

This PR loosens validation to allow not configuring any listeners, with the expectation that underlying infra would be provisioned, then a user would add specific listeners.

Does this PR introduce a user-facing change?:

Gateway.Spec.Listeners field is now optional

This changes `listeners` field to be optional (i.e. allow 0 length
list).

The motivation here is that a Gateway actually provisions underlying
infrastructure. This can take a long time, too, depending on the set.
 It may be useful to initially provision a Gateway with
*zero* listeners, and then later add them.

This PR loosens validation to allow not configuring any listeners, with
the expectation that underlying infra would be provisioned, then a user
would add specific listeners.
@k8s-ci-robot k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 13, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: howardjohn
Once this PR has been reviewed and has the lgtm label, please assign caseydavenport for approval by writing /assign @caseydavenport in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 13, 2022
@hzxuzhonghu
Copy link
Member

The motivation here is that a Gateway actually provisions underlying infrastructure.

Isn't gatewayClass that does provision?

@howardjohn
Copy link
Contributor Author

Gateway class just says what controllers are installed (more or less). Gateway is an actual instance of the infra

@robscott
Copy link
Member

Need more time to think about this, but I think it's too big of a change for v0.6.0, adding a hold on this until we get that released.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 13, 2022
@shaneutt
Copy link
Member

I'm not opposed to this, but I do have some general questions about the use case: it sounds like the intention here is to allow a CREATE followed by an UPDATE to add the Listeners after the Gateway becomes Ready=True, is that accurate? Can you help me to better understand the dilemma? What would be the downside of requiring the Listeners but then having the controller implementation wait until the underlying provisioning reaches a point where it can accept listeners before eventually applying those listeners, and then calling the Gateway Ready=True last? It seems to me that it would overall be better to not have the Gateway marked as Ready=True until it has listeners ready?

@howardjohn
Copy link
Contributor Author

howardjohn commented Dec 13, 2022 via email

@hzxuzhonghu
Copy link
Member

That can make the ready not really ready if a listener can be updated later

@howardjohn
Copy link
Contributor Author

howardjohn commented Dec 14, 2022 via email

@robscott
Copy link
Member

@howardjohn I think I see the value of pre-warming infra whenever it's possible, but have a few questions:

  1. Why not just provision a Gateway with a dummy listener and change it later? If the infra you're provisioning is a Service type=LB, wouldn't you need to specify ports there?
  2. What would this mean for conformance? Not every implementation is going to be able to provision infrastructure without ports/protocols specified?
  3. Are there other use cases a Gateway without listeners would enable?

@shaneutt
Copy link
Member

@howardjohn looks like we have some outstanding questions here, let us know if you need anything from us in order to move this one forward 🖖

@howardjohn
Copy link
Contributor Author

  1. Why not just provision a Gateway with a dummy listener and change it later? If the infra you're provisioning is a Service type=LB, wouldn't you need to specify ports there?

Exposing a dummy port on public internet doesn't seem like a good idea and maybe conflict with security guidelines, etc. There is also a difference between "No listener" and "Listener without routes" (not to mention risk of accidentally attached routes)

  1. What would this mean for conformance? Not every implementation is going to be able to provision infrastructure without ports/protocols specified?

I think like many other fields - implementation specific. We can declare regardless of support they should report status properly ("scheduled" but 0 listeners, or "not scheduled") .

  1. Are there other use cases a Gateway without listeners would enable?

Yes, we want to use Gateway to deploy instances of waypoint proxies in ambient mesh. These have an implicit listener that isn't reliable to represent as a listener ("all protocols on all ports"), but also allow users adding explicit listeners using standard GW mechanism (ie port 443 HTTPS with some cert). Requiring a listener is fitting a square peg in a round hole and adds complexity for the user.

@youngnick
Copy link
Contributor

Coming back to this one, I should note that changing a previously required field to optional is a breaking API change, sadly. In this case, controllers can currently expect the slice to have at least one entry, changing that is a breaking change. And breaking changes are supposed to have an API revision bump.

I don't necessarily think this is impossible, but I think it would need something to run through the use cases in different deployment models (what should an ingress controller like Contour or Emissary do with an empty list? Should it be accepted? And so on).

@howardjohn What would you like to do here?

@howardjohn
Copy link
Contributor Author

I think at this point it may make sense to close this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants