Skip to content
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

Add DOWNSTREAM_LOCAL_ADDRESS header variable #2487

Merged
merged 4 commits into from
Jan 31, 2018

Conversation

mandarjog
Copy link
Contributor

Adds ORIGINAL_DST header variable which is an alias for
DOWNSTREAM_LOCAL_ADDRESS_WITHOUT_PORT.

ORIGINAL_DST is the address that the client initially expected to reach.

Signed-off-by: Mandar U Jog [email protected]

fixes #2484

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comment. Thanks! Please do doc PR that we can review at the same time.

@@ -126,6 +126,12 @@ RequestInfoHeaderFormatter::RequestInfoHeaderFormatter(absl::string_view field_n
return RequestInfo::Utility::formatDownstreamAddressNoPort(
*request_info.downstreamRemoteAddress());
};
} else if (field_name == "ORIGINAL_DST" ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need "ORIGINAL_DST"? Would prefer to only have one and the other is more common with the new layout.

@mandarjog
Copy link
Contributor Author

I think ORiGin_dst is more in-line with the cluster type of the same meaning. It also better describes the content.

@mattklein123
Copy link
Member

mattklein123 commented Jan 31, 2018

I think ORiGin_dst is more in-line with the cluster type of the same meaning. It also better describes the content.

I'm sorry I disagree. We already have precedent for this in the access log formatter and I would like to be consistent: https://github.com/envoyproxy/envoy/blob/master/source/common/access_log/access_log_formatter.cc#L234

You are forwarding the local address, which is useful either in the case of original_dst or not. Feel free to provide ample links in the docs to the original_dst filter, describe the scenario, etc.

Also, if you add a "WITHOUT_PORT" option please also add it to the access log formatter (with tests) so we have parity.

@mandarjog
Copy link
Contributor Author

Ok, I will remove original_dst alias.

@mandarjog mandarjog force-pushed the original_dst branch 3 times, most recently from a51e65b to b07b504 Compare January 31, 2018 12:33
@mandarjog mandarjog changed the title Add ORIGINAL_DST header variable Add DOWNSTREAM_LOCAL_ADDRESS header variable Jan 31, 2018
@htuch
Copy link
Member

htuch commented Jan 31, 2018

Can you merge master to pickup #2490?

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM! Can you add a note to RAW_RELEASE_NOTES.md please?

Adds ORIGINAL_DST header variable which is an alias for
DOWNSTREAM_LOCAL_ADDRESS_WITHOUT_PORT.

ORIGINAL_DST is the address that the client initially expected to reach.

Signed-off-by: Mandar U Jog <[email protected]>
Add DOWNSTREAM_LOCAL_ADDRESS_WITHOUT_PORT to header_formatter
and access_log_formatter.

Signed-off-by: Mandar U Jog <[email protected]>
Signed-off-by: Mandar U Jog <[email protected]>
@mandarjog
Copy link
Contributor Author

@mattklein123 Updated RAW_RELEASE_NOTES.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mattklein123 mattklein123 merged commit 0a3960f into envoyproxy:master Jan 31, 2018
@mandarjog mandarjog deleted the original_dst branch January 31, 2018 21:55
Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add %ORIGINAL_DST% as a header variable
3 participants