Skip to content

feat: support multiple GatewayClass per controller#2298

Merged
zirain merged 7 commits intoenvoyproxy:mainfrom
cnvergence:feature-multiple-gc
Feb 17, 2024
Merged

feat: support multiple GatewayClass per controller#2298
zirain merged 7 commits intoenvoyproxy:mainfrom
cnvergence:feature-multiple-gc

Conversation

@cnvergence
Copy link
Copy Markdown
Member

@cnvergence cnvergence commented Dec 12, 2023 •
edited
Loading

What type of PR is this?

What this PR does / why we need it:

  • supporting multiple GatewayClass with different infrastructure deployment types
  • extend Provider Resources watchable interface with GatewayClassResources map
  • initial e2e test for multiple GatewayClass scenario

Which issue(s) this PR fixes:

Fixes #1231

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 12, 2023

🚀 Thank you for contributing to the Envoy Gateway project! 🚀

Before merging, please ensure to follow the process below:

  1. Requesting Reviews:
  • cc @envoyproxy/gateway-reviewers team for an initial review.
  • After the initial review, reviewers should request the @envoyproxy/gateway-maintainers team for further review.
  1. Review Approval:
  • Your PR needs to receive at least two approvals.
  • At least one approval must come from a member of the gateway-maintainers team.

NOTE: Once your PR is under review, please do not rebase and force push it. Otherwise, it will force your reviewers to review the PR from scratch rather than simply look at your latest changes.

What's more, you can help expedite the processing of your PR by
  • Ensuring you have self-reviewed your work according to the project's Contribution Guidelines.
  • If your PR addresses a specific issue, make sure to mention it in the PR description.
  • Respond promptly if there are any test failures or suggestions for improvements that we comment on.

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 12, 2023 •
edited
Loading

Codecov Report

Attention: 203 lines in your changes are missing coverage. Please review.

Comparison is base (f7df0e2) 63.53% compared to head (18f3cc8) 63.41%.

Files Patch % Lines
internal/provider/kubernetes/controller.go 42.85% 67 Missing and 17 partials ⚠️
internal/gatewayapi/runner/runner.go 0.00% 79 Missing and 1 partial ⚠️
internal/gatewayapi/resource.go 0.00% 22 Missing ⚠️
internal/provider/kubernetes/predicates.go 11.11% 14 Missing and 2 partials ⚠️
internal/message/types.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2298      +/-   ##
==========================================
- Coverage   63.53%   63.41%   -0.12%     
==========================================
  Files         119      119              
  Lines       19236    19227       -9     
==========================================
- Hits        12221    12193      -28     
- Misses       6200     6222      +22     
+ Partials      815      812       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Xunzhuo
Copy link
Copy Markdown
Member

Xunzhuo commented Dec 21, 2023

/milestone v1.0.0-rc1

@github-actions github-actions bot added this to the v1.0.0-rc1 milestone Dec 21, 2023
@Xunzhuo
Copy link
Copy Markdown
Member

Xunzhuo commented Dec 21, 2023

/assign

@Xunzhuo Xunzhuo added kind/feature new feature and removed kind/feature new feature labels Dec 21, 2023
@cnvergence cnvergence force-pushed the feature-multiple-gc branch 3 times, most recently from 2213aa3 to b04f6b2 Compare January 15, 2024 17:12
@cnvergence cnvergence force-pushed the feature-multiple-gc branch 2 times, most recently from c7c4535 to bd1588b Compare January 18, 2024 16:06
@cnvergence
Copy link
Copy Markdown
Member Author

cnvergence commented Jan 25, 2024

/retest

@cnvergence cnvergence marked this pull request as ready for review January 26, 2024 09:35
@cnvergence cnvergence requested a review from a team as a code owner January 26, 2024 09:35
@cnvergence cnvergence added the area/infra-mgr Issues related to the provisioner used for provisioning the managed Envoy Proxy fleet. label Jan 26, 2024
arkodg
arkodg reviewed Jan 26, 2024
View reviewed changes
internal/provider/kubernetes/controller.go Outdated
Copy link
Copy Markdown
Contributor

@arkodg arkodg Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

seeing a bug

Possible solution

  • filter on relevant keys (try and match GC) when cleaning
  • or keep it flat, and pass 1 provider resource to the gateway-api layer with a list of GCs (the gateway-api translator can still work on 1 GC at a time)

Copy link
Copy Markdown
Member Author

@cnvergence cnvergence Jan 29, 2024

Choose a reason for hiding this comment

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

right I checked it out, thanks @arkodg

@cnvergence cnvergence added the hold do not merge label Jan 29, 2024
@cnvergence
Copy link
Copy Markdown
Member Author

cnvergence commented Feb 1, 2024

/retest

@cnvergence cnvergence force-pushed the feature-multiple-gc branch 2 times, most recently from a4efdc9 to b0c19d2 Compare February 5, 2024 19:17
@cnvergence cnvergence removed the hold do not merge label Feb 5, 2024
@cnvergence
Copy link
Copy Markdown
Member Author

cnvergence commented Feb 6, 2024

/retest

@cnvergence cnvergence added the kind/enhancement New feature or request label Feb 6, 2024
arkodg
arkodg reviewed Feb 13, 2024
View reviewed changes
internal/gatewayapi/runner/runner.go Outdated
Copy link
Copy Markdown
Contributor

@arkodg arkodg Feb 13, 2024

Choose a reason for hiding this comment

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

this logic looks great, thanks !

arkodg
arkodg reviewed Feb 13, 2024
View reviewed changes
internal/gatewayapi/resource.go Outdated
Copy link
Copy Markdown
Contributor

@arkodg arkodg Feb 13, 2024

Choose a reason for hiding this comment

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

non blocking comment that can be tackled in a new PR - would a list *Resources[] be a better option here ?

Copy link
Copy Markdown
Member Author

@cnvergence cnvergence Feb 14, 2024

Choose a reason for hiding this comment

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

could be easier to handle indeed, for merged gateways we can still match it differently

arkodg
arkodg reviewed Feb 13, 2024
View reviewed changes
@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Feb 13, 2024

minor comments, but this is looking good @cnvergence ! massive feature, that im hoping lands in v1.
0.0

@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Feb 15, 2024

@cnvergence looks like e2e is failing

@cnvergence
Copy link
Copy Markdown
Member Author

cnvergence commented Feb 15, 2024

/retest

@cnvergence
Copy link
Copy Markdown
Member Author

cnvergence commented Feb 15, 2024

fixed those issues

arkodg
arkodg approved these changes Feb 16, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@arkodg arkodg requested review from a team February 16, 2024 03:29
@arkodg arkodg assigned cnvergence and unassigned Xunzhuo Feb 16, 2024
@cnvergence
reconcile multiple gatewayclasses per controller
d5c89c1 
Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
@cnvergence
filter gateway-api infra layer by gc label
6b9507c 
Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
@cnvergence
wip: modify watchable message for gatewayapi translate
1943467 
Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
@cnvergence
gen deepcopy for gatewayclassresources
b92939b 
Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
@cnvergence
add comments to deepcopy gen
20f5f8c 
Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
@cnvergence
fix store order
21970e0 
Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
shawnh2
shawnh2 approved these changes Feb 17, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@shawnh2 shawnh2 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@zirain zirain merged commit a5125bf into envoyproxy:main Feb 17, 2024
@shawnh2 shawnh2 mentioned this pull request Feb 17, 2024
Closed
@cnvergence cnvergence deleted the feature-multiple-gc branch February 19, 2024 14:57
@cnvergence cnvergence mentioned this pull request May 10, 2024
Closed
@zhaohuabing zhaohuabing mentioned this pull request May 22, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zirain zirain zirain approved these changes

@arkodg arkodg arkodg approved these changes

@shawnh2 shawnh2 shawnh2 approved these changes

Assignees

@cnvergence cnvergence

Labels

area/infra-mgr Issues related to the provisioner used for provisioning the managed Envoy Proxy fleet. kind/enhancement New feature or request road-to-ga

Projects

No open projects
Envoy Gateway: The Road to GA
Status: Done

Milestone

v1.0.0-rc1

Development

Successfully merging this pull request may close these issues.

Support multiple GatewayClass

5 participants

@cnvergence @Xunzhuo @arkodg @zirain @shawnh2