api: Support Server header transformations by liorokman · Pull Request #2500 · envoyproxy/gateway

liorokman · 2024-01-25T09:03:56Z

What this PR does / why we need it:

This PR adds support for defining what should be done with the Server header.

The relevant Envoy configuration is here.

This PR additionally moves the SuppressEnvoyHeaders attribute into a headers container in the client traffic policy document.

codecov · 2024-01-25T09:07:50Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (f21f9ff) 63.94% compared to head (b771af6) 63.94%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
...frastructure/kubernetes/proxy/resource_provider.go	50.00%	0 Missing and 4 partials ⚠️
internal/gatewayapi/clienttrafficpolicy.go	83.33%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2500   +/-   ##
=======================================
  Coverage   63.94%   63.94%           
=======================================
  Files         117      117           
  Lines       18057    18062    +5     
=======================================
+ Hits        11546    11550    +4     
- Misses       5758     5761    +3     
+ Partials      753      751    -2

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

arkodg · 2024-02-01T20:40:09Z

api/v1alpha1/clienttrafficpolicy_types.go

we discussed this in the community meeting today, here are some follow up questions

can this use case we solved with the ResponseHeaderFilter ?
https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.HTTPHeaderFilter

can the suppressEnvoyHeaders be represented with another name closer to the goal of the field such as passThrough etc ?

we discussed this in the community meeting today, here are some follow up questions

can this use case we solved with the ResponseHeaderFilter ?
https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.HTTPHeaderFilter

No. The ResponseHeaderFilter unconditionally adds/overwrites a header whose value is given in the filter. This feature's passThrough option will keep the value of the Server header provided by the upstream, and there's no way to express the appendIfAbsent option with ResponseHeaderFilter.

can the suppressEnvoyHeaders be represented with another name closer to the goal of the field such as passThrough etc ?

You meant serverHeaderTransformation, not suppressEnvoyHeaders, right? suppressEnvoyHeaders has nothing to do with passing anything through and isn't the topic of this PR.

I used the name of the underlying feature in Envoy Proxy for the serverHeaderTransformation field. I think passThrough isn't a good name - it's only one of three options, and is exactly the opposite of the overwrite option.

I'm open to any other name you have that you think fits better.

from a user perspective, (correct me if im wrong), what you're really trying to solve here is - hey envoy, dont add any new response headers like x-envoy-* or server
we should come up with a name for this knob to define this opt in behavior

how about a knob called addEnvoyResponseHeaders, making adding envoy headers along with server as opt in, so we supress by default

maybe istio/api#2240 will give a little help.

thanks for sharing this @zirain, this is super useful
how about removing this API and disabling

x-envoy-*

server (passthrough)
by default

and we can introduce APIs in the future to opt into this

So, to summarise the above:

The HeaderSettings structure is removed, and the default behaviour becomes that the X-Envoy- headers are removed and the Server header is defined as passthrough. This is done to try to effectively hide the presence of an Envoy Proxy between the client and the upstream target.

There is no support at this time for allowing the X-Envoy headers. This makes it harder for developers to perform certain tasks, for example when trying to measure latency with the x-envoy-upstream-service-time, or debugging various rewrite rules with the x-envoy-original-path header.

How to proceed? Should I close this PR and create a new PR removing the suppressEnvoyHeaders option and implementing the behavioural changes listed here?

hey @liorokman, agree with 1.
for 2. we can make a opt in API e.g.

headers: enableEnvoyHeaders: true ``

Updated to reflect this thread.

This PR changes the API and keeps the project building, a different PR will actually implement the feature.

Signed-off-by: Lior Okman <lior.okman@sap.com>

arkodg · 2024-02-07T20:48:48Z

api/v1alpha1/clienttrafficpolicy_types.go

+	Headers *HeaderSettings `json:"headers,omitempty"`
+}
+
+// ServerHeaderTransformation specifies the transformation required for the Server header


this is not needed

Missed that. Removed.

internal/gatewayapi/clienttrafficpolicy.go

Signed-off-by: Lior Okman <lior.okman@sap.com>

arkodg

LGTM thanks !

shawnh2 · 2024-02-09T07:53:48Z

/retest

liorokman requested a review from a team as a code owner January 25, 2024 09:03

liorokman mentioned this pull request Jan 30, 2024

feat: Support Client IP Detection using XFF on ClientTrafficPolicy #2535

Merged

arkodg reviewed Feb 1, 2024

View reviewed changes

liorokman added 3 commits February 7, 2024 11:49

api: support Server header transformations

8fb86d2

Signed-off-by: Lior Okman <lior.okman@sap.com>

Fix typo, make linter happy

21a4036

Signed-off-by: Lior Okman <lior.okman@sap.com>

Getting the API into shape based on the reviews.

e775ba9

Signed-off-by: Lior Okman <lior.okman@sap.com>

liorokman force-pushed the ctp-header-settings branch from 9ba81dd to e775ba9 Compare February 7, 2024 10:03

arkodg reviewed Feb 7, 2024

View reviewed changes

internal/gatewayapi/clienttrafficpolicy.go Show resolved Hide resolved

Removed the unneeded enum.

b771af6

Signed-off-by: Lior Okman <lior.okman@sap.com>

arkodg approved these changes Feb 7, 2024

View reviewed changes

arkodg requested review from a team and zirain February 7, 2024 21:20

shawnh2 approved these changes Feb 9, 2024

View reviewed changes

arkodg merged commit 53c9758 into envoyproxy:main Feb 9, 2024

liorokman mentioned this pull request Feb 10, 2024

feat: Suppress 'X-Envoy' headers and pass-through the upstream 'Server' header by default #2585

Merged

davidalger mentioned this pull request Mar 13, 2024

Supression of X-Envoy headers breaks expected output of default log format #2903

Open

liorokman deleted the ctp-header-settings branch August 17, 2024 06:47

Conversation

liorokman commented Jan 25, 2024

Uh oh!

codecov bot commented Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkodg Feb 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkodg Feb 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

shawnh2 commented Feb 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jan 25, 2024 •

edited

Loading

arkodg Feb 3, 2024 •

edited

Loading

arkodg Feb 3, 2024 •

edited

Loading