Ability to disable Envoy adding server headers to responses #4906

vishal-chdhry · 2022-12-07T18:27:01Z

Signed-off-by: Vishal Choudhary contactvishaltech@gmail.com

adds server_header_transformation on HTTPConnectionManager
Fixes: #4359

Signed-off-by: Vishal Choudhary <contactvishaltech@gmail.com>

apis/projectcontour/v1alpha1/contourconfig.go

pkg/config/parameters.go

Signed-off-by: Vishal Choudhary <contactvishaltech@gmail.com>

github-actions · 2023-01-07T00:20:13Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

apis/projectcontour/v1alpha1/contourconfig.go

Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

skriss

Also need to add the new field to https://github.com/projectcontour/contour/blob/main/site/content/docs/main/configuration.md for the config file
We'll need to add some unit testing as well, can add pointers in the next round of review

internal/contourconfig/contourconfiguration.go

skriss · 2023-01-17T15:53:30Z

internal/xdscache/v3/listener.go

@@ -125,6 +125,9 @@ type ListenerConfig struct {
 	// MergeSlashes toggles Envoy's non-standard merge_slashes path transformation option for all listeners.
 	MergeSlashes bool

+	// ServerHeaderTransformation signifies we will not modify the Server header.


Update godoc to reflect enum nature

Suggested change

// ServerHeaderTransformation signifies we will not modify the Server header.

// ServerHeaderTransformation defines the action to be applied to the Server header on the response path.

apis/projectcontour/v1alpha1/contourconfig.go

pkg/config/parameters.go

skriss · 2023-01-18T23:53:12Z

@vishal-chdhry just wanted to let you know that we are planning to cut a release candidate for 1.24 ~next Monday, so if possible it'd be great to get this updated by then.

vishal-chdhry · 2023-01-19T02:00:55Z

@vishal-chdhry just wanted to let you know that we are planning to cut a release candidate for 1.24 ~next Monday, so if possible it'd be great to get this updated by then.

sure, i will be on it right away

Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

vishal-chdhry · 2023-01-19T05:21:09Z

@skriss how should I add the unit tests?

apis/projectcontour/v1alpha1/contourconfig.go

pkg/config/parameters.go

site/content/docs/main/configuration.md

skriss · 2023-01-19T15:59:20Z

Unit tests to add:

https://github.com/projectcontour/contour/blob/main/cmd/contour/servecontext_test.go#L365 (new test case for this field)
https://github.com/projectcontour/contour/blob/main/internal/envoy/v3/listener_test.go#L529 (new test case for this field)
https://github.com/projectcontour/contour/blob/main/internal/xdscache/v3/listener_test.go#L143 (new test case for this field)
https://github.com/projectcontour/contour/blob/main/pkg/config/parameters_test.go (new test function testing the Validate method)
https://github.com/projectcontour/contour/blob/main/internal/featuretests/v3/listeners_test.go#L1245-L1297 (should be able to follow the pattern used for the highlighted test to add a new test for this field)

Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

vishal-chdhry · 2023-01-20T05:23:49Z

Unit tests to add:

@skriss done

skriss

@vishal-chdhry one more round of minor feedback and then I think this is about ready to merge.

cmd/contour/servecontext_test.go

skriss · 2023-01-20T17:03:31Z

internal/xdscache/v3/listener.go

@@ -125,6 +125,9 @@ type ListenerConfig struct {
 	// MergeSlashes toggles Envoy's non-standard merge_slashes path transformation option for all listeners.
 	MergeSlashes bool

+	// ServerHeaderTransformation signifies we will not modify the Server header.


Suggested change

// ServerHeaderTransformation signifies we will not modify the Server header.

// ServerHeaderTransformation defines the action to be applied to the Server header on the response path.

internal/envoy/v3/listener.go

internal/xdscache/v3/listener_test.go

internal/envoy/v3/listener_test.go

internal/featuretests/v3/listeners_test.go

skriss · 2023-01-20T17:23:49Z

apis/projectcontour/v1alpha1/contourconfig.go

+	// When configured as APPEND_IF_ABSENT, ⁣If no Server header is present, append Server server_name If a Server header is present, pass it through.
+	// When configured as PASS_THROUGH, ⁣Pass through the value of the server header, and do not append a header if none is present.
+	//
+	// Values: `OVERWRITE` (default), `APPEND_IF_ABSENT`, `PASS_THROUGH`


@sunjayBhatia WDYT about casing here of the values here? For DNS lookup family we lowercased, but I don't see existing multi-word options. I guess we could switch these to lowercase, but honestly not sure if there's any point in doing so.

actually it's an integer in envoy

hm yeah as you say many if not all the other relevant consts in this package and pkg/config we define are lowercase, so for consistency we could switch to that, since we're not really going to pass the value straight through to Envoy it doesn't really matter

changelogs/unreleased/4906-Vishal-Chdhry-small.md

skriss · 2023-01-20T17:37:14Z

Looks like you need to re-run make generate and commit the results as well.

Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

… disable-envoy-adding-server

Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

skriss

LGTM other than the enum value casing change, which we can change in a follow-up PR so we can get this in. Thanks for the work on this @vishal-chdhry!

unit tests failing

skriss · 2023-01-23T16:17:05Z

Just noticed that there are some unit test failures that need to be addressed.

Signed-off-by: Steve Kriss <krisss@vmware.com>

skriss · 2023-01-23T16:52:53Z

I pushed fixes so we can get this in for the RC.

Signed-off-by: Steve Kriss <krisss@vmware.com>

codecov · 2023-01-23T17:01:37Z

Codecov Report

Merging #4906 (b7cf999) into main (33964d6) will increase coverage by 0.05%.
The diff coverage is 89.79%.

❗ Current head b7cf999 differs from pull request most recent head 147e890. Consider uploading reports for the commit 147e890 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4906      +/-   ##
==========================================
+ Coverage   77.55%   77.60%   +0.05%     
==========================================
  Files         138      138              
  Lines       16789    16837      +48     
==========================================
+ Hits        13021    13067      +46     
- Misses       3513     3516       +3     
+ Partials      255      254       -1

Impacted Files	Coverage Δ
cmd/contour/serve.go	`22.28% <0.00%> (-0.04%)`	⬇️
internal/envoy/v3/listener.go	`98.40% <83.33%> (-0.30%)`	⬇️
cmd/contour/servecontext.go	`80.83% <84.61%> (-0.08%)`	⬇️
internal/contourconfig/contourconfiguration.go	`96.93% <100.00%> (+0.01%)`	⬆️
internal/xdscache/v3/listener.go	`91.41% <100.00%> (+0.07%)`	⬆️
pkg/config/parameters.go	`86.97% <100.00%> (+0.35%)`	⬆️
internal/xds/v3/callbacks.go	`100.00% <0.00%> (ø)`
internal/xds/v3/contour.go	`100.00% <0.00%> (+3.22%)`	⬆️

skriss · 2023-01-23T17:01:39Z

Pushed the lowercasing changes here too.

skriss

LGTM, will wait for @sunjayBhatia to take one more look

sunjayBhatia · 2023-01-23T17:03:19Z

apis/projectcontour/v1alpha1/contourconfig.go

@@ -336,6 +336,18 @@ type EnvoyListenerConfig struct {
 	// +optional
 	DisableMergeSlashes *bool `json:"disableMergeSlashes,omitempty"`

+	// Defines the action to be applied to the Server header on the response path
+	// When configured as overwrite, overwrites any Server header with the contents of server_name.


in this section probably replace server_name with envoy? since we don't make it configurable (i think this was copied from the envoy docs)

sunjayBhatia · 2023-01-23T17:03:32Z

apis/projectcontour/v1alpha1/contourconfig.go

+	// Defines the action to be applied to the Server header on the response path
+	// When configured as overwrite, overwrites any Server header with the contents of server_name.
+	// When configured as append_if_absent, ⁣If no Server header is present, append Server server_name If a Server header is present, pass it through.
+	// When configured as pass_through, pPass through the value of the server header, and do not append a header if none is present.


Suggested change

// When configured as pass_through, pPass through the value of the server header, and do not append a header if none is present.

// When configured as pass_through, pass through the value of the server header, and do not append a header if none is present.

sunjayBhatia · 2023-01-23T17:03:56Z

apis/projectcontour/v1alpha1/contourconfig.go

+type ServerHeaderTransformationType string
+
+const (
+	// Overwrite any Server header with the contents of server_name.


maybe similar here with server_name

sunjayBhatia · 2023-01-23T17:07:22Z

pkg/config/parameters.go

@@ -489,6 +505,14 @@ type Parameters struct {
 	// which strips duplicate slashes from request URL paths.
 	DisableMergeSlashes bool `yaml:"disableMergeSlashes,omitempty"`

+	// Defines the action to be applied to the Server header on the response path
+	// When configured as overwrite, overwrites any Server header with the contents of server_name.


similar comment about server_name

Signed-off-by: Steve Kriss <krisss@vmware.com>

…rojectcontour#4906) Adds support for configuring Envoy's server header transformation, which customizes how Envoy treats the Server header on responses. The Server header can now be passed through as-is or only set to "envoy" if no other value is present, in addition to the default behavior of always setting the Server header to "envoy". Closes projectcontour#4359. Signed-off-by: Vishal Choudhary <contactvishaltech@gmail.com> Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com> Signed-off-by: Steve Kriss <krisss@vmware.com> Co-authored-by: Steve Kriss <krisss@vmware.com>

WIP

487bd4c

Signed-off-by: Vishal Choudhary <contactvishaltech@gmail.com>

vishal-chdhry requested a review from a team as a code owner December 7, 2022 18:27

vishal-chdhry requested review from stevesloka and sunjayBhatia and removed request for a team December 7, 2022 18:27

NIT

7613b45

Signed-off-by: Vishal Choudhary <contactvishaltech@gmail.com>

sunjayBhatia reviewed Dec 21, 2022

View reviewed changes

apis/projectcontour/v1alpha1/contourconfig.go Outdated Show resolved Hide resolved

sunjayBhatia reviewed Dec 21, 2022

View reviewed changes

apis/projectcontour/v1alpha1/contourconfig.go Outdated Show resolved Hide resolved

sunjayBhatia reviewed Dec 21, 2022

View reviewed changes

pkg/config/parameters.go Outdated Show resolved Hide resolved

NIT

d779ee5

Signed-off-by: Vishal Choudhary <contactvishaltech@gmail.com>

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 7, 2023

skriss removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 11, 2023

skriss self-requested a review January 11, 2023 16:30

skriss reviewed Jan 11, 2023

View reviewed changes

apis/projectcontour/v1alpha1/contourconfig.go Outdated Show resolved Hide resolved

vishal-chdhry added 2 commits January 14, 2023 20:22

Merge branch 'main' into disable-envoy-adding-server

73fc2eb

Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

changed server header transformation to enum

5e3b212

Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

vishal-chdhry changed the title ~~WIP: Ability to disable Envoy adding server headers to responses~~ Ability to disable Envoy adding server headers to responses Jan 14, 2023

skriss reviewed Jan 17, 2023

View reviewed changes

vishal-chdhry added 3 commits January 19, 2023 10:39

wrote serverHeaderTransformationType

8b8aa12

Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

added serverHeaderTransformation to config file

0af9fc9

Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

added changelog

b12cce2

Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

vishal-chdhry requested a review from skriss January 19, 2023 05:21

skriss requested changes Jan 19, 2023

View reviewed changes

skriss added the release-note/small A small change that needs one line of explanation in the release notes. label Jan 19, 2023

changed serverheader transform type to string

7022e23

Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

vishal-chdhry added 2 commits January 20, 2023 09:40

NIT

00ec3ec

Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

tests

965212c

Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

vishal-chdhry requested a review from skriss January 20, 2023 05:23

skriss requested changes Jan 20, 2023

View reviewed changes

skriss added release-note/minor A minor change that needs about a paragraph of explanation in the release notes. and removed release-note/small A small change that needs one line of explanation in the release notes. labels Jan 20, 2023

vishal-chdhry added 3 commits January 21, 2023 21:03

test changes

6635ef4

Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

Merge branch 'main' of https://github.com/projectcontour/contour into…

48f434e

… disable-envoy-adding-server

make generate

48b5224

Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

vishal-chdhry requested a review from skriss January 21, 2023 16:03

skriss previously approved these changes Jan 23, 2023

View reviewed changes

fix unit test failures

f74bb55

Signed-off-by: Steve Kriss <krisss@vmware.com>

lowercase field values

b7cf999

Signed-off-by: Steve Kriss <krisss@vmware.com>

skriss approved these changes Jan 23, 2023

View reviewed changes

sunjayBhatia reviewed Jan 23, 2023

View reviewed changes

docs updates

147e890

Signed-off-by: Steve Kriss <krisss@vmware.com>

sunjayBhatia approved these changes Jan 23, 2023

View reviewed changes

skriss merged commit cf769c1 into projectcontour:main Jan 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to disable Envoy adding server headers to responses #4906

Ability to disable Envoy adding server headers to responses #4906

vishal-chdhry commented Dec 7, 2022

github-actions bot commented Jan 7, 2023

skriss left a comment

skriss Jan 17, 2023

skriss Jan 20, 2023 •

edited

Loading

skriss commented Jan 18, 2023

vishal-chdhry commented Jan 19, 2023

vishal-chdhry commented Jan 19, 2023

skriss commented Jan 19, 2023

vishal-chdhry commented Jan 20, 2023

skriss left a comment

skriss Jan 20, 2023 •

edited

Loading

skriss Jan 20, 2023

vishal-chdhry Jan 20, 2023

sunjayBhatia Jan 23, 2023

skriss commented Jan 20, 2023

skriss left a comment

skriss commented Jan 23, 2023

skriss commented Jan 23, 2023

codecov bot commented Jan 23, 2023 •

edited

Loading

skriss commented Jan 23, 2023

skriss left a comment

sunjayBhatia Jan 23, 2023

sunjayBhatia Jan 23, 2023

sunjayBhatia Jan 23, 2023

sunjayBhatia Jan 23, 2023

	// ServerHeaderTransformation signifies we will not modify the Server header.
	// ServerHeaderTransformation defines the action to be applied to the Server header on the response path.

	// When configured as pass_through, pPass through the value of the server header, and do not append a header if none is present.
	// When configured as pass_through, pass through the value of the server header, and do not append a header if none is present.

Ability to disable Envoy adding server headers to responses #4906

Ability to disable Envoy adding server headers to responses #4906

Conversation

vishal-chdhry commented Dec 7, 2022

github-actions bot commented Jan 7, 2023

skriss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

skriss commented Jan 18, 2023

vishal-chdhry commented Jan 19, 2023

vishal-chdhry commented Jan 19, 2023

skriss commented Jan 19, 2023

vishal-chdhry commented Jan 20, 2023

skriss left a comment

Choose a reason for hiding this comment

skriss Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss commented Jan 20, 2023

skriss left a comment

Choose a reason for hiding this comment

skriss commented Jan 23, 2023

skriss commented Jan 23, 2023

codecov bot commented Jan 23, 2023 • edited Loading

Codecov Report

skriss commented Jan 23, 2023

skriss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss Jan 20, 2023 •

edited

Loading

skriss Jan 20, 2023 •

edited

Loading

codecov bot commented Jan 23, 2023 •

edited

Loading