nginx-gateway-fabric icon indicating copy to clipboard operation
nginx-gateway-fabric copied to clipboard

Add Support for Gateway-API HTTP Request Mirror

Open amimimor opened this issue 10 months ago • 3 comments

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:

  1. Create a MirrorPath field in the dataplane.Location struct.
  2. 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

amimimor avatar Apr 04 '24 15:04 amimimor

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.

nginx-bot[bot] avatar Apr 04 '24 15:04 nginx-bot[bot]

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

sjberman avatar Apr 11 '24 17:04 sjberman

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.

github-actions[bot] avatar Apr 30 '24 02:04 github-actions[bot]

This PR was closed because it has been stalled for 14 days with no activity.

github-actions[bot] avatar May 14 '24 02:05 github-actions[bot]

Hi maintainers, I was off to a long vacation... can you reopen this draft so we could continue?

amimimor avatar May 15 '24 07:05 amimimor

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.

github-actions[bot] avatar May 30 '24 02:05 github-actions[bot]

This PR was closed because it has been stalled for 14 days with no activity.

github-actions[bot] avatar Jun 13 '24 02:06 github-actions[bot]