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

feat: support mergeGateways in EnvoyPatchPolicy #2320

Merged
merged 4 commits into from
Dec 21, 2023

Conversation

cnvergence
Copy link
Member

@cnvergence cnvergence commented Dec 18, 2023

What type of PR is this?

feat: support mergeGateways in EnvoyPatchPolicy

What this PR does / why we need it:
Support EnvoyPatchPolicy when MergeGateways is enabled and make sure that it targets GatewayClass instead Gateway. As when mergeGateways is enabled, there will be one Envoy deployment for all Gateways in the cluster.

Which issue(s) this PR fixes:

Fixes #2308

Copy link

🚀 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.

Copy link

codecov bot commented Dec 18, 2023

Codecov Report

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

Comparison is base (da0d0dc) 64.72% compared to head (24587c7) 64.73%.

Files Patch % Lines
internal/gatewayapi/envoypatchpolicy.go 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2320      +/-   ##
==========================================
+ Coverage   64.72%   64.73%   +0.01%     
==========================================
  Files         113      113              
  Lines       16442    16445       +3     
==========================================
+ Hits        10642    10646       +4     
+ Misses       5130     5129       -1     
  Partials      670      670              

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

@cnvergence cnvergence changed the title support mergeGateways in EnvoyPatchPolicy fix: support mergeGateways in EnvoyPatchPolicy Dec 18, 2023
@cnvergence cnvergence marked this pull request as ready for review December 18, 2023 13:58
@cnvergence cnvergence requested a review from a team as a code owner December 18, 2023 13:58
@arkodg
Copy link
Contributor

arkodg commented Dec 18, 2023

@cnvergence this does make sense, can you please expand on the WHY in the PR description ?

you will also need to update the validation logic

// Ensure policy can only target a Gateway
to make sure targetRef type is KindGatewayClass when mergeGateways is true

this feature will also require some additional docs to explain this to the end user

@cnvergence
Copy link
Member Author

@arkodg right, I will add this to the GatewayClass level
Also need to fix this #2308 (comment)

@cnvergence cnvergence force-pushed the patch-policy-merge-gateways branch from 45cf433 to 5daefc1 Compare December 21, 2023 10:33
@cnvergence cnvergence changed the title fix: support mergeGateways in EnvoyPatchPolicy feat: support mergeGateways in EnvoyPatchPolicy Dec 21, 2023
@cnvergence
Copy link
Member Author

nvm, it works as expected

Copy link
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 !

@zirain
Copy link
Member

zirain commented Dec 21, 2023

/retest

@zirain zirain merged commit a4f0396 into envoyproxy:main Dec 21, 2023
18 checks passed
@cnvergence cnvergence deleted the patch-policy-merge-gateways branch February 6, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

support EnvoyPatchPolicy when enabling mergeGateways
4 participants