fix: handle multi-value response headers as comma-separated per RFC 7230
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
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 sure, let me adjust the tests then
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.
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!
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!
@patrostkowski still working on this ?
@patrostkowski any news/time to finish your fix ?
This still seems to be worked on right?
@jasmin-terrien @VonNao feel free to pick this one up, there's been inactivity for a long time
fixed with #7436