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 how to roll out and handle breaking changes to the XFF header #3611

Closed
rgs1 opened this Issue Jun 12, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@rgs1
Copy link
Contributor

rgs1 commented Jun 12, 2018

This is a follow-up for #3607.

#3597 was reverted because it breaks Utility::getLastAddressFromXFF(). We should document that #3610 is a pre-requisite for safely unreverting #3597 and rolling it out.

@mattklein123 mattklein123 added this to the 1.8.0 milestone Jun 12, 2018

@alyssawilk

This comment has been minimized.

Copy link
Contributor

alyssawilk commented Jun 13, 2018

This includes clear docs in the release notes as @rgs1 called out, and reapplying #3609

This should not be done until mid-July to minimize rollout dependencies mentioned in #3610

@alyssawilk

This comment has been minimized.

Copy link
Contributor

alyssawilk commented Jul 9, 2018

Ok, it has been about a month and we've done an intermediate release. Do we think it's worth rolling this forward, or shall we wait a bit longer to lower the odds of folks running up against this? Do we know if anyone does less than monthly releases who isn't picking up the actual release builds?

@mattklein123

This comment has been minimized.

Copy link
Member

mattklein123 commented Jul 9, 2018

I think it's probably fine to just do it, especially since we just crossed a version boundary.

@alyssawilk

This comment has been minimized.

Copy link
Contributor

alyssawilk commented Jul 10, 2018

Ok, just to clearly document in one place.

The original PR #3587 removed whitespace in XFF headers, changing them from the form

X-Forwarded-For: 127.0.0.1, 127.0.0.2
to
X-Forwarded-For: 127.0.0.1,127.0.0.2

Unfortunately, as documented in #3607, Envoy's getLastAddressFromXFF utility assumed there was whitespace, so did not properly parse the new terse form of headers. This resulted in Envoy not properly determining addresses, failing ip tagging, etc.

The parsing issue was fixed with #3610, which landed Wed Jun 13 and made the Envoy 1.7.0 build. It's important to note that if running a multi-Envoy deployment and upgrading from binaries prior to #3610 to binaries after #3828, it is essential to upgrade the upstream Envoys (willing to parse terse XFF) before the downstream Envoys (sending terse XFF).

@rgs1

This comment has been minimized.

Copy link
Contributor

rgs1 commented Jul 10, 2018

lgtm, thanks!

@alyssawilk

This comment has been minimized.

Copy link
Contributor

alyssawilk commented Jul 10, 2018

As mentioned on the bug and seconded by @rgs1 I'm going to err on the side of paranoia and wait for a full release. I've added a calendar reminder to pick this up come October so it doesn't linger forever :-P

@mattklein123 mattklein123 modified the milestones: 1.8.0, 1.9.0 Sep 24, 2018

@alyssawilk alyssawilk self-assigned this Oct 10, 2018

alyssawilk added a commit that referenced this issue Oct 10, 2018

http: removing whitespace when appending x-forwarded-for headers (#4671)
Risk Level: Medium
Testing: updated unit tests
Docs Changes: nope
Release Notes: called out a warning in the release notes
Fixes: #3611

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

aa-stripe added a commit to aa-stripe/envoy that referenced this issue Oct 11, 2018

http: removing whitespace when appending x-forwarded-for headers (env…
…oyproxy#4671)

Risk Level: Medium
Testing: updated unit tests
Docs Changes: nope
Release Notes: called out a warning in the release notes
Fixes: envoyproxy#3611

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment