-
Notifications
You must be signed in to change notification settings - Fork 267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Document the new xff_num_trusted_hops feature #479
Conversation
Signed-off-by: Brian Pane <[email protected]>
Signed-off-by: Brian Pane <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the excellent docs. I wasn't involved in the original xff_num_trusted_hops reviews, so I should be a good sounding board to make sure this makes sense to the casual reader.
It is a common case where a service wants to perform analytics based on the origin client's IP | ||
address. Per the lengthy discussion on :ref:`XFF <config_http_conn_man_headers_x-forwarded-for>`, | ||
this can get quite complicated, so Envoy simplifies this by setting *x-envoy-external-address* | ||
to the *trusted client address* (as defined in that XFF discussion) if the request is from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an anchor there and just link with :ref: rather than with description?
additional addresses from XFF: | ||
|
||
* If *use_remote_address* is false and *xff_num_trusted_hops* is set to a value *N* that is | ||
greater than zero, the trusted client address is the *N+1* th address from the right end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if N is larger than the length of the XFF list? Can you state what happens then?
|
||
Example 4: Envoy as internal proxy, with the edge proxy from Example 3 in front of it | ||
Settings: | ||
| use_remote_address = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be false if we're an internal proxy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a bug on my part; I'll fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great docs, some comments on top of @htuch comments.
greater than zero, the trusted client address is the *N+1* th address from the right end | ||
of XFF. | ||
* If *use_remote_address* is true and *xff_num_trusted_hops* is set to a value *N* that is | ||
greater than zero, the trusted client address is the *N* th address from the right end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "Nth"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sphinx noob question: how do I do Nth in Sphinx markup? The doc generation engine didn't like it when I tried *N*th
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry no idea. There is probably some escape character, or just do "Nth". It's not a big deal either way.
|
||
Request details: | ||
| Downstream IP address = 192.0.2.5 | ||
| XFF = "198.51.100.128, 198.51.100.10, 198.51.100.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: I might use something other than "198" when talking about external addresses, just because it's pretty close to "192" for the quick reader. I would use something like "50" or whatever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, especially in this context where differentiating internal and external addresses is important. I picked addresses in the 198.51.100.0/24 range reserved for documentation in RFC5737, but that RFC also allows 203.0.113.0/24 so I'll switch to that.
Result: | ||
| Trusted client address = 192.0.2.5 (last address in XFF is trusted) | ||
| X-Envoy-External-Address is not modified | ||
| Request type = internal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an internal request, but in this case x-envoy-internal will be set to false, because XFF has multiple addresses in it. I realize this is confusing, potentially wrong, etc., but it's the way it is. Not sure how you want to discuss in docs, but I think actually saying what x-envoy-internal would be in each case is probably worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching that! I'll fix the example and add x-envoy-internal.
Signed-off-by: Brian Pane <[email protected]>
greater than zero, the trusted client address is the Nth address from the right end | ||
of XFF. (If the XFF contains fewer than N addresses, Envoy falls back to using the | ||
immediate downstream connection's source address as trusted client address.) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So first off, thank you for this amazing documentation, it makes it really clear what is going on over in Envoy #2559
The goal here is to preserve existing Envoy behavior while allowing configuring xff_num_trusted_hops, right? The one thing I'm still struggling with is the use of the combinations and permutations. If we were designing this from scratch, would there be a gain in having the two separate config options, or would we only want one, where we always considered the first hop to be the direct connected host?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brian-pane LGTM, can you merge master? |
Conflicts: docs/root/configuration/http_conn_man/headers.rst Signed-off-by: Brian Pane <[email protected]>
Envoy issue #2503
Corresponding code PR #2559
Signed-off-by: Brian Pane [email protected]