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 request header filter support for gRPC #1909

Merged
merged 8 commits into from May 16, 2024

Conversation

ciarams87
Copy link
Member

@ciarams87 ciarams87 commented Apr 30, 2024

Proposed changes

Problem: As a user of NGF with applications that require GRPC traffic
I want NGF to support a GRPCRouteRule that can handle a request HTTPHeaderFilter
So that I can append specific headers required by my applications.

Solution: Add support for RequestHeaderFilters for GRPCRoutes. This mostly reuses the logic that already exists for HTTPRoutes by converting the GRPCFilter to HTTPFilter, and adapting the existing validation. This makes most of the request header validation shared, so it has been moved to the route_common.go (as well as the corresponding unit tests).

Also added base gRPC headers - Host (which maps to the :authority pseudo-header under the hood), Authority (which needs to be the same value as Host) and X-Forwarded-For which works the same as HTTP and preserves the client IP.

Testing: As well as unit test coverage, local testing using the conformance test echo server:

Config:

apiVersion: gateway.networking.k8s.io/v1alpha2
kind: GRPCRoute
metadata:
  name: backend-v1
spec:
  parentRefs:
  - name: grpcroute-listener-hostname-matching
    sectionName: listener-1
  rules:
  - matches:
    filters:
    - type: RequestHeaderModifier
      requestHeaderModifier:
        set:
        - name: My-Overwrite-Header
          value: this-is-the-only-value
        add:
        - name: Accept-Encoding
          value: compress
        - name: My-cool-header
          value: this-is-an-appended-value
        remove:
        - User-Agent
    backendRefs:
    - name: grpc-infra-backend-v1
      port: 8080

Request and response:

grpcurl -plaintext -proto grpc.proto -authority bar.com -H "my-cool-header: hello" -H "my-overwrite-header: don't see this" ${GW_IP}:${GW_PORT} gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo

{
  "assertions": {
    "fullyQualifiedMethod": "/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo",
    "headers": [
      {
        "key": "grpc-accept-encoding",
        "value": "gzip"
      },
      {
        "key": ":authority",
        "value": "bar.com"
      },
      {
        "key": "accept-encoding",
        "value": "compress"
      },
      {
        "key": "my-cool-header",
        "value": "hello,this-is-an-appended-value"
      },
      {
        "key": "content-type",
        "value": "application/grpc"
      },
      {
        "key": "my-overwrite-header",
        "value": "this-is-the-only-value"
      },
      {
        "key": "x-forwarded-for",
        "value": "172.18.0.3"
      },
      {
        "key": "authority",
        "value": "bar.com"
      }
    ],
    "authority": "bar.com",
    "context": {
      "namespace": "default",
      "pod": "grpc-infra-backend-v1-68866c4db9-vd4ln"
    }
  }
}

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 #1766

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.

Added support for RequestHeaderFilter for GRPCRoutes

@ciarams87 ciarams87 requested review from a team as code owners April 30, 2024 20:49
@github-actions github-actions bot added documentation Improvements or additions to documentation enhancement New feature or request labels Apr 30, 2024
Copy link

codecov bot commented Apr 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.90%. Comparing base (bc01fdf) to head (44f565a).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1909      +/-   ##
==========================================
+ Coverage   86.83%   86.90%   +0.06%     
==========================================
  Files          88       88              
  Lines        6025     6056      +31     
  Branches       50       50              
==========================================
+ Hits         5232     5263      +31     
  Misses        741      741              
  Partials       52       52              

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

@ciarams87 ciarams87 force-pushed the feat/request-header-filter-grpc branch from 3e47a3e to 7116b06 Compare May 1, 2024 18:19
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.

it's great that we don't need to change anything in internal/mode/static/nginx/config package

internal/mode/static/state/graph/grpcroute.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/grpcroute_test.go Outdated Show resolved Hide resolved
@ciarams87 ciarams87 force-pushed the feat/request-header-filter-grpc branch from 211e59a to 60bead2 Compare May 2, 2024 12:34
@ciarams87 ciarams87 force-pushed the feat/request-header-filter-grpc branch from 60bead2 to a414dcb Compare May 2, 2024 18:04
@ciarams87 ciarams87 requested a review from pleshakov May 2, 2024 18:04
@ciarams87 ciarams87 force-pushed the feat/request-header-filter-grpc branch from a414dcb to 1f10820 Compare May 3, 2024 14:07
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.

LGTM docs-wise but this PR is primarily code based so I will not give an approval.

@ciarams87 ciarams87 requested a review from salonichf5 May 14, 2024 08:18
@ciarams87 ciarams87 force-pushed the feat/request-header-filter-grpc branch 2 times, most recently from 50d1102 to 75f1e06 Compare May 14, 2024 14:47
@ciarams87 ciarams87 force-pushed the feat/request-header-filter-grpc branch from 75f1e06 to ebb3ecf Compare May 14, 2024 16:38
@ciarams87 ciarams87 force-pushed the feat/request-header-filter-grpc branch from ebb3ecf to 06416d8 Compare May 15, 2024 08:09
Copy link
Contributor

@salonichf5 salonichf5 left a comment

Choose a reason for hiding this comment

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

are we adding or updating any examples to test this?

LGTM otherwise!

@ciarams87 ciarams87 force-pushed the feat/request-header-filter-grpc branch from a339eb5 to 44f565a Compare May 16, 2024 09:28
@ciarams87
Copy link
Member Author

are we adding or updating any examples to test this?

LGTM otherwise!

@salonichf5 No, configuring this filter is identical to how it's done for HTTPRoute, so adding a further example wouldn't add much value. Additionally, we don't have an easy way to create an echo server like we do for HTTP (in my testing I am using the echo server from the conformance test suite), so there would be additional overhead in creating and maintaining one for use in examples where we want to see the headers.

@ciarams87 ciarams87 enabled auto-merge (squash) May 16, 2024 09:33
@ciarams87 ciarams87 merged commit 2e3a07e into nginxinc:main May 16, 2024
40 checks passed
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 release-notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

GRPCRouteFilter Request HTTPHeaderFilter for GRPCRouteRule
6 participants