ServerConfigSelector: clean up API (#31683)

* ServerConfigSelector: clean up API

* Automated change: Fix sanity tests

Co-authored-by: markdroth <markdroth@users.noreply.github.com>
pull/26480/head
Mark D. Roth 2 years ago committed by GitHub
parent a9e5508cdb
commit 808347ffe8
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
  1. 1
      CMakeLists.txt
  2. 2
      Makefile
  3. 1
      build_autogenerated.yaml
  4. 1
      config.m4
  5. 1
      config.w32
  6. 1
      gRPC-Core.podspec
  7. 1
      grpc.gemspec
  8. 1
      grpc.gyp
  9. 1
      package.xml
  10. 4
      src/core/BUILD
  11. 62
      src/core/ext/filters/server_config_selector/server_config_selector.cc
  12. 11
      src/core/ext/filters/server_config_selector/server_config_selector.h
  13. 6
      src/core/ext/filters/server_config_selector/server_config_selector_filter.cc
  14. 33
      src/core/ext/xds/xds_server_config_fetcher.cc
  15. 1
      src/python/grpcio/grpc_core_dependencies.py
  16. 34
      test/core/server_config_selector/server_config_selector_test.cc
  17. 1
      tools/doxygen/Doxyfile.c++.internal
  18. 1
      tools/doxygen/Doxyfile.core.internal

1
CMakeLists.txt generated

@ -1726,7 +1726,6 @@ add_library(grpc
src/core/ext/filters/message_size/message_size_filter.cc
src/core/ext/filters/rbac/rbac_filter.cc
src/core/ext/filters/rbac/rbac_service_config_parser.cc
src/core/ext/filters/server_config_selector/server_config_selector.cc
src/core/ext/filters/server_config_selector/server_config_selector_filter.cc
src/core/ext/transport/chttp2/alpn/alpn.cc
src/core/ext/transport/chttp2/client/chttp2_connector.cc

2
Makefile generated

@ -1028,7 +1028,6 @@ LIBGRPC_SRC = \
src/core/ext/filters/message_size/message_size_filter.cc \
src/core/ext/filters/rbac/rbac_filter.cc \
src/core/ext/filters/rbac/rbac_service_config_parser.cc \
src/core/ext/filters/server_config_selector/server_config_selector.cc \
src/core/ext/filters/server_config_selector/server_config_selector_filter.cc \
src/core/ext/transport/chttp2/alpn/alpn.cc \
src/core/ext/transport/chttp2/client/chttp2_connector.cc \
@ -2907,7 +2906,6 @@ src/core/ext/filters/client_channel/resolver/google_c2p/google_c2p_resolver.cc:
src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc: $(OPENSSL_DEP)
src/core/ext/filters/rbac/rbac_filter.cc: $(OPENSSL_DEP)
src/core/ext/filters/rbac/rbac_service_config_parser.cc: $(OPENSSL_DEP)
src/core/ext/filters/server_config_selector/server_config_selector.cc: $(OPENSSL_DEP)
src/core/ext/filters/server_config_selector/server_config_selector_filter.cc: $(OPENSSL_DEP)
src/core/ext/transport/chttp2/alpn/alpn.cc: $(OPENSSL_DEP)
src/core/ext/upb-generated/envoy/admin/v3/certs.upb.c: $(OPENSSL_DEP)

@ -1114,7 +1114,6 @@ libs:
- src/core/ext/filters/message_size/message_size_filter.cc
- src/core/ext/filters/rbac/rbac_filter.cc
- src/core/ext/filters/rbac/rbac_service_config_parser.cc
- src/core/ext/filters/server_config_selector/server_config_selector.cc
- src/core/ext/filters/server_config_selector/server_config_selector_filter.cc
- src/core/ext/transport/chttp2/alpn/alpn.cc
- src/core/ext/transport/chttp2/client/chttp2_connector.cc

1
config.m4 generated

@ -110,7 +110,6 @@ if test "$PHP_GRPC" != "no"; then
src/core/ext/filters/message_size/message_size_filter.cc \
src/core/ext/filters/rbac/rbac_filter.cc \
src/core/ext/filters/rbac/rbac_service_config_parser.cc \
src/core/ext/filters/server_config_selector/server_config_selector.cc \
src/core/ext/filters/server_config_selector/server_config_selector_filter.cc \
src/core/ext/transport/chttp2/alpn/alpn.cc \
src/core/ext/transport/chttp2/client/chttp2_connector.cc \

1
config.w32 generated

@ -76,7 +76,6 @@ if (PHP_GRPC != "no") {
"src\\core\\ext\\filters\\message_size\\message_size_filter.cc " +
"src\\core\\ext\\filters\\rbac\\rbac_filter.cc " +
"src\\core\\ext\\filters\\rbac\\rbac_service_config_parser.cc " +
"src\\core\\ext\\filters\\server_config_selector\\server_config_selector.cc " +
"src\\core\\ext\\filters\\server_config_selector\\server_config_selector_filter.cc " +
"src\\core\\ext\\transport\\chttp2\\alpn\\alpn.cc " +
"src\\core\\ext\\transport\\chttp2\\client\\chttp2_connector.cc " +

1
gRPC-Core.podspec generated

@ -322,7 +322,6 @@ Pod::Spec.new do |s|
'src/core/ext/filters/rbac/rbac_filter.h',
'src/core/ext/filters/rbac/rbac_service_config_parser.cc',
'src/core/ext/filters/rbac/rbac_service_config_parser.h',
'src/core/ext/filters/server_config_selector/server_config_selector.cc',
'src/core/ext/filters/server_config_selector/server_config_selector.h',
'src/core/ext/filters/server_config_selector/server_config_selector_filter.cc',
'src/core/ext/filters/server_config_selector/server_config_selector_filter.h',

1
grpc.gemspec generated

@ -233,7 +233,6 @@ Gem::Specification.new do |s|
s.files += %w( src/core/ext/filters/rbac/rbac_filter.h )
s.files += %w( src/core/ext/filters/rbac/rbac_service_config_parser.cc )
s.files += %w( src/core/ext/filters/rbac/rbac_service_config_parser.h )
s.files += %w( src/core/ext/filters/server_config_selector/server_config_selector.cc )
s.files += %w( src/core/ext/filters/server_config_selector/server_config_selector.h )
s.files += %w( src/core/ext/filters/server_config_selector/server_config_selector_filter.cc )
s.files += %w( src/core/ext/filters/server_config_selector/server_config_selector_filter.h )

1
grpc.gyp generated

@ -440,7 +440,6 @@
'src/core/ext/filters/message_size/message_size_filter.cc',
'src/core/ext/filters/rbac/rbac_filter.cc',
'src/core/ext/filters/rbac/rbac_service_config_parser.cc',
'src/core/ext/filters/server_config_selector/server_config_selector.cc',
'src/core/ext/filters/server_config_selector/server_config_selector_filter.cc',
'src/core/ext/transport/chttp2/alpn/alpn.cc',
'src/core/ext/transport/chttp2/client/chttp2_connector.cc',

1
package.xml generated

@ -215,7 +215,6 @@
<file baseinstalldir="/" name="src/core/ext/filters/rbac/rbac_filter.h" role="src" />
<file baseinstalldir="/" name="src/core/ext/filters/rbac/rbac_service_config_parser.cc" role="src" />
<file baseinstalldir="/" name="src/core/ext/filters/rbac/rbac_service_config_parser.h" role="src" />
<file baseinstalldir="/" name="src/core/ext/filters/server_config_selector/server_config_selector.cc" role="src" />
<file baseinstalldir="/" name="src/core/ext/filters/server_config_selector/server_config_selector.h" role="src" />
<file baseinstalldir="/" name="src/core/ext/filters/server_config_selector/server_config_selector_filter.cc" role="src" />
<file baseinstalldir="/" name="src/core/ext/filters/server_config_selector/server_config_selector_filter.h" role="src" />

@ -2327,9 +2327,6 @@ grpc_cc_library(
grpc_cc_library(
name = "grpc_server_config_selector",
srcs = [
"ext/filters/server_config_selector/server_config_selector.cc",
],
hdrs = [
"ext/filters/server_config_selector/server_config_selector.h",
],
@ -2339,7 +2336,6 @@ grpc_cc_library(
],
language = "c++",
deps = [
"channel_args",
"dual_ref_counted",
"error",
"grpc_service_config",

@ -1,62 +0,0 @@
//
// Copyright 2021 gRPC authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//
#include <grpc/support/port_platform.h>
#include "src/core/ext/filters/server_config_selector/server_config_selector.h"
#include "src/core/lib/channel/channel_args.h"
#include "src/core/lib/gpr/useful.h"
namespace grpc_core {
namespace {
void* ServerConfigSelectorProviderArgCopy(void* p) {
ServerConfigSelectorProvider* arg =
static_cast<ServerConfigSelectorProvider*>(p);
return arg->Ref().release();
}
void ServerConfigSelectorProviderArgDestroy(void* p) {
ServerConfigSelectorProvider* arg =
static_cast<ServerConfigSelectorProvider*>(p);
arg->Unref();
}
int ServerConfigSelectorProviderArgCmp(void* p, void* q) {
return QsortCompare(p, q);
}
const grpc_arg_pointer_vtable kChannelArgVtable = {
ServerConfigSelectorProviderArgCopy, ServerConfigSelectorProviderArgDestroy,
ServerConfigSelectorProviderArgCmp};
const char* kServerConfigSelectorProviderChannelArgName =
"grpc.internal.server_config_selector_provider";
} // namespace
grpc_arg ServerConfigSelectorProvider::MakeChannelArg() const {
return grpc_channel_arg_pointer_create(
const_cast<char*>(kServerConfigSelectorProviderChannelArgName),
const_cast<ServerConfigSelectorProvider*>(this), &kChannelArgVtable);
}
absl::string_view ServerConfigSelectorProvider::ChannelArgName() {
return kServerConfigSelectorProviderChannelArgName;
}
} // namespace grpc_core

@ -43,14 +43,15 @@ class ServerConfigSelector : public RefCounted<ServerConfigSelector> {
public:
// Configuration to apply to an incoming call
struct CallConfig {
grpc_error_handle error;
const ServiceConfigParser::ParsedConfigVector* method_configs = nullptr;
RefCountedPtr<ServiceConfig> service_config;
};
~ServerConfigSelector() override = default;
// Returns the CallConfig to apply to a call based on the incoming \a metadata
virtual CallConfig GetCallConfig(grpc_metadata_batch* metadata) = 0;
virtual absl::StatusOr<CallConfig> GetCallConfig(
grpc_metadata_batch* metadata) = 0;
};
// ServerConfigSelectorProvider allows for subscribers to watch for updates on
@ -71,13 +72,13 @@ class ServerConfigSelectorProvider
std::unique_ptr<ServerConfigSelectorWatcher> watcher) = 0;
virtual void CancelWatch() = 0;
static absl::string_view ChannelArgName();
static absl::string_view ChannelArgName() {
return "grpc.internal.server_config_selector_provider";
}
static int ChannelArgsCompare(const ServerConfigSelectorProvider* a,
const ServerConfigSelectorProvider* b) {
return QsortCompare(a, b);
}
grpc_arg MakeChannelArg() const;
};
} // namespace grpc_core

@ -135,15 +135,15 @@ ArenaPromise<ServerMetadataHandle> ServerConfigSelectorFilter::MakeCallPromise(
if (!sel.ok()) return Immediate(ServerMetadataFromStatus(sel.status()));
auto call_config =
sel.value()->GetCallConfig(call_args.client_initial_metadata.get());
if (!call_config.error.ok()) {
if (!call_config.ok()) {
auto r = Immediate(ServerMetadataFromStatus(
absl::UnavailableError(StatusToString(call_config.error))));
absl::UnavailableError(StatusToString(call_config.status()))));
return std::move(r);
}
auto& ctx = GetContext<
grpc_call_context_element>()[GRPC_CONTEXT_SERVICE_CONFIG_CALL_DATA];
ctx.value = GetContext<Arena>()->New<ServiceConfigCallData>(
std::move(call_config.service_config), call_config.method_configs,
std::move(call_config->service_config), call_config->method_configs,
ServiceConfigCallData::CallAttributes{});
ctx.destroy = [](void* p) {
static_cast<ServiceConfigCallData*>(p)->~ServiceConfigCallData();

@ -319,7 +319,8 @@ class XdsServerConfigFetcher::ListenerWatcher::FilterChainMatchManager::
http_filters);
~XdsServerConfigSelector() override = default;
CallConfig GetCallConfig(grpc_metadata_batch* metadata) override;
absl::StatusOr<CallConfig> GetCallConfig(
grpc_metadata_batch* metadata) override;
private:
struct VirtualHost {
@ -1188,30 +1189,26 @@ XdsServerConfigFetcher::ListenerWatcher::FilterChainMatchManager::
return config_selector;
}
ServerConfigSelector::CallConfig XdsServerConfigFetcher::ListenerWatcher::
FilterChainMatchManager::XdsServerConfigSelector::GetCallConfig(
grpc_metadata_batch* metadata) {
absl::StatusOr<ServerConfigSelector::CallConfig>
XdsServerConfigFetcher::ListenerWatcher::FilterChainMatchManager::
XdsServerConfigSelector::GetCallConfig(grpc_metadata_batch* metadata) {
CallConfig call_config;
if (metadata->get_pointer(HttpPathMetadata()) == nullptr) {
call_config.error = GRPC_ERROR_CREATE("No path found");
return call_config;
return absl::InternalError("no path found");
}
absl::string_view path =
metadata->get_pointer(HttpPathMetadata())->as_string_view();
if (metadata->get_pointer(HttpAuthorityMetadata()) == nullptr) {
call_config.error = GRPC_ERROR_CREATE("No authority found");
return call_config;
return absl::InternalError("no authority found");
}
absl::string_view authority =
metadata->get_pointer(HttpAuthorityMetadata())->as_string_view();
auto vhost_index = XdsRouting::FindVirtualHostForDomain(
VirtualHostListIterator(&virtual_hosts_), authority);
if (!vhost_index.has_value()) {
call_config.error = grpc_error_set_int(
GRPC_ERROR_CREATE(absl::StrCat("could not find VirtualHost for ",
authority, " in RouteConfiguration")),
StatusIntProperty::kRpcStatus, GRPC_STATUS_UNAVAILABLE);
return call_config;
return absl::UnavailableError(
absl::StrCat("could not find VirtualHost for ", authority,
" in RouteConfiguration"));
}
auto& virtual_host = virtual_hosts_[vhost_index.value()];
auto route_index = XdsRouting::GetRouteForRequest(
@ -1220,10 +1217,7 @@ ServerConfigSelector::CallConfig XdsServerConfigFetcher::ListenerWatcher::
auto& route = virtual_host.routes[route_index.value()];
// Found the matching route
if (route.unsupported_action) {
call_config.error = grpc_error_set_int(
GRPC_ERROR_CREATE("Matching route has unsupported action"),
StatusIntProperty::kRpcStatus, GRPC_STATUS_UNAVAILABLE);
return call_config;
return absl::UnavailableError("matching route has unsupported action");
}
if (route.method_config != nullptr) {
call_config.method_configs =
@ -1232,10 +1226,7 @@ ServerConfigSelector::CallConfig XdsServerConfigFetcher::ListenerWatcher::
}
return call_config;
}
call_config.error = grpc_error_set_int(GRPC_ERROR_CREATE("No route matched"),
StatusIntProperty::kRpcStatus,
GRPC_STATUS_UNAVAILABLE);
return call_config;
return absl::UnavailableError("no route matched");
}
//

@ -85,7 +85,6 @@ CORE_SOURCE_FILES = [
'src/core/ext/filters/message_size/message_size_filter.cc',
'src/core/ext/filters/rbac/rbac_filter.cc',
'src/core/ext/filters/rbac/rbac_service_config_parser.cc',
'src/core/ext/filters/server_config_selector/server_config_selector.cc',
'src/core/ext/filters/server_config_selector/server_config_selector_filter.cc',
'src/core/ext/transport/chttp2/alpn/alpn.cc',
'src/core/ext/transport/chttp2/client/chttp2_connector.cc',

@ -26,16 +26,13 @@
#include "src/core/lib/channel/channel_args.h"
#include "test/core/util/test_config.h"
namespace grpc {
namespace grpc_core {
namespace testing {
namespace {
using grpc_core::ServerConfigSelector;
using grpc_core::ServerConfigSelectorProvider;
class TestServerConfigSelectorProvider : public ServerConfigSelectorProvider {
absl::StatusOr<grpc_core::RefCountedPtr<ServerConfigSelector>> Watch(
std::unique_ptr<ServerConfigSelectorWatcher> /* watcher */) override {
absl::StatusOr<RefCountedPtr<ServerConfigSelector>> Watch(
std::unique_ptr<ServerConfigSelectorWatcher> /*watcher*/) override {
return absl::UnavailableError("Test ServerConfigSelector");
}
@ -48,33 +45,24 @@ class TestServerConfigSelectorProvider : public ServerConfigSelectorProvider {
// and destroyed
TEST(ServerConfigSelectorProviderTest, CopyChannelArgs) {
auto server_config_selector_provider =
grpc_core::MakeRefCounted<TestServerConfigSelectorProvider>();
grpc_arg arg = server_config_selector_provider->MakeChannelArg();
grpc_channel_args* args = grpc_channel_args_copy_and_add(nullptr, &arg, 1);
MakeRefCounted<TestServerConfigSelectorProvider>();
auto args = ChannelArgs().SetObject(server_config_selector_provider);
EXPECT_EQ(server_config_selector_provider,
grpc_core::ChannelArgs::FromC(args)
.GetObject<ServerConfigSelectorProvider>());
grpc_channel_args_destroy(args);
args.GetObject<ServerConfigSelectorProvider>());
}
// Test compare on channel args with the same ServerConfigSelectorProvider
TEST(ServerConfigSelectorProviderTest, ChannelArgsCompare) {
auto server_config_selector_provider =
grpc_core::MakeRefCounted<TestServerConfigSelectorProvider>();
grpc_arg arg = server_config_selector_provider->MakeChannelArg();
grpc_channel_args* args = grpc_channel_args_copy_and_add(nullptr, &arg, 1);
grpc_channel_args* new_args = grpc_channel_args_copy(args);
EXPECT_EQ(grpc_core::ChannelArgs::FromC(new_args)
.GetObject<ServerConfigSelectorProvider>(),
grpc_core::ChannelArgs::FromC(args)
.GetObject<ServerConfigSelectorProvider>());
grpc_channel_args_destroy(args);
grpc_channel_args_destroy(new_args);
MakeRefCounted<TestServerConfigSelectorProvider>();
auto args = ChannelArgs().SetObject(server_config_selector_provider);
auto args2 = ChannelArgs().SetObject(server_config_selector_provider);
EXPECT_EQ(args, args2);
}
} // namespace
} // namespace testing
} // namespace grpc
} // namespace grpc_core
int main(int argc, char** argv) {
::testing::InitGoogleTest(&argc, argv);

@ -1192,7 +1192,6 @@ src/core/ext/filters/rbac/rbac_filter.cc \
src/core/ext/filters/rbac/rbac_filter.h \
src/core/ext/filters/rbac/rbac_service_config_parser.cc \
src/core/ext/filters/rbac/rbac_service_config_parser.h \
src/core/ext/filters/server_config_selector/server_config_selector.cc \
src/core/ext/filters/server_config_selector/server_config_selector.h \
src/core/ext/filters/server_config_selector/server_config_selector_filter.cc \
src/core/ext/filters/server_config_selector/server_config_selector_filter.h \

@ -1000,7 +1000,6 @@ src/core/ext/filters/rbac/rbac_filter.cc \
src/core/ext/filters/rbac/rbac_filter.h \
src/core/ext/filters/rbac/rbac_service_config_parser.cc \
src/core/ext/filters/rbac/rbac_service_config_parser.h \
src/core/ext/filters/server_config_selector/server_config_selector.cc \
src/core/ext/filters/server_config_selector/server_config_selector.h \
src/core/ext/filters/server_config_selector/server_config_selector_filter.cc \
src/core/ext/filters/server_config_selector/server_config_selector_filter.h \

Loading…
Cancel
Save