Skip to content

Commit

Permalink
comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
  • Loading branch information
@adisuissa
adisuissa committed Oct 28, 2024
1 parent 735d6a8 commit eb373c2
Show file tree
Hide file tree
Showing 8 changed files with 22 additions and 21 deletions.
4 changes: 2 additions & 2 deletions envoy/config/xds_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ class XdsManager {
* Set the ADS ConfigSource Envoy should use that will replace the current ADS
* server.
* @param ads_config the ADS config of the new server.
* @return true if the ADS config is valid (points to a valid static server),
* or false otherwise.
* @return Ok if the ADS config is valid (points to a valid static server),
* or an error otherwise.
*/
virtual absl::Status
setAdsConfigSource(const envoy::config::core::v3::ApiConfigSource& config_source) PURE;
Expand Down
5 changes: 3 additions & 2 deletions envoy/upstream/cluster_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -369,11 +369,12 @@ class ClusterManager {

/**
* Replaces the current ADS mux with a new one based on the given config.
* Assumes that the given ads_config is yntactically valid (according to the PGV constraints).
* Assumes that the given ads_config is syntactically valid (according to the PGV constraints).
* @param ads_config an ADS config source to use.
* @return the status of the operation.
*/
virtual absl::Status replaceAds(const envoy::config::core::v3::ApiConfigSource& ads_config) PURE;
virtual absl::Status
replaceAdsMux(const envoy::config::core::v3::ApiConfigSource& ads_config) PURE;

/**
* @return Grpc::AsyncClientManager& the cluster manager's gRPC client manager.
Expand Down
2 changes: 1 addition & 1 deletion source/common/config/xds_manager_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ XdsManagerImpl::setAdsConfigSource(const envoy::config::core::v3::ApiConfigSourc
ASSERT_IS_MAIN_OR_TEST_THREAD();
RETURN_IF_NOT_OK(validateAdsConfig(config_source));

return cm_.replaceAds(config_source);
return cm_.replaceAdsMux(config_source);
}

absl::Status
Expand Down
2 changes: 1 addition & 1 deletion source/common/upstream/cluster_manager_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,7 @@ ClusterManagerImpl::initialize(const envoy::config::bootstrap::v3::Bootstrap& bo
}

absl::Status
ClusterManagerImpl::replaceAds(const envoy::config::core::v3::ApiConfigSource& ads_config) {
ClusterManagerImpl::replaceAdsMux(const envoy::config::core::v3::ApiConfigSource& ads_config) {
// If there was no ADS defined, reject replacement.
const auto& bootstrap = server_.bootstrap();
if (!bootstrap.has_dynamic_resources() || !bootstrap.dynamic_resources().has_ads_config()) {
Expand Down
2 changes: 1 addition & 1 deletion source/common/upstream/cluster_manager_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ class ClusterManagerImpl : public ClusterManager,
}

Config::GrpcMuxSharedPtr adsMux() override { return ads_mux_; }
absl::Status replaceAds(const envoy::config::core::v3::ApiConfigSource& ads_config) override;
absl::Status replaceAdsMux(const envoy::config::core::v3::ApiConfigSource& ads_config) override;
Grpc::AsyncClientManager& grpcAsyncClientManager() override { return *async_client_manager_; }

const absl::optional<std::string>& localClusterName() const override {
Expand Down
4 changes: 2 additions & 2 deletions test/common/config/xds_manager_impl_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class XdsManagerImplTest : public testing::Test {
TEST_F(XdsManagerImplTest, AdsConfigSourceSetterSuccess) {
envoy::config::core::v3::ApiConfigSource config_source;
config_source.set_api_type(envoy::config::core::v3::ApiConfigSource::GRPC);
EXPECT_CALL(cm_, replaceAds(ProtoEq(config_source))).WillOnce(Return(absl::OkStatus()));
EXPECT_CALL(cm_, replaceAdsMux(ProtoEq(config_source))).WillOnce(Return(absl::OkStatus()));
absl::Status res = xds_manager_impl_.setAdsConfigSource(config_source);
EXPECT_TRUE(res.ok());
}
Expand All @@ -42,7 +42,7 @@ TEST_F(XdsManagerImplTest, AdsConfigSourceSetterSuccess) {
TEST_F(XdsManagerImplTest, AdsConfigSourceSetterFailure) {
envoy::config::core::v3::ApiConfigSource config_source;
config_source.set_api_type(envoy::config::core::v3::ApiConfigSource::GRPC);
EXPECT_CALL(cm_, replaceAds(ProtoEq(config_source)))
EXPECT_CALL(cm_, replaceAdsMux(ProtoEq(config_source)))
.WillOnce(Return(absl::InternalError("error")));
absl::Status res = xds_manager_impl_.setAdsConfigSource(config_source);
EXPECT_THAT(res, StatusCodeIs(absl::StatusCode::kInternal));
Expand Down
22 changes: 11 additions & 11 deletions test/common/upstream/cluster_manager_impl_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4778,7 +4778,7 @@ TEST_F(ClusterManagerImplTest, AdsReplacementNoPriorAdsRejection) {
)EOF",
new_ads_config);

const auto res = cluster_manager_->replaceAds(new_ads_config);
const auto res = cluster_manager_->replaceAdsMux(new_ads_config);
EXPECT_THAT(res, StatusCodeIs(absl::StatusCode::kInternal));
EXPECT_EQ(res.message(),
"Cannot replace an ADS config when one wasn't previously configured in the bootstrap");
Expand Down Expand Up @@ -4871,7 +4871,7 @@ TEST_F(ClusterManagerImplTest, AdsReplacementPrimaryOnly) {
EXPECT_EQ(failover_async_client, nullptr);
return absl::OkStatus();
}));
const auto res = cluster_manager_->replaceAds(new_ads_config);
const auto res = cluster_manager_->replaceAdsMux(new_ads_config);
EXPECT_TRUE(res.ok());
}

Expand Down Expand Up @@ -4966,7 +4966,7 @@ TEST_F(ClusterManagerImplTest, AdsReplacementPrimaryAndFailover) {
EXPECT_NE(failover_async_client, nullptr);
return absl::OkStatus();
}));
const auto res = cluster_manager_->replaceAds(new_ads_config);
const auto res = cluster_manager_->replaceAdsMux(new_ads_config);
EXPECT_TRUE(res.ok());
}

Expand Down Expand Up @@ -5014,7 +5014,7 @@ TEST_F(ClusterManagerImplTest, AdsReplacementUnknownCluster) {
)EOF",
new_ads_config);

const auto res = cluster_manager_->replaceAds(new_ads_config);
const auto res = cluster_manager_->replaceAdsMux(new_ads_config);
EXPECT_THAT(res, StatusCodeIs(absl::StatusCode::kInvalidArgument));
EXPECT_EQ(res.message(), "Unknown gRPC client cluster 'ads_cluster2'");
}
Expand Down Expand Up @@ -5096,7 +5096,7 @@ TEST_F(ClusterManagerImplTest, AdsReplacementUnknownFailoverCluster) {
)EOF",
new_ads_config);

const auto res = cluster_manager_->replaceAds(new_ads_config);
const auto res = cluster_manager_->replaceAdsMux(new_ads_config);
EXPECT_THAT(res, StatusCodeIs(absl::StatusCode::kInvalidArgument));
EXPECT_EQ(res.message(), "Unknown gRPC client cluster 'non_existent_failover_ads_cluster'");
}
Expand Down Expand Up @@ -5154,7 +5154,7 @@ TEST_F(ClusterManagerImplTest, AdsReplacementDifferentAdsTypeRejection) {
cluster_name: ads_cluster
)EOF",
new_ads_config);
const auto res = cluster_manager_->replaceAds(new_ads_config);
const auto res = cluster_manager_->replaceAdsMux(new_ads_config);
EXPECT_THAT(res, StatusCodeIs(absl::StatusCode::kInternal));
EXPECT_EQ(res.message(),
"Cannot replace an ADS config with a different api_type (expected: GRPC)");
Expand Down Expand Up @@ -5216,7 +5216,7 @@ TEST_F(ClusterManagerImplTest, AdsReplacementInvalidBackoffRejection) {
max_interval: 10s
)EOF",
new_ads_config);
const auto res = cluster_manager_->replaceAds(new_ads_config);
const auto res = cluster_manager_->replaceAdsMux(new_ads_config);
EXPECT_THAT(res, StatusCodeIs(absl::StatusCode::kInvalidArgument));
EXPECT_EQ(res.message(), "max_interval must be greater than or equal to the base_interval");
}
Expand All @@ -5238,7 +5238,7 @@ TEST_F(ClusterManagerImplTest, AdsReplacementUnsupportedTypeRejection) {
}));

// Use GRPC type which is supported, but make sure that it will return
// AGGREGATED_GRPC when invoking replaceAds() to emulate a type which is not
// AGGREGATED_GRPC when invoking replaceAdsMux() to emulate a type which is not
// supported.
const std::string yaml = R"EOF(
dynamic_resources:
Expand Down Expand Up @@ -5276,7 +5276,7 @@ TEST_F(ClusterManagerImplTest, AdsReplacementUnsupportedTypeRejection) {
cluster_name: ads_cluster
)EOF",
new_ads_config);
const auto res = cluster_manager_->replaceAds(new_ads_config);
const auto res = cluster_manager_->replaceAdsMux(new_ads_config);
EXPECT_THAT(res, StatusCodeIs(absl::StatusCode::kInternal));
EXPECT_EQ(res.message(),
"Cannot replace an ADS config with a different api_type (expected: GRPC)");
Expand Down Expand Up @@ -5341,7 +5341,7 @@ TEST_F(ClusterManagerImplTest, AdsReplacementNumberOfCustomValidatorsRejection)
cluster_name: ads_cluster
)EOF",
new_ads_config);
const auto res = cluster_manager_->replaceAds(new_ads_config);
const auto res = cluster_manager_->replaceAdsMux(new_ads_config);
EXPECT_THAT(res, StatusCodeIs(absl::StatusCode::kInternal));
EXPECT_THAT(res.message(),
HasSubstr("Cannot replace config_validators in ADS config (different size)"));
Expand Down Expand Up @@ -5414,7 +5414,7 @@ TEST_F(ClusterManagerImplTest, AdsReplacementContentsOfCustomValidatorsRejection
number_value: 7.0
)EOF",
new_ads_config);
const auto res = cluster_manager_->replaceAds(new_ads_config);
const auto res = cluster_manager_->replaceAdsMux(new_ads_config);
EXPECT_THAT(res, StatusCodeIs(absl::StatusCode::kInternal));
EXPECT_THAT(res.message(),
HasSubstr("Cannot replace config_validators in ADS config (different contents)"));
Expand Down
2 changes: 1 addition & 1 deletion test/mocks/upstream/cluster_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class MockClusterManager : public ClusterManager {
MOCK_METHOD(bool, isShutdown, ());
MOCK_METHOD(const absl::optional<envoy::config::core::v3::BindConfig>&, bindConfig, (), (const));
MOCK_METHOD(Config::GrpcMuxSharedPtr, adsMux, ());
MOCK_METHOD(absl::Status, replaceAds,
MOCK_METHOD(absl::Status, replaceAdsMux,
(const envoy::config::core::v3::ApiConfigSource& ads_config));
MOCK_METHOD(Grpc::AsyncClientManager&, grpcAsyncClientManager, ());
MOCK_METHOD(const std::string, versionInfo, (), (const));
Expand Down

0 comments on commit eb373c2

Please sign in to comment.