Skip to content

Commit

Permalink
Revert "implement per-route override for rbac stat prefixes (envoypro…
Browse files Browse the repository at this point in the history 
…xy#35531)" (envoyproxy#35655)

Revert "implement per-route override for rbac stat prefixes (envoyproxy#35531)"
    
Causing //test/extensions/filters/network/rbac:integration_test to
flakily time out.

This reverts commit 751217c.
    
Signed-off-by: Ryan Hamilton <[email protected]>

Signed-off-by: Ryan Hamilton <[email protected]>
  • Loading branch information
@RyanTheOptimist
RyanTheOptimist authored Aug 9, 2024
1 parent ee56715 commit 7ae5ca5
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 174 deletions.
4 changes: 0 additions & 4 deletions changelogs/current.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,6 @@ 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: 6 additions & 33 deletions source/extensions/filters/http/rbac/rbac_filter.cc
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
#include "rbac_filter.h"
#include "source/extensions/filters/http/rbac/rbac_filter.h"

#include "envoy/stats/scope.h"
Expand Down Expand Up @@ -75,29 +74,6 @@ 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 @@ -127,9 +103,7 @@ RoleBasedAccessControlRouteSpecificFilterConfig::RoleBasedAccessControlRouteSpec
const envoy::extensions::filters::http::rbac::v3::RBACPerRoute& per_route_config,
Server::Configuration::ServerFactoryContext& context,
ProtobufMessage::ValidationVisitor& validation_visitor)
: 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()) {
: 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 @@ -191,11 +165,10 @@ RoleBasedAccessControlFilter::decodeHeaders(Http::RequestHeaderMap& headers, boo
}

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

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

const auto engine = config_->engine(callbacks_, Filters::Common::RBAC::EnforcementMode::Enforced);
Expand All @@ -205,7 +178,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(callbacks_)].mutable_string_value() =
*fields[config_->enforcedEffectivePolicyIdField()].mutable_string_value() =
effective_policy_id;
}
if (allowed) {
Expand All @@ -215,7 +188,7 @@ RoleBasedAccessControlFilter::decodeHeaders(Http::RequestHeaderMap& headers, boo
config_->stats().incPolicyAllowed(effective_policy_id);
}

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

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

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

Expand Down
25 changes: 16 additions & 9 deletions source/extensions/filters/http/rbac/rbac_filter.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,10 @@ 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 @@ -62,11 +57,23 @@ class RoleBasedAccessControlFilterConfig {
ProtobufMessage::ValidationVisitor& validation_visitor);

Filters::Common::RBAC::RoleBasedAccessControlFilterStats& stats() { return stats_; }
std::string shadowEffectivePolicyIdField(const Http::StreamFilterCallbacks* callbacks) const;
std::string shadowEngineResultField(const Http::StreamFilterCallbacks* callbacks) const;
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 enforcedEffectivePolicyIdField(const Http::StreamFilterCallbacks* callbacks) const;
std::string enforcedEngineResultField(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;
}

const Filters::Common::RBAC::RoleBasedAccessControlEngine*
engine(const Http::StreamFilterCallbacks* callbacks,
Expand Down
128 changes: 0 additions & 128 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,134 +576,6 @@ 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 7ae5ca5

Please sign in to comment.