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

GEP-1713: Standard Mechanism to Merge Multiple Gateways #1863

Closed
wants to merge 36 commits into from

Conversation

dprotaso
Copy link
Contributor

@dprotaso dprotaso commented Mar 22, 2023

Note - this refactors out the 'merging' aspect of Gateways from #1757 - but tweaks the approach

What type of PR is this?

/kind gep

What this PR does / why we need it:

Outlines a mechanism to merge Gateways

Which issue(s) this PR fixes:

Fixes #1713

Does this PR introduce a user-facing change?:


@k8s-ci-robot k8s-ci-robot added kind/gep PRs related to Gateway Enhancement Proposal(GEP) cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 22, 2023
@k8s-ci-robot k8s-ci-robot requested a review from bowei March 22, 2023 01:09
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 22, 2023
geps/gep-1713.md Outdated Show resolved Hide resolved
Copy link
Member

@hzxuzhonghu hzxuzhonghu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can validate the invalid gateways conflicts?

@dprotaso
Copy link
Contributor Author

How can validate the invalid gateways conflicts?

Can you provide some examples that come to mind?

Generally, I think a first step is to decide the expectations for gateway ports and listeners (within a single gateway) - #1842

Then this GEP should decide if merging across gateways allows us to relax some of those requirements.
ie. Multiple listeners on the same port should definitely be allowed. Knative need this to support adding certificates.

geps/gep-1713.md Outdated Show resolved Hide resolved
@hzxuzhonghu
Copy link
Member

Can you provide some examples that come to mind?

Just come to mind because we met this in istio, istio's gateway has server fields which is like listener here. If all the listners in one gateway, it can be validated at least. But if they are in at least two gateways, once they are created at the same time, we have no way to validate them. This is the mechanism of eventual consistence limitation, the only way is to detect any conflict on running

@dprotaso
Copy link
Contributor Author

This is the mechanism of eventual consistence limitation, the only way is to detect any conflict on running

I agree - I think when it comes to the scenario you mentioned I would suggest that if a child gateway cannot merge into a parent gateway then the child gateway status conditions Accepted and Ready should be set to False

geps/gep-1713.md Outdated Show resolved Hide resolved
geps/gep-1713.md Outdated Show resolved Hide resolved
@dprotaso
Copy link
Contributor Author

dprotaso commented Mar 28, 2023

Can folks take another look at the GEP - I hope to have addressed the topics in the discussions and I've flushed out a lot of the semantics.

Copy link
Contributor

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small comments but LGTM!

geps/gep-1713.md Outdated Show resolved Hide resolved
geps/gep-1713.md Outdated Show resolved Hide resolved
geps/gep-1713.md Outdated Show resolved Hide resolved
geps/gep-1713.md Outdated Show resolved Hide resolved
@dprotaso dprotaso changed the title GEP-1713 - Standard Mechanism to Merge Multiple Gateways GEP-1713: Standard Mechanism to Merge Multiple Gateways Apr 4, 2023
Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the teams I work with does some merging of Gateways which may be relevant here and I would be interested to have their review on the subject:

/cc @mlavacca @pmalek @rainest

If we could give a little bit of time for them to check this out they might have some good thoughts on the matter.

/hold

@k8s-ci-robot k8s-ci-robot requested a review from mlavacca May 3, 2023 12:21
@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 May 3, 2023
@kubernetes-sigs kubernetes-sigs deleted a comment from k8s-ci-robot May 3, 2023
Copy link
Contributor

@pmalek pmalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No substiantial comments from my side but I just want to ask this to be sure: (maybe I've missed this in the comments somewhere):

Can a child attach to a parent in a different namespace? My gut tells me that this is not meant to be allowed but it wasn't explicitly mentioned here.

geps/gep-1713.md Outdated Show resolved Hide resolved
@dprotaso
Copy link
Contributor Author

dprotaso commented May 4, 2023

Can a child attach to a parent in a different namespace? My gut tells me that this is not meant to be allowed but it wasn't explicitly mentioned here.

I added a sentence to make this explicit. The API uses an LocalObjectReference which means it can only reference Gateway in the same namespace.

[updated Feb 7 2024] I added support for cross namespace

geps/gep-1713.md Outdated Show resolved Hide resolved
geps/gep-1713.md Outdated Show resolved Hide resolved
geps/gep-1713.md Outdated Show resolved Hide resolved
geps/gep-1713.md Outdated Show resolved Hide resolved
geps/gep-1713.md Outdated Show resolved Hide resolved
@dprotaso
Copy link
Contributor Author

dprotaso commented Jul 3, 2024

@interpixelstudios your comment is sorta off topic for this PR. It might be better to create a discussion here - https://github.com/kubernetes-sigs/gateway-api/discussions

geps/gep-1713/index.md Outdated Show resolved Hide resolved
geps/gep-1713/index.md Outdated Show resolved Hide resolved
geps/gep-1713/index.md Outdated Show resolved Hide resolved
geps/gep-1713/index.md Outdated Show resolved Hide resolved

#### Policy Attachment

Policies attached to a parent Gateway apply to both the parent and all children listeners.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to consider or rule out an alternative that is defined in a policy that could be attached to the parent Gateway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate what you mean? I don't understand.

@dprotaso
Copy link
Contributor Author

Hey folks - feedback has been to change approaches in terms of the Gateway <> Gateway relationship this GEP proposed.

Given the large # of comments and commits on this PR it didn't make sense to continue further revisions here. This PR has now been usurped by #3213

Going to close this out - please review the new PR when you get a chance.

@dprotaso dprotaso closed this Jul 23, 2024
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/gep PRs related to Gateway Enhancement Proposal(GEP) size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GEP: Standard Mechanism to Merge Multiple Gateways