Skip to content

Commit

Permalink
Reapply "implement per-route override for rbac stat prefixes (#35531)… (
Browse files Browse the repository at this point in the history 
#35693)

…" (#35655)

This reverts commit 7ae5ca5.

The flakiness in #35653 is not
caused by this PR. If you look at [the
failure](https://btx.cloud.google.com/invocations/08f19e64-5934-4ed9-9f4c-e0d14e113360/targets/%2F%2Ftest%2Fextensions%2Ffilters%2Fnetwork%2Frbac:integration_test;config=a48d3689347bfb6aaa2bb9b4c87edaab1bb2d2c4710bec9b6690537c48f15140/log)
(you can also find this by going to the issue and clicking the link),
the failing test is
`RoleBasedAccessControlNetworkFilterIntegrationTest.DelayDenied`. That
test was added in #33875 and is
still flaky at head.

Signed-off-by: antoniovleonti <[email protected]>
  • Loading branch information
@antoniovleonti
antoniovleonti authored Aug 15, 2024
1 parent 9263d88 commit 40dddc9
Show file tree
Hide file tree
Showing 4 changed files with 174 additions and 22 deletions.
4 changes: 4 additions & 0 deletions changelogs/current.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ bug_fixes:
change: |
Add runtime guard for timeout error code 504 Gateway Timeout that is returned to downstream. If runtime flag
``envoy.reloadable_features.ext_proc_timeout_error`` is set to false, old error code 500 Internal Server Error will be returned.
- area: rbac
change: |
RBAC will now allow stat prefixes configured in per-route config to override the base config's
stat prefix.
removed_config_or_runtime:
# *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`
Expand Down
39 changes: 33 additions & 6 deletions source/extensions/filters/http/rbac/rbac_filter.cc
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include "rbac_filter.h"
#include "source/extensions/filters/http/rbac/rbac_filter.h"

#include "envoy/stats/scope.h"
Expand Down Expand Up @@ -74,6 +75,29 @@ RoleBasedAccessControlFilterConfig::RoleBasedAccessControlFilterConfig(
shadow_engine_(Filters::Common::RBAC::createShadowEngine(
proto_config, context, validation_visitor, action_validation_visitor_)) {}

#define DEFINE_DYNAMIC_METADATA_STAT_KEY_GETTER(GETTER_NAME, PREFIX, ROUTE_LOCAL_PREFIX_OVERRIDE, \
DYNAMIC_METADATA_KEY) \
std::string RoleBasedAccessControlFilterConfig::GETTER_NAME( \
const Http::StreamFilterCallbacks* callbacks) const { \
const auto* route_local = Http::Utility::resolveMostSpecificPerFilterConfig< \
RoleBasedAccessControlRouteSpecificFilterConfig>(callbacks); \
std::string prefix = PREFIX; \
if (route_local && !route_local->ROUTE_LOCAL_PREFIX_OVERRIDE().empty()) { \
prefix = route_local->ROUTE_LOCAL_PREFIX_OVERRIDE(); \
} \
return prefix + \
Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().DYNAMIC_METADATA_KEY; \
}

DEFINE_DYNAMIC_METADATA_STAT_KEY_GETTER(shadowEffectivePolicyIdField, shadow_rules_stat_prefix_,
shadowRulesStatPrefix, ShadowEffectivePolicyIdField)
DEFINE_DYNAMIC_METADATA_STAT_KEY_GETTER(shadowEngineResultField, shadow_rules_stat_prefix_,
shadowRulesStatPrefix, ShadowEngineResultField)
DEFINE_DYNAMIC_METADATA_STAT_KEY_GETTER(enforcedEffectivePolicyIdField, rules_stat_prefix_,
rulesStatPrefix, EnforcedEffectivePolicyIdField)
DEFINE_DYNAMIC_METADATA_STAT_KEY_GETTER(enforcedEngineResultField, rules_stat_prefix_,
rulesStatPrefix, EnforcedEngineResultField)

const Filters::Common::RBAC::RoleBasedAccessControlEngine*
RoleBasedAccessControlFilterConfig::engine(const Http::StreamFilterCallbacks* callbacks,
Filters::Common::RBAC::EnforcementMode mode) const {
Expand Down Expand Up @@ -103,7 +127,9 @@ RoleBasedAccessControlRouteSpecificFilterConfig::RoleBasedAccessControlRouteSpec
const envoy::extensions::filters::http::rbac::v3::RBACPerRoute& per_route_config,
Server::Configuration::ServerFactoryContext& context,
ProtobufMessage::ValidationVisitor& validation_visitor)
: per_rule_stats_(per_route_config.rbac().track_per_rule_stats()) {
: rules_stat_prefix_(per_route_config.rbac().rules_stat_prefix()),
shadow_rules_stat_prefix_(per_route_config.rbac().shadow_rules_stat_prefix()),
per_rule_stats_(per_route_config.rbac().track_per_rule_stats()) {
// Moved from member initializer to ctor body to overcome clang false warning about memory
// leak (clang-analyzer-cplusplus.NewDeleteLeaks,-warnings-as-errors).
// Potentially https://lists.llvm.org/pipermail/llvm-bugs/2018-July/066769.html
Expand Down Expand Up @@ -165,10 +191,11 @@ RoleBasedAccessControlFilter::decodeHeaders(Http::RequestHeaderMap& headers, boo
}

if (!effective_policy_id.empty()) {
*fields[config_->shadowEffectivePolicyIdField()].mutable_string_value() = effective_policy_id;
*fields[config_->shadowEffectivePolicyIdField(callbacks_)].mutable_string_value() =
effective_policy_id;
}

*fields[config_->shadowEngineResultField()].mutable_string_value() = shadow_resp_code;
*fields[config_->shadowEngineResultField(callbacks_)].mutable_string_value() = shadow_resp_code;
}

const auto engine = config_->engine(callbacks_, Filters::Common::RBAC::EnforcementMode::Enforced);
Expand All @@ -178,7 +205,7 @@ RoleBasedAccessControlFilter::decodeHeaders(Http::RequestHeaderMap& headers, boo
callbacks_->streamInfo(), &effective_policy_id);
const std::string log_policy_id = effective_policy_id.empty() ? "none" : effective_policy_id;
if (!effective_policy_id.empty()) {
*fields[config_->enforcedEffectivePolicyIdField()].mutable_string_value() =
*fields[config_->enforcedEffectivePolicyIdField(callbacks_)].mutable_string_value() =
effective_policy_id;
}
if (allowed) {
Expand All @@ -188,7 +215,7 @@ RoleBasedAccessControlFilter::decodeHeaders(Http::RequestHeaderMap& headers, boo
config_->stats().incPolicyAllowed(effective_policy_id);
}

*fields[config_->enforcedEngineResultField()].mutable_string_value() =
*fields[config_->enforcedEngineResultField(callbacks_)].mutable_string_value() =
Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().EngineResultAllowed;
callbacks_->streamInfo().setDynamicMetadata("envoy.filters.http.rbac", metrics);

Expand All @@ -203,7 +230,7 @@ RoleBasedAccessControlFilter::decodeHeaders(Http::RequestHeaderMap& headers, boo
config_->stats().incPolicyDenied(effective_policy_id);
}

*fields[config_->enforcedEngineResultField()].mutable_string_value() =
*fields[config_->enforcedEngineResultField(callbacks_)].mutable_string_value() =
Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().EngineResultDenied;
callbacks_->streamInfo().setDynamicMetadata("envoy.filters.http.rbac", metrics);

Expand Down
25 changes: 9 additions & 16 deletions source/extensions/filters/http/rbac/rbac_filter.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,15 @@ class RoleBasedAccessControlRouteSpecificFilterConfig : public Router::RouteSpec
return mode == Filters::Common::RBAC::EnforcementMode::Enforced ? engine_.get()
: shadow_engine_.get();
}
std::string rulesStatPrefix() const { return rules_stat_prefix_; }

std::string shadowRulesStatPrefix() const { return shadow_rules_stat_prefix_; }

bool perRuleStatsEnabled() const { return per_rule_stats_; }

private:
const std::string rules_stat_prefix_;
const std::string shadow_rules_stat_prefix_;
ActionValidationVisitor action_validation_visitor_;
std::unique_ptr<Filters::Common::RBAC::RoleBasedAccessControlEngine> engine_;
std::unique_ptr<Filters::Common::RBAC::RoleBasedAccessControlEngine> shadow_engine_;
Expand All @@ -57,23 +62,11 @@ class RoleBasedAccessControlFilterConfig {
ProtobufMessage::ValidationVisitor& validation_visitor);

Filters::Common::RBAC::RoleBasedAccessControlFilterStats& stats() { return stats_; }
std::string shadowEffectivePolicyIdField() const {
return shadow_rules_stat_prefix_ +
Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().ShadowEffectivePolicyIdField;
}
std::string shadowEngineResultField() const {
return shadow_rules_stat_prefix_ +
Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().ShadowEngineResultField;
}
std::string shadowEffectivePolicyIdField(const Http::StreamFilterCallbacks* callbacks) const;
std::string shadowEngineResultField(const Http::StreamFilterCallbacks* callbacks) const;

std::string enforcedEffectivePolicyIdField() const {
return rules_stat_prefix_ + Filters::Common::RBAC::DynamicMetadataKeysSingleton::get()
.EnforcedEffectivePolicyIdField;
}
std::string enforcedEngineResultField() const {
return rules_stat_prefix_ +
Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().EnforcedEngineResultField;
}
std::string enforcedEffectivePolicyIdField(const Http::StreamFilterCallbacks* callbacks) const;
std::string enforcedEngineResultField(const Http::StreamFilterCallbacks* callbacks) const;

const Filters::Common::RBAC::RoleBasedAccessControlEngine*
engine(const Http::StreamFilterCallbacks* callbacks,
Expand Down
128 changes: 128 additions & 0 deletions test/extensions/filters/http/rbac/rbac_filter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -576,6 +576,134 @@ TEST_F(RoleBasedAccessControlFilterTest, RouteLocalOverride) {
checkAccessLogMetadata(LogResult::Undecided);
}

TEST_F(RoleBasedAccessControlFilterTest, RouteLocalOverrideDynamicMetadataStats) {
setupPolicy(envoy::config::rbac::v3::RBAC::DENY, "original_rbac_rules_prefix_");

setDestinationPort(123);
setMetadata();

// Set up an allow route_config that overrides the deny policy.
envoy::extensions::filters::http::rbac::v3::RBACPerRoute route_config;
route_config.mutable_rbac()->mutable_rules()->set_action(envoy::config::rbac::v3::RBAC::ALLOW);
route_config.mutable_rbac()->mutable_shadow_rules()->set_action(
envoy::config::rbac::v3::RBAC::ALLOW);
route_config.mutable_rbac()->set_rules_stat_prefix("override_rules_stat_prefix_");
route_config.mutable_rbac()->set_shadow_rules_stat_prefix("override_shadow_rules_stat_prefix_");

envoy::config::rbac::v3::Policy policy;
auto policy_rules = policy.add_permissions()->mutable_or_rules();
policy_rules->add_rules()->set_destination_port(123);
policy.add_principals()->set_any(true);

envoy::config::rbac::v3::Policy shadow_policy;
auto shadow_policy_rules = shadow_policy.add_permissions()->mutable_or_rules();
shadow_policy_rules->add_rules()->set_destination_port(123);
shadow_policy.add_principals()->set_any(true);

(*route_config.mutable_rbac()->mutable_rules()->mutable_policies())["foobar"] = policy;
(*route_config.mutable_rbac()->mutable_shadow_rules()->mutable_policies())["foobar"] =
shadow_policy;
NiceMock<Server::Configuration::MockServerFactoryContext> factory_context;
NiceMock<Filters::Common::RBAC::MockEngine> engine{route_config.rbac().rules(), factory_context};
NiceMock<MockRoleBasedAccessControlRouteSpecificFilterConfig> per_route_config_{route_config,
context_};

EXPECT_CALL(engine, handleAction(_, _, _, _)).WillRepeatedly(Return(true));
EXPECT_CALL(per_route_config_, engine()).WillRepeatedly(ReturnRef(engine));

EXPECT_CALL(*callbacks_.route_, mostSpecificPerFilterConfig(_))
.WillRepeatedly(Return(&per_route_config_));

// Filter iteration should continue since the route-specific policy is ALLOW
// and there are enforced and shadow rules.
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(headers_, true));
ASSERT_TRUE(req_info_.dynamicMetadata().filter_metadata().contains("envoy.filters.http.rbac"));
auto filter_meta = req_info_.dynamicMetadata().filter_metadata().at("envoy.filters.http.rbac");

// We expect the route-specific rules and prefix to be used for the enforced
// engine and the shadow rules and prefix to be used for the shadow engine.
ASSERT_TRUE(filter_meta.fields().contains("override_rules_stat_prefix_enforced_engine_result"));
EXPECT_EQ(
"allowed",
filter_meta.fields().at("override_rules_stat_prefix_enforced_engine_result").string_value());
ASSERT_TRUE(
filter_meta.fields().contains("override_rules_stat_prefix_enforced_effective_policy_id"));
EXPECT_EQ("foobar", filter_meta.fields()
.at("override_rules_stat_prefix_enforced_effective_policy_id")
.string_value());

ASSERT_TRUE(
filter_meta.fields().contains("override_shadow_rules_stat_prefix_shadow_engine_result"));
EXPECT_EQ("allowed", filter_meta.fields()
.at("override_shadow_rules_stat_prefix_shadow_engine_result")
.string_value());
ASSERT_TRUE(filter_meta.fields().contains(
"override_shadow_rules_stat_prefix_shadow_effective_policy_id"));
EXPECT_EQ("foobar", filter_meta.fields()
.at("override_shadow_rules_stat_prefix_shadow_effective_policy_id")
.string_value());
}

TEST_F(RoleBasedAccessControlFilterTest, NoRouteLocalOverrideDynamicMetadataStatsEmpty) {
setupPolicy(envoy::config::rbac::v3::RBAC::DENY, "rules_stat_prefix_");

setDestinationPort(123);
setMetadata();

// Set up an allow route_config that overrides the deny policy. But do not set up stat prefixes.
envoy::extensions::filters::http::rbac::v3::RBACPerRoute route_config;
route_config.mutable_rbac()->mutable_rules()->set_action(envoy::config::rbac::v3::RBAC::ALLOW);
route_config.mutable_rbac()->mutable_shadow_rules()->set_action(
envoy::config::rbac::v3::RBAC::ALLOW);

envoy::config::rbac::v3::Policy policy;
auto policy_rules = policy.add_permissions()->mutable_or_rules();
policy_rules->add_rules()->set_destination_port(123);
policy.add_principals()->set_any(true);

envoy::config::rbac::v3::Policy shadow_policy;
auto shadow_policy_rules = shadow_policy.add_permissions()->mutable_or_rules();
shadow_policy_rules->add_rules()->set_destination_port(123);
shadow_policy.add_principals()->set_any(true);

(*route_config.mutable_rbac()->mutable_rules()->mutable_policies())["foobar"] = policy;
(*route_config.mutable_rbac()->mutable_shadow_rules()->mutable_policies())["foobar"] =
shadow_policy;
NiceMock<Server::Configuration::MockServerFactoryContext> factory_context;
NiceMock<Filters::Common::RBAC::MockEngine> engine{route_config.rbac().rules(), factory_context};
NiceMock<MockRoleBasedAccessControlRouteSpecificFilterConfig> per_route_config_{route_config,
context_};

EXPECT_CALL(engine, handleAction(_, _, _, _)).WillRepeatedly(Return(true));
EXPECT_CALL(per_route_config_, engine()).WillRepeatedly(ReturnRef(engine));

EXPECT_CALL(*callbacks_.route_, mostSpecificPerFilterConfig(_))
.WillRepeatedly(Return(&per_route_config_));

// Filter iteration should continue since the route-specific policy is ALLOW and there are
// enforced and shadow rules.
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(headers_, true));
ASSERT_TRUE(req_info_.dynamicMetadata().filter_metadata().contains("envoy.filters.http.rbac"));
auto filter_meta = req_info_.dynamicMetadata().filter_metadata().at("envoy.filters.http.rbac");

// We expect the base rules and prefix to be used since no route-specific stat was set up.
ASSERT_TRUE(filter_meta.fields().contains("rules_stat_prefix_enforced_engine_result"));
EXPECT_EQ("allowed",
filter_meta.fields().at("rules_stat_prefix_enforced_engine_result").string_value());
ASSERT_TRUE(filter_meta.fields().contains("rules_stat_prefix_enforced_effective_policy_id"));
EXPECT_EQ(
"foobar",
filter_meta.fields().at("rules_stat_prefix_enforced_effective_policy_id").string_value());

ASSERT_TRUE(filter_meta.fields().contains("shadow_rules_prefix_shadow_engine_result"));
EXPECT_EQ("allowed",
filter_meta.fields().at("shadow_rules_prefix_shadow_engine_result").string_value());
ASSERT_TRUE(filter_meta.fields().contains("shadow_rules_prefix_shadow_effective_policy_id"));
EXPECT_EQ(
"foobar",
filter_meta.fields().at("shadow_rules_prefix_shadow_effective_policy_id").string_value());
}

TEST_F(RoleBasedAccessControlFilterTest, RouteLocalOverrideWithPerRuleStats) {
setupPolicy(envoy::config::rbac::v3::RBAC::ALLOW);

Expand Down

0 comments on commit 40dddc9

Please sign in to comment.