nginx-gateway-fabric
nginx-gateway-fabric copied to clipboard
Add Support for Gateway-API HTTP Request Mirror
Proposed changes
Add support for Gateway-API HTTP Request Mirroring spec, as described in this guide.
Problem: currently, NGF, does not support http-request mirroring filter.
Solution:
The end goal was to modify the dataplane.Location struct so that servers_template.go file will be able to include a MirrorPath field populated in the dataplane.Location. This field specifies the path to where mirrored requests should be sent:
{{- if $l.MirrorPath }}
mirror {{ $l.MirrorPath }};
{{- end }}
To achieve this, the following steps were taken:
- Create a
MirrorPathfield in thedataplane.Locationstruct. - Create an actual location route entry as target for the mirror directive call:
- With the same specification as the "source" location route (headers, filters, etc.)
- Without the mirror filter that caused the route creation
- With a distinct name based on the "target" service name, namespace, and route
Additionally, the gateway-api specification uses a backendRef structure as the mechanism for backend specification.
Therefore, the filter also creates a new BackendRef (if it doesn't exist) mapped to the created target location route.
The biggest challenge, which was solved in a not-so-elegant way, was that target route creation is done by adding new path matches to the Graph (separately from the path holding the filter information). So, when the Graph is converted to a dataplane.Location and the mirror /{target} directive needs to be populated in the template (as shown above), the source path needs to perform similar logic as it did during the Graph creation step for building the target path. In short, two pieces of code from different packages (graph and dataplane ) needed to be doing identical logic at different phases. The not-so-elegant solution was to call methods created in the helpers package.
Testing: Numerous but probably not enough. A lot of investment made to test the graph creation. I manually tested the implementation locally over k8s (actually I'm using k3s), using the resources defined in the examples/cafe-mirror-example, dug into the NGF pod to cheerfully witness the creation of the mirror directive, and the actual mirroring to be working.
Please focus on (optional): Think of another approach to solving the creation of the target path and the source path mirror {/target} directive at different phases. During the testing in graph_test.go I didn't quite understand the logic behind the tls policies and I just made it work, so possibly there are mistakes there
Closes #533
Checklist
Before creating a PR, run through this checklist and mark each as complete.
- [X] I have read the CONTRIBUTING doc
- [X] I have added tests that prove my fix is effective or that my feature works
- [X] I have checked that all unit tests pass after adding my changes
- [X] I have updated necessary documentation
- [X] I have rebased my branch onto main
- [X] I will ensure my PR is targeting the main branch and pulling from my branch from my own fork
Release notes
If this PR introduces a change that affects users and needs to be mentioned in the release notes, please add a brief note that summarizes the change.
Add support for Gateway-API http-request-mirror filter
Hi @amimimor! Welcome to the project! 🎉
Thanks for opening this pull request! Be sure to check out our Contributing Guidelines while you wait for someone on the team to review this.
@amimimor Can you enable the conformance tests for mirroring? Add HTTPRouteRequestMirror and HTTPRouteRequestMultipleMirrors to the list of supported features in the conformance Makefile. You can then follow the steps in the conformance README to run the tests and verify everything passes.
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.
This PR was closed because it has been stalled for 14 days with no activity.
Hi maintainers, I was off to a long vacation... can you reopen this draft so we could continue?
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.
This PR was closed because it has been stalled for 14 days with no activity.