Skip to content

Commit

Permalink
rbac: rules_stat_prefix to emit metrics with a prefix (envoyproxy#31835)
Browse files Browse the repository at this point in the history
This is akin to shadow_rules_stat_prefix but for non-shadowing rules.
Since only shadow rules emit dynamic metadata, this prefix only applies
to metrics.

---------

Signed-off-by: Thomas van Noort <[email protected]>
  • Loading branch information
thomasvnoort authored Mar 19, 2024
1 parent 307218e commit 7fec609
Show file tree
Hide file tree
Showing 12 changed files with 152 additions and 72 deletions.
7 changes: 6 additions & 1 deletion api/envoy/extensions/filters/http/rbac/v3/rbac.proto
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
// [#extension: envoy.filters.http.rbac]

// RBAC filter config.
// [#next-free-field: 6]
// [#next-free-field: 7]
message RBAC {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.filter.http.rbac.v2.RBAC";
Expand All @@ -34,6 +34,11 @@ message RBAC {
config.rbac.v3.RBAC rules = 1
[(udpa.annotations.field_migrate).oneof_promotion = "rules_specifier"];

// If specified, rules will emit stats with the given prefix.
// This is useful to distinguish the stat when there are more than 1 RBAC filter configured with
// rules.
string rules_stat_prefix = 6;

// The match tree to use when resolving RBAC action for incoming requests. Requests do not
// match any matcher will be denied.
// If absent, no enforcing RBAC matcher will be applied.
Expand Down
4 changes: 4 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,10 @@ new_features:
Update ``aws_request_signing`` filter to support optionally sending the aws signature in query parameters rather than headers,
by specifying the :ref:`query_string <envoy_v3_api_field_extensions.filters.http.aws_request_signing.v3.AwsRequestSigning.query_string>`
configuration section.
- area: rbac
change: |
Added :ref:`rules_stat_prefix <envoy_v3_api_field_extensions.filters.http.rbac.v3.RBAC.rules_stat_prefix>`
to allow adding custom prefix to the stats emitted by rules.
deprecated:
- area: listener
Expand Down
7 changes: 6 additions & 1 deletion docs/root/configuration/http/http_filters/rbac_filter.rst
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,12 @@ The RBAC filter outputs statistics in the ``http.<stat_prefix>.rbac.`` namespace
<envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.stat_prefix>` comes from the
owning HTTP connection manager.

For the shadow rule statistics ``shadow_allowed`` and ``shadow_denied``, the :ref:`shadow_rules_stat_prefix <envoy_v3_api_field_extensions.filters.http.rbac.v3.RBAC.shadow_rules_stat_prefix>`
For the rule statistics ``allowed`` and ``denied``,
the :ref:`rules_stat_prefix <envoy_v3_api_field_extensions.filters.http.rbac.v3.RBAC.rules_stat_prefix>`
can be used to add an extra prefix to output the statistics in the ``http.<stat_prefix>.rbac.<rules_stat_prefix>.`` namespace.

For the shadow rule statistics ``shadow_allowed`` and ``shadow_denied``,
the :ref:`shadow_rules_stat_prefix <envoy_v3_api_field_extensions.filters.http.rbac.v3.RBAC.shadow_rules_stat_prefix>`
can be used to add an extra prefix to output the statistics in the ``http.<stat_prefix>.rbac.<shadow_rules_stat_prefix>.`` namespace.

.. csv-table::
Expand Down
3 changes: 3 additions & 0 deletions source/common/config/well_known_names.cc
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,9 @@ TagNameValues::TagNameValues() {

// (<stat_prefix>.).rbac.**
addTokenized(RBAC_PREFIX, "$.rbac.**");

// http.<stat_prefix>.rbac.(<rules_stat_prefix>.)*
addTokenized(RBAC_HTTP_PREFIX, "http.*.rbac.$.**");
}

void TagNameValues::addRe2(const std::string& name, const std::string& regex,
Expand Down
2 changes: 2 additions & 0 deletions source/common/config/well_known_names.h
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ class TagNameValues {
const std::string CONNECTION_LIMIT_PREFIX = "envoy.connection_limit_prefix";
// Stats prefix for the RBAC network filter
const std::string RBAC_PREFIX = "envoy.rbac_prefix";
// Stats prefix for the RBAC http filter
const std::string RBAC_HTTP_PREFIX = "envoy.rbac_http_prefix";
// Stats prefix for the TCP Proxy network filter
const std::string TCP_PREFIX = "envoy.tcp_prefix";
// Stats prefix for the UDP Proxy network filter
Expand Down
11 changes: 7 additions & 4 deletions source/extensions/filters/common/rbac/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,14 @@ namespace Filters {
namespace Common {
namespace RBAC {

RoleBasedAccessControlFilterStats
generateStats(const std::string& prefix, const std::string& shadow_prefix, Stats::Scope& scope) {
RoleBasedAccessControlFilterStats generateStats(const std::string& prefix,
const std::string& rules_prefix,
const std::string& shadow_rules_prefix,
Stats::Scope& scope) {
const std::string final_prefix = Envoy::statPrefixJoin(prefix, "rbac.");
return {ENFORCE_RBAC_FILTER_STATS(POOL_COUNTER_PREFIX(scope, final_prefix))
SHADOW_RBAC_FILTER_STATS(POOL_COUNTER_PREFIX(scope, final_prefix + shadow_prefix))};
return {
ENFORCE_RBAC_FILTER_STATS(POOL_COUNTER_PREFIX(scope, final_prefix + rules_prefix))
SHADOW_RBAC_FILTER_STATS(POOL_COUNTER_PREFIX(scope, final_prefix + shadow_rules_prefix))};
}

std::string responseDetail(const std::string& policy_id) {
Expand Down
6 changes: 4 additions & 2 deletions source/extensions/filters/common/rbac/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,10 @@ struct RoleBasedAccessControlFilterStats {
SHADOW_RBAC_FILTER_STATS(GENERATE_COUNTER_STRUCT)
};

RoleBasedAccessControlFilterStats
generateStats(const std::string& prefix, const std::string& shadow_prefix, Stats::Scope& scope);
RoleBasedAccessControlFilterStats generateStats(const std::string& prefix,
const std::string& rules_prefix,
const std::string& shadow_rules_prefix,
Stats::Scope& scope);

template <class ConfigType>
std::unique_ptr<RoleBasedAccessControlEngine>
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/filters/http/rbac/rbac_filter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ RoleBasedAccessControlFilterConfig::RoleBasedAccessControlFilterConfig(
const std::string& stats_prefix, Stats::Scope& scope,
Server::Configuration::ServerFactoryContext& context,
ProtobufMessage::ValidationVisitor& validation_visitor)
: stats_(Filters::Common::RBAC::generateStats(stats_prefix,
: stats_(Filters::Common::RBAC::generateStats(stats_prefix, proto_config.rules_stat_prefix(),
proto_config.shadow_rules_stat_prefix(), scope)),
shadow_rules_stat_prefix_(proto_config.shadow_rules_stat_prefix()),
engine_(Filters::Common::RBAC::createEngine(proto_config, context, validation_visitor,
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/filters/network/rbac/rbac_filter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ RoleBasedAccessControlFilterConfig::RoleBasedAccessControlFilterConfig(
const envoy::extensions::filters::network::rbac::v3::RBAC& proto_config, Stats::Scope& scope,
Server::Configuration::ServerFactoryContext& context,
ProtobufMessage::ValidationVisitor& validation_visitor)
: stats_(Filters::Common::RBAC::generateStats(proto_config.stat_prefix(),
: stats_(Filters::Common::RBAC::generateStats(proto_config.stat_prefix(), "",
proto_config.shadow_rules_stat_prefix(), scope)),
shadow_rules_stat_prefix_(proto_config.shadow_rules_stat_prefix()),
engine_(Filters::Common::RBAC::createEngine(proto_config, context, validation_visitor,
Expand Down
11 changes: 11 additions & 0 deletions test/common/stats/tag_extractor_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,17 @@ TEST(TagExtractorTest, DefaultTagExtractors) {
rbac_prefix.name_ = tag_names.RBAC_PREFIX;
rbac_prefix.value_ = "my_rbac_prefix";
regex_tester.testRegex("my_rbac_prefix.rbac.allowed", "rbac.allowed", {rbac_prefix});

// RBAC HTTP Filter Prefix
Tag rbac_http_hcm_prefix;
rbac_http_hcm_prefix.name_ = tag_names.HTTP_CONN_MANAGER_PREFIX;
rbac_http_hcm_prefix.value_ = "hcm_prefix";

Tag rbac_http_prefix;
rbac_http_prefix.name_ = tag_names.RBAC_HTTP_PREFIX;
rbac_http_prefix.value_ = "prefix";
regex_tester.testRegex("http.hcm_prefix.rbac.prefix.allowed", "http.rbac.allowed",
{rbac_http_hcm_prefix, rbac_http_prefix});
}

TEST(TagExtractorTest, ExtAuthzTagExtractors) {
Expand Down
81 changes: 53 additions & 28 deletions test/extensions/filters/http/rbac/rbac_filter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ enum class LogResult { Yes, No, Undecided };

class RoleBasedAccessControlFilterTest : public testing::Test {
public:
void setupPolicy(envoy::config::rbac::v3::RBAC::Action action) {
void setupPolicy(envoy::config::rbac::v3::RBAC::Action action,
std::string rules_stat_prefix = "") {
envoy::extensions::filters::http::rbac::v3::RBAC config;

envoy::config::rbac::v3::Policy policy;
Expand All @@ -47,6 +48,7 @@ class RoleBasedAccessControlFilterTest : public testing::Test {
policy.add_principals()->set_any(true);
config.mutable_rules()->set_action(action);
(*config.mutable_rules()->mutable_policies())["foo"] = policy;
config.set_rules_stat_prefix(rules_stat_prefix);

envoy::config::rbac::v3::Policy shadow_policy;
auto shadow_policy_rules = shadow_policy.add_permissions()->mutable_or_rules();
Expand All @@ -55,7 +57,7 @@ class RoleBasedAccessControlFilterTest : public testing::Test {
shadow_policy.add_principals()->set_any(true);
config.mutable_shadow_rules()->set_action(action);
(*config.mutable_shadow_rules()->mutable_policies())["bar"] = shadow_policy;
config.set_shadow_rules_stat_prefix("prefix_");
config.set_shadow_rules_stat_prefix("shadow_rules_prefix_");

setupConfig(std::make_shared<RoleBasedAccessControlFilterConfig>(
config, "test", *store_.rootScope(), context_,
Expand Down Expand Up @@ -156,7 +158,7 @@ class RoleBasedAccessControlFilterTest : public testing::Test {
TestUtility::loadFromYaml(fmt::format(shadow_matcher_yaml, action, on_no_match_action),
shadow_matcher);
*config.mutable_shadow_matcher() = shadow_matcher;
config.set_shadow_rules_stat_prefix("prefix_");
config.set_shadow_rules_stat_prefix("shadow_rules_prefix_");

setupConfig(std::make_shared<RoleBasedAccessControlFilterConfig>(
config, "test", *store_.rootScope(), context_,
Expand Down Expand Up @@ -238,7 +240,7 @@ class RoleBasedAccessControlFilterTest : public testing::Test {
*config.mutable_matcher() = matcher;

*config.mutable_shadow_matcher() = matcher;
config.set_shadow_rules_stat_prefix("prefix_");
config.set_shadow_rules_stat_prefix("shadow_rules_prefix_");

setupConfig(std::make_shared<RoleBasedAccessControlFilterConfig>(
config, "test", *store_.rootScope(), context_,
Expand Down Expand Up @@ -335,8 +337,9 @@ TEST_F(RoleBasedAccessControlFilterTest, Allowed) {
EXPECT_EQ(1U, config_->stats().shadow_denied_.value());
EXPECT_EQ("test.rbac.allowed", config_->stats().allowed_.name());
EXPECT_EQ("test.rbac.denied", config_->stats().denied_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_allowed",
config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_denied", config_->stats().shadow_denied_.name());

Buffer::OwnedImpl data("");
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data, false));
Expand All @@ -359,8 +362,9 @@ TEST_F(RoleBasedAccessControlFilterTest, RequestedServerName) {
EXPECT_EQ(1U, config_->stats().shadow_denied_.value());
EXPECT_EQ("test.rbac.allowed", config_->stats().allowed_.name());
EXPECT_EQ("test.rbac.denied", config_->stats().denied_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_allowed",
config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_denied", config_->stats().shadow_denied_.name());

Buffer::OwnedImpl data("");
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data, false));
Expand Down Expand Up @@ -404,12 +408,16 @@ TEST_F(RoleBasedAccessControlFilterTest, Denied) {
EXPECT_EQ(1U, config_->stats().shadow_allowed_.value());
EXPECT_EQ("test.rbac.allowed", config_->stats().allowed_.name());
EXPECT_EQ("test.rbac.denied", config_->stats().denied_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_allowed",
config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_denied", config_->stats().shadow_denied_.name());

auto filter_meta = req_info_.dynamicMetadata().filter_metadata().at("envoy.filters.http.rbac");
EXPECT_EQ("allowed", filter_meta.fields().at("prefix_shadow_engine_result").string_value());
EXPECT_EQ("bar", filter_meta.fields().at("prefix_shadow_effective_policy_id").string_value());
EXPECT_EQ("allowed",
filter_meta.fields().at("shadow_rules_prefix_shadow_engine_result").string_value());
EXPECT_EQ(
"bar",
filter_meta.fields().at("shadow_rules_prefix_shadow_effective_policy_id").string_value());
EXPECT_EQ("rbac_access_denied_matched_policy[none]", callbacks_.details());
checkAccessLogMetadata(LogResult::Undecided);
}
Expand Down Expand Up @@ -496,8 +504,9 @@ TEST_F(RoleBasedAccessControlFilterTest, MatcherAllowed) {
EXPECT_EQ(1U, config_->stats().shadow_denied_.value());
EXPECT_EQ("test.rbac.allowed", config_->stats().allowed_.name());
EXPECT_EQ("test.rbac.denied", config_->stats().denied_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_allowed",
config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_denied", config_->stats().shadow_denied_.name());

Buffer::OwnedImpl data("");
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data, false));
Expand All @@ -520,8 +529,9 @@ TEST_F(RoleBasedAccessControlFilterTest, RequestedServerNameMatcher) {
EXPECT_EQ(1U, config_->stats().shadow_denied_.value());
EXPECT_EQ("test.rbac.allowed", config_->stats().allowed_.name());
EXPECT_EQ("test.rbac.denied", config_->stats().denied_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_allowed",
config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_denied", config_->stats().shadow_denied_.name());

Buffer::OwnedImpl data("");
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data, false));
Expand Down Expand Up @@ -565,12 +575,16 @@ TEST_F(RoleBasedAccessControlFilterTest, MatcherDenied) {
EXPECT_EQ(1U, config_->stats().shadow_allowed_.value());
EXPECT_EQ("test.rbac.allowed", config_->stats().allowed_.name());
EXPECT_EQ("test.rbac.denied", config_->stats().denied_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_allowed",
config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_denied", config_->stats().shadow_denied_.name());

auto filter_meta = req_info_.dynamicMetadata().filter_metadata().at("envoy.filters.http.rbac");
EXPECT_EQ("allowed", filter_meta.fields().at("prefix_shadow_engine_result").string_value());
EXPECT_EQ("bar", filter_meta.fields().at("prefix_shadow_effective_policy_id").string_value());
EXPECT_EQ("allowed",
filter_meta.fields().at("shadow_rules_prefix_shadow_engine_result").string_value());
EXPECT_EQ(
"bar",
filter_meta.fields().at("shadow_rules_prefix_shadow_effective_policy_id").string_value());
EXPECT_EQ("rbac_access_denied_matched_policy[none]", callbacks_.details());
checkAccessLogMetadata(LogResult::Undecided);
}
Expand Down Expand Up @@ -618,8 +632,9 @@ TEST_F(RoleBasedAccessControlFilterTest, ShouldLog) {
EXPECT_EQ(0U, config_->stats().shadow_denied_.value());
EXPECT_EQ("test.rbac.allowed", config_->stats().allowed_.name());
EXPECT_EQ("test.rbac.denied", config_->stats().denied_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_allowed",
config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_denied", config_->stats().shadow_denied_.name());

Buffer::OwnedImpl data("");
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data, false));
Expand All @@ -639,8 +654,9 @@ TEST_F(RoleBasedAccessControlFilterTest, ShouldNotLog) {
EXPECT_EQ(0U, config_->stats().shadow_denied_.value());
EXPECT_EQ("test.rbac.allowed", config_->stats().allowed_.name());
EXPECT_EQ("test.rbac.denied", config_->stats().denied_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_allowed",
config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_denied", config_->stats().shadow_denied_.name());

Buffer::OwnedImpl data("");
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data, false));
Expand All @@ -660,8 +676,9 @@ TEST_F(RoleBasedAccessControlFilterTest, MatcherShouldLog) {
EXPECT_EQ(0U, config_->stats().shadow_denied_.value());
EXPECT_EQ("test.rbac.allowed", config_->stats().allowed_.name());
EXPECT_EQ("test.rbac.denied", config_->stats().denied_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_allowed",
config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_denied", config_->stats().shadow_denied_.name());

Buffer::OwnedImpl data("");
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data, false));
Expand All @@ -681,8 +698,9 @@ TEST_F(RoleBasedAccessControlFilterTest, MatcherShouldNotLog) {
EXPECT_EQ(0U, config_->stats().shadow_denied_.value());
EXPECT_EQ("test.rbac.allowed", config_->stats().allowed_.name());
EXPECT_EQ("test.rbac.denied", config_->stats().denied_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_allowed",
config_->stats().shadow_allowed_.name());
EXPECT_EQ("test.rbac.shadow_rules_prefix_.shadow_denied", config_->stats().shadow_denied_.name());

Buffer::OwnedImpl data("");
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data, false));
Expand All @@ -691,6 +709,13 @@ TEST_F(RoleBasedAccessControlFilterTest, MatcherShouldNotLog) {
checkAccessLogMetadata(LogResult::No);
}

TEST_F(RoleBasedAccessControlFilterTest, RulesStatPrefix) {
setupPolicy(envoy::config::rbac::v3::RBAC::ALLOW, "rules_prefix_");

EXPECT_EQ("test.rbac.rules_prefix_.allowed", config_->stats().allowed_.name());
EXPECT_EQ("test.rbac.rules_prefix_.denied", config_->stats().denied_.name());
}

// Upstream Ip and Port matcher tests.
class UpstreamIpPortMatcherTests : public RoleBasedAccessControlFilterTest {
public:
Expand Down
Loading

0 comments on commit 7fec609

Please sign in to comment.