From 6638506bfa4917f659f668e41a4a97e607c044b2 Mon Sep 17 00:00:00 2001 From: Yangmin Zhu Date: Tue, 9 Mar 2021 20:18:19 -0800 Subject: [PATCH 1/2] rbac: apply the shadow_rules_stat_prefix to shadow stats in addition to dynamic metadata Signed-off-by: Yangmin Zhu --- .../http/http_filters/rbac_filter.rst | 6 ++++ .../listeners/network_filters/rbac_filter.rst | 6 ++++ .../extensions/filters/common/rbac/utility.cc | 6 ++-- .../extensions/filters/common/rbac/utility.h | 19 ++++++++---- .../filters/http/rbac/rbac_filter.cc | 3 +- .../filters/network/rbac/rbac_filter.cc | 3 +- .../filters/http/rbac/rbac_filter_test.cc | 20 +++++++++++++ .../filters/network/rbac/filter_test.cc | 30 ++++++++++++++++++- 8 files changed, 82 insertions(+), 11 deletions(-) diff --git a/docs/root/configuration/http/http_filters/rbac_filter.rst b/docs/root/configuration/http/http_filters/rbac_filter.rst index e8cd4b1b69ab3..a41d033e071bc 100644 --- a/docs/root/configuration/http/http_filters/rbac_filter.rst +++ b/docs/root/configuration/http/http_filters/rbac_filter.rst @@ -33,6 +33,9 @@ The RBAC filter outputs statistics in the *http..rbac.* namespace. ` comes from the owning HTTP connection manager. +For the shadow rule statistics `shadow_allowed` and `shadow_denied`, the :ref:`shadow_rules_stat_prefix ` +can be used to add an extra prefix to output the statistics in the *http..rbac..* namespace. + .. csv-table:: :header: Name, Type, Description :widths: 1, 1, 2 @@ -51,6 +54,9 @@ Dynamic Metadata The RBAC filter emits the following dynamic metadata. +For the shadow rules dynamic metadata `shadow_effective_policy_id` and `shadow_engine_result`, the :ref:`shadow_rules_stat_prefix ` +can be used to add an extra prefix to the corresponding dynamic metadata key. + .. csv-table:: :header: Name, Type, Description :widths: 1, 1, 2 diff --git a/docs/root/configuration/listeners/network_filters/rbac_filter.rst b/docs/root/configuration/listeners/network_filters/rbac_filter.rst index 0324e5153c897..251f3deb0aecc 100644 --- a/docs/root/configuration/listeners/network_filters/rbac_filter.rst +++ b/docs/root/configuration/listeners/network_filters/rbac_filter.rst @@ -23,6 +23,9 @@ Statistics The RBAC network filter outputs statistics in the *.rbac.* namespace. +For the shadow rule statistics `shadow_allowed` and `shadow_denied`, the :ref:`shadow_rules_stat_prefix ` +can be used to add an extra prefix to output the statistics in the *.rbac..* namespace. + .. csv-table:: :header: Name, Type, Description :widths: 1, 1, 2 @@ -41,6 +44,9 @@ Dynamic Metadata The RBAC filter emits the following dynamic metadata. +For the shadow rules dynamic metadata `shadow_effective_policy_id` and `shadow_engine_result`, the :ref:`shadow_rules_stat_prefix ` +can be used to add an extra prefix to the corresponding dynamic metadata key. + .. csv-table:: :header: Name, Type, Description :widths: 1, 1, 2 diff --git a/source/extensions/filters/common/rbac/utility.cc b/source/extensions/filters/common/rbac/utility.cc index 0895e0156e13f..8273e9659c207 100644 --- a/source/extensions/filters/common/rbac/utility.cc +++ b/source/extensions/filters/common/rbac/utility.cc @@ -10,9 +10,11 @@ namespace Filters { namespace Common { namespace RBAC { -RoleBasedAccessControlFilterStats generateStats(const std::string& prefix, Stats::Scope& scope) { +RoleBasedAccessControlFilterStats +generateStats(const std::string& prefix, const std::string& shadow_prefix, Stats::Scope& scope) { const std::string final_prefix = prefix + "rbac."; - return {ALL_RBAC_FILTER_STATS(POOL_COUNTER_PREFIX(scope, final_prefix))}; + return {ENFORCE_RBAC_FILTER_STATS(POOL_COUNTER_PREFIX(scope, final_prefix)) + SHADOW_RBAC_FILTER_STATS(POOL_COUNTER_PREFIX(scope, final_prefix + shadow_prefix))}; } std::string responseDetail(const std::string& policy_id) { diff --git a/source/extensions/filters/common/rbac/utility.h b/source/extensions/filters/common/rbac/utility.h index e690b3a297eca..91f8711f2210a 100644 --- a/source/extensions/filters/common/rbac/utility.h +++ b/source/extensions/filters/common/rbac/utility.h @@ -14,22 +14,29 @@ namespace Common { namespace RBAC { /** - * All stats for the RBAC filter. @see stats_macros.h + * All stats for the enforced rules in RBAC filter. @see stats_macros.h */ -#define ALL_RBAC_FILTER_STATS(COUNTER) \ +#define ENFORCE_RBAC_FILTER_STATS(COUNTER) \ COUNTER(allowed) \ - COUNTER(denied) \ + COUNTER(denied) + +/** + * All stats for the shadow rules in RBAC filter. @see stats_macros.h + */ +#define SHADOW_RBAC_FILTER_STATS(COUNTER) \ COUNTER(shadow_allowed) \ COUNTER(shadow_denied) /** - * Wrapper struct for RBAC filter stats. @see stats_macros.h + * Wrapper struct for shadow rules in RBAC filter stats. @see stats_macros.h */ struct RoleBasedAccessControlFilterStats { - ALL_RBAC_FILTER_STATS(GENERATE_COUNTER_STRUCT) + ENFORCE_RBAC_FILTER_STATS(GENERATE_COUNTER_STRUCT) + SHADOW_RBAC_FILTER_STATS(GENERATE_COUNTER_STRUCT) }; -RoleBasedAccessControlFilterStats generateStats(const std::string& prefix, Stats::Scope& scope); +RoleBasedAccessControlFilterStats +generateStats(const std::string& prefix, const std::string& shadow_prefix, Stats::Scope& scope); template std::unique_ptr createEngine(const ConfigType& config) { diff --git a/source/extensions/filters/http/rbac/rbac_filter.cc b/source/extensions/filters/http/rbac/rbac_filter.cc index d8b6505131e8d..9d75db7f661fb 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.cc +++ b/source/extensions/filters/http/rbac/rbac_filter.cc @@ -17,7 +17,8 @@ namespace RBACFilter { RoleBasedAccessControlFilterConfig::RoleBasedAccessControlFilterConfig( const envoy::extensions::filters::http::rbac::v3::RBAC& proto_config, const std::string& stats_prefix, Stats::Scope& scope) - : stats_(Filters::Common::RBAC::generateStats(stats_prefix, scope)), + : stats_(Filters::Common::RBAC::generateStats(stats_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)), shadow_engine_(Filters::Common::RBAC::createShadowEngine(proto_config)) {} diff --git a/source/extensions/filters/network/rbac/rbac_filter.cc b/source/extensions/filters/network/rbac/rbac_filter.cc index 2ed05efd73d74..3892600c77dcd 100644 --- a/source/extensions/filters/network/rbac/rbac_filter.cc +++ b/source/extensions/filters/network/rbac/rbac_filter.cc @@ -15,7 +15,8 @@ namespace RBACFilter { RoleBasedAccessControlFilterConfig::RoleBasedAccessControlFilterConfig( const envoy::extensions::filters::network::rbac::v3::RBAC& proto_config, Stats::Scope& scope) - : stats_(Filters::Common::RBAC::generateStats(proto_config.stat_prefix(), scope)), + : 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)), shadow_engine_(Filters::Common::RBAC::createShadowEngine(proto_config)), diff --git a/test/extensions/filters/http/rbac/rbac_filter_test.cc b/test/extensions/filters/http/rbac/rbac_filter_test.cc index 6161882a7436e..07d01f679647d 100644 --- a/test/extensions/filters/http/rbac/rbac_filter_test.cc +++ b/test/extensions/filters/http/rbac/rbac_filter_test.cc @@ -128,6 +128,10 @@ TEST_F(RoleBasedAccessControlFilterTest, Allowed) { EXPECT_EQ(Http::FilterMetadataStatus::Continue, filter_.decodeMetadata(metadata_map)); EXPECT_EQ(1U, config_->stats().allowed_.value()); EXPECT_EQ(1U, config_->stats().shadow_denied_.value()); + EXPECT_EQ("testrbac.allowed", config_->stats().allowed_.name()); + EXPECT_EQ("testrbac.denied", config_->stats().denied_.name()); + EXPECT_EQ("testrbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name()); + EXPECT_EQ("testrbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name()); Buffer::OwnedImpl data(""); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(data, false)); @@ -146,6 +150,10 @@ TEST_F(RoleBasedAccessControlFilterTest, RequestedServerName) { EXPECT_EQ(0U, config_->stats().denied_.value()); EXPECT_EQ(0U, config_->stats().shadow_allowed_.value()); EXPECT_EQ(1U, config_->stats().shadow_denied_.value()); + EXPECT_EQ("testrbac.allowed", config_->stats().allowed_.name()); + EXPECT_EQ("testrbac.denied", config_->stats().denied_.name()); + EXPECT_EQ("testrbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name()); + EXPECT_EQ("testrbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name()); Buffer::OwnedImpl data(""); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(data, false)); @@ -183,6 +191,10 @@ TEST_F(RoleBasedAccessControlFilterTest, Denied) { EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_.decodeHeaders(headers_, true)); EXPECT_EQ(1U, config_->stats().denied_.value()); EXPECT_EQ(1U, config_->stats().shadow_allowed_.value()); + EXPECT_EQ("testrbac.allowed", config_->stats().allowed_.name()); + EXPECT_EQ("testrbac.denied", config_->stats().denied_.name()); + EXPECT_EQ("testrbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name()); + EXPECT_EQ("testrbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name()); auto filter_meta = req_info_.dynamicMetadata().filter_metadata().at(HttpFilterNames::get().Rbac); EXPECT_EQ("allowed", filter_meta.fields().at("prefix_shadow_engine_result").string_value()); @@ -222,6 +234,10 @@ TEST_F(RoleBasedAccessControlFilterTest, ShouldLog) { EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(headers_, false)); EXPECT_EQ(1U, config_->stats().allowed_.value()); EXPECT_EQ(0U, config_->stats().shadow_denied_.value()); + EXPECT_EQ("testrbac.allowed", config_->stats().allowed_.name()); + EXPECT_EQ("testrbac.denied", config_->stats().denied_.name()); + EXPECT_EQ("testrbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name()); + EXPECT_EQ("testrbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name()); Buffer::OwnedImpl data(""); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(data, false)); @@ -241,6 +257,10 @@ TEST_F(RoleBasedAccessControlFilterTest, ShouldNotLog) { EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(headers_, false)); EXPECT_EQ(1U, config_->stats().allowed_.value()); EXPECT_EQ(0U, config_->stats().shadow_denied_.value()); + EXPECT_EQ("testrbac.allowed", config_->stats().allowed_.name()); + EXPECT_EQ("testrbac.denied", config_->stats().denied_.name()); + EXPECT_EQ("testrbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name()); + EXPECT_EQ("testrbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name()); Buffer::OwnedImpl data(""); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(data, false)); diff --git a/test/extensions/filters/network/rbac/filter_test.cc b/test/extensions/filters/network/rbac/filter_test.cc index 45a973e586f63..72b84b2098405 100644 --- a/test/extensions/filters/network/rbac/filter_test.cc +++ b/test/extensions/filters/network/rbac/filter_test.cc @@ -28,6 +28,7 @@ class RoleBasedAccessControlNetworkFilterTest : public testing::Test { envoy::extensions::filters::network::rbac::v3::RBAC config; config.set_stat_prefix("tcp."); + config.set_shadow_rules_stat_prefix("prefix_"); if (with_policy) { envoy::config::rbac::v3::Policy policy; @@ -45,7 +46,6 @@ class RoleBasedAccessControlNetworkFilterTest : public testing::Test { shadow_policy_rules->add_rules()->set_destination_port(456); shadow_policy.add_principals()->set_any(true); config.mutable_shadow_rules()->set_action(action); - config.set_shadow_rules_stat_prefix("prefix_"); (*config.mutable_shadow_rules()->mutable_policies())["bar"] = shadow_policy; } @@ -125,6 +125,10 @@ TEST_F(RoleBasedAccessControlNetworkFilterTest, AllowedWithOneTimeEnforcement) { EXPECT_EQ(0U, config_->stats().denied_.value()); EXPECT_EQ(0U, config_->stats().shadow_allowed_.value()); EXPECT_EQ(1U, config_->stats().shadow_denied_.value()); + EXPECT_EQ("tcp.rbac.allowed", config_->stats().allowed_.name()); + EXPECT_EQ("tcp.rbac.denied", config_->stats().denied_.name()); + EXPECT_EQ("tcp.rbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name()); + EXPECT_EQ("tcp.rbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name()); } TEST_F(RoleBasedAccessControlNetworkFilterTest, AllowedWithContinuousEnforcement) { @@ -142,6 +146,10 @@ TEST_F(RoleBasedAccessControlNetworkFilterTest, AllowedWithContinuousEnforcement EXPECT_EQ(0U, config_->stats().denied_.value()); EXPECT_EQ(0U, config_->stats().shadow_allowed_.value()); EXPECT_EQ(2U, config_->stats().shadow_denied_.value()); + EXPECT_EQ("tcp.rbac.allowed", config_->stats().allowed_.name()); + EXPECT_EQ("tcp.rbac.denied", config_->stats().denied_.name()); + EXPECT_EQ("tcp.rbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name()); + EXPECT_EQ("tcp.rbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name()); } TEST_F(RoleBasedAccessControlNetworkFilterTest, RequestedServerName) { @@ -157,6 +165,10 @@ TEST_F(RoleBasedAccessControlNetworkFilterTest, RequestedServerName) { EXPECT_EQ(0U, config_->stats().denied_.value()); EXPECT_EQ(0U, config_->stats().shadow_allowed_.value()); EXPECT_EQ(1U, config_->stats().shadow_denied_.value()); + EXPECT_EQ("tcp.rbac.allowed", config_->stats().allowed_.name()); + EXPECT_EQ("tcp.rbac.denied", config_->stats().denied_.name()); + EXPECT_EQ("tcp.rbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name()); + EXPECT_EQ("tcp.rbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name()); } TEST_F(RoleBasedAccessControlNetworkFilterTest, AllowedWithNoPolicy) { @@ -172,6 +184,10 @@ TEST_F(RoleBasedAccessControlNetworkFilterTest, AllowedWithNoPolicy) { EXPECT_EQ(0U, config_->stats().denied_.value()); EXPECT_EQ(0U, config_->stats().shadow_allowed_.value()); EXPECT_EQ(0U, config_->stats().shadow_denied_.value()); + EXPECT_EQ("tcp.rbac.allowed", config_->stats().allowed_.name()); + EXPECT_EQ("tcp.rbac.denied", config_->stats().denied_.name()); + EXPECT_EQ("tcp.rbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name()); + EXPECT_EQ("tcp.rbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name()); } TEST_F(RoleBasedAccessControlNetworkFilterTest, Denied) { @@ -187,6 +203,10 @@ TEST_F(RoleBasedAccessControlNetworkFilterTest, Denied) { EXPECT_EQ(1U, config_->stats().denied_.value()); EXPECT_EQ(1U, config_->stats().shadow_allowed_.value()); EXPECT_EQ(0U, config_->stats().shadow_denied_.value()); + EXPECT_EQ("tcp.rbac.allowed", config_->stats().allowed_.name()); + EXPECT_EQ("tcp.rbac.denied", config_->stats().denied_.name()); + EXPECT_EQ("tcp.rbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name()); + EXPECT_EQ("tcp.rbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name()); auto filter_meta = stream_info_.dynamicMetadata().filter_metadata().at(NetworkFilterNames::get().Rbac); @@ -206,6 +226,10 @@ TEST_F(RoleBasedAccessControlNetworkFilterTest, ShouldLog) { EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data_, false)); EXPECT_EQ(1U, config_->stats().allowed_.value()); EXPECT_EQ(0U, config_->stats().shadow_denied_.value()); + EXPECT_EQ("tcp.rbac.allowed", config_->stats().allowed_.name()); + EXPECT_EQ("tcp.rbac.denied", config_->stats().denied_.name()); + EXPECT_EQ("tcp.rbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name()); + EXPECT_EQ("tcp.rbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name()); checkAccessLogMetadata(true); } @@ -221,6 +245,10 @@ TEST_F(RoleBasedAccessControlNetworkFilterTest, ShouldNotLog) { EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data_, false)); EXPECT_EQ(1U, config_->stats().allowed_.value()); EXPECT_EQ(0U, config_->stats().shadow_denied_.value()); + EXPECT_EQ("tcp.rbac.allowed", config_->stats().allowed_.name()); + EXPECT_EQ("tcp.rbac.denied", config_->stats().denied_.name()); + EXPECT_EQ("tcp.rbac.prefix_.shadow_allowed", config_->stats().shadow_allowed_.name()); + EXPECT_EQ("tcp.rbac.prefix_.shadow_denied", config_->stats().shadow_denied_.name()); checkAccessLogMetadata(false); } From fcdfbe2fd39b17d1e7aff0dd05655b6bada0b30f Mon Sep 17 00:00:00 2001 From: Yangmin Zhu Date: Thu, 11 Mar 2021 14:34:02 -0800 Subject: [PATCH 2/2] update link Signed-off-by: Yangmin Zhu --- .../configuration/listeners/network_filters/rbac_filter.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/root/configuration/listeners/network_filters/rbac_filter.rst b/docs/root/configuration/listeners/network_filters/rbac_filter.rst index 251f3deb0aecc..3fb15d98d4ff0 100644 --- a/docs/root/configuration/listeners/network_filters/rbac_filter.rst +++ b/docs/root/configuration/listeners/network_filters/rbac_filter.rst @@ -23,7 +23,7 @@ Statistics The RBAC network filter outputs statistics in the *.rbac.* namespace. -For the shadow rule statistics `shadow_allowed` and `shadow_denied`, the :ref:`shadow_rules_stat_prefix ` +For the shadow rule statistics `shadow_allowed` and `shadow_denied`, the :ref:`shadow_rules_stat_prefix ` can be used to add an extra prefix to output the statistics in the *.rbac..* namespace. .. csv-table:: @@ -44,7 +44,7 @@ Dynamic Metadata The RBAC filter emits the following dynamic metadata. -For the shadow rules dynamic metadata `shadow_effective_policy_id` and `shadow_engine_result`, the :ref:`shadow_rules_stat_prefix ` +For the shadow rules dynamic metadata `shadow_effective_policy_id` and `shadow_engine_result`, the :ref:`shadow_rules_stat_prefix ` can be used to add an extra prefix to the corresponding dynamic metadata key. .. csv-table::