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

feat: Gateway API RequestRedirect filter translated to combination of kong plugins #3702

Merged
merged 3 commits into from
Mar 10, 2023
Merged

feat: Gateway API RequestRedirect filter translated to combination of kong plugins #3702

merged 3 commits into from
Mar 10, 2023

Conversation

mlavacca
Copy link
Member

@mlavacca mlavacca commented Mar 8, 2023 •
edited
Loading

What this PR does / why we need it:

Which issue this PR fixes:

Fixes #3673 #2955

Special notes for your reviewer:

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@mlavacca mlavacca force-pushed the mlavacca/conformance/redirect-host-and-status branch from 8686891 to 1d753ed Compare March 8, 2023 17:33
@codecov
Copy link

codecov bot commented Mar 9, 2023 •
edited
Loading

Codecov Report

Patch coverage: 67.7% and project coverage change: -0.2 ⚠️

Comparison is base (0fc96e9) 73.4% compared to head (a568d9c) 73.2%.

Additional details and impacted files 
@@           Coverage Diff           @@
##            main   #3702     +/-   ##
=======================================
- Coverage   73.4%   73.2%   -0.2%     
=======================================
  Files        133     133             
  Lines      15817   15885     +68     
=======================================
+ Hits       11621   11642     +21     
- Misses      3443    3480     +37     
- Partials     753     763     +10
Impacted Files Coverage Δ
internal/dataplane/parser/translate_httproute.go 85.7% <67.4%> (-5.0%) ⬇️
internal/dataplane/parser/translate_utils.go 88.5% <100.0%> (-0.4%) ⬇️

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mlavacca mlavacca force-pushed the mlavacca/conformance/redirect-host-and-status branch from 7c5afc2 to b276615 Compare March 9, 2023 13:53
@mlavacca mlavacca marked this pull request as ready for review March 9, 2023 14:12
@mlavacca mlavacca requested a review from a team as a code owner March 9, 2023 14:12
@mlavacca
feat: RequestRedirect filter implementation
b5046f5 
The Gateway API filter RequestRedirect is now supported and translated
to the proper set of kong plugins.

Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
@mlavacca mlavacca force-pushed the mlavacca/conformance/redirect-host-and-status branch from b276615 to b5046f5 Compare March 9, 2023 14:16
@mlavacca mlavacca changed the title Gateway API RequestRedirect filter translated to combination of kong plugins feat: Gateway API RequestRedirect filter translated to combination of kong plugins Mar 9, 2023
pmalek
pmalek reviewed Mar 9, 2023
View reviewed changes
internal/dataplane/parser/translate_httproute_test.go Outdated Show resolved Hide resolved
internal/dataplane/parser/translate_httproute.go Show resolved Hide resolved
internal/dataplane/parser/translate_httproute.go Outdated Show resolved Hide resolved
internal/dataplane/parser/translate_httproute.go Outdated Show resolved Hide resolved
internal/dataplane/parser/translate_httproute.go Outdated Show resolved Hide resolved
internal/dataplane/parser/translate_httproute.go Outdated Show resolved Hide resolved
internal/dataplane/parser/translate_httproute.go Outdated Show resolved Hide resolved
internal/dataplane/parser/translate_httproute.go Show resolved Hide resolved
@mlavacca
review's comments addressed
a6d3851 
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
@mlavacca mlavacca requested a review from pmalek March 9, 2023 17:21
rainest
rainest approved these changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

Approving since points other than https://github.com/Kong/kubernetes-ingress-controller/pull/3702/files#r1131141596 are addressed. That last one I think is a symptom of a broader UX issue that we need to address as GWAPI moves towards GA--I don't think we're handling optional not implemented stuff in any consistent way at present, and there's no immediate way to address it well here, so probably needs an n+1 or n+2 release follow-up.

Not merging immediately given that's maybe an open discussion, but I think a good solution is likely out of scope for 2.9.

@mlavacca mlavacca enabled auto-merge (squash) March 10, 2023 11:27
@mlavacca mlavacca merged commit f628552 into main Mar 10, 2023
@mlavacca mlavacca deleted the mlavacca/conformance/redirect-host-and-status branch March 10, 2023 13:12
@mlavacca mlavacca mentioned this pull request Mar 14, 2023
Closed
5 tasks
This was referenced May 5, 2023
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rainest rainest rainest approved these changes

@pmalek pmalek pmalek approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Standard conformance failures
3 participants
@mlavacca @rainest @pmalek