Skip to content

Conversation

@LiorLieberman
Copy link
Contributor

@LiorLieberman LiorLieberman commented Aug 2, 2023

Adding support for multiple RequestMirror filters per rule in GW API. both GRPC and HTTP

Fixes: #4566

ref: kubernetes-sigs/gateway-api#2199

@github-actions
Copy link

github-actions bot commented Aug 2, 2023

Hi @LiorLieberman! Welcome to our community and thank you for opening your first Pull Request. Someone will review it soon. Thank you for committing to making Contour better. You can also join us on our mailing list and in our channel in the Kubernetes Slack Workspace

Signed-off-by: Lior Lieberman <[email protected]>
@LiorLieberman LiorLieberman force-pushed the gw-api-multiple-mirror-filter branch from 3303086 to e9f451d Compare August 2, 2023 14:22
Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

so far looks good, just one question 👍🏽

@LiorLieberman
Copy link
Contributor Author

@skriss do you have any feedback before I go ahead and add tests for it?

@LiorLieberman LiorLieberman marked this pull request as ready for review August 7, 2023 08:14
@LiorLieberman LiorLieberman requested a review from a team as a code owner August 7, 2023 08:14
@LiorLieberman LiorLieberman requested review from skriss and tsaarni and removed request for a team August 7, 2023 08:14
@LiorLieberman LiorLieberman force-pushed the gw-api-multiple-mirror-filter branch from 3249d4b to c76f0e3 Compare August 7, 2023 08:15
@LiorLieberman LiorLieberman changed the title [gw-api] Support multiple RequestMirror filters per rule in GW API gw-api: Support multiple RequestMirror filters per rule in GW API Aug 8, 2023
Signed-off-by: Lior Lieberman <[email protected]>
@LiorLieberman LiorLieberman force-pushed the gw-api-multiple-mirror-filter branch from 30d9290 to aa37e0b Compare August 8, 2023 08:07
@LiorLieberman
Copy link
Contributor Author

I am not sure I followed all needed. I added the changelog now.
Let me know if something is missing

@sunjayBhatia sunjayBhatia added the release-note/minor A minor change that needs about a paragraph of explanation in the release notes. label Aug 8, 2023
Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

looks like a minor lint issue to fix (can remove the offending comment?)

couple other comments to clean up:

and one to modify according to the new guidance on mirror policy:

  • // Per Gateway API docs: "Specifying a core filter multiple times
    // has unspecified or implementation-specific conformance." Contour
    // chooses to use the first instance of each filter type and ignore
    // subsequent instances.

@LiorLieberman
Copy link
Contributor Author

Thanks @sunjayBhatia, done!

@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #5652 (413e476) into main (d60959e) will decrease coverage by 0.03%.
Report is 8 commits behind head on main.
The diff coverage is 75.43%.

❗ Current head 413e476 differs from pull request most recent head 91e5942. Consider uploading reports for the commit 91e5942 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5652      +/-   ##
==========================================
- Coverage   78.54%   78.51%   -0.03%     
==========================================
  Files         138      138              
  Lines       19046    19058      +12     
==========================================
+ Hits        14959    14964       +5     
- Misses       3803     3808       +5     
- Partials      284      286       +2     
Files Changed Coverage Δ
internal/dag/dag.go 98.70% <ø> (ø)
internal/debug/dot.go 48.67% <0.00%> (-0.88%) ⬇️
internal/dag/accessors.go 86.66% <50.00%> (-0.33%) ⬇️
internal/dag/gatewayapi_processor.go 93.71% <91.30%> (-0.25%) ⬇️
internal/dag/httpproxy_processor.go 91.80% <100.00%> (ø)
internal/envoy/v3/route.go 80.68% <100.00%> (+0.09%) ⬆️

Signed-off-by: Sunjay Bhatia <[email protected]>
Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

made a tiny unit test nit change to prevent another back and forth on comments, otherwise LGTM

@sunjayBhatia sunjayBhatia merged commit 3231bf7 into projectcontour:main Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note/minor A minor change that needs about a paragraph of explanation in the release notes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Gateway API: support multiple RequestMirror filters per rule in HTTPRoute [extended]

2 participants