Skip to content

Commit

Permalink
Remove filter name from GenerateServiceConfig
Browse files Browse the repository at this point in the history
  • Loading branch information
@rockspore
rockspore committed Jul 28, 2023
1 parent 9c30f67 commit cba31bd
Show file tree
Hide file tree
Showing 12 changed files with 41 additions and 108 deletions.
12 changes: 5 additions & 7 deletions src/core/ext/filters/rbac/rbac_service_config_parser.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,12 +215,11 @@ struct RbacConfig {
Rules(Rules&&) = default;
Rules& operator=(Rules&&) = default;

Rbac TakeAsRbac(std::string name);
Rbac TakeAsRbac();
static const JsonLoaderInterface* JsonLoader(const JsonArgs&);
void JsonPostLoad(const Json&, const JsonArgs&, ValidationErrors* errors);
};

std::string name;
absl::optional<Rules> rules;

Rbac TakeAsRbac();
Expand Down Expand Up @@ -773,9 +772,9 @@ void RbacConfig::RbacPolicy::Rules::AuditLogger::JsonPostLoad(
// RbacConfig::RbacPolicy::Rules
//

Rbac RbacConfig::RbacPolicy::Rules::TakeAsRbac(std::string name) {
Rbac RbacConfig::RbacPolicy::Rules::TakeAsRbac() {
Rbac rbac;
rbac.name = std::move(name);
rbac.name = "";
rbac.action = static_cast<Rbac::Action>(action);
rbac.audit_condition = audit_condition;
for (auto& p : policies) {
Expand Down Expand Up @@ -849,15 +848,14 @@ Rbac RbacConfig::RbacPolicy::TakeAsRbac() {
if (!rules.has_value()) {
// No enforcing to be applied. An empty deny policy with an empty map
// is equivalent to no enforcing.
return Rbac(std::move(name), Rbac::Action::kDeny, {});
return Rbac("", Rbac::Action::kDeny, {});
}
return rules->TakeAsRbac(std::move(name));
return rules->TakeAsRbac();
}

const JsonLoaderInterface* RbacConfig::RbacPolicy::JsonLoader(const JsonArgs&) {
static const auto* loader = JsonObjectLoader<RbacPolicy>()
.OptionalField("rules", &RbacPolicy::rules)
.Field("filter_name", &RbacPolicy::name)
.Finish();
return loader;
}
Expand Down
3 changes: 1 addition & 2 deletions src/core/ext/xds/xds_http_fault_filter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,8 +227,7 @@ ChannelArgs XdsHttpFaultFilter::ModifyChannelArgs(
absl::StatusOr<XdsHttpFilterImpl::ServiceConfigJsonEntry>
XdsHttpFaultFilter::GenerateServiceConfig(
const FilterConfig& hcm_filter_config,
const FilterConfig* filter_config_override,
absl::string_view /*filter_name*/) const {
const FilterConfig* filter_config_override) const {
Json policy_json = filter_config_override != nullptr
? filter_config_override->config
: hcm_filter_config.config;
Expand Down
3 changes: 1 addition & 2 deletions src/core/ext/xds/xds_http_fault_filter.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,7 @@ class XdsHttpFaultFilter : public XdsHttpFilterImpl {
ChannelArgs ModifyChannelArgs(const ChannelArgs& args) const override;
absl::StatusOr<ServiceConfigJsonEntry> GenerateServiceConfig(
const FilterConfig& hcm_filter_config,
const FilterConfig* filter_config_override,
absl::string_view filter_name) const override;
const FilterConfig* filter_config_override) const override;
bool IsSupportedOnClients() const override { return true; }
bool IsSupportedOnServers() const override { return false; }
};
Expand Down
6 changes: 2 additions & 4 deletions src/core/ext/xds/xds_http_filters.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,7 @@ class XdsHttpFilterImpl {
// there is no override in any of those locations.
virtual absl::StatusOr<ServiceConfigJsonEntry> GenerateServiceConfig(
const FilterConfig& hcm_filter_config,
const FilterConfig* filter_config_override,
absl::string_view filter_name) const = 0;
const FilterConfig* filter_config_override) const = 0;

// Returns true if the filter is supported on clients; false otherwise
virtual bool IsSupportedOnClients() const = 0;
Expand All @@ -139,8 +138,7 @@ class XdsHttpRouterFilter : public XdsHttpFilterImpl {
const grpc_channel_filter* channel_filter() const override { return nullptr; }
absl::StatusOr<ServiceConfigJsonEntry> GenerateServiceConfig(
const FilterConfig& /*hcm_filter_config*/,
const FilterConfig* /*filter_config_override*/,
absl::string_view /*filter_name*/) const override {
const FilterConfig* /*filter_config_override*/) const override {
// This will never be called, since channel_filter() returns null.
return absl::UnimplementedError("router filter should never be called");
}
Expand Down
11 changes: 3 additions & 8 deletions src/core/ext/xds/xds_http_rbac_filter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -575,17 +575,12 @@ ChannelArgs XdsHttpRbacFilter::ModifyChannelArgs(
absl::StatusOr<XdsHttpFilterImpl::ServiceConfigJsonEntry>
XdsHttpRbacFilter::GenerateServiceConfig(
const FilterConfig& hcm_filter_config,
const FilterConfig* filter_config_override,
absl::string_view filter_name) const {
const FilterConfig* filter_config_override) const {
const Json& policy_json = filter_config_override != nullptr
? filter_config_override->config
: hcm_filter_config.config;
auto json_object = policy_json.object();
json_object.emplace("filter_name",
Json::FromString(std::string(filter_name)));
// The policy JSON may be empty other than the filter name, that's allowed.
return ServiceConfigJsonEntry{"rbacPolicy",
JsonDump(Json::FromObject(json_object))};
// The policy JSON may be empty and that's allowed.
return ServiceConfigJsonEntry{"rbacPolicy", JsonDump(policy_json)};
}

} // namespace grpc_core
3 changes: 1 addition & 2 deletions src/core/ext/xds/xds_http_rbac_filter.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,7 @@ class XdsHttpRbacFilter : public XdsHttpFilterImpl {
ChannelArgs ModifyChannelArgs(const ChannelArgs& args) const override;
absl::StatusOr<ServiceConfigJsonEntry> GenerateServiceConfig(
const FilterConfig& hcm_filter_config,
const FilterConfig* filter_config_override,
absl::string_view filter_name) const override;
const FilterConfig* filter_config_override) const override;
bool IsSupportedOnClients() const override { return false; }
bool IsSupportedOnServers() const override { return true; }
};
Expand Down
3 changes: 1 addition & 2 deletions src/core/ext/xds/xds_http_stateful_session_filter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,8 +211,7 @@ ChannelArgs XdsHttpStatefulSessionFilter::ModifyChannelArgs(
absl::StatusOr<XdsHttpFilterImpl::ServiceConfigJsonEntry>
XdsHttpStatefulSessionFilter::GenerateServiceConfig(
const FilterConfig& hcm_filter_config,
const FilterConfig* filter_config_override,
absl::string_view /*filter_name*/) const {
const FilterConfig* filter_config_override) const {
const Json& config = filter_config_override != nullptr
? filter_config_override->config
: hcm_filter_config.config;
Expand Down
3 changes: 1 addition & 2 deletions src/core/ext/xds/xds_http_stateful_session_filter.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,7 @@ class XdsHttpStatefulSessionFilter : public XdsHttpFilterImpl {
ChannelArgs ModifyChannelArgs(const ChannelArgs& args) const override;
absl::StatusOr<ServiceConfigJsonEntry> GenerateServiceConfig(
const FilterConfig& hcm_filter_config,
const FilterConfig* filter_config_override,
absl::string_view filter_name) const override;
const FilterConfig* filter_config_override) const override;
bool IsSupportedOnClients() const override { return true; }
bool IsSupportedOnServers() const override { return false; }
};
Expand Down
2 changes: 1 addition & 1 deletion src/core/ext/xds/xds_routing.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ XdsRouting::GeneratePerHTTPFilterConfigs(
cluster_weight);
// Generate service config for filter.
auto method_config_field = filter_impl->GenerateServiceConfig(
http_filter.config, config_override, http_filter.name);
http_filter.config, config_override);
if (!method_config_field.ok()) {
return absl::FailedPreconditionError(absl::StrCat(
"failed to generate method config for HTTP filter ", http_filter.name,
Expand Down
48 changes: 6 additions & 42 deletions test/core/ext/filters/rbac/rbac_service_config_parser_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,37 +99,15 @@ class RbacServiceConfigParsingTest : public ::testing::Test {
std::map<absl::string_view, std::string> logger_configs_;
};

// Filter name is required in RBAC policy.
TEST_F(RbacServiceConfigParsingTest, EmptyRbacPolicy) {
const char* test_json =
"{\n"
" \"methodConfig\": [ {\n"
" \"name\": [\n"
" {}\n"
" ],\n"
" \"rbacPolicy\": [ {\n"
" } ]"
" } ]\n"
"}";
ChannelArgs args = ChannelArgs().Set(GRPC_ARG_PARSE_RBAC_METHOD_CONFIG, 1);
auto service_config = ServiceConfigImpl::Create(args, test_json);
EXPECT_EQ(service_config.status().code(), absl::StatusCode::kInvalidArgument);
EXPECT_EQ(service_config.status().message(),
"errors validating service config: ["
"field:methodConfig[0].rbacPolicy[0].filter_name error:field not "
"present]")
<< service_config.status();
}

// Test basic parsing of RBAC policy
// Test parsing of an empty RBAC policy
TEST_F(RbacServiceConfigParsingTest, RbacPolicyWithoutRules) {
const char* test_json =
"{\n"
" \"methodConfig\": [ {\n"
" \"name\": [\n"
" {}\n"
" ],\n"
" \"rbacPolicy\": [ {\"filter_name\": \"rbac\"} ]\n"
" \"rbacPolicy\": [ {} ]\n"
" } ]\n"
"}";
ChannelArgs args = ChannelArgs().Set(GRPC_ARG_PARSE_RBAC_METHOD_CONFIG, 1);
Expand Down Expand Up @@ -201,9 +179,9 @@ TEST_F(RbacServiceConfigParsingTest, MultipleRbacPolicies) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [\n"
" { \"filter_name\": \"rbac-1\" },\n"
" { \"filter_name\": \"rbac-2\" },\n"
" { \"filter_name\": \"rbac-3\" }\n"
" {},\n"
" {},\n"
" {}\n"
" ]"
" } ]\n"
"}";
Expand Down Expand Up @@ -250,7 +228,7 @@ TEST_F(RbacServiceConfigParsingTest, BadRulesType) {
" \"name\": [\n"
" {}\n"
" ],\n"
" \"rbacPolicy\": [{\"filter_name\": \"rbac\", \"rules\":1}]\n"
" \"rbacPolicy\": [{\"rules\":1}]\n"
" } ]\n"
"}";
ChannelArgs args = ChannelArgs().Set(GRPC_ARG_PARSE_RBAC_METHOD_CONFIG, 1);
Expand All @@ -270,7 +248,6 @@ TEST_F(RbacServiceConfigParsingTest, BadActionAndPolicyType) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [{\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":{},\n"
" \"policies\":123\n"
Expand Down Expand Up @@ -298,7 +275,6 @@ TEST_F(RbacServiceConfigParsingTest, MissingPermissionAndPrincipals) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [{\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"policies\":{\n"
Expand Down Expand Up @@ -329,7 +305,6 @@ TEST_F(RbacServiceConfigParsingTest, EmptyPrincipalAndPermission) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [{\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"policies\":{\n"
Expand Down Expand Up @@ -362,7 +337,6 @@ TEST_F(RbacServiceConfigParsingTest, VariousPermissionsAndPrincipalsTypes) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [{\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"policies\":{\n"
Expand Down Expand Up @@ -420,7 +394,6 @@ TEST_F(RbacServiceConfigParsingTest, VariousPermissionsAndPrincipalsBadTypes) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [{\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"policies\":{\n"
Expand Down Expand Up @@ -515,7 +488,6 @@ TEST_F(RbacServiceConfigParsingTest, HeaderMatcherVariousTypes) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [{\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"policies\":{\n"
Expand Down Expand Up @@ -560,7 +532,6 @@ TEST_F(RbacServiceConfigParsingTest, HeaderMatcherBadTypes) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [{\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"policies\":{\n"
Expand Down Expand Up @@ -617,7 +588,6 @@ TEST_F(RbacServiceConfigParsingTest, StringMatcherVariousTypes) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [{\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"policies\":{\n"
Expand Down Expand Up @@ -659,7 +629,6 @@ TEST_F(RbacServiceConfigParsingTest, StringMatcherBadTypes) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [{\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"policies\":{\n"
Expand Down Expand Up @@ -715,7 +684,6 @@ TEST_F(RbacServiceConfigParsingTest, AuditConditionOnDenyWithMultipleLoggers) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [ {\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"audit_condition\":1,\n"
Expand Down Expand Up @@ -760,7 +728,6 @@ TEST_F(RbacServiceConfigParsingTest, BadAuditLoggerConfig) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [ {\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"audit_condition\":1,\n"
Expand Down Expand Up @@ -791,7 +758,6 @@ TEST_F(RbacServiceConfigParsingTest, UnknownAuditLoggerConfig) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [ {\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"audit_condition\":1,\n"
Expand Down Expand Up @@ -822,7 +788,6 @@ TEST_F(RbacServiceConfigParsingTest, BadAuditConditionAndLoggersTypes) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [ {\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"audit_condition\":{},\n"
Expand Down Expand Up @@ -851,7 +816,6 @@ TEST_F(RbacServiceConfigParsingTest, BadAuditConditionEnum) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [ {\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"audit_condition\":100\n"
Expand Down
21 changes: 8 additions & 13 deletions test/core/xds/xds_http_filters_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -305,8 +305,7 @@ TEST_F(XdsFaultInjectionFilterTest, ModifyChannelArgs) {
TEST_F(XdsFaultInjectionFilterTest, GenerateServiceConfigTopLevelConfig) {
XdsHttpFilterImpl::FilterConfig config;
config.config = Json::FromObject({{"foo", Json::FromString("bar")}});
auto service_config =
filter_->GenerateServiceConfig(config, nullptr, /*filter_name=*/"");
auto service_config = filter_->GenerateServiceConfig(config, nullptr);
ASSERT_TRUE(service_config.ok()) << service_config.status();
EXPECT_EQ(service_config->service_config_field_name, "faultInjectionPolicy");
EXPECT_EQ(service_config->element, "{\"foo\":\"bar\"}");
Expand All @@ -318,8 +317,8 @@ TEST_F(XdsFaultInjectionFilterTest, GenerateServiceConfigOverrideConfig) {
XdsHttpFilterImpl::FilterConfig override_config;
override_config.config =
Json::FromObject({{"baz", Json::FromString("quux")}});
auto service_config = filter_->GenerateServiceConfig(
top_config, &override_config, /*filter_name=*/"");
auto service_config =
filter_->GenerateServiceConfig(top_config, &override_config);
ASSERT_TRUE(service_config.ok()) << service_config.status();
EXPECT_EQ(service_config->service_config_field_name, "faultInjectionPolicy");
EXPECT_EQ(service_config->element, "{\"baz\":\"quux\"}");
Expand Down Expand Up @@ -599,13 +598,11 @@ TEST_F(XdsRbacFilterTest, GenerateServiceConfig) {
XdsHttpFilterImpl::FilterConfig hcm_config = {
filter_->ConfigProtoName(),
Json::FromObject({{"name", Json::FromString("foo")}})};
auto config = filter_->GenerateServiceConfig(hcm_config, nullptr, "rbac");
auto config = filter_->GenerateServiceConfig(hcm_config, nullptr);
ASSERT_TRUE(config.ok()) << config.status();
EXPECT_EQ(config->service_config_field_name, "rbacPolicy");
EXPECT_EQ(
config->element,
JsonDump(Json::FromObject({{"name", Json::FromString("foo")},
{"filter_name", Json::FromString("rbac")}})));
EXPECT_EQ(config->element,
JsonDump(Json::FromObject({{"name", Json::FromString("foo")}})));
}

// For the RBAC filter, the override config is a superset of the
Expand Down Expand Up @@ -1169,8 +1166,7 @@ TEST_F(XdsStatefulSessionFilterTest, GenerateServiceConfigNoOverride) {
XdsHttpFilterImpl::FilterConfig hcm_config = {
filter_->ConfigProtoName(),
Json::FromObject({{"name", Json::FromString("foo")}})};
auto config =
filter_->GenerateServiceConfig(hcm_config, nullptr, /*filter_name=*/"");
auto config = filter_->GenerateServiceConfig(hcm_config, nullptr);
ASSERT_TRUE(config.ok()) << config.status();
EXPECT_EQ(config->service_config_field_name, "stateful_session");
EXPECT_EQ(config->element,
Expand All @@ -1184,8 +1180,7 @@ TEST_F(XdsStatefulSessionFilterTest, GenerateServiceConfigWithOverride) {
XdsHttpFilterImpl::FilterConfig override_config = {
filter_->OverrideConfigProtoName(),
Json::FromObject({{"name", Json::FromString("bar")}})};
auto config = filter_->GenerateServiceConfig(hcm_config, &override_config,
/*filter_name=*/"");
auto config = filter_->GenerateServiceConfig(hcm_config, &override_config);
ASSERT_TRUE(config.ok()) << config.status();
EXPECT_EQ(config->service_config_field_name, "stateful_session");
EXPECT_EQ(config->element,
Expand Down
Loading

0 comments on commit cba31bd

Please sign in to comment.