[xDS] Remove filter name from GenerateServiceConfig (#33915)

We decided to not populate `policy_name` with the HTTP filter name in
xDS case. So removing it from `GenerateServiceConfig`. This will be
consistent across languages. The gRFC
[PR](https://github.com/grpc/proposal/pull/346) has been updated.
pull/34011/head
Luwei Ge 1 year ago committed by GitHub
parent b235f20f47
commit a5f1121982
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
  1. 11
      src/core/ext/filters/rbac/rbac_service_config_parser.cc
  2. 3
      src/core/ext/xds/xds_http_fault_filter.cc
  3. 3
      src/core/ext/xds/xds_http_fault_filter.h
  4. 6
      src/core/ext/xds/xds_http_filters.h
  5. 11
      src/core/ext/xds/xds_http_rbac_filter.cc
  6. 3
      src/core/ext/xds/xds_http_rbac_filter.h
  7. 3
      src/core/ext/xds/xds_http_stateful_session_filter.cc
  8. 3
      src/core/ext/xds/xds_http_stateful_session_filter.h
  9. 4
      src/core/ext/xds/xds_routing.cc
  10. 2
      src/core/lib/security/authorization/rbac_policy.h
  11. 48
      test/core/ext/filters/rbac/rbac_service_config_parser_test.cc
  12. 21
      test/core/xds/xds_http_filters_test.cc
  13. 34
      test/cpp/end2end/xds/xds_end2end_test.cc

@ -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();
@ -773,9 +772,8 @@ 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.action = static_cast<Rbac::Action>(action);
rbac.audit_condition = audit_condition;
for (auto& p : policies) {
@ -849,15 +847,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;
}

@ -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;

@ -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; }
};

@ -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;
@ -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");
}

@ -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

@ -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; }
};

@ -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;

@ -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; }
};

@ -248,8 +248,8 @@ XdsRouting::GeneratePerHTTPFilterConfigs(
FindFilterConfigOverride(http_filter.name, vhost, route,
cluster_weight);
// Generate service config for filter.
auto method_config_field = filter_impl->GenerateServiceConfig(
http_filter.config, config_override, http_filter.name);
auto method_config_field =
filter_impl->GenerateServiceConfig(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,

@ -179,7 +179,7 @@ struct Rbac {
std::string ToString() const;
// The authorization policy name or the HTTP RBAC filter name.
// The authorization policy name or empty string in xDS case.
std::string name;
Action action;

@ -99,29 +99,7 @@ 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"
@ -129,7 +107,7 @@ TEST_F(RbacServiceConfigParsingTest, RbacPolicyWithoutRules) {
" \"name\": [\n"
" {}\n"
" ],\n"
" \"rbacPolicy\": [ {\"filter_name\": \"rbac\"} ]\n"
" \"rbacPolicy\": [ {} ]\n"
" } ]\n"
"}";
ChannelArgs args = ChannelArgs().Set(GRPC_ARG_PARSE_RBAC_METHOD_CONFIG, 1);
@ -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"
"}";
@ -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);
@ -270,7 +248,6 @@ TEST_F(RbacServiceConfigParsingTest, BadActionAndPolicyType) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [{\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":{},\n"
" \"policies\":123\n"
@ -298,7 +275,6 @@ TEST_F(RbacServiceConfigParsingTest, MissingPermissionAndPrincipals) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [{\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"policies\":{\n"
@ -329,7 +305,6 @@ TEST_F(RbacServiceConfigParsingTest, EmptyPrincipalAndPermission) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [{\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"policies\":{\n"
@ -362,7 +337,6 @@ TEST_F(RbacServiceConfigParsingTest, VariousPermissionsAndPrincipalsTypes) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [{\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"policies\":{\n"
@ -420,7 +394,6 @@ TEST_F(RbacServiceConfigParsingTest, VariousPermissionsAndPrincipalsBadTypes) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [{\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"policies\":{\n"
@ -515,7 +488,6 @@ TEST_F(RbacServiceConfigParsingTest, HeaderMatcherVariousTypes) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [{\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"policies\":{\n"
@ -560,7 +532,6 @@ TEST_F(RbacServiceConfigParsingTest, HeaderMatcherBadTypes) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [{\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"policies\":{\n"
@ -617,7 +588,6 @@ TEST_F(RbacServiceConfigParsingTest, StringMatcherVariousTypes) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [{\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"policies\":{\n"
@ -659,7 +629,6 @@ TEST_F(RbacServiceConfigParsingTest, StringMatcherBadTypes) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [{\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"policies\":{\n"
@ -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"
@ -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"
@ -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"
@ -822,7 +788,6 @@ TEST_F(RbacServiceConfigParsingTest, BadAuditConditionAndLoggersTypes) {
" {}\n"
" ],\n"
" \"rbacPolicy\": [ {\n"
" \"filter_name\": \"rbac\",\n"
" \"rules\":{\n"
" \"action\":1,\n"
" \"audit_condition\":{},\n"
@ -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"

@ -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\"}");
@ -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\"}");
@ -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
@ -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,
@ -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,

@ -2882,20 +2882,11 @@ TEST_P(XdsRbacTestWithActionPermutations,
grpc::StatusCode::PERMISSION_DENIED);
// If the second rbac denies the rpc, only one log from the first rbac.
// Otherwise, all three rbacs log.
std::vector<absl::string_view> expected = {
std::vector<absl::string_view> expected(
GetParam().rbac_action() != RBAC_Action_DENY ? 3 : 1,
"{\"authorized\":true,\"matched_rule\":\"policy\","
"\"policy_name\":\"rbac1\",\"principal\":\"\",\"rpc_"
"method\":\"/grpc.testing.EchoTestService/Echo\"}"};
if (GetParam().rbac_action() != RBAC_Action_DENY) {
expected.push_back(
"{\"authorized\":true,\"matched_rule\":\"policy\","
"\"policy_name\":\"rbac2\",\"principal\":\"\",\"rpc_"
"method\":\"/grpc.testing.EchoTestService/Echo\"}");
expected.push_back(
"{\"authorized\":true,\"matched_rule\":\"policy\","
"\"policy_name\":\"rbac3\",\"principal\":\"\",\"rpc_"
"method\":\"/grpc.testing.EchoTestService/Echo\"}");
}
"\"policy_name\":\"\",\"principal\":\"\",\"rpc_"
"method\":\"/grpc.testing.EchoTestService/Echo\"}");
EXPECT_THAT(audit_logs_, ::testing::ElementsAreArray(expected));
}
@ -2941,7 +2932,7 @@ TEST_P(XdsRbacTestWithActionPermutations, MultipleRbacPoliciesWithAuditOnDeny) {
if (GetParam().rbac_action() == RBAC_Action_DENY) {
expected.push_back(
"{\"authorized\":false,\"matched_rule\":\"policy\",\"policy_name\":"
"\"rbac2\",\"principal\":\"\",\"rpc_method\":\"/"
"\"\",\"principal\":\"\",\"rpc_method\":\"/"
"grpc.testing.EchoTestService/Echo\"}");
}
EXPECT_THAT(audit_logs_, ::testing::ElementsAreArray(expected));
@ -2989,21 +2980,18 @@ TEST_P(XdsRbacTestWithActionPermutations,
// all rbacs log.
std::vector<absl::string_view> expected = {
"{\"authorized\":true,\"matched_rule\":\"policy\",\"policy_name\":"
"\"rbac1\",\"principal\":\"\",\"rpc_method\":\"/"
"\"\",\"principal\":\"\",\"rpc_method\":\"/"
"grpc.testing.EchoTestService/Echo\"}"};
if (GetParam().rbac_action() == RBAC_Action_DENY) {
expected.push_back(
"{\"authorized\":false,\"matched_rule\":\"policy\",\"policy_name\":"
"\"rbac2\",\"principal\":\"\",\"rpc_method\":\"/"
"\"\",\"principal\":\"\",\"rpc_method\":\"/"
"grpc.testing.EchoTestService/Echo\"}");
} else {
expected.push_back(
"{\"authorized\":true,\"matched_rule\":\"policy\",\"policy_name\":"
"\"rbac2\",\"principal\":\"\",\"rpc_method\":\"/"
"grpc.testing.EchoTestService/Echo\"}");
expected.push_back(
expected = std::vector<absl::string_view>(
3,
"{\"authorized\":true,\"matched_rule\":\"policy\",\"policy_name\":"
"\"rbac3\",\"principal\":\"\",\"rpc_method\":\"/"
"\"\",\"principal\":\"\",\"rpc_method\":\"/"
"grpc.testing.EchoTestService/Echo\"}");
}
EXPECT_THAT(audit_logs_, ::testing::ElementsAreArray(expected));
@ -3084,7 +3072,7 @@ TEST_P(XdsRbacTestWithActionAndAuditConditionPermutations, MultipleLoggers) {
EXPECT_THAT(audit_logs_,
::testing::ElementsAre(absl::StrFormat(
"{\"authorized\":%s,\"matched_rule\":\"policy\","
"\"policy_name\":\"rbac1\",\"principal\":\"\","
"\"policy_name\":\"\",\"principal\":\"\","
"\"rpc_"
"method\":\"/grpc.testing.EchoTestService/Echo\"}",
action == RBAC_Action_DENY ? "false" : "true")));

Loading…
Cancel
Save