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

ip-tagging filter: add support for an optional ip-tag-header field #36434

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
+44 −2
Open

ip-tagging filter: add support for an optional ip-tag-header field #36434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
49b77ae
add the new header field in proto
Radha13 Oct 2, 2024
7febf6a
use the new header filed + make a getter
Radha13 Oct 2, 2024
0da4790
use the new header if it's provided instead of x-envoy-ip-tags
Radha13 Oct 2, 2024
131ee4d
fix format
Radha13 Oct 2, 2024
68bb688
fix syntax here as well
Radha13 Oct 2, 2024
e59ea3a
add tests
Radha13 Oct 3, 2024
5ea7f0f
change the name of header field to something more consistent with the
Radha13 Oct 3, 2024
ab02a10
fix format
Radha13 Oct 3, 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
5 changes: 5 additions & 0 deletions api/envoy/extensions/filters/http/ip_tagging/v3/ip_tagging.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
// IP tagging :ref:`configuration overview <config_http_filters_ip_tagging>`.
// [#extension: envoy.filters.http.ip_tagging]

// [#next-free-field: 6]
message IPTagging {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.filter.http.ip_tagging.v2.IPTagging";
Expand Down Expand Up @@ -52,6 +53,10 @@ message IPTagging {
repeated config.core.v3.CidrRange ip_list = 2;
}

// optional header to use for ip-tagging instead of default
Copy link
Contributor

Choose a reason for hiding this comment

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

style-nit: sentence starts with capital letter and ends with '.'.

I suggest explicitly writing the name of the default header name.

string ip_tag_header = 5
[(validate.rules).string = {well_known_regex: HTTP_HEADER_NAME strict: false}];

// The type of request the filter should apply to.
RequestType request_type = 1 [(validate.rules).enum = {defined_only: true}];

Expand Down
10 changes: 8 additions & 2 deletions source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ IpTaggingFilterConfig::IpTaggingFilterConfig(
stat_name_set_(scope.symbolTable().makeSet("IpTagging")),
stats_prefix_(stat_name_set_->add(stat_prefix + "ip_tagging")),
no_hit_(stat_name_set_->add("no_hit")), total_(stat_name_set_->add("total")),
unknown_tag_(stat_name_set_->add("unknown_tag.hit")) {
unknown_tag_(stat_name_set_->add("unknown_tag.hit")), header_(config.ip_tag_header()) {

// Once loading IP tags from a file system is supported, the restriction on the size
// of the set should be removed and observability into what tags are loaded needs
Expand Down Expand Up @@ -82,7 +82,13 @@ Http::FilterHeadersStatus IpTaggingFilter::decodeHeaders(Http::RequestHeaderMap&

if (!tags.empty()) {
const std::string tags_join = absl::StrJoin(tags, ",");
headers.appendEnvoyIpTags(tags_join, ",");

// use the optional header instead of the default if it's provided.
Copy link
Contributor

Choose a reason for hiding this comment

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

style-nit: start comment with capital letter.

if (!config_->ip_tag_header().get().empty()) {
headers.appendCopy(config_->ip_tag_header(), tags_join);
} else {
headers.appendEnvoyIpTags(tags_join, ",");
}

// We must clear the route cache or else we can't match on x-envoy-ip-tags.
callbacks_->downstreamCallbacks()->clearRouteCache();
Expand Down
3 changes: 3 additions & 0 deletions source/extensions/filters/http/ip_tagging/ip_tagging_filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ class IpTaggingFilterConfig {
FilterRequestType requestType() const { return request_type_; }
const Network::LcTrie::LcTrie<std::string>& trie() const { return *trie_; }

const Http::LowerCaseString& ip_tag_header() const { return header_; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment here, stating that empty string implies that no ip_tag_header is set.

I do wonder if it just makes more sense to return the correct ip_tag_header here (either the default or the alternative). On the one hand I think it may make the code a bit more readable, but then the calls to the headers.append will need to be a bit different.


void incHit(absl::string_view tag) {
incCounter(stat_name_set_->getBuiltin(absl::StrCat(tag, ".hit"), unknown_tag_));
}
Expand Down Expand Up @@ -71,6 +73,7 @@ class IpTaggingFilterConfig {
const Stats::StatName total_;
const Stats::StatName unknown_tag_;
std::unique_ptr<Network::LcTrie::LcTrie<std::string>> trie_;
const Http::LowerCaseString header_;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: rename to alt_header_ or something ip_tag_header_ to make it more readable.

};

using IpTaggingFilterConfigSharedPtr = std::shared_ptr<IpTaggingFilterConfig>;
Expand Down
28 changes: 28 additions & 0 deletions test/extensions/filters/http/ip_tagging/ip_tagging_filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,34 @@ TEST_F(IpTaggingFilterTest, AppendEntry) {
EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers));
}

TEST_F(IpTaggingFilterTest, AppendEntryOptionalHeader) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
TEST_F(IpTaggingFilterTest, AppendEntryOptionalHeader) {
TEST_F(IpTaggingFilterTest, AppendEntryAlternateHeader) {

const std::string internal_request_yaml = R"EOF(
request_type: internal
ip_tag_header: x-envoy-optional-header
ip_tags:
- ip_tag_name: internal_request_with_optional_header
ip_list:
- {address_prefix: 1.2.3.4, prefix_len: 32}
)EOF";

initializeFilter(internal_request_yaml);

Http::TestRequestHeaderMapImpl request_headers{{"x-envoy-internal", "true"},
{"x-envoy-optional-header", "foo"}};
Network::Address::InstanceConstSharedPtr remote_address =
Network::Utility::parseInternetAddressNoThrow("1.2.3.4");
filter_callbacks_.stream_info_.downstream_connection_info_provider_->setRemoteAddress(
remote_address);

EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false));
EXPECT_EQ("foo,internal_request_with_optional_header",
request_headers.get_("x-envoy-optional-header"));

EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data_, false));
Http::TestRequestTrailerMapImpl request_trailers;
EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers));
}

TEST_F(IpTaggingFilterTest, NestedPrefixes) {
const std::string duplicate_request_yaml = R"EOF(
request_type: both
Expand Down