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

Add Support for Gateway-API HTTP Request Mirror #1806

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

amimimor
Copy link

@amimimor amimimor commented Apr 4, 2024

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.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • 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

Signed-off-by: Amit Mor <amitm@at-bay.com>
Signed-off-by: Amit Mor <amit.mor@hotmail.com>
Signed-off-by: Amit Mor <amitm@at-bay.com>
Copy link

nginx-bot bot commented Apr 4, 2024

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 nginx-bot bot added the community label Apr 4, 2024
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Apr 4, 2024
Problem: Users need a way to configure the behavior 
between the downstream client and NGF. For example, 
be able to set the client max body size.

Solution: Add the ClientSettingsPolicy CRD, which 
provides a way to configure this behavior. 
Note: this PR contains the CRD only. 
A subsequent PR will add the implementation.
@sjberman sjberman added the enhancement New feature or request label Apr 5, 2024
dependabot bot and others added 5 commits April 5, 2024 09:44
Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 4.1.1 to 4.2.0.
- [Release notes](https://github.com/codecov/codecov-action/releases)
- [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md)
- [Commits](codecov/codecov-action@c16abc2...7afa10e)

---
updated-dependencies:
- dependency-name: codecov/codecov-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [github/codeql-action](https://github.com/github/codeql-action) from 3.24.9 to 3.24.10.
- [Release notes](https://github.com/github/codeql-action/releases)
- [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md)
- [Commits](github/codeql-action@1b1aada...4355270)

---
updated-dependencies:
- dependency-name: github/codeql-action
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Problem: We want a design for GatewayClass level settings that applies to all Gateways, such as `otel_exporter`.

Solution: Add enhancement proposal introducing the `NginxProxy` CRD, which will contain the Gateway settings.

Enhancement Proposal: nginxinc#1775
…from 1.24.0 to 1.25.0 (nginxinc#1812)

Bump go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc

Bumps [go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc](https://github.com/open-telemetry/opentelemetry-go) from 1.24.0 to 1.25.0.
- [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases)
- [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md)
- [Commits](open-telemetry/opentelemetry-go@v1.24.0...v1.25.0)

---
updated-dependencies:
- dependency-name: go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [docker/setup-buildx-action](https://github.com/docker/setup-buildx-action) from 3.2.0 to 3.3.0.
- [Release notes](https://github.com/docker/setup-buildx-action/releases)
- [Commits](docker/setup-buildx-action@2b51285...d70bba7)

---
updated-dependencies:
- dependency-name: docker/setup-buildx-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@@ -0,0 +1,123 @@
# Example
Copy link
Contributor

Choose a reason for hiding this comment

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

We're preferring to write tutorial documentation on new traffic features going forward instead of providing in the examples directory. Here is one example of a doc for redirects. (the DOCS-XXX header can just be DOCS-000)

internal/mode/static/nginx/config/generator.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers_template.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/backend_refs.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/httproute.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/httproute.go Show resolved Hide resolved
@github-actions github-actions bot removed the enhancement New feature or request label Apr 11, 2024
amimimor and others added 2 commits April 11, 2024 08:10
…inc#1817)

Bumps [DavidAnson/markdownlint-cli2-action](https://github.com/davidanson/markdownlint-cli2-action) from 15.0.0 to 16.0.0.
- [Release notes](https://github.com/davidanson/markdownlint-cli2-action/releases)
- [Commits](DavidAnson/markdownlint-cli2-action@510b996...b4c9fea)

---
updated-dependencies:
- dependency-name: DavidAnson/markdownlint-cli2-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@sjberman
Copy link
Contributor

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

dependabot bot and others added 10 commits April 11, 2024 17:29
)

Bumps [sigs.k8s.io/controller-runtime](https://github.com/kubernetes-sigs/controller-runtime) from 0.17.2 to 0.17.3.
- [Release notes](https://github.com/kubernetes-sigs/controller-runtime/releases)
- [Changelog](https://github.com/kubernetes-sigs/controller-runtime/blob/main/RELEASE.md)
- [Commits](kubernetes-sigs/controller-runtime@v0.17.2...v0.17.3)

---
updated-dependencies:
- dependency-name: sigs.k8s.io/controller-runtime
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 4.2.0 to 4.3.0.
- [Release notes](https://github.com/codecov/codecov-action/releases)
- [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md)
- [Commits](codecov/codecov-action@7afa10e...8450866)

---
updated-dependencies:
- dependency-name: codecov/codecov-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [github.com/prometheus/common](https://github.com/prometheus/common) from 0.52.2 to 0.52.3.
- [Release notes](https://github.com/prometheus/common/releases)
- [Commits](prometheus/common@v0.52.2...v0.52.3)

---
updated-dependencies:
- dependency-name: github.com/prometheus/common
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [sigstore/cosign-installer](https://github.com/sigstore/cosign-installer) from 3.4.0 to 3.5.0.
- [Release notes](https://github.com/sigstore/cosign-installer/releases)
- [Commits](sigstore/cosign-installer@e1523de...59acb62)

---
updated-dependencies:
- dependency-name: sigstore/cosign-installer
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [peter-evans/create-pull-request](https://github.com/peter-evans/create-pull-request) from 6.0.2 to 6.0.3.
- [Release notes](https://github.com/peter-evans/create-pull-request/releases)
- [Commits](peter-evans/create-pull-request@70a41ab...c55203c)

---
updated-dependencies:
- dependency-name: peter-evans/create-pull-request
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…/nginx/modules (nginxinc#1825)

Bump @vitest/coverage-v8 in /internal/mode/static/nginx/modules

Bumps [@vitest/coverage-v8](https://github.com/vitest-dev/vitest/tree/HEAD/packages/coverage-v8) from 1.4.0 to 1.5.0.
- [Release notes](https://github.com/vitest-dev/vitest/releases)
- [Commits](https://github.com/vitest-dev/vitest/commits/v1.5.0/packages/coverage-v8)

---
updated-dependencies:
- dependency-name: "@vitest/coverage-v8"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Amit Mor <amitm@at-bay.com>
Signed-off-by: Amit Mor <amit.mor@hotmail.com>
Signed-off-by: Amit Mor <amitm@at-bay.com>
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Apr 15, 2024
Copy link
Contributor

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 github-actions bot added the stale Pull requests/issues with no activity label Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation release-notes stale Pull requests/issues with no activity
Projects
Status: External Pull Requests
Development

Successfully merging this pull request may close these issues.

HTTPRequestMirrorFilter (Traffic Mirroring)
3 participants