Skip to content

Commit b8e2eee

Browse files
ambuchtuch
ambuc
authored and
htuch
committed
Support api config source without cluster names (#3030)
This reverts #3018, which reverted #2999. Therefore this PR is similar to #2999, except that 77b292c allows REST/gRPC config mismatch errors to warn instead of throwing an error. Hopefully this solves the issue for users who were unable to roll forward their binaries while keeping the same config. Changelist for #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 b2e54ba commit b8e2eee

File tree

9 files changed

+383
-155
lines changed

9 files changed

+383
-155
lines changed

‎DEPRECATED.md‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,16 +10,16 @@ The following features have been DEPRECATED and will be removed in the specified
1010
* Admin mutations should be sent as POSTs rather than GETs. HTTP GETs will result in an error
1111
status code and will not have their intended effect. Prior to 1.7, GETs can be used for
1212
admin mutations, but a warning is logged.
13+
* Rate limit service configuration via the `cluster_name` field is deprecated. Use `grpc_service`
14+
instead.
15+
* gRPC service configuration via the `cluster_names` field in `ApiConfigSource` is deprecated. Use
16+
`grpc_services` instead. Prior to 1.7, a warning is logged.
1317

1418
## Version 1.6.0 (March 20, 2018)
1519

1620
* DOWNSTREAM_ADDRESS log formatter is deprecated. Use DOWNSTREAM_REMOTE_ADDRESS_WITHOUT_PORT
1721
instead.
1822
* CLIENT_IP header formatter is deprecated. Use DOWNSTREAM_REMOTE_ADDRESS_WITHOUT_PORT instead.
19-
* Rate limit service configuration via the `cluster_name` field is deprecated. Use `grpc_service`
20-
instead.
21-
* gRPC service configuration via the `cluster_names` field in `ApiConfigSource` is deprecated. Use
22-
`grpc_services` instead.
2323
* 'use_original_dst' field in the v2 LDS API is deprecated. Use listener filters and filter chain
2424
matching instead.
2525
* `value` and `regex` fields in the `HeaderMatcher` message is deprecated. Use the `exact_match`

‎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: 65 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -78,16 +78,43 @@ 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 (api_config_source.cluster_names().size() == 0 &&
87+
api_config_source.grpc_services().size() == 0) {
88+
throw EnvoyException("API configs must have either a gRPC service or a cluster name defined");
8889
}
8990

90-
const auto& cluster_name = api_config_source.cluster_names()[0];
91+
if (is_grpc) {
92+
if (api_config_source.cluster_names().size() != 0) {
93+
ENVOY_LOG_MISC(warn, "Setting a cluster name for API config source type "
94+
"envoy::api::v2::core::ConfigSource::GRPC is deprecated");
95+
}
96+
if (api_config_source.cluster_names().size() > 1) {
97+
throw EnvoyException(
98+
"envoy::api::v2::core::ConfigSource must have a singleton cluster name specified");
99+
}
100+
if (api_config_source.grpc_services().size() > 1) {
101+
throw EnvoyException(
102+
"envoy::api::v2::core::ConfigSource::GRPC must have a single gRPC service specified");
103+
}
104+
} else {
105+
if (api_config_source.grpc_services().size() != 0) {
106+
throw EnvoyException("envoy::api::v2::core::ConfigSource, if not of type gRPC, must not have "
107+
"a gRPC service specified");
108+
}
109+
if (api_config_source.cluster_names().size() != 1) {
110+
throw EnvoyException(
111+
"envoy::api::v2::core::ConfigSource must have a singleton cluster name specified");
112+
}
113+
}
114+
}
115+
116+
void Utility::validateClusterName(const Upstream::ClusterManager::ClusterInfoMap& clusters,
117+
const std::string& cluster_name) {
91118
const auto& it = clusters.find(cluster_name);
92119
if (it == clusters.end() || it->second.get().info()->addedViaApi() ||
93120
it->second.get().info()->type() == envoy::api::v2::Cluster::EDS) {
@@ -98,6 +125,31 @@ void Utility::checkApiConfigSourceSubscriptionBackingCluster(
98125
}
99126
}
100127

128+
void Utility::checkApiConfigSourceSubscriptionBackingCluster(
129+
const Upstream::ClusterManager::ClusterInfoMap& clusters,
130+
const envoy::api::v2::core::ApiConfigSource& api_config_source) {
131+
Utility::checkApiConfigSourceNames(api_config_source);
132+
133+
const bool is_grpc =
134+
(api_config_source.api_type() == envoy::api::v2::core::ApiConfigSource::GRPC);
135+
136+
if (!api_config_source.cluster_names().empty()) {
137+
// All API configs of type REST and REST_LEGACY should have cluster names.
138+
// Additionally, some gRPC API configs might have a cluster name set instead
139+
// of an envoy gRPC.
140+
Utility::validateClusterName(clusters, api_config_source.cluster_names()[0]);
141+
} else if (is_grpc) {
142+
// Some ApiConfigSources of type GRPC won't have a cluster name, such as if
143+
// they've been configured with google_grpc.
144+
if (api_config_source.grpc_services()[0].has_envoy_grpc()) {
145+
// If an Envoy gRPC exists, we take its cluster name.
146+
Utility::validateClusterName(
147+
clusters, api_config_source.grpc_services()[0].envoy_grpc().cluster_name());
148+
}
149+
}
150+
// Otherwise, there is no cluster name to validate.
151+
}
152+
101153
std::chrono::milliseconds Utility::apiConfigSourceRefreshDelay(
102154
const envoy::api::v2::core::ApiConfigSource& api_config_source) {
103155
if (!api_config_source.has_refresh_delay()) {
@@ -161,36 +213,13 @@ void Utility::checkObjNameLength(const std::string& error_prefix, const std::str
161213
}
162214
}
163215

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-
}
216+
Grpc::AsyncClientFactoryPtr Utility::factoryForGrpcApiConfigSource(
217+
Grpc::AsyncClientManager& async_client_manager,
218+
const envoy::api::v2::core::ApiConfigSource& api_config_source, Stats::Scope& scope) {
219+
Utility::checkApiConfigSourceNames(api_config_source);
192220

193-
return async_client_manager.factoryForGrpcService(grpc_service, scope, false);
221+
return async_client_manager.factoryForGrpcService(api_config_source.grpc_services(0), scope,
222+
false);
194223
}
195224

196225
} // namespace Config

‎source/common/config/utility.h‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -121,10 +121,29 @@ 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.
127136
* @param api_config_source the config source to validate.
137+
* @throws EnvoyException when an API config doesn't have a statically defined non-EDS cluster.
138+
*/
139+
static void validateClusterName(const Upstream::ClusterManager::ClusterInfoMap& clusters,
140+
const std::string& cluster_name);
141+
142+
/**
143+
* Potentially calls Utility::validateClusterName, if a cluster name can be found.
144+
* @param clusters the clusters currently loaded in the cluster manager.
145+
* @param api_config_source the config source to validate.
146+
* @throws EnvoyException when an API config doesn't have a statically defined non-EDS cluster.
128147
*/
129148
static void checkApiConfigSourceSubscriptionBackingCluster(
130149
const Upstream::ClusterManager::ClusterInfoMap& clusters,
@@ -240,9 +259,9 @@ class Utility {
240259
* @return Grpc::AsyncClientFactoryPtr gRPC async client factory.
241260
*/
242261
static Grpc::AsyncClientFactoryPtr
243-
factoryForApiConfigSource(Grpc::AsyncClientManager& async_client_manager,
244-
const envoy::api::v2::core::ApiConfigSource& api_config_source,
245-
Stats::Scope& scope);
262+
factoryForGrpcApiConfigSource(Grpc::AsyncClientManager& async_client_manager,
263+
const envoy::api::v2::core::ApiConfigSource& api_config_source,
264+
Stats::Scope& scope);
246265
};
247266

248267
} // 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)