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

Add request header filter support for gRPC

Open ciarams87 opened this issue 9 months ago • 1 comments

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.

  • [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.

Added support for RequestHeaderFilter for GRPCRoutes

ciarams87 avatar Apr 30 '24 20:04 ciarams87

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Apr 30 '24 20:04 codecov[bot]

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 avatar May 16 '24 09:05 ciarams87