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

docs: Clarify UPSTREAM_REMOTE_ADDRESS and UPSTREAM_HOST access log variables are the same #29800

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

GUI
Copy link
Contributor

@GUI GUI commented Sep 25, 2023

These access log variables use identical code so they should always be equivalent. I was initially wondering how these variables differed, but after digging in, I found they have identical source code, so I thought this might be worth pointing out in the docs that these reflect the same value.

I suppose one of these could be deprecated if you all wanted to eliminate the duplication, but that would obviously be a bigger change. In the meantime, I thought clarifying the fact that they are the same might help clear up any confusion, like my own. It seems like UPSTREAM_REMOTE_ADDRESS was added more recently (#19613) and is part of a more consistent set of variables, but UPSTREAM_HOST has been around much longer.

Current identical implementation between these two variables on main:

{"UPSTREAM_HOST",
{CommandSyntaxChecker::COMMAND_ONLY,
[](const std::string&, absl::optional<size_t>) {
return StreamInfoAddressFormatterProvider::withPort(
[](const StreamInfo::StreamInfo& stream_info)
-> std::shared_ptr<const Envoy::Network::Address::Instance> {
if (stream_info.upstreamInfo() && stream_info.upstreamInfo()->upstreamHost()) {
return stream_info.upstreamInfo()->upstreamHost()->address();
}
return nullptr;
});
}}},

{"UPSTREAM_REMOTE_ADDRESS",
{CommandSyntaxChecker::COMMAND_ONLY,
[](const std::string&, absl::optional<size_t>) {
return StreamInfoAddressFormatterProvider::withPort(
[](const StreamInfo::StreamInfo& stream_info)
-> std::shared_ptr<const Envoy::Network::Address::Instance> {
if (stream_info.upstreamInfo() && stream_info.upstreamInfo()->upstreamHost()) {
return stream_info.upstreamInfo()->upstreamHost()->address();
}
return nullptr;
});
}}},

Also the same implementation on the v1.27.0 tag:

https://github.com/envoyproxy/envoy/blob/v1.27.0/source/common/formatter/substitution_formatter.cc#L1128-L1140
https://github.com/envoyproxy/envoy/blob/v1.27.0/source/common/formatter/substitution_formatter.cc#L1205-L1217

Commit Message: docs: Clarify UPSTREAM_REMOTE_ADDRESS and UPSTREAM_HOST are the same
Additional Description: N/A
Risk Level: Low (docs only)
Testing: N/A
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

@repokitteh-read-only
Copy link

Docs for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/29800/docs/index.html

The docs are (re-)rendered each time the CI envoy-presubmit (precheck docs) job completes.

🐱

Caused by: #29800 was opened by GUI.

see: more, trace.

@repokitteh-read-only
Copy link

Hi @GUI, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #29800 was opened by GUI.

see: more, trace.

Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

Yep, they are identical. Thanks for clarifying the docs!

Please fix DCO in your PR, as described here: https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md#fixing-dco

/wait

These access log variables use identical code so they should always be
equivalent.

Signed-off-by: Nick Muerdter <12112+GUI@users.noreply.github.com>
@GUI
Copy link
Contributor Author

GUI commented Sep 26, 2023

@ggreenway: Ah, sorry I missed the DCO stuff. That should be fixed in the latest commit. Thanks for pointing that out!

@ggreenway
Copy link
Contributor

I suppose one of these could be deprecated if you all wanted to eliminate the duplication

I think it's probably not worthwhile. It would require getting people using the deprecated one to change their config, and it's not very difficult to keep supporting both forever.

@ggreenway ggreenway enabled auto-merge (squash) September 26, 2023 20:49
@ggreenway ggreenway merged commit f1369ae into envoyproxy:main Sep 26, 2023
94 checks passed
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.

2 participants