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

Expand HTTPRoute Matching #109

Merged
merged 23 commits into from
May 31, 2022
Merged

Expand HTTPRoute Matching #109

merged 23 commits into from
May 31, 2022

Conversation

kate-osborn
Copy link
Contributor

Proposed changes

Adds support for HTTPRoute matching based on headers, query parameters, and method.

The generator package now generates both an external and internal location block for routes that have headers, query parameters, and/or a method defined as a match condition.

The external location block handles the request for a given path and invokes the httpmatches njs module to handle the matching logic. The njs module will compare the request object's attributes to the match conditions. If the request satisfies all the match conditions, the request is routed to the internal location block. Otherwise, a 405 or 404 is returned.

The internal location block proxies the request to the backend service.

If a route's HTTPRouteMatch only defines a path, no internal location block is generated. In this case, the njs module is not called, and the external location block will proxy the request to the backend service.

Other notes:

  • If you'd like to test out the changes, follow the README in the cafe-example
  • The njs module is mounted to the nginx container as a configmap. This requires a couple extra steps on setup. See the README for directions.

Limitations and future work:

  • Only one route per path is configured. This means that only one HTTPRouteMatch will be processed for a given path. Eventually we will need to support multiple routes per path and multiple HTTPRouteMatches per path.
  • Only Exact matching for headers and query parameters is supported.
  • No input validation -- outside of the kubebuilder annotations on the CRDs -- is performed on the HTTPRouteMatch fields. We will need to add custom validation for headers and query parameters.
  • No support for repeated headers.
  • We should use typescript in NJS module. However, this requires additional files to be mounted to the NGINX container and will be easier to implement if we build our own NGINX container image.

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

Adds an njs module to support routing requests based on the
HTTP method, headers, and or query parameters of the request.

Currently, only processes one HTTPRouteMatch per route.
README.md Show resolved Hide resolved
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

Hi @kate-osborn

This looks great! Please see some questions and suggestions

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
examples/cafe-example/cafe-routes.yaml Outdated Show resolved Hide resolved
deploy/manifests/nginx-gateway.yaml Outdated Show resolved Hide resolved
internal/nginx/config/generator.go Outdated Show resolved Hide resolved
internal/nginx/modules/httpmatches.js Outdated Show resolved Hide resolved
internal/nginx/modules/httpmatches.js Outdated Show resolved Hide resolved
internal/nginx/modules/httpmatches_test.js Outdated Show resolved Hide resolved
internal/nginx/modules/httpmatches_test.js Show resolved Hide resolved
Makefile Show resolved Hide resolved
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

@kate-osborn
The changes look good! Just a few suggestions/comments for the new stuff

examples/advanced-routing/cafe-routes.yaml Outdated Show resolved Hide resolved
examples/advanced-routing/README.md Outdated Show resolved Hide resolved
internal/nginx/config/generator.go Outdated Show resolved Hide resolved
internal/nginx/config/generator.go Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

👍

@f5yacobucci f5yacobucci self-requested a review May 27, 2022 22:07
@kate-osborn kate-osborn merged commit cc9210d into nginxinc:main May 31, 2022
@lucacome lucacome added the enhancement New feature or request label Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants