-
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
ip-tagging filter: add support for an optional ip-tag-header field #36434
Changes from 31 commits
49b77ae
7febf6a
0da4790
131ee4d
68bb688
e59ea3a
5ea7f0f
ab02a10
8da6c4f
eccb613
8b21da5
3711e4b
5f62aea
8c17a44
a02c2f5
921226a
d0adb65
8ce800f
2f7ddec
c93feaa
813fb6c
89ca195
437802e
90750fe
664f46e
80988fb
84716e8
7d74269
1bcb45b
7d59e1f
9303f56
c1cc54b
8dc3bdd
649d54d
3033d27
a9c92b7
159fcf0
97b059c
d6b807b
c611942
99819b6
44f3d4e
7f9ccea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -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: 7] | ||||||||
message IPTagging { | ||||||||
option (udpa.annotations.versioning).previous_message_type = | ||||||||
"envoy.config.filter.http.ip_tagging.v2.IPTagging"; | ||||||||
|
@@ -39,6 +40,22 @@ message IPTagging { | |||||||
EXTERNAL = 2; | ||||||||
} | ||||||||
|
||||||||
// Describes how to apply the tags to the headers. | ||||||||
enum HeaderAction { | ||||||||
// (DEFAULT) The header specified in :ref:`ip_tag_header <envoy_v3_api_field_extensions.filters.http.ip_tagging.v3.IPTagging.ip_tag_header>` | ||||||||
// will be dropped, before the tags are applied. Sanitize will be run regardless of if the source is a trusted IP or not. | ||||||||
// | ||||||||
// Note that the header will be visible unsanitized to any filters downstream of the ip-tag-header filter. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, mind adding |
||||||||
SANITIZE = 0; | ||||||||
alyssawilk marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
||||||||
// Tags will be appended to the header specified in | ||||||||
// :ref:`ip_tag_header <envoy_v3_api_field_extensions.filters.http.ip_tagging.v3.IPTagging.ip_tag_header>`, | ||||||||
// leaving original values in place. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
// | ||||||||
// Please note that this could cause the header to retain values set by the http client even if the client is from an untrusted IP. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RE "untrusted IP", see comment above regarding the terms. |
||||||||
APPEND_FORWARD = 1; | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does "FORWARD" imply? I'm assuming the goal is to have something similar to APPEND_IF_EXISTS_OR_ADD. Will it make sense to either use the same enum (probably not as the one suggested adds sanitization), or use the same enum name? |
||||||||
} | ||||||||
|
||||||||
// Supplies the IP tag name and the IP address subnets. | ||||||||
message IPTag { | ||||||||
option (udpa.annotations.versioning).previous_message_type = | ||||||||
|
@@ -52,6 +69,23 @@ message IPTagging { | |||||||
repeated config.core.v3.CidrRange ip_list = 2; | ||||||||
} | ||||||||
|
||||||||
// Optional header to use for ip-tagging instead of using | ||||||||
// default header ``x-envoy-ip-tags``. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again if sanitization is different for CustomHeader than DefaultHeader let's call out here. "This header will be sanitized based on the config in |
||||||||
// | ||||||||
// This header will be sanitized based on the config in | ||||||||
// :ref:`ip_tag_header <envoy_v3_api_field_extensions.filters.http.ip_tagging.v3.IPTagging.ip_tag_header_action>` | ||||||||
// rather than the defaults for x-envoy prefixed headers. | ||||||||
string ip_tag_header = 5 | ||||||||
[(validate.rules).string = {well_known_regex: HTTP_HEADER_NAME strict: false}]; | ||||||||
|
||||||||
// Control if the header in :ref:`ip_tag_header <envoy_v3_api_field_extensions.filters.http.ip_tagging.v3.IPTagging.ip_tag_header>` | ||||||||
// will be sanitized, or be appended to. | ||||||||
alyssawilk marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
// | ||||||||
// This is ignored if :ref:`ip_tag_header <envoy_v3_api_field_extensions.filters.http.ip_tagging.v3.IPTagging.ip_tag_header>` is empty. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this field is closely related to the |
||||||||
// | ||||||||
// Default: *SANITIZE*. | ||||||||
HeaderAction ip_tag_header_action = 6; | ||||||||
|
||||||||
// The type of request the filter should apply to. | ||||||||
RequestType request_type = 1 [(validate.rules).enum = {defined_only: true}]; | ||||||||
|
||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,8 @@ 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")), ip_tag_header_(config.ip_tag_header()), | ||
ip_tag_header_action_(config.ip_tag_header_action()) { | ||
|
||
// 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 | ||
|
@@ -80,13 +81,8 @@ Http::FilterHeadersStatus IpTaggingFilter::decodeHeaders(Http::RequestHeaderMap& | |
std::vector<std::string> tags = | ||
config_->trie().getData(callbacks_->streamInfo().downstreamAddressProvider().remoteAddress()); | ||
|
||
applyTags(headers, tags); | ||
if (!tags.empty()) { | ||
const std::string tags_join = absl::StrJoin(tags, ","); | ||
headers.appendEnvoyIpTags(tags_join, ","); | ||
|
||
// We must clear the route cache or else we can't match on x-envoy-ip-tags. | ||
callbacks_->downstreamCallbacks()->clearRouteCache(); | ||
|
||
// For a large number(ex > 1000) of tags, stats cardinality will be an issue. | ||
// If there are use cases with a large set of tags, a way to opt into these stats | ||
// should be exposed and other observability options like logging tags need to be implemented. | ||
|
@@ -112,6 +108,46 @@ void IpTaggingFilter::setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbac | |
callbacks_ = &callbacks; | ||
} | ||
|
||
void IpTaggingFilter::applyTags(Http::RequestHeaderMap& headers, | ||
const std::vector<std::string>& tags) { | ||
using HeaderAction = IpTaggingFilterConfig::HeaderAction; | ||
|
||
OptRef<const Http::LowerCaseString> header_name = config_->ip_tag_header(); | ||
|
||
if (tags.empty()) { | ||
bool mustSanitize = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this isn't really mustSanitize given we'll only do it if there's a header name configured. Also snake case for Envoy variables There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (I wish there was a linter for this.) <3 |
||
config_->ip_tag_header_action() == HeaderAction::IPTagging_HeaderAction_SANITIZE; | ||
if (header_name.has_value() && mustSanitize) { | ||
if (headers.remove(header_name.value()) != 0) { | ||
// We must clear the route cache in case it held a decision based on the now-removed header. | ||
callbacks_->downstreamCallbacks()->clearRouteCache(); | ||
} | ||
} | ||
return; | ||
} | ||
|
||
const std::string tags_join = absl::StrJoin(tags, ","); | ||
if (!header_name.has_value()) { | ||
// The x-envoy-ip-tags header was cleared at the start of the filter chain. | ||
// We only do append here, so that if multiple ip-tagging filters are run sequentially, | ||
// the behaviour will be backwards compatible. | ||
headers.appendEnvoyIpTags(tags_join, ","); | ||
} else { | ||
switch (config_->ip_tag_header_action()) { | ||
PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; | ||
case HeaderAction::IPTagging_HeaderAction_SANITIZE: | ||
headers.setCopy(header_name.value(), tags_join); | ||
break; | ||
case HeaderAction::IPTagging_HeaderAction_APPEND_FORWARD: | ||
headers.appendCopy(header_name.value(), tags_join); | ||
break; | ||
} | ||
} | ||
|
||
// We must clear the route cache so it can match on the updated value of the header. | ||
callbacks_->downstreamCallbacks()->clearRouteCache(); | ||
} | ||
|
||
} // namespace IpTagging | ||
} // namespace HttpFilters | ||
} // namespace Extensions | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
#include <vector> | ||
|
||
#include "envoy/common/exception.h" | ||
#include "envoy/common/optref.h" | ||
#include "envoy/extensions/filters/http/ip_tagging/v3/ip_tagging.pb.h" | ||
#include "envoy/http/filter.h" | ||
#include "envoy/runtime/runtime.h" | ||
|
@@ -31,6 +32,8 @@ enum class FilterRequestType { INTERNAL, EXTERNAL, BOTH }; | |
*/ | ||
class IpTaggingFilterConfig { | ||
public: | ||
using HeaderAction = envoy::extensions::filters::http::ip_tagging::v3::IPTagging::HeaderAction; | ||
|
||
IpTaggingFilterConfig(const envoy::extensions::filters::http::ip_tagging::v3::IPTagging& config, | ||
const std::string& stat_prefix, Stats::Scope& scope, | ||
Runtime::Loader& runtime); | ||
|
@@ -39,6 +42,14 @@ class IpTaggingFilterConfig { | |
FilterRequestType requestType() const { return request_type_; } | ||
const Network::LcTrie::LcTrie<std::string>& trie() const { return *trie_; } | ||
|
||
OptRef<const Http::LowerCaseString> ip_tag_header() const { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and for functions ipTagHeader and ipTagHeaderAction |
||
if (ip_tag_header_.get().empty()) { | ||
return absl::nullopt; | ||
} | ||
return ip_tag_header_; | ||
} | ||
HeaderAction ip_tag_header_action() const { return ip_tag_header_action_; } | ||
|
||
void incHit(absl::string_view tag) { | ||
incCounter(stat_name_set_->getBuiltin(absl::StrCat(tag, ".hit"), unknown_tag_)); | ||
} | ||
|
@@ -71,6 +82,9 @@ class IpTaggingFilterConfig { | |
const Stats::StatName total_; | ||
const Stats::StatName unknown_tag_; | ||
std::unique_ptr<Network::LcTrie::LcTrie<std::string>> trie_; | ||
const Http::LowerCaseString | ||
ip_tag_header_; // An empty string indicates that no ip_tag_header is set. | ||
const HeaderAction ip_tag_header_action_; | ||
}; | ||
|
||
using IpTaggingFilterConfigSharedPtr = std::shared_ptr<IpTaggingFilterConfig>; | ||
|
@@ -95,6 +109,8 @@ class IpTaggingFilter : public Http::StreamDecoderFilter { | |
void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override; | ||
|
||
private: | ||
void applyTags(Http::RequestHeaderMap& headers, const std::vector<std::string>& tags); | ||
|
||
IpTaggingFilterConfigSharedPtr config_; | ||
Http::StreamDecoderFilterCallbacks* callbacks_{}; | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -192,6 +192,148 @@ TEST_F(IpTaggingFilterTest, AppendEntry) { | |
EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers)); | ||
} | ||
|
||
TEST_F(IpTaggingFilterTest, ReplaceAlternateHeaderWhenActionIsDefaulted) { | ||
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"}}; // foo will be removed | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mind adding multiple just so we regression test that rmove does remove all? |
||
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("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, ReplaceAlternateHeader) { | ||
const std::string internal_request_yaml = R"EOF( | ||
request_type: internal | ||
ip_tag_header: x-envoy-optional-header | ||
ip_tag_header_action: SANITIZE | ||
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"}}; // foo will be removed | ||
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("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, ClearAlternateHeaderWhenUnmatchedAndSanitized) { | ||
const std::string internal_request_yaml = R"EOF( | ||
request_type: internal | ||
ip_tag_header: x-envoy-optional-header | ||
ip_tag_header_action: SANITIZE | ||
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"}}; // header will be removed | ||
Network::Address::InstanceConstSharedPtr remote_address = | ||
Network::Utility::parseInternetAddressNoThrow("1.2.3.5"); | ||
filter_callbacks_.stream_info_.downstream_connection_info_provider_->setRemoteAddress( | ||
remote_address); | ||
|
||
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); | ||
EXPECT_FALSE(request_headers.has("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, AppendForwardAlternateHeader) { | ||
const std::string internal_request_yaml = R"EOF( | ||
request_type: internal | ||
ip_tag_header: x-envoy-optional-header | ||
ip_tag_header_action: APPEND_FORWARD | ||
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, RetainAlternateHeaderWhenUnmatchedAndAppendForwarded) { | ||
const std::string internal_request_yaml = R"EOF( | ||
request_type: internal | ||
ip_tag_header: x-envoy-optional-header | ||
ip_tag_header_action: APPEND_FORWARD | ||
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.5"); | ||
filter_callbacks_.stream_info_.downstream_connection_info_provider_->setRemoteAddress( | ||
remote_address); | ||
|
||
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); | ||
EXPECT_EQ("foo", 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 | ||
|
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'm trying to stick to the same terminology introduced in https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_conn_man/header_sanitizing. I'm assuming that is the goal, and if so, I suggest trying to stick with the same terms.