-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
access log: make %DOWNSTREAM_DIRECT_REMOTE_ADDRESS% work correctly with PROXY protocol #10419
access log: make %DOWNSTREAM_DIRECT_REMOTE_ADDRESS% work correctly with PROXY protocol #10419
Conversation
…th PROXY protocol Previously, this access log field would return the value read from the PROXY protocol header instead of using the direct address, as documented. Fixes: envoyproxy#10328 Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
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.
One small typo, but otherwise looks good!
@@ -92,6 +92,32 @@ TEST_P(ProxyProtoIntegrationTest, RouterProxyUnknownLongRequestAndResponseWithBo | |||
testRouterRequestAndResponseWithBody(1024, 512, false, &creator); | |||
} | |||
|
|||
// Test that %DOWNSTREAM_DIRECT_REMOTE_ADDRESS*% returns the direct address, and |
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: extra asterisk here
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.
I added that to note that it also applies to DOWNSTREAM_DIRECT_REMOTE_ADDRESS_WITHOUT_PORT, but maybe I should just enumerate the two.
Signed-off-by: Greg Greenway <ggreenway@apple.com>
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 for the run-around!
Previously, this access log field would return the value read from the
PROXY protocol header instead of using the direct address, as
documented.
Risk Level: Low
Testing: Added integration test
Docs Changes: none
Release Notes: added
Fixes #10328