From c56caa1388058a180b5e9e41d9f03d95d0af4ca6 Mon Sep 17 00:00:00 2001 From: Quanjie Lin Date: Sun, 5 Aug 2018 19:03:12 -0700 Subject: [PATCH 1/6] collect metrics for RBAC shadow policy Signed-off-by: Quanjie Lin --- source/extensions/filters/common/rbac/engine.h | 3 ++- .../extensions/filters/common/rbac/engine_impl.cc | 15 +++++++++------ .../extensions/filters/common/rbac/engine_impl.h | 5 +++-- .../extensions/filters/http/rbac/rbac_filter.cc | 14 ++++++++++++-- .../filters/common/rbac/engine_impl_test.cc | 12 ++++++------ test/extensions/filters/common/rbac/mocks.h | 5 +++-- .../filters/http/rbac/rbac_filter_test.cc | 5 ++++- 7 files changed, 39 insertions(+), 20 deletions(-) diff --git a/source/extensions/filters/common/rbac/engine.h b/source/extensions/filters/common/rbac/engine.h index 1281f6319bcf2..cde3977b9fc76 100644 --- a/source/extensions/filters/common/rbac/engine.h +++ b/source/extensions/filters/common/rbac/engine.h @@ -27,7 +27,8 @@ class RoleBasedAccessControlEngine { * @param metadata the metadata with additional information about the action/principal. */ virtual bool allowed(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata& metadata) const PURE; + const envoy::api::v2::core::Metadata& metadata, + std::string& effectivePolicyID) const PURE; }; } // namespace RBAC diff --git a/source/extensions/filters/common/rbac/engine_impl.cc b/source/extensions/filters/common/rbac/engine_impl.cc index 1608476f8efff..3b5a500cc803c 100644 --- a/source/extensions/filters/common/rbac/engine_impl.cc +++ b/source/extensions/filters/common/rbac/engine_impl.cc @@ -11,17 +11,20 @@ RoleBasedAccessControlEngineImpl::RoleBasedAccessControlEngineImpl( : allowed_if_matched_(rules.action() == envoy::config::rbac::v2alpha::RBAC_Action::RBAC_Action_ALLOW) { for (const auto& policy : rules.policies()) { - policies_.emplace_back(policy.second); + policies_.insert(std::pair(policy.first, policy.second)); } } -bool RoleBasedAccessControlEngineImpl::allowed( - const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata& metadata) const { +bool RoleBasedAccessControlEngineImpl::allowed(const Network::Connection& connection, + const Envoy::Http::HeaderMap& headers, + const envoy::api::v2::core::Metadata& metadata, + std::string& effectivePolicyID) const { bool matched = false; - for (const auto& policy : policies_) { - if (policy.matches(connection, headers, metadata)) { + + for (auto it = policies_.begin(); it != policies_.end(); it++) { + if (it->second.matches(connection, headers, metadata)) { matched = true; + effectivePolicyID = it->first; break; } } diff --git a/source/extensions/filters/common/rbac/engine_impl.h b/source/extensions/filters/common/rbac/engine_impl.h index 6be4d18798d27..c2ea93441e823 100644 --- a/source/extensions/filters/common/rbac/engine_impl.h +++ b/source/extensions/filters/common/rbac/engine_impl.h @@ -16,12 +16,13 @@ class RoleBasedAccessControlEngineImpl : public RoleBasedAccessControlEngine { RoleBasedAccessControlEngineImpl(const envoy::config::rbac::v2alpha::RBAC& rules); bool allowed(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata& metadata) const override; + const envoy::api::v2::core::Metadata& metadata, + std::string& effectivePolicyID) const override; private: const bool allowed_if_matched_; - std::vector policies_; + std::map policies_; }; } // namespace RBAC diff --git a/source/extensions/filters/http/rbac/rbac_filter.cc b/source/extensions/filters/http/rbac/rbac_filter.cc index 08a347a79aa9b..52fecc22f01e5 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.cc +++ b/source/extensions/filters/http/rbac/rbac_filter.cc @@ -77,24 +77,34 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head callbacks_->connection()->ssl()->subjectPeerCertificate() : "none", headers, callbacks_->requestInfo().dynamicMetadata().DebugString()); + std::string effective_policyID; const absl::optional& shadow_engine = config_->engine(callbacks_->route(), EnforcementMode::Shadow); if (shadow_engine.has_value()) { + std::string shadow_resp_code = "200"; if (shadow_engine->allowed(*callbacks_->connection(), headers, - callbacks_->requestInfo().dynamicMetadata())) { + callbacks_->requestInfo().dynamicMetadata(), effective_policyID)) { ENVOY_LOG(debug, "shadow allowed"); config_->stats().shadow_allowed_.inc(); } else { ENVOY_LOG(debug, "shadow denied"); config_->stats().shadow_denied_.inc(); + shadow_resp_code = "403"; } + + auto filter_meta = callbacks_->requestInfo().dynamicMetadata().filter_metadata(); + if (effective_policyID != "") { + filter_meta["shadow"].MergeFrom( + MessageUtil::keyValueStruct("effective_policyID", effective_policyID)); + } + filter_meta["shadow"].MergeFrom(MessageUtil::keyValueStruct("response_code", shadow_resp_code)); } const absl::optional& engine = config_->engine(callbacks_->route(), EnforcementMode::Enforced); if (engine.has_value()) { if (engine->allowed(*callbacks_->connection(), headers, - callbacks_->requestInfo().dynamicMetadata())) { + callbacks_->requestInfo().dynamicMetadata(), effective_policyID)) { ENVOY_LOG(debug, "enforced allowed"); config_->stats().allowed_.inc(); return Http::FilterHeadersStatus::Continue; diff --git a/test/extensions/filters/common/rbac/engine_impl_test.cc b/test/extensions/filters/common/rbac/engine_impl_test.cc index b4f441f44cab1..f58696daef6a2 100644 --- a/test/extensions/filters/common/rbac/engine_impl_test.cc +++ b/test/extensions/filters/common/rbac/engine_impl_test.cc @@ -20,12 +20,12 @@ namespace Common { namespace RBAC { namespace { -void checkEngine( - const RBAC::RoleBasedAccessControlEngineImpl& engine, bool expected, - const Envoy::Network::Connection& connection = Envoy::Network::MockConnection(), - const Envoy::Http::HeaderMap& headers = Envoy::Http::HeaderMapImpl(), - const envoy::api::v2::core::Metadata& metadata = envoy::api::v2::core::Metadata()) { - EXPECT_EQ(expected, engine.allowed(connection, headers, metadata)); +void checkEngine(const RBAC::RoleBasedAccessControlEngineImpl& engine, bool expected, + const Envoy::Network::Connection& connection = Envoy::Network::MockConnection(), + const Envoy::Http::HeaderMap& headers = Envoy::Http::HeaderMapImpl(), + const envoy::api::v2::core::Metadata& metadata = envoy::api::v2::core::Metadata(), + std::string policyID = "") { + EXPECT_EQ(expected, engine.allowed(connection, headers, metadata, policyID)); } TEST(RoleBasedAccessControlEngineImpl, Disabled) { diff --git a/test/extensions/filters/common/rbac/mocks.h b/test/extensions/filters/common/rbac/mocks.h index a289b6cea8c37..6156337868a72 100644 --- a/test/extensions/filters/common/rbac/mocks.h +++ b/test/extensions/filters/common/rbac/mocks.h @@ -15,8 +15,9 @@ class MockEngine : public RoleBasedAccessControlEngineImpl { MockEngine(const envoy::config::rbac::v2alpha::RBAC& rules) : RoleBasedAccessControlEngineImpl(rules){}; - MOCK_CONST_METHOD3(allowed, bool(const Envoy::Network::Connection&, const Envoy::Http::HeaderMap&, - const envoy::api::v2::core::Metadata&)); + MOCK_CONST_METHOD4(allowed, + bool(const Envoy::Network::Connection&, const Envoy::Http::HeaderMap&, + const envoy::api::v2::core::Metadata&, std::string& effectivePolicyID)); }; } // namespace RBAC diff --git a/test/extensions/filters/http/rbac/rbac_filter_test.cc b/test/extensions/filters/http/rbac/rbac_filter_test.cc index 7e1d89ff852b3..df4cbefdc211b 100644 --- a/test/extensions/filters/http/rbac/rbac_filter_test.cc +++ b/test/extensions/filters/http/rbac/rbac_filter_test.cc @@ -1,3 +1,4 @@ +#include "common/config/metadata.h" #include "common/network/utility.h" #include "extensions/filters/http/rbac/rbac_filter.h" @@ -43,6 +44,7 @@ class RoleBasedAccessControlFilterTest : public testing::Test { void SetUp() { EXPECT_CALL(callbacks_, connection()).WillRepeatedly(Return(&connection_)); + EXPECT_CALL(callbacks_, requestInfo()).WillRepeatedly(ReturnRef(req_info_)); filter_.setDecoderFilterCallbacks(callbacks_); } @@ -53,6 +55,7 @@ class RoleBasedAccessControlFilterTest : public testing::Test { NiceMock callbacks_; NiceMock connection_{}; + NiceMock req_info_; Stats::IsolatedStoreImpl store_; RoleBasedAccessControlFilterConfigSharedPtr config_; @@ -98,7 +101,7 @@ TEST_F(RoleBasedAccessControlFilterTest, RouteLocalOverride) { NiceMock engine{route_config.rbac().rules()}; NiceMock per_route_config_{route_config}; - EXPECT_CALL(engine, allowed(_, _, _)).WillRepeatedly(Return(true)); + EXPECT_CALL(engine, allowed(_, _, _, _)).WillRepeatedly(Return(true)); EXPECT_CALL(per_route_config_, engine()).WillRepeatedly(ReturnRef(engine)); EXPECT_CALL(callbacks_.route_->route_entry_, perFilterConfig(HttpFilterNames::get().Rbac)) From eae1cad18feec76361690b9e9e0b03ef748ec6ac Mon Sep 17 00:00:00 2001 From: Quanjie Lin Date: Mon, 6 Aug 2018 13:38:35 -0700 Subject: [PATCH 2/6] address comment Signed-off-by: Quanjie Lin --- .../extensions/filters/common/rbac/engine.h | 2 +- .../filters/common/rbac/engine_impl.cc | 6 ++--- .../filters/common/rbac/engine_impl.h | 2 +- .../filters/http/rbac/rbac_filter.cc | 22 ++++++++++++++----- .../filters/http/rbac/rbac_filter_test.cc | 1 + 5 files changed, 23 insertions(+), 10 deletions(-) diff --git a/source/extensions/filters/common/rbac/engine.h b/source/extensions/filters/common/rbac/engine.h index cde3977b9fc76..6665f29374137 100644 --- a/source/extensions/filters/common/rbac/engine.h +++ b/source/extensions/filters/common/rbac/engine.h @@ -28,7 +28,7 @@ class RoleBasedAccessControlEngine { */ virtual bool allowed(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, const envoy::api::v2::core::Metadata& metadata, - std::string& effectivePolicyID) const PURE; + std::string& effective_policyID) const PURE; }; } // namespace RBAC diff --git a/source/extensions/filters/common/rbac/engine_impl.cc b/source/extensions/filters/common/rbac/engine_impl.cc index 3b5a500cc803c..9c4a6437cd573 100644 --- a/source/extensions/filters/common/rbac/engine_impl.cc +++ b/source/extensions/filters/common/rbac/engine_impl.cc @@ -11,20 +11,20 @@ RoleBasedAccessControlEngineImpl::RoleBasedAccessControlEngineImpl( : allowed_if_matched_(rules.action() == envoy::config::rbac::v2alpha::RBAC_Action::RBAC_Action_ALLOW) { for (const auto& policy : rules.policies()) { - policies_.insert(std::pair(policy.first, policy.second)); + policies_.insert(std::make_pair(policy.first, policy.second)); } } bool RoleBasedAccessControlEngineImpl::allowed(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, const envoy::api::v2::core::Metadata& metadata, - std::string& effectivePolicyID) const { + std::string& effective_policyID) const { bool matched = false; for (auto it = policies_.begin(); it != policies_.end(); it++) { if (it->second.matches(connection, headers, metadata)) { matched = true; - effectivePolicyID = it->first; + effective_policyID = it->first; break; } } diff --git a/source/extensions/filters/common/rbac/engine_impl.h b/source/extensions/filters/common/rbac/engine_impl.h index c2ea93441e823..283ed522d21c5 100644 --- a/source/extensions/filters/common/rbac/engine_impl.h +++ b/source/extensions/filters/common/rbac/engine_impl.h @@ -17,7 +17,7 @@ class RoleBasedAccessControlEngineImpl : public RoleBasedAccessControlEngine { bool allowed(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, const envoy::api::v2::core::Metadata& metadata, - std::string& effectivePolicyID) const override; + std::string& effective_policyID) const override; private: const bool allowed_if_matched_; diff --git a/source/extensions/filters/http/rbac/rbac_filter.cc b/source/extensions/filters/http/rbac/rbac_filter.cc index 52fecc22f01e5..6aa6a818d5d5b 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.cc +++ b/source/extensions/filters/http/rbac/rbac_filter.cc @@ -92,12 +92,24 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head shadow_resp_code = "403"; } - auto filter_meta = callbacks_->requestInfo().dynamicMetadata().filter_metadata(); - if (effective_policyID != "") { - filter_meta["shadow"].MergeFrom( - MessageUtil::keyValueStruct("effective_policyID", effective_policyID)); + const auto& filter_metadata = callbacks_->requestInfo().dynamicMetadata().filter_metadata(); + const auto filter_it = filter_metadata.find(HttpFilterNames::get().Rbac); + if (filter_it != filter_metadata.end()) { + ProtobufWkt::Struct metrics; + + if (effective_policyID != "") { + ProtobufWkt::Value policy_id; + policy_id.set_string_value(effective_policyID); + (*metrics.mutable_fields())["shadow_effective_policyID"] = policy_id; + } + + ProtobufWkt::Value resp_code; + resp_code.set_string_value(shadow_resp_code); + (*metrics.mutable_fields())["shadow_response_code"] = resp_code; + + auto filter_meta = filter_metadata.at(HttpFilterNames::get().Rbac); + filter_meta.MergeFrom(metrics); } - filter_meta["shadow"].MergeFrom(MessageUtil::keyValueStruct("response_code", shadow_resp_code)); } const absl::optional& engine = diff --git a/test/extensions/filters/http/rbac/rbac_filter_test.cc b/test/extensions/filters/http/rbac/rbac_filter_test.cc index df4cbefdc211b..4febdda18835d 100644 --- a/test/extensions/filters/http/rbac/rbac_filter_test.cc +++ b/test/extensions/filters/http/rbac/rbac_filter_test.cc @@ -59,6 +59,7 @@ class RoleBasedAccessControlFilterTest : public testing::Test { Stats::IsolatedStoreImpl store_; RoleBasedAccessControlFilterConfigSharedPtr config_; + envoy::api::v2::core::Metadata metadata_; RoleBasedAccessControlFilter filter_; Network::Address::InstanceConstSharedPtr address_; Http::TestHeaderMapImpl headers_; From 9d70d385e951427bb253372a3b6d5591c991b98c Mon Sep 17 00:00:00 2001 From: Quanjie Lin Date: Mon, 6 Aug 2018 16:14:21 -0700 Subject: [PATCH 3/6] rename Signed-off-by: Quanjie Lin --- source/extensions/filters/common/rbac/engine.h | 2 +- source/extensions/filters/common/rbac/engine_impl.cc | 4 ++-- source/extensions/filters/common/rbac/engine_impl.h | 2 +- source/extensions/filters/http/rbac/rbac_filter.cc | 10 +++++----- .../extensions/filters/common/rbac/engine_impl_test.cc | 4 ++-- test/extensions/filters/common/rbac/mocks.h | 2 +- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/source/extensions/filters/common/rbac/engine.h b/source/extensions/filters/common/rbac/engine.h index 6665f29374137..b2de48e3c49b4 100644 --- a/source/extensions/filters/common/rbac/engine.h +++ b/source/extensions/filters/common/rbac/engine.h @@ -28,7 +28,7 @@ class RoleBasedAccessControlEngine { */ virtual bool allowed(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, const envoy::api::v2::core::Metadata& metadata, - std::string& effective_policyID) const PURE; + std::string& effective_policy_id) const PURE; }; } // namespace RBAC diff --git a/source/extensions/filters/common/rbac/engine_impl.cc b/source/extensions/filters/common/rbac/engine_impl.cc index 9c4a6437cd573..5c8d595e84ffa 100644 --- a/source/extensions/filters/common/rbac/engine_impl.cc +++ b/source/extensions/filters/common/rbac/engine_impl.cc @@ -18,13 +18,13 @@ RoleBasedAccessControlEngineImpl::RoleBasedAccessControlEngineImpl( bool RoleBasedAccessControlEngineImpl::allowed(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, const envoy::api::v2::core::Metadata& metadata, - std::string& effective_policyID) const { + std::string& effective_policy_id) const { bool matched = false; for (auto it = policies_.begin(); it != policies_.end(); it++) { if (it->second.matches(connection, headers, metadata)) { matched = true; - effective_policyID = it->first; + effective_policy_id = it->first; break; } } diff --git a/source/extensions/filters/common/rbac/engine_impl.h b/source/extensions/filters/common/rbac/engine_impl.h index 283ed522d21c5..bc987454642b2 100644 --- a/source/extensions/filters/common/rbac/engine_impl.h +++ b/source/extensions/filters/common/rbac/engine_impl.h @@ -17,7 +17,7 @@ class RoleBasedAccessControlEngineImpl : public RoleBasedAccessControlEngine { bool allowed(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, const envoy::api::v2::core::Metadata& metadata, - std::string& effective_policyID) const override; + std::string& effective_policy_id) const override; private: const bool allowed_if_matched_; diff --git a/source/extensions/filters/http/rbac/rbac_filter.cc b/source/extensions/filters/http/rbac/rbac_filter.cc index 6aa6a818d5d5b..982275acd7c42 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.cc +++ b/source/extensions/filters/http/rbac/rbac_filter.cc @@ -77,13 +77,13 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head callbacks_->connection()->ssl()->subjectPeerCertificate() : "none", headers, callbacks_->requestInfo().dynamicMetadata().DebugString()); - std::string effective_policyID; + std::string effective_policy_id; const absl::optional& shadow_engine = config_->engine(callbacks_->route(), EnforcementMode::Shadow); if (shadow_engine.has_value()) { std::string shadow_resp_code = "200"; if (shadow_engine->allowed(*callbacks_->connection(), headers, - callbacks_->requestInfo().dynamicMetadata(), effective_policyID)) { + callbacks_->requestInfo().dynamicMetadata(), effective_policy_id)) { ENVOY_LOG(debug, "shadow allowed"); config_->stats().shadow_allowed_.inc(); } else { @@ -97,9 +97,9 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head if (filter_it != filter_metadata.end()) { ProtobufWkt::Struct metrics; - if (effective_policyID != "") { + if (effective_policy_id != "") { ProtobufWkt::Value policy_id; - policy_id.set_string_value(effective_policyID); + policy_id.set_string_value(effective_policy_id); (*metrics.mutable_fields())["shadow_effective_policyID"] = policy_id; } @@ -116,7 +116,7 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head config_->engine(callbacks_->route(), EnforcementMode::Enforced); if (engine.has_value()) { if (engine->allowed(*callbacks_->connection(), headers, - callbacks_->requestInfo().dynamicMetadata(), effective_policyID)) { + callbacks_->requestInfo().dynamicMetadata(), effective_policy_id)) { ENVOY_LOG(debug, "enforced allowed"); config_->stats().allowed_.inc(); return Http::FilterHeadersStatus::Continue; diff --git a/test/extensions/filters/common/rbac/engine_impl_test.cc b/test/extensions/filters/common/rbac/engine_impl_test.cc index f58696daef6a2..63c23a07da3f5 100644 --- a/test/extensions/filters/common/rbac/engine_impl_test.cc +++ b/test/extensions/filters/common/rbac/engine_impl_test.cc @@ -24,8 +24,8 @@ void checkEngine(const RBAC::RoleBasedAccessControlEngineImpl& engine, bool expe const Envoy::Network::Connection& connection = Envoy::Network::MockConnection(), const Envoy::Http::HeaderMap& headers = Envoy::Http::HeaderMapImpl(), const envoy::api::v2::core::Metadata& metadata = envoy::api::v2::core::Metadata(), - std::string policyID = "") { - EXPECT_EQ(expected, engine.allowed(connection, headers, metadata, policyID)); + std::string policy_id = "") { + EXPECT_EQ(expected, engine.allowed(connection, headers, metadata, policy_id)); } TEST(RoleBasedAccessControlEngineImpl, Disabled) { diff --git a/test/extensions/filters/common/rbac/mocks.h b/test/extensions/filters/common/rbac/mocks.h index 6156337868a72..c40a36cae18f4 100644 --- a/test/extensions/filters/common/rbac/mocks.h +++ b/test/extensions/filters/common/rbac/mocks.h @@ -17,7 +17,7 @@ class MockEngine : public RoleBasedAccessControlEngineImpl { MOCK_CONST_METHOD4(allowed, bool(const Envoy::Network::Connection&, const Envoy::Http::HeaderMap&, - const envoy::api::v2::core::Metadata&, std::string& effectivePolicyID)); + const envoy::api::v2::core::Metadata&, std::string& effective_policy_id)); }; } // namespace RBAC From fabbe929a8284b26968f940fac9c261fc4533f9c Mon Sep 17 00:00:00 2001 From: Quanjie Lin Date: Tue, 7 Aug 2018 09:29:41 -0700 Subject: [PATCH 4/6] address comments Signed-off-by: Quanjie Lin --- source/extensions/filters/common/rbac/engine.h | 1 + .../extensions/filters/http/rbac/rbac_filter.cc | 15 ++++++++++----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/source/extensions/filters/common/rbac/engine.h b/source/extensions/filters/common/rbac/engine.h index b2de48e3c49b4..3b7f3dd00b29f 100644 --- a/source/extensions/filters/common/rbac/engine.h +++ b/source/extensions/filters/common/rbac/engine.h @@ -25,6 +25,7 @@ class RoleBasedAccessControlEngine { * @param headers the headers of the incoming request used to identify the action/principal. An * empty map should be used if there are no headers available. * @param metadata the metadata with additional information about the action/principal. + * @param effective_policy_id the matching policy's ID to identity the source of the allow/deny. */ virtual bool allowed(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, const envoy::api::v2::core::Metadata& metadata, diff --git a/source/extensions/filters/http/rbac/rbac_filter.cc b/source/extensions/filters/http/rbac/rbac_filter.cc index 982275acd7c42..66ab031895022 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.cc +++ b/source/extensions/filters/http/rbac/rbac_filter.cc @@ -9,6 +9,11 @@ namespace Extensions { namespace HttpFilters { namespace RBACFilter { +static const std::string resp_code_200 = "200"; +static const std::string resp_code_403 = "403"; +static const std::string shadow_policy_id_field = "shadow_effective_policyID"; +static const std::string shadow_resp_code_field = "shadow_response_code"; + RoleBasedAccessControlFilterConfig::RoleBasedAccessControlFilterConfig( const envoy::config::filter::http::rbac::v2::RBAC& proto_config, const std::string& stats_prefix, Stats::Scope& scope) @@ -81,7 +86,7 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head const absl::optional& shadow_engine = config_->engine(callbacks_->route(), EnforcementMode::Shadow); if (shadow_engine.has_value()) { - std::string shadow_resp_code = "200"; + std::string shadow_resp_code = resp_code_200; if (shadow_engine->allowed(*callbacks_->connection(), headers, callbacks_->requestInfo().dynamicMetadata(), effective_policy_id)) { ENVOY_LOG(debug, "shadow allowed"); @@ -89,7 +94,7 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head } else { ENVOY_LOG(debug, "shadow denied"); config_->stats().shadow_denied_.inc(); - shadow_resp_code = "403"; + shadow_resp_code = resp_code_403; } const auto& filter_metadata = callbacks_->requestInfo().dynamicMetadata().filter_metadata(); @@ -97,15 +102,15 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head if (filter_it != filter_metadata.end()) { ProtobufWkt::Struct metrics; - if (effective_policy_id != "") { + if (!effective_policy_id.empty()) { ProtobufWkt::Value policy_id; policy_id.set_string_value(effective_policy_id); - (*metrics.mutable_fields())["shadow_effective_policyID"] = policy_id; + (*metrics.mutable_fields())[shadow_policy_id_field] = policy_id; } ProtobufWkt::Value resp_code; resp_code.set_string_value(shadow_resp_code); - (*metrics.mutable_fields())["shadow_response_code"] = resp_code; + (*metrics.mutable_fields())[shadow_resp_code_field] = resp_code; auto filter_meta = filter_metadata.at(HttpFilterNames::get().Rbac); filter_meta.MergeFrom(metrics); From 287d3a62cf3a22818309513f2ba26e052d816ea7 Mon Sep 17 00:00:00 2001 From: Quanjie Lin Date: Tue, 7 Aug 2018 10:12:08 -0700 Subject: [PATCH 5/6] address comments Signed-off-by: Quanjie Lin --- source/extensions/filters/common/rbac/engine.h | 2 +- source/extensions/filters/common/rbac/engine_impl.cc | 6 ++++-- source/extensions/filters/common/rbac/engine_impl.h | 2 +- source/extensions/filters/http/rbac/rbac_filter.cc | 4 ++-- test/extensions/filters/common/rbac/engine_impl_test.cc | 2 +- test/extensions/filters/common/rbac/mocks.h | 2 +- 6 files changed, 10 insertions(+), 8 deletions(-) diff --git a/source/extensions/filters/common/rbac/engine.h b/source/extensions/filters/common/rbac/engine.h index 3b7f3dd00b29f..534dad1b173b1 100644 --- a/source/extensions/filters/common/rbac/engine.h +++ b/source/extensions/filters/common/rbac/engine.h @@ -29,7 +29,7 @@ class RoleBasedAccessControlEngine { */ virtual bool allowed(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, const envoy::api::v2::core::Metadata& metadata, - std::string& effective_policy_id) const PURE; + std::string* effective_policy_id) const PURE; }; } // namespace RBAC diff --git a/source/extensions/filters/common/rbac/engine_impl.cc b/source/extensions/filters/common/rbac/engine_impl.cc index 5c8d595e84ffa..0d3a01fdde89e 100644 --- a/source/extensions/filters/common/rbac/engine_impl.cc +++ b/source/extensions/filters/common/rbac/engine_impl.cc @@ -18,13 +18,15 @@ RoleBasedAccessControlEngineImpl::RoleBasedAccessControlEngineImpl( bool RoleBasedAccessControlEngineImpl::allowed(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, const envoy::api::v2::core::Metadata& metadata, - std::string& effective_policy_id) const { + std::string* effective_policy_id) const { bool matched = false; for (auto it = policies_.begin(); it != policies_.end(); it++) { if (it->second.matches(connection, headers, metadata)) { matched = true; - effective_policy_id = it->first; + if (effective_policy_id != nullptr) { + *effective_policy_id = it->first; + } break; } } diff --git a/source/extensions/filters/common/rbac/engine_impl.h b/source/extensions/filters/common/rbac/engine_impl.h index bc987454642b2..e8343d737d94b 100644 --- a/source/extensions/filters/common/rbac/engine_impl.h +++ b/source/extensions/filters/common/rbac/engine_impl.h @@ -17,7 +17,7 @@ class RoleBasedAccessControlEngineImpl : public RoleBasedAccessControlEngine { bool allowed(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, const envoy::api::v2::core::Metadata& metadata, - std::string& effective_policy_id) const override; + std::string* effective_policy_id) const override; private: const bool allowed_if_matched_; diff --git a/source/extensions/filters/http/rbac/rbac_filter.cc b/source/extensions/filters/http/rbac/rbac_filter.cc index 66ab031895022..277ef15d614eb 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.cc +++ b/source/extensions/filters/http/rbac/rbac_filter.cc @@ -88,7 +88,7 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head if (shadow_engine.has_value()) { std::string shadow_resp_code = resp_code_200; if (shadow_engine->allowed(*callbacks_->connection(), headers, - callbacks_->requestInfo().dynamicMetadata(), effective_policy_id)) { + callbacks_->requestInfo().dynamicMetadata(), &effective_policy_id)) { ENVOY_LOG(debug, "shadow allowed"); config_->stats().shadow_allowed_.inc(); } else { @@ -121,7 +121,7 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head config_->engine(callbacks_->route(), EnforcementMode::Enforced); if (engine.has_value()) { if (engine->allowed(*callbacks_->connection(), headers, - callbacks_->requestInfo().dynamicMetadata(), effective_policy_id)) { + callbacks_->requestInfo().dynamicMetadata(), nullptr)) { ENVOY_LOG(debug, "enforced allowed"); config_->stats().allowed_.inc(); return Http::FilterHeadersStatus::Continue; diff --git a/test/extensions/filters/common/rbac/engine_impl_test.cc b/test/extensions/filters/common/rbac/engine_impl_test.cc index 63c23a07da3f5..cf848847728c9 100644 --- a/test/extensions/filters/common/rbac/engine_impl_test.cc +++ b/test/extensions/filters/common/rbac/engine_impl_test.cc @@ -24,7 +24,7 @@ void checkEngine(const RBAC::RoleBasedAccessControlEngineImpl& engine, bool expe const Envoy::Network::Connection& connection = Envoy::Network::MockConnection(), const Envoy::Http::HeaderMap& headers = Envoy::Http::HeaderMapImpl(), const envoy::api::v2::core::Metadata& metadata = envoy::api::v2::core::Metadata(), - std::string policy_id = "") { + std::string* policy_id = nullptr) { EXPECT_EQ(expected, engine.allowed(connection, headers, metadata, policy_id)); } diff --git a/test/extensions/filters/common/rbac/mocks.h b/test/extensions/filters/common/rbac/mocks.h index c40a36cae18f4..b0e0a4de82a10 100644 --- a/test/extensions/filters/common/rbac/mocks.h +++ b/test/extensions/filters/common/rbac/mocks.h @@ -17,7 +17,7 @@ class MockEngine : public RoleBasedAccessControlEngineImpl { MOCK_CONST_METHOD4(allowed, bool(const Envoy::Network::Connection&, const Envoy::Http::HeaderMap&, - const envoy::api::v2::core::Metadata&, std::string& effective_policy_id)); + const envoy::api::v2::core::Metadata&, std::string* effective_policy_id)); }; } // namespace RBAC From 1005003a5d79193fc968ba2d12ea7d46015ac5af Mon Sep 17 00:00:00 2001 From: Quanjie Lin Date: Tue, 7 Aug 2018 11:14:15 -0700 Subject: [PATCH 6/6] update comment Signed-off-by: Quanjie Lin --- source/extensions/filters/common/rbac/engine.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/common/rbac/engine.h b/source/extensions/filters/common/rbac/engine.h index 534dad1b173b1..e966fc3e54898 100644 --- a/source/extensions/filters/common/rbac/engine.h +++ b/source/extensions/filters/common/rbac/engine.h @@ -25,7 +25,8 @@ class RoleBasedAccessControlEngine { * @param headers the headers of the incoming request used to identify the action/principal. An * empty map should be used if there are no headers available. * @param metadata the metadata with additional information about the action/principal. - * @param effective_policy_id the matching policy's ID to identity the source of the allow/deny. + * @param effective_policy_id it will be filled by the matching policy's ID, + * which is used to identity the source of the allow/deny. */ virtual bool allowed(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, const envoy::api::v2::core::Metadata& metadata,