Skip to content

Commit

Permalink
router: removing a few exceptions (envoyproxy#35346)
Browse files Browse the repository at this point in the history 
Risk Level: low
Testing: updated tests
Docs Changes: n/a
Release Notes: n/a
envoyproxy/envoy-mobile#176

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Martin Duke <martin.h.duke@gmail.com>
  • Loading branch information
@alyssawilk @martinduke
alyssawilk authored and martinduke committed Aug 8, 2024
1 parent 3894b39 commit e91c587
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 21 deletions.
43 changes: 31 additions & 12 deletions source/common/router/config_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,14 +159,15 @@ class RouteActionValidationVisitor
}
};

const envoy::config::route::v3::WeightedCluster::ClusterWeight& validateWeightedClusterSpecifier(
absl::Status validateWeightedClusterSpecifier(
const envoy::config::route::v3::WeightedCluster::ClusterWeight& cluster) {
if (!cluster.name().empty() && !cluster.cluster_header().empty()) {
throwEnvoyExceptionOrPanic("Only one of name or cluster_header can be specified");
return absl::InvalidArgumentError("Only one of name or cluster_header can be specified");
} else if (cluster.name().empty() && cluster.cluster_header().empty()) {
throwEnvoyExceptionOrPanic("At least one of name or cluster_header need to be specified");
return absl::InvalidArgumentError(
"At least one of name or cluster_header need to be specified");
}
return cluster;
return absl::OkStatus();
}

// Returns a vector of header parsers, sorted by specificity. The `specificity_ascend` parameter
Expand Down Expand Up @@ -261,13 +262,17 @@ RetryPolicyImpl::create(const envoy::config::route::v3::RetryPolicy& retry_polic
ProtobufMessage::ValidationVisitor& validation_visitor,
Upstream::RetryExtensionFactoryContext& factory_context,
Server::Configuration::CommonFactoryContext& common_context) {
return std::unique_ptr<RetryPolicyImpl>(
new RetryPolicyImpl(retry_policy, validation_visitor, factory_context, common_context));
absl::Status creation_status = absl::OkStatus();
auto ret = std::unique_ptr<RetryPolicyImpl>(new RetryPolicyImpl(
retry_policy, validation_visitor, factory_context, common_context, creation_status));
RETURN_IF_NOT_OK(creation_status);
return ret;
}
RetryPolicyImpl::RetryPolicyImpl(const envoy::config::route::v3::RetryPolicy& retry_policy,
ProtobufMessage::ValidationVisitor& validation_visitor,
Upstream::RetryExtensionFactoryContext& factory_context,
Server::Configuration::CommonFactoryContext& common_context)
Server::Configuration::CommonFactoryContext& common_context,
absl::Status& creation_status)
: retriable_headers_(Http::HeaderUtility::buildHeaderMatcherVector(
retry_policy.retriable_headers(), common_context)),
retriable_request_headers_(Http::HeaderUtility::buildHeaderMatcherVector(
Expand Down Expand Up @@ -332,8 +337,9 @@ RetryPolicyImpl::RetryPolicyImpl(const envoy::config::route::v3::RetryPolicy& re
}

if ((*max_interval_).count() < (*base_interval_).count()) {
throwEnvoyExceptionOrPanic(
creation_status = absl::InvalidArgumentError(
"retry_policy.max_interval must greater than or equal to the base_interval");
return;
}
}
}
Expand Down Expand Up @@ -644,8 +650,10 @@ RouteEntryImplBase::RouteEntryImplBase(const CommonVirtualHostSharedPtr& vhost,
std::vector<WeightedClusterEntrySharedPtr> weighted_clusters;
weighted_clusters.reserve(route.route().weighted_clusters().clusters().size());
for (const auto& cluster : route.route().weighted_clusters().clusters()) {
auto cluster_entry = std::make_unique<WeightedClusterEntry>(
this, runtime_key_prefix + "." + cluster.name(), factory_context, validator, cluster);
auto cluster_entry = THROW_OR_RETURN_VALUE(
WeightedClusterEntry::create(this, runtime_key_prefix + "." + cluster.name(),
factory_context, validator, cluster),
std::unique_ptr<WeightedClusterEntry>);
weighted_clusters.emplace_back(std::move(cluster_entry));
total_weight += weighted_clusters.back()->clusterWeight();
if (total_weight > std::numeric_limits<uint32_t>::max()) {
Expand Down Expand Up @@ -1475,13 +1483,24 @@ const Envoy::Config::TypedMetadata& RouteEntryImplBase::typedMetadata() const {
: DefaultRouteMetadataPack::get().typed_metadata_;
}

absl::StatusOr<std::unique_ptr<RouteEntryImplBase::WeightedClusterEntry>>
RouteEntryImplBase::WeightedClusterEntry::create(
const RouteEntryImplBase* parent, const std::string& runtime_key,
Server::Configuration::ServerFactoryContext& factory_context,
ProtobufMessage::ValidationVisitor& validator,
const envoy::config::route::v3::WeightedCluster::ClusterWeight& cluster) {
RETURN_IF_NOT_OK(validateWeightedClusterSpecifier(cluster));
return std::unique_ptr<WeightedClusterEntry>(
new WeightedClusterEntry(parent, runtime_key, factory_context, validator, cluster));
}

RouteEntryImplBase::WeightedClusterEntry::WeightedClusterEntry(
const RouteEntryImplBase* parent, const std::string& runtime_key,
Server::Configuration::ServerFactoryContext& factory_context,
ProtobufMessage::ValidationVisitor& validator,
const envoy::config::route::v3::WeightedCluster::ClusterWeight& cluster)
: DynamicRouteEntry(parent, nullptr, validateWeightedClusterSpecifier(cluster).name()),
runtime_key_(runtime_key), loader_(factory_context.runtime()),
: DynamicRouteEntry(parent, nullptr, cluster.name()), runtime_key_(runtime_key),
loader_(factory_context.runtime()),
cluster_weight_(PROTOBUF_GET_WRAPPED_REQUIRED(cluster, weight)),
per_filter_configs_(THROW_OR_RETURN_VALUE(
PerFilterConfigs::create(cluster.typed_per_filter_config(), factory_context, validator),
Expand Down
17 changes: 12 additions & 5 deletions source/common/router/config_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,8 @@ class RetryPolicyImpl : public RetryPolicy {
RetryPolicyImpl(const envoy::config::route::v3::RetryPolicy& retry_policy,
ProtobufMessage::ValidationVisitor& validation_visitor,
Upstream::RetryExtensionFactoryContext& factory_context,
Server::Configuration::CommonFactoryContext& common_context);
Server::Configuration::CommonFactoryContext& common_context,
absl::Status& creation_status);
std::chrono::milliseconds per_try_timeout_{0};
std::chrono::milliseconds per_try_idle_timeout_{0};
// We set the number of retries to 1 by default (i.e. when no route or vhost level retry policy is
Expand Down Expand Up @@ -992,10 +993,11 @@ class RouteEntryImplBase : public RouteEntryAndRoute,
*/
class WeightedClusterEntry : public DynamicRouteEntry {
public:
WeightedClusterEntry(const RouteEntryImplBase* parent, const std::string& rutime_key,
Server::Configuration::ServerFactoryContext& factory_context,
ProtobufMessage::ValidationVisitor& validator,
const envoy::config::route::v3::WeightedCluster::ClusterWeight& cluster);
static absl::StatusOr<std::unique_ptr<WeightedClusterEntry>>
create(const RouteEntryImplBase* parent, const std::string& rutime_key,
Server::Configuration::ServerFactoryContext& factory_context,
ProtobufMessage::ValidationVisitor& validator,
const envoy::config::route::v3::WeightedCluster::ClusterWeight& cluster);

uint64_t clusterWeight() const {
return loader_.snapshot().getInteger(runtime_key_, cluster_weight_);
Expand Down Expand Up @@ -1064,6 +1066,11 @@ class RouteEntryImplBase : public RouteEntryAndRoute,
const Http::LowerCaseString& clusterHeaderName() const { return cluster_header_name_; }

private:
WeightedClusterEntry(const RouteEntryImplBase* parent, const std::string& rutime_key,
Server::Configuration::ServerFactoryContext& factory_context,
ProtobufMessage::ValidationVisitor& validator,
const envoy::config::route::v3::WeightedCluster::ClusterWeight& cluster);

const std::string runtime_key_;
Runtime::Loader& loader_;
const uint64_t cluster_weight_;
Expand Down
7 changes: 3 additions & 4 deletions test/common/router/config_impl_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4973,10 +4973,9 @@ TEST_F(RouteMatcherTest, InvalidRetryBackOff) {
)EOF";

factory_context_.cluster_manager_.initializeClusters({"backoff"}, {});
EXPECT_THROW_WITH_MESSAGE(
TestConfigImpl(parseRouteConfigurationFromYaml(yaml), factory_context_, true,
creation_status_),
EnvoyException, "retry_policy.max_interval must greater than or equal to the base_interval");
TestConfigImpl(parseRouteConfigurationFromYaml(yaml), factory_context_, true, creation_status_);
EXPECT_EQ(creation_status_.message(),
"retry_policy.max_interval must greater than or equal to the base_interval");
}

TEST_F(RouteMatcherTest, RateLimitedRetryBackOff) {
Expand Down

0 comments on commit e91c587

Please sign in to comment.