gateway icon indicating copy to clipboard operation
gateway copied to clipboard

fix: handle multi-value response headers as comma-separated per RFC 7230

Open patrostkowski opened this issue 7 months ago • 3 comments

What type of PR is this?

fix

What this PR does / why we need it:

This change updates the translation logic of ResponseHeaderModifier so that when multiple values are specified for a header, they are joined into a single, comma-separated value as required by RFC 7230. As a result, the generated Envoy config now emits one HeaderValueOption per header, with all values joined by commas.

Tested using following config:

apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: backend
  namespace: default
spec:
  hostnames:
  - www.example.com
  parentRefs:
  - group: gateway.networking.k8s.io
    kind: Gateway
    name: eg
  rules:
  - backendRefs:
    - group: ""
      kind: Service
      name: backend
      port: 3000
      weight: 1
    filters:
    - responseHeaderModifier:
        set:
        - name: Cache-Control
          value: private, no-store
      type: ResponseHeaderModifier
    matches:
    - path:
        type: PathPrefix
        value: /
# curl latest image
root@ubuntu:/$ curl -i http://10.96.20.98/ -H 'Host: www.example.com' -H 'X-Echo-Set-Header: Cache-control: value1' | grep ^cache-control
cache-control:  no-store

# curl e4c8f89 image
root@ubuntu:/$ curl -i http://10.96.20.98/ -H 'Host: www.example.com' -H 'X-Echo-Set-Header: Cache-control: value1' | grep ^cache-control
cache-control: private, no-store

Which issue(s) this PR fixes:

Fixes https://github.com/envoyproxy/gateway/issues/5733

Release Notes: Yes/No

patrostkowski avatar Jun 09 '25 20:06 patrostkowski

hey @patrostkowski we use YAML based testcases for better readability

  • e.g. for gateway-api lib - internal/gatewayapi/testdata/grpcroute-with-request-header-modifier.in.yaml
  • for xds translator - internal/xds/translator/testdata/in/xds-ir/http-route-request-headers.yaml

if you run make testdata it will even generate the out files for you

arkodg avatar Jun 12 '25 18:06 arkodg

@arkodg sure, let me adjust the tests then

patrostkowski avatar Jun 13 '25 11:06 patrostkowski

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 70.66%. Comparing base (e0e8a29) to head (2b8a179). :warning: Report is 589 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6281      +/-   ##
==========================================
+ Coverage   70.60%   70.66%   +0.06%     
==========================================
  Files         220      220              
  Lines       36829    36832       +3     
==========================================
+ Hits        26004    26029      +25     
+ Misses       9292     9275      -17     
+ Partials     1533     1528       -5     

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

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Jun 13 '25 11:06 codecov[bot]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions!

github-actions[bot] avatar Jul 13 '25 12:07 github-actions[bot]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions!

github-actions[bot] avatar Aug 29 '25 12:08 github-actions[bot]

@patrostkowski still working on this ?

arkodg avatar Sep 15 '25 23:09 arkodg

@patrostkowski any news/time to finish your fix ?

jasmin-terrien avatar Oct 14 '25 13:10 jasmin-terrien

This still seems to be worked on right?

VonNao avatar Oct 30 '25 16:10 VonNao

@jasmin-terrien @VonNao feel free to pick this one up, there's been inactivity for a long time

arkodg avatar Oct 30 '25 17:10 arkodg

fixed with #7436

arkodg avatar Nov 14 '25 22:11 arkodg