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.

Sign up for GitHub

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

Jump to bottom

ratelimit: masked_remote_address action don't return descriptor_entry with statsd key words #36275

Closed
wants to merge 3 commits into from
Closed

ratelimit: masked_remote_address action don't return descriptor_entry with statsd key words #36275

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9cbe9d8
ratelimit: masked_remote_address action don't return descriptor_entry…
zirain Sep 21, 2024
5363124
nit
zirain Sep 21, 2024
09dab76
fix test
zirain Sep 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion source/common/router/router_ratelimit.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
#include "source/common/matcher/matcher.h"
#include "source/common/protobuf/utility.h"

#include "absl/strings/str_replace.h"

namespace Envoy {
namespace Router {

Expand Down Expand Up @@ -174,7 +176,14 @@ bool MaskedRemoteAddressAction::populateDescriptor(RateLimit::DescriptorEntry& d
// from addressAsString this is a valid address.
Network::Address::CidrRange cidr_entry =
*Network::Address::CidrRange::create(remote_address->ip()->addressAsString(), mask_len);
descriptor_entry = {"masked_remote_address", cidr_entry.asString()};
std::string masked_address = cidr_entry.asString();
if (remote_address->ip()->version() == Network::Address::IpVersion::v4) {
// For Ipv4, masked_address is returned in a format similar to '192.168.1.0/32', '.' It's a key
// word in statsd, This makes it difficult to export measurements in Promethean format in the
// RateLimit server.
masked_address = absl::StrReplaceAll(masked_address, {{".", "_"}});
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it might be a breaking change for many people.

Envoy supports many consumers of stats and I feel like we shouldn't tweak naming in a common subsystem to tune to just one of them.

Regarding Prometheus: why not just generate Prom format directly? Why go via statsd?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems like it might be a breaking change for many people.

we can either add a runtime feature flag or add an optional API field for this.

Envoy supports many consumers of stats and I feel like we shouldn't tweak naming in a common subsystem to tune to just one of them.

this problem mainly related to envoyproxy/ratelimit, I spend a lot of time in the past trying to bring native promemtheus to it but failed, becase it requires to change a lot of libs used by envoyproxy/ratelimit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this change is going to spawn a lot of other changes as well. THere are a ton of places where IP addresses and other things with dots show up in stat names and I'm unsure why this one is different.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general I'd like to see specific needs of individual sinks addressed in the sink, ideally, not in code that generates stat names that would also be used by other sinks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you're right. After rethinking, any ratelimit action that support custom descriptor value will have same problem when user specify ..

let me try to fix this in evnoyproxy/ratelimit.

}
descriptor_entry = {"masked_remote_address", masked_address};

return true;
}
Expand Down
8 changes: 4 additions & 4 deletions test/common/router/router_ratelimit_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,10 +376,10 @@ TEST_F(RateLimitPolicyEntryTest, MaskedRemoteAddressIpv4Default) {
rate_limit_entry_->populateDescriptors(descriptors_, "", header_, stream_info_);
rate_limit_entry_->populateLocalDescriptors(local_descriptors_, "", header_, stream_info_);
EXPECT_THAT(
std::vector<Envoy::RateLimit::Descriptor>({{{{"masked_remote_address", "10.0.0.1/32"}}}}),
std::vector<Envoy::RateLimit::Descriptor>({{{{"masked_remote_address", "10_0_0_1/32"}}}}),
testing::ContainerEq(descriptors_));
EXPECT_THAT(std::vector<Envoy::RateLimit::LocalDescriptor>(
{{{{"masked_remote_address", "10.0.0.1/32"}}}}),
{{{{"masked_remote_address", "10_0_0_1/32"}}}}),
testing::ContainerEq(local_descriptors_));
}

Expand All @@ -395,10 +395,10 @@ TEST_F(RateLimitPolicyEntryTest, MaskedRemoteAddressIpv4) {
rate_limit_entry_->populateDescriptors(descriptors_, "", header_, stream_info_);
rate_limit_entry_->populateLocalDescriptors(local_descriptors_, "", header_, stream_info_);
EXPECT_THAT(
std::vector<Envoy::RateLimit::Descriptor>({{{{"masked_remote_address", "10.0.0.0/16"}}}}),
std::vector<Envoy::RateLimit::Descriptor>({{{{"masked_remote_address", "10_0_0_0/16"}}}}),
testing::ContainerEq(descriptors_));
EXPECT_THAT(std::vector<Envoy::RateLimit::LocalDescriptor>(
{{{{"masked_remote_address", "10.0.0.0/16"}}}}),
{{{{"masked_remote_address", "10_0_0_0/16"}}}}),
testing::ContainerEq(local_descriptors_));
}

Expand Down
Loading