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 regex matching for headers and query params for HTTPRoute and GRPCRoute #3093

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

salonichf5
Copy link
Contributor

@salonichf5 salonichf5 commented Feb 5, 2025

Proposed changes

Write a clear and concise description that helps reviewers understand the purpose and impact of your changes. Use the
following format:

Problem: Users want to be able to specify RegularExpression as headers and query params type.

Solution: Adds functionality to add RegularExpression type for headers in HTTPRoutes and GRPCRoutes and query params in HTTPRoutes.

Testing: Manual Testing

For HTTPRoutes, manual testing done using Advanced Routing examples

This route configures headers and query params with regex matching for coffee-v3 svc. If the regex match succeeds we receive a response from coffee-v3 svc , otherwise all request are responded by coffee-v1

apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: coffee
spec:
  parentRefs:
  - name: cafe
  hostnames:
  - cafe.example.com
  rules:
  - matches:
    - path:
        type: PathPrefix
        value: /coffee
    backendRefs:
    - name: coffee-v1-svc
      port: 80
  - matches:
    - path:
        type: PathPrefix
        value: /coffee
      headers:
      - name: version
        value: v2
    - path:
        type: PathPrefix
        value: /coffee
      queryParams:
      - name: TEST
        value: v2
    backendRefs:
    - name: coffee-v2-svc
      port: 80
  - matches:
    - path:
        type: PathPrefix
        value: /coffee
      headers:
      - name: headerRegex
        type: RegularExpression
        value: "header-[a-z]{1}"
    - path:
        type: PathPrefix
        value: /coffee
      queryParams:
      - name: queryRegex
        type: RegularExpression
        value: "query-[a-z]{1}"
    backendRefs:
    - name: coffee-v3-svc
      port: 80

Matches the regular expression, responds from coffee-v3

curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/coffee -H "headerRegex:header-a"
Handling connection for 8080
Server address: 10.244.0.111:8080
Server name: coffee-v3-66d58645f4-t2xwq
Date: 05/Feb/2025:01:52:23 +0000
URI: /coffee
Request ID: f37a03870179ced04332162c0ee30c02

Does not match the regular expression, response from coffee-v1

curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/coffee -H "headerRegex:header"
Handling connection for 8080
Server address: 10.244.0.109:8080
Server name: coffee-v1-767764946-jsql5
Date: 05/Feb/2025:01:52:36 +0000
URI: /coffee
Request ID: 96e3902a554461d3d48f082f77bca3a1

For GRPCRoute, header matching with regular expression we are configuring a headerRegex header and request is received by grpc-backend-2

apiVersion: gateway.networking.k8s.io/v1
kind: GRPCRoute
metadata:
  name: grpc-header-matching
spec:
  parentRefs:
  - name: same-namespace
  rules:
  # Matches "version: one"
  - matches:
    - headers:
      - name: version
        value: one
    backendRefs:
    - name: grpc-infra-backend-v1
      port: 8080
  # Matches "version: two"
  - matches:
    - headers:
      - name: version
        value: two
    backendRefs:
    - name: grpc-infra-backend-v2
      port: 8080
  # Matches "headerRegex: grpc-header-[a-z]{1}"
  - matches:
    - headers:
      - name: headerRegex
        value: "grpc-header-[a-z]{1}"
        type: RegularExpression
    backendRefs:
    - name: grpc-infra-backend-v2
      port: 8080

Matches the regular expression

grpcurl -plaintext -proto grpc.proto -authority bar.com -d '{"name": "version two regex"}' -H 'headerRegex: grpc-header-a' ${GW_IP}:${GW_PORT} helloworld.Greeter/SayHello
Handling connection for 8080
{
  "message": "Hello version two regex"
}

Does not match the regular expression

grpcurl -plaintext -proto grpc.proto -authority bar.com -d '{"name": "version two regex"}' -H 'headerRegex: two' ${GW_IP}:${GW_PORT} helloworld.Greeter/SayHello
Handling connection for 8080
ERROR:
  Code: Unimplemented
  Message: unexpected HTTP status code received from server: 404 (Not Found); transport: received unexpected content-type "text/html"

Functional Tests

  • Adds tests to verify HTTPRoute and GRPCRoute with advanced route settings are configured with True/Accepted/True condition.
  • Add test to verify working traffic to the HTTPRoute using headers and query params with type Exact and RegularExpression match type.

Please focus on (optional): If you any specific areas where you would like reviewers to focus their attention or provide
specific feedback, add them here.

Closes #1965
Closes #3101

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.

Adds regex matching for headers and query params for HTTPRoutes and headers for GRPCRoutes.

@github-actions github-actions bot added documentation Improvements or additions to documentation enhancement New feature or request labels Feb 5, 2025
Copy link
Contributor

github-actions bot commented Feb 5, 2025

Deploy Preview will be available once build job completes!

Name Link
😎 Deploy Preview https://frontdoor-test-docs.nginx.com/previews/nginx-gateway-fabric/3093/

@salonichf5 salonichf5 force-pushed the feat/regex-match branch 2 times, most recently from ef6d054 to cd63396 Compare February 5, 2025 16:00
@salonichf5 salonichf5 marked this pull request as ready for review February 5, 2025 16:00
@salonichf5 salonichf5 requested review from a team as code owners February 5, 2025 16:00
@nginx nginx deleted a comment from codecov bot Feb 5, 2025
Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.08%. Comparing base (c9bd90c) to head (4e9fbde).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3093      +/-   ##
==========================================
+ Coverage   90.06%   90.08%   +0.02%     
==========================================
  Files         112      112              
  Lines       11570    11596      +26     
  Branches       50       62      +12     
==========================================
+ Hits        10420    10446      +26     
  Misses       1089     1089              
  Partials       61       61              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sjberman
Copy link
Collaborator

sjberman commented Feb 5, 2025

Since there are no conformance tests for this, it feels like we should probably have a basic functional test for it.

Also, the docs changes should be in the docs repo, not here.

internal/mode/static/nginx/config/servers.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/modules/src/httpmatches.js Outdated Show resolved Hide resolved
internal/mode/static/nginx/modules/src/httpmatches.js Outdated Show resolved Hide resolved
internal/mode/static/state/dataplane/types.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/grpcroute.go Outdated Show resolved Hide resolved
@salonichf5
Copy link
Contributor Author

Since there are no conformance tests for this, it feels like we should probably have a basic functional test for it.
Let me work on that.

Also, the docs changes should be in the docs repo, not here.

Yeah put them here to be reviewed. Once I have approvals for this PR, i'll open it in the docs repo.

Copy link
Contributor

@ADubhlaoich ADubhlaoich left a comment

Choose a reason for hiding this comment

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

Doc changes LGTM, but don't forget to make them in the documentation repository!

https://github.com/nginx/documentation

It should target the ngf release branch, ngf-release-2.0.

site/content/how-to/traffic-management/advanced-routing.md Outdated Show resolved Hide resolved
@github-actions github-actions bot added the tests Pull requests that update tests label Feb 7, 2025
internal/mode/static/nginx/config/servers_test.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/grpcroute.go Show resolved Hide resolved
tests/suite/advanced_routing_test.go Outdated Show resolved Hide resolved
tests/suite/advanced_routing_test.go Outdated Show resolved Hide resolved
tests/suite/advanced_routing_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@bjee19 bjee19 left a comment

Choose a reason for hiding this comment

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

lgtm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request tests Pull requests that update tests
Projects
Status: 🆕 New
4 participants