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

Jump to bottom

Remove experimental from gateway infrastructure #3272

Merged
merged 13 commits into from
Sep 4, 2024
Merged

Remove experimental from gateway infrastructure #3272

merged 13 commits into from
Sep 4, 2024

Conversation

keithmattix
Copy link
Contributor

@keithmattix keithmattix commented Aug 16, 2024 •
edited
Loading

What type of PR is this?

/kind gep
/kind cleanup
What this PR does / why we need it:
Removes experimental from gateway infrastructure type.

Which issue(s) this PR fixes:

Fixes #1867

Does this PR introduce a user-facing change?:

Promoted GEP-1867 Per Gateway Infrastructure to Standard Channel

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 16, 2024
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 16, 2024
@youngnick
Copy link
Contributor

#3197 actually probably blocks this to some extent, because we need to resolve that before we release. Doesn't stop us merging this, but we need to make sure we do both.

@keithmattix
Copy link
Contributor Author

Thanks for the reminder! @snorwin happy to wait on your change or make add the new LabelValue field in this PR. Up to you 🙂

@snorwin
Copy link
Contributor

snorwin commented Aug 20, 2024

@keithmattix I created a proposal in #3284. It would be great if you have a look at it too.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 26, 2024
@keithmattix keithmattix mentioned this pull request Aug 28, 2024
Merged
@robscott
Copy link
Member

@keithmattix @howardjohn We got a good question about this GEP at SIG-Network yesterday. How do we handle mutability? If these labels were to map to Pods, it could be rather disruptive to update labels.

@keithmattix
Copy link
Contributor Author

In the Istio implementation, we create a deployment for the gateway, so any pod restarts would be via a rolling restart. I think as long as you're not changing a label used for deployment or service selection, you're probably fine in most cases. I think the GEP reserves the gateway.networking.k8s.io prefix (i.e. users can't set labels with that prefix), and that's what we use for deployment + service selection in Istio.

@robscott
Copy link
Member

@keithmattix do you mind adding a note somewhere in the GEP as part of this PR to at least call out the risk that changing labels could lead to underlying dataplane Pods being replaced, depending on the underlying implementation? I'm assuming that at least some Gateway implementations may not be able to do that without at least some minor disruptions to traffic.

robscott
robscott reviewed Sep 3, 2024
View reviewed changes
apis/v1/gateway_types.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added 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/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 3, 2024
@keithmattix
Copy link
Contributor Author

@robscott I added some language about dataplane pods; let me know if that's sufficient. I also removed references to gatewayclass.infrastructure because I believe that direction was abandoned

robscott
robscott reviewed Sep 3, 2024
View reviewed changes
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks @keithmattix!

geps/gep-1867/index.md Outdated Show resolved Hide resolved
geps/gep-1867/index.md Show resolved Hide resolved
@keithmattix keithmattix force-pushed the promote-incluster-gateway-infra branch from d8aee5a to ef0ca93 Compare September 3, 2024 21:14
@robscott
Copy link
Member

robscott commented Sep 3, 2024

Thanks @keithmattix! LGTM once presubmits pass - looks like another round of codegen is needed.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: keithmattix, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 3, 2024
@youngnick
Copy link
Contributor

/lgtm

once the presubmits pass. Nice work @keithmattix!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 3, 2024
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2024
@keithmattix
Copy link
Contributor Author

Oh the cel isn't getting copied over hmmm

keithmattix and others added 12 commits September 4, 2024 16:56
@keithmattix
Remove experimental from gateway infrastructure
74028fe 
Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
@keithmattix
Codegen
3caa89a 
Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
@keithmattix
Change version of protoc-gen-grpc
395b77a 
Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
@keithmattix
Change protoc version
4bdbe39 
Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
@keithmattix @robscott
Update apis/v1/gateway_types.go
b3e1f13 
Co-authored-by: Rob Scott <rob.scott87@gmail.com>
@keithmattix
Add a warning about how modifying infrastructure.labels can result in…
768d7c9 
… pod churn

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
@keithmattix
Codegen
b493895 
Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
@keithmattix
Address PR feedback
736a277 
Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
@keithmattix
Fix whitespace
4083970 
Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
@keithmattix
Fixup
ecc6b1b 
Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
@keithmattix
Regen
dbf921f 
Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
@keithmattix
Add a git diff
257cdc1 
Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
@keithmattix keithmattix force-pushed the promote-incluster-gateway-infra branch from db13417 to 257cdc1 Compare September 4, 2024 16:57
@keithmattix
Regen after rebase
307d302 
Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
@keithmattix
Copy link
Contributor Author

It was because I didn't rebase on top of #3197 🤦🏾 presubmit should pass now

@robscott
Copy link
Member

robscott commented Sep 4, 2024

It was because I didn't rebase on top of #3197 🤦🏾 presubmit should pass now

Good catch, thanks @keithmattix!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2024
@robscott robscott added this to the v1.2.0 milestone Sep 4, 2024
@keithmattix
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 4, 2024
@k8s-ci-robot k8s-ci-robot merged commit ce6d2c6 into kubernetes-sigs:main Sep 4, 2024
8 checks passed
@keithmattix keithmattix deleted the promote-incluster-gateway-infra branch September 4, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mlavacca mlavacca Awaiting requested review from mlavacca

@youngnick youngnick Awaiting requested review from youngnick

@robscott robscott Awaiting requested review from robscott

Assignees

@robscott robscott

@youngnick youngnick

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/gep PRs related to Gateway Enhancement Proposal(GEP) lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2.0
Development

Successfully merging this pull request may close these issues.

GEP: Provide a standard way to configure per-Gateway Infrastructure
5 participants
@keithmattix @youngnick @snorwin @robscott @k8s-ci-robot