Skip to content

Commit

Permalink
[xDS] pass HTTP filter name to GenerateServiceConfig() method. (#32976
Browse files Browse the repository at this point in the history
)

We need the RBAC filter name as the `policy_name` field in audit logging
context.
  • Loading branch information
rockspore authored and wanlin31 committed May 18, 2023
1 parent 3c73fbc commit ce3d5d5
Show file tree
Hide file tree
Showing 9 changed files with 41 additions and 17 deletions.
3 changes: 2 additions & 1 deletion src/core/ext/xds/xds_http_fault_filter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,8 @@ ChannelArgs XdsHttpFaultFilter::ModifyChannelArgs(
absl::StatusOr<XdsHttpFilterImpl::ServiceConfigJsonEntry>
XdsHttpFaultFilter::GenerateServiceConfig(
const FilterConfig& hcm_filter_config,
const FilterConfig* filter_config_override) const {
const FilterConfig* filter_config_override,
absl::string_view /*filter_name*/) const {
Json policy_json = filter_config_override != nullptr
? filter_config_override->config
: hcm_filter_config.config;
Expand Down
3 changes: 2 additions & 1 deletion src/core/ext/xds/xds_http_fault_filter.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ 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) const override;
const FilterConfig* filter_config_override,
absl::string_view filter_name) const override;
bool IsSupportedOnClients() const override { return true; }
bool IsSupportedOnServers() const override { return false; }
};
Expand Down
6 changes: 4 additions & 2 deletions src/core/ext/xds/xds_http_filters.h
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ 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) const = 0;
const FilterConfig* filter_config_override,
absl::string_view filter_name) const = 0;

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

} // namespace grpc_core
3 changes: 2 additions & 1 deletion src/core/ext/xds/xds_http_rbac_filter.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ 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) const override;
const FilterConfig* filter_config_override,
absl::string_view filter_name) const override;
bool IsSupportedOnClients() const override { return false; }
bool IsSupportedOnServers() const override { return true; }
};
Expand Down
3 changes: 2 additions & 1 deletion src/core/ext/xds/xds_http_stateful_session_filter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,8 @@ ChannelArgs XdsHttpStatefulSessionFilter::ModifyChannelArgs(
absl::StatusOr<XdsHttpFilterImpl::ServiceConfigJsonEntry>
XdsHttpStatefulSessionFilter::GenerateServiceConfig(
const FilterConfig& hcm_filter_config,
const FilterConfig* filter_config_override) const {
const FilterConfig* filter_config_override,
absl::string_view /*filter_name*/) const {
Json config = filter_config_override != nullptr
? filter_config_override->config
: hcm_filter_config.config;
Expand Down
3 changes: 2 additions & 1 deletion src/core/ext/xds/xds_http_stateful_session_filter.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ 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) const override;
const FilterConfig* filter_config_override,
absl::string_view filter_name) const override;
bool IsSupportedOnClients() const override { return true; }
bool IsSupportedOnServers() const override { return false; }
};
Expand Down
4 changes: 2 additions & 2 deletions src/core/ext/xds/xds_routing.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
auto method_config_field = filter_impl->GenerateServiceConfig(
http_filter.config, config_override, http_filter.name);
if (!method_config_field.ok()) {
return absl::FailedPreconditionError(absl::StrCat(
"failed to generate method config for HTTP filter ", http_filter.name,
Expand Down
24 changes: 19 additions & 5 deletions test/core/xds/xds_http_filters_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,8 @@ TEST_F(XdsFaultInjectionFilterTest, ModifyChannelArgs) {
TEST_F(XdsFaultInjectionFilterTest, GenerateServiceConfigTopLevelConfig) {
XdsHttpFilterImpl::FilterConfig config;
config.config = Json::Object{{"foo", "bar"}};
auto service_config = filter_->GenerateServiceConfig(config, nullptr);
auto service_config =
filter_->GenerateServiceConfig(config, nullptr, /*filter_name=*/"");
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 @@ -314,8 +315,8 @@ TEST_F(XdsFaultInjectionFilterTest, GenerateServiceConfigOverrideConfig) {
top_config.config = Json::Object{{"foo", "bar"}};
XdsHttpFilterImpl::FilterConfig override_config;
override_config.config = Json::Object{{"baz", "quux"}};
auto service_config =
filter_->GenerateServiceConfig(top_config, &override_config);
auto service_config = filter_->GenerateServiceConfig(
top_config, &override_config, /*filter_name=*/"");
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 @@ -591,6 +592,17 @@ TEST_F(XdsRbacFilterTest, GenerateFilterConfigOverrideUnparseable) {
<< status;
}

TEST_F(XdsRbacFilterTest, GenerateServiceConfig) {
XdsHttpFilterImpl::FilterConfig hcm_config = {filter_->ConfigProtoName(),
Json::Object{{"name", "foo"}}};
auto config = filter_->GenerateServiceConfig(hcm_config, nullptr, "rbac");
ASSERT_TRUE(config.ok()) << config.status();
EXPECT_EQ(config->service_config_field_name, "rbacPolicy");
EXPECT_EQ(
config->element,
JsonDump(Json(Json::Object{{"name", "foo"}, {"filter_name", "rbac"}})));
}

// For the RBAC filter, the override config is a superset of the
// top-level config, so we test all of the common fields as input for
// both GenerateFilterConfig() and GenerateFilterConfigOverride().
Expand Down Expand Up @@ -1125,7 +1137,8 @@ TEST_F(XdsStatefulSessionFilterTest, OverrideConfigDisabled) {
TEST_F(XdsStatefulSessionFilterTest, GenerateServiceConfigNoOverride) {
XdsHttpFilterImpl::FilterConfig hcm_config = {filter_->ConfigProtoName(),
Json::Object{{"name", "foo"}}};
auto config = filter_->GenerateServiceConfig(hcm_config, nullptr);
auto config =
filter_->GenerateServiceConfig(hcm_config, nullptr, /*filter_name=*/"");
ASSERT_TRUE(config.ok()) << config.status();
EXPECT_EQ(config->service_config_field_name, "stateful_session");
EXPECT_EQ(config->element, JsonDump(Json(Json::Object{{"name", "foo"}})));
Expand All @@ -1136,7 +1149,8 @@ TEST_F(XdsStatefulSessionFilterTest, GenerateServiceConfigWithOverride) {
Json::Object{{"name", "foo"}}};
XdsHttpFilterImpl::FilterConfig override_config = {
filter_->OverrideConfigProtoName(), Json::Object{{"name", "bar"}}};
auto config = filter_->GenerateServiceConfig(hcm_config, &override_config);
auto config = filter_->GenerateServiceConfig(hcm_config, &override_config,
/*filter_name=*/"");
ASSERT_TRUE(config.ok()) << config.status();
EXPECT_EQ(config->service_config_field_name, "stateful_session");
EXPECT_EQ(config->element, JsonDump(Json(Json::Object{{"name", "bar"}})));
Expand Down

0 comments on commit ce3d5d5

Please sign in to comment.