This is a follow-up for #3607.
Utility::getLastAddressFromXFF(). We should document that #3610 is a pre-requisite for safely unreverting #3597 and rolling it out.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
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?
I think it's probably fine to just do it, especially since we just crossed a version boundary.
Ok, just to clearly document in one place.
The original PR https://github.com/envoyproxy/envoy/pull/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 https://github.com/envoyproxy/envoy/issues/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 https://github.com/envoyproxy/envoy/pull/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 https://github.com/envoyproxy/envoy/pull/3610 to binaries after https://github.com/envoyproxy/envoy/pull/3828, it is essential to upgrade the upstream Envoys (willing to parse terse XFF) before the downstream Envoys (sending terse XFF).
lgtm, thanks!
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