internal/envoy: Set XffNumTrustedHops to keep header proto intact #3293
Conversation
stevesloka
requested review from
jpeach and
youngnick
and removed request for
a team
|
This still needs docs & feature tests. |
| @@ -86,7 +86,7 @@ default-http-versions: [] | |||
| cluster: | |||
| dns-lookup-family: auto | |||
| ` | |||
| assert.Equal(t, strings.TrimSpace(string(data)), strings.TrimSpace(expected)) | |||
| assert.Equal(t, strings.TrimSpace(expected), strings.TrimSpace(string(data))) | |||
There was a problem hiding this comment.
Debugging tests I noticed this was swapped.
Codecov Report
@@ Coverage Diff @@
## main #3293 +/- ##
==========================================
+ Coverage 75.36% 75.43% +0.06%
==========================================
Files 98 98
Lines 6154 6162 +8
==========================================
+ Hits 4638 4648 +10
+ Misses 1411 1409 -2
Partials 105 105
|
stevesloka
force-pushed
the
numTrustedHops
branch
from
7957655
to
076ed5d
Compare
There was a problem hiding this comment.
LGTM pending an addition I’m guessing you’re going to add here https://github.com/projectcontour/contour/blob/main/site/docs/main/configuration.md
|
@sunjayBhatia yup added docs and a feature test to cover. |
If a user has an external load balancer that terminates TLS, the X-Forwarded-Proto header gets overwritten unless the downstream connection is trusted. This adds a config file option to set the number of trusted hops which will allow the headers to be instact already set from downstream. Fixes projectcontour#3294 Signed-off-by: Steve Sloka <slokas@vmware.com>
stevesloka
force-pushed
the
numTrustedHops
branch
from
25f3c66
to
436659d
Compare
| @@ -106,6 +107,16 @@ The cluster configuration block can be used to configure various parameters for | |||
| {: class="table thead-dark table-bordered"} | |||
| <br> | |||
|
|
|||
| ### Network Configuration | |||
There was a problem hiding this comment.
Do we have any issues we can earmark so we make sure to add configuration to this object?
There was a problem hiding this comment.
There's an issue linked for the max hops, are you referring to the Network Configuration section?
There was a problem hiding this comment.
Yeah, just wondering if we already had other things to add in the network hash we already know about
There was a problem hiding this comment.
Yeah at the moment not sure, things that the chunked length maybe should have gone into there. We can remove the network parent, but was trying to group stuff together in a way.
There was a problem hiding this comment.
I think having the network top-level item is great, we should definitely keep that, and look out for things to put in there.
stevesloka
added
the
release-note
If a user has an external load balancer that terminates TLS, the X-Forwarded-Proto
header gets overwritten unless the downstream connection is trusted.
This adds a config file option to set the number of trusted hops which will
allow the headers to be instact already set from downstream.
Fixes #3294
Signed-off-by: Steve Sloka slokas@vmware.com