Skip to content

Commit 01e75f9

Browse files
ambuchtuch
ambuc
authored and
htuch
committed
Support api config source without cluster names (#2999)
To support the deprecation of cluster_names in the api_config_source (#2860), I added a more verbose error in utility.cc for GRPC api_config_sources with any named clusters hunted down places in tests where api_config_source.cluster_names()[0] was implicitly used (assuming that API would be GRPC and would have cluster names). renamed factoryForApiConfigSource to factoryForGrpcApiConfigSource, as it already implicitly returns a Grpc::AsyncClientFactoryPtr, and gave a more verbose error for when the user passes a config with cluster_names set. separated out checkApiConfigSourceSubscriptionBackingCluster into checkApiConfigSourceNames, which does the gRPC services v. cluster names validation, and checkApiConfigSourceSubscriptionBackingCluster, which actually validates the cluster name against the clusters map. more completely covered the tests for the branching cases for non-gRPC API configs which happened to have grpc services defined. Risk Level: Low Testing: bazel test test/... all passed. Fixed #2680 Fixed #2902 Signed-off-by: James Buckland <[email protected]>
1 parent 6d2cdff commit 01e75f9

File tree

8 files changed

+270
-113
lines changed

8 files changed

+270
-113
lines changed

‎source/common/config/subscription_factory.h‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,22 +52,21 @@ class SubscriptionFactory {
5252
case envoy::api::v2::core::ConfigSource::kApiConfigSource: {
5353
const envoy::api::v2::core::ApiConfigSource& api_config_source = config.api_config_source();
5454
Utility::checkApiConfigSourceSubscriptionBackingCluster(cm.clusters(), api_config_source);
55-
const std::string& cluster_name = api_config_source.cluster_names()[0];
5655
switch (api_config_source.api_type()) {
5756
case envoy::api::v2::core::ApiConfigSource::REST_LEGACY:
5857
result.reset(rest_legacy_constructor());
5958
break;
6059
case envoy::api::v2::core::ApiConfigSource::REST:
6160
result.reset(new HttpSubscriptionImpl<ResourceType>(
62-
node, cm, cluster_name, dispatcher, random,
61+
node, cm, api_config_source.cluster_names()[0], dispatcher, random,
6362
Utility::apiConfigSourceRefreshDelay(api_config_source),
6463
*Protobuf::DescriptorPool::generated_pool()->FindMethodByName(rest_method), stats));
6564
break;
6665
case envoy::api::v2::core::ApiConfigSource::GRPC: {
6766
result.reset(new GrpcSubscriptionImpl<ResourceType>(
6867
node,
69-
Config::Utility::factoryForApiConfigSource(cm.grpcAsyncClientManager(),
70-
config.api_config_source(), scope)
68+
Config::Utility::factoryForGrpcApiConfigSource(cm.grpcAsyncClientManager(),
69+
config.api_config_source(), scope)
7170
->create(),
7271
dispatcher, *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(grpc_method),
7372
stats));

‎source/common/config/utility.cc‎

Lines changed: 64 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -78,23 +78,67 @@ void Utility::checkFilesystemSubscriptionBackingPath(const std::string& path) {
7878
}
7979
}
8080

81-
void Utility::checkApiConfigSourceSubscriptionBackingCluster(
82-
const Upstream::ClusterManager::ClusterInfoMap& clusters,
81+
void Utility::checkApiConfigSourceNames(
8382
const envoy::api::v2::core::ApiConfigSource& api_config_source) {
84-
if (api_config_source.cluster_names().size() != 1) {
85-
// TODO(htuch): Add support for multiple clusters, #1170.
86-
throw EnvoyException(
87-
"envoy::api::v2::core::ConfigSource must have a singleton cluster name specified");
83+
const bool is_grpc =
84+
(api_config_source.api_type() == envoy::api::v2::core::ApiConfigSource::GRPC);
85+
86+
if (is_grpc) {
87+
if (api_config_source.cluster_names().size() != 0) {
88+
throw EnvoyException(
89+
"envoy::api::v2::core::ConfigSource::GRPC must not have a cluster name specified");
90+
}
91+
if (api_config_source.grpc_services().size() != 1) {
92+
throw EnvoyException(
93+
"envoy::api::v2::core::ConfigSource::GRPC must have a single gRPC service specified");
94+
}
95+
} else {
96+
if (api_config_source.grpc_services().size() != 0) {
97+
throw EnvoyException("envoy::api::v2::core::ConfigSource, if not of type gRPC, must not have "
98+
"a gRPC service specified");
99+
}
100+
if (api_config_source.cluster_names().size() != 1) {
101+
throw EnvoyException(
102+
"envoy::api::v2::core::ConfigSource must have a singleton cluster name specified");
103+
}
88104
}
105+
}
89106

90-
const auto& cluster_name = api_config_source.cluster_names()[0];
91-
const auto& it = clusters.find(cluster_name);
92-
if (it == clusters.end() || it->second.get().info()->addedViaApi() ||
93-
it->second.get().info()->type() == envoy::api::v2::Cluster::EDS) {
94-
throw EnvoyException(fmt::format(
95-
"envoy::api::v2::core::ConfigSource must have a statically "
96-
"defined non-EDS cluster: '{}' does not exist, was added via api, or is an EDS cluster",
97-
cluster_name));
107+
void Utility::checkApiConfigSourceSubscriptionBackingCluster(
108+
const Upstream::ClusterManager::ClusterInfoMap& clusters,
109+
const envoy::api::v2::core::ApiConfigSource& api_config_source) {
110+
Utility::checkApiConfigSourceNames(api_config_source);
111+
112+
const bool is_grpc =
113+
(api_config_source.api_type() == envoy::api::v2::core::ApiConfigSource::GRPC);
114+
115+
// We ought to validate the cluster name if and only if there is a cluster name.
116+
if (is_grpc) {
117+
// Some ApiConfigSources of type GRPC won't have a cluster name, such as if
118+
// they've been configured with google_grpc.
119+
if (api_config_source.grpc_services()[0].has_envoy_grpc()) {
120+
const std::string& cluster_name =
121+
api_config_source.grpc_services()[0].envoy_grpc().cluster_name();
122+
const auto& it = clusters.find(cluster_name);
123+
if (it == clusters.end() || it->second.get().info()->addedViaApi() ||
124+
it->second.get().info()->type() == envoy::api::v2::Cluster::EDS) {
125+
throw EnvoyException(fmt::format(
126+
"envoy::api::v2::core::ConfigSource must have a statically "
127+
"defined non-EDS cluster: '{}' does not exist, was added via api, or is an EDS cluster",
128+
cluster_name));
129+
}
130+
}
131+
} else {
132+
// All ApiConfigSources of type REST and REST_LEGACY should have cluster_names.
133+
const std::string& cluster_name = api_config_source.cluster_names()[0];
134+
const auto& it = clusters.find(cluster_name);
135+
if (it == clusters.end() || it->second.get().info()->addedViaApi() ||
136+
it->second.get().info()->type() == envoy::api::v2::Cluster::EDS) {
137+
throw EnvoyException(fmt::format(
138+
"envoy::api::v2::core::ConfigSource must have a statically "
139+
"defined non-EDS cluster: '{}' does not exist, was added via api, or is an EDS cluster",
140+
cluster_name));
141+
}
98142
}
99143
}
100144

@@ -161,36 +205,13 @@ void Utility::checkObjNameLength(const std::string& error_prefix, const std::str
161205
}
162206
}
163207

164-
Grpc::AsyncClientFactoryPtr
165-
Utility::factoryForApiConfigSource(Grpc::AsyncClientManager& async_client_manager,
166-
const envoy::api::v2::core::ApiConfigSource& api_config_source,
167-
Stats::Scope& scope) {
168-
ASSERT(api_config_source.api_type() == envoy::api::v2::core::ApiConfigSource::GRPC);
169-
envoy::api::v2::core::GrpcService grpc_service;
170-
if (api_config_source.cluster_names().empty()) {
171-
if (api_config_source.grpc_services().empty()) {
172-
throw EnvoyException(
173-
fmt::format("Missing gRPC services in envoy::api::v2::core::ApiConfigSource: {}",
174-
api_config_source.DebugString()));
175-
}
176-
// TODO(htuch): Implement multiple gRPC services.
177-
if (api_config_source.grpc_services().size() != 1) {
178-
throw EnvoyException(fmt::format("Only singleton gRPC service lists supported in "
179-
"envoy::api::v2::core::ApiConfigSource: {}",
180-
api_config_source.DebugString()));
181-
}
182-
grpc_service.MergeFrom(api_config_source.grpc_services(0));
183-
} else {
184-
// TODO(htuch): cluster_names is deprecated, remove after 1.6.0.
185-
if (api_config_source.cluster_names().size() != 1) {
186-
throw EnvoyException(fmt::format("Only singleton cluster name lists supported in "
187-
"envoy::api::v2::core::ApiConfigSource: {}",
188-
api_config_source.DebugString()));
189-
}
190-
grpc_service.mutable_envoy_grpc()->set_cluster_name(api_config_source.cluster_names(0));
191-
}
208+
Grpc::AsyncClientFactoryPtr Utility::factoryForGrpcApiConfigSource(
209+
Grpc::AsyncClientManager& async_client_manager,
210+
const envoy::api::v2::core::ApiConfigSource& api_config_source, Stats::Scope& scope) {
211+
Utility::checkApiConfigSourceNames(api_config_source);
192212

193-
return async_client_manager.factoryForGrpcService(grpc_service, scope, false);
213+
return async_client_manager.factoryForGrpcService(api_config_source.grpc_services(0), scope,
214+
false);
194215
}
195216

196217
} // namespace Config

‎source/common/config/utility.h‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,15 @@ class Utility {
121121
*/
122122
static void checkFilesystemSubscriptionBackingPath(const std::string& path);
123123

124+
/**
125+
* Check the grpc_services and cluster_names for API config sanity. Throws on error.
126+
* @param api_config_source the config source to validate.
127+
* @throws EnvoyException when an API config has the wrong number of gRPC
128+
* services or cluster names, depending on expectations set by its API type.
129+
*/
130+
static void
131+
checkApiConfigSourceNames(const envoy::api::v2::core::ApiConfigSource& api_config_source);
132+
124133
/**
125134
* Check the validity of a cluster backing an api config source. Throws on error.
126135
* @param clusters the clusters currently loaded in the cluster manager.
@@ -240,9 +249,9 @@ class Utility {
240249
* @return Grpc::AsyncClientFactoryPtr gRPC async client factory.
241250
*/
242251
static Grpc::AsyncClientFactoryPtr
243-
factoryForApiConfigSource(Grpc::AsyncClientManager& async_client_manager,
244-
const envoy::api::v2::core::ApiConfigSource& api_config_source,
245-
Stats::Scope& scope);
252+
factoryForGrpcApiConfigSource(Grpc::AsyncClientManager& async_client_manager,
253+
const envoy::api::v2::core::ApiConfigSource& api_config_source,
254+
Stats::Scope& scope);
246255
};
247256

248257
} // namespace Config

‎source/common/upstream/cluster_manager_impl.cc‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ ClusterManagerImpl::ClusterManagerImpl(const envoy::config::bootstrap::v2::Boots
204204
if (bootstrap.dynamic_resources().has_ads_config()) {
205205
ads_mux_.reset(new Config::GrpcMuxImpl(
206206
bootstrap.node(),
207-
Config::Utility::factoryForApiConfigSource(
207+
Config::Utility::factoryForGrpcApiConfigSource(
208208
*async_client_manager_, bootstrap.dynamic_resources().ads_config(), stats)
209209
->create(),
210210
main_thread_dispatcher,
@@ -291,11 +291,12 @@ ClusterManagerImpl::ClusterManagerImpl(const envoy::config::bootstrap::v2::Boots
291291

292292
if (cm_config.has_load_stats_config()) {
293293
const auto& load_stats_config = cm_config.load_stats_config();
294-
load_stats_reporter_.reset(new LoadStatsReporter(
295-
bootstrap.node(), *this, stats,
296-
Config::Utility::factoryForApiConfigSource(*async_client_manager_, load_stats_config, stats)
297-
->create(),
298-
main_thread_dispatcher));
294+
load_stats_reporter_.reset(
295+
new LoadStatsReporter(bootstrap.node(), *this, stats,
296+
Config::Utility::factoryForGrpcApiConfigSource(
297+
*async_client_manager_, load_stats_config, stats)
298+
->create(),
299+
main_thread_dispatcher));
299300
}
300301
}
301302

0 commit comments

Comments
 (0)