From 5cba72cede219d21c962f9fa7b3d111044497fc2 Mon Sep 17 00:00:00 2001 From: Takeshi Yoneda Date: Tue, 4 Feb 2025 18:56:01 +0000 Subject: [PATCH] Make admission_control exception free Signed-off-by: Takeshi Yoneda --- .../filters/http/admission_control/config.cc | 12 ++++--- .../evaluators/success_criteria_evaluator.cc | 18 ++++++++-- .../evaluators/success_criteria_evaluator.h | 8 ++++- .../http/admission_control/config_test.cc | 26 +++++++-------- .../success_criteria_evaluator_test.cc | 33 +++++++++++-------- tools/code_format/config.yaml | 1 - 6 files changed, 61 insertions(+), 37 deletions(-) diff --git a/source/extensions/filters/http/admission_control/config.cc b/source/extensions/filters/http/admission_control/config.cc index a20bf1be0812..40d4767fe78e 100644 --- a/source/extensions/filters/http/admission_control/config.cc +++ b/source/extensions/filters/http/admission_control/config.cc @@ -23,7 +23,7 @@ AdmissionControlFilterFactory::createFilterFactoryFromProtoTyped( const std::string& stats_prefix, DualInfo dual_info, Server::Configuration::ServerFactoryContext& context) { if (config.has_sr_threshold() && config.sr_threshold().default_value().value() < 1.0) { - throw EnvoyException("Success rate threshold cannot be less than 1.0%."); + return absl::InvalidArgumentError("Success rate threshold cannot be less than 1.0%."); } const std::string prefix = stats_prefix + "admission_control."; @@ -39,11 +39,15 @@ AdmissionControlFilterFactory::createFilterFactoryFromProtoTyped( std::unique_ptr response_evaluator; switch (config.evaluation_criteria_case()) { - case AdmissionControlProto::EvaluationCriteriaCase::kSuccessCriteria: - response_evaluator = std::make_unique(config.success_criteria()); + case AdmissionControlProto::EvaluationCriteriaCase::kSuccessCriteria: { + absl::StatusOr> response_evaluator_or = + SuccessCriteriaEvaluator::create(config.success_criteria()); + RETURN_IF_NOT_OK(response_evaluator_or.status()); + response_evaluator = std::move(response_evaluator_or.value()); break; + } case AdmissionControlProto::EvaluationCriteriaCase::EVALUATION_CRITERIA_NOT_SET: - throw EnvoyException("Evaluation criteria not set"); + return absl::InvalidArgumentError("Evaluation criteria not set"); } AdmissionControlFilterConfigSharedPtr filter_config = diff --git a/source/extensions/filters/http/admission_control/evaluators/success_criteria_evaluator.cc b/source/extensions/filters/http/admission_control/evaluators/success_criteria_evaluator.cc index 5a7ae7fc915c..1d1277ff8bcb 100644 --- a/source/extensions/filters/http/admission_control/evaluators/success_criteria_evaluator.cc +++ b/source/extensions/filters/http/admission_control/evaluators/success_criteria_evaluator.cc @@ -13,13 +13,24 @@ namespace Extensions { namespace HttpFilters { namespace AdmissionControl { -SuccessCriteriaEvaluator::SuccessCriteriaEvaluator(const SuccessCriteria& success_criteria) { +absl::StatusOr> +SuccessCriteriaEvaluator::create(const SuccessCriteria& success_criteria) { + absl::Status status = absl::OkStatus(); + auto evaluator = std::unique_ptr( + new SuccessCriteriaEvaluator(success_criteria, status)); + RETURN_IF_NOT_OK_REF(status); + return evaluator; +} + +SuccessCriteriaEvaluator::SuccessCriteriaEvaluator(const SuccessCriteria& success_criteria, + absl::Status& creation_status) { // HTTP status. if (success_criteria.has_http_criteria()) { for (const auto& range : success_criteria.http_criteria().http_success_status()) { if (!validHttpRange(range.start(), range.end())) { - throw EnvoyException( + creation_status = absl::InvalidArgumentError( fmt::format("invalid HTTP range: [{}, {})", range.start(), range.end())); + return; } const auto start = static_cast(range.start()); @@ -36,7 +47,8 @@ SuccessCriteriaEvaluator::SuccessCriteriaEvaluator(const SuccessCriteria& succes if (success_criteria.has_grpc_criteria()) { for (const auto& status : success_criteria.grpc_criteria().grpc_success_status()) { if (status > 16) { - throw EnvoyException(fmt::format("invalid gRPC code {}", status)); + creation_status = absl::InvalidArgumentError(fmt::format("invalid gRPC code {}", status)); + return; } grpc_success_codes_.emplace_back(status); diff --git a/source/extensions/filters/http/admission_control/evaluators/success_criteria_evaluator.h b/source/extensions/filters/http/admission_control/evaluators/success_criteria_evaluator.h index f55c2fce2464..df95dadd926b 100644 --- a/source/extensions/filters/http/admission_control/evaluators/success_criteria_evaluator.h +++ b/source/extensions/filters/http/admission_control/evaluators/success_criteria_evaluator.h @@ -7,6 +7,8 @@ #include "source/extensions/filters/http/admission_control/evaluators/response_evaluator.h" +#include "absl/status/statusor.h" + namespace Envoy { namespace Extensions { namespace HttpFilters { @@ -16,12 +18,16 @@ class SuccessCriteriaEvaluator : public ResponseEvaluator { public: using SuccessCriteria = envoy::extensions::filters::http::admission_control::v3::AdmissionControl::SuccessCriteria; - SuccessCriteriaEvaluator(const SuccessCriteria& evaluation_criteria); + static absl::StatusOr> + create(const SuccessCriteria& success_criteria); + // ResponseEvaluator bool isHttpSuccess(uint64_t code) const override; bool isGrpcSuccess(uint32_t status) const override; private: + SuccessCriteriaEvaluator(const SuccessCriteria& evaluation_criteria, absl::Status& status); + bool validHttpRange(const int32_t start, const int32_t end) const { return start <= end && start < 600 && start >= 100 && end <= 600 && end >= 100; } diff --git a/test/extensions/filters/http/admission_control/config_test.cc b/test/extensions/filters/http/admission_control/config_test.cc index 8e6990e8c344..062fe9c2dd0a 100644 --- a/test/extensions/filters/http/admission_control/config_test.cc +++ b/test/extensions/filters/http/admission_control/config_test.cc @@ -35,7 +35,9 @@ class AdmissionControlConfigTest : public testing::Test { TestUtility::loadFromYamlAndValidate(yaml, proto); auto tls = ThreadLocal::TypedSlot::makeUnique( context_.server_factory_context_.threadLocal()); - auto evaluator = std::make_unique(proto.success_criteria()); + auto evaluator_or = SuccessCriteriaEvaluator::create(proto.success_criteria()); + EXPECT_TRUE(evaluator_or.ok()); + auto evaluator = std::move(evaluator_or.value()); return std::make_shared(proto, runtime_, random_, scope_, std::move(tls), std::move(evaluator)); } @@ -74,13 +76,10 @@ sampling_window: 1337s AdmissionControlProto proto; TestUtility::loadFromYamlAndValidate(yaml, proto); NiceMock factory_context; - EXPECT_THROW_WITH_MESSAGE( - admission_control_filter_factory - .createFilterFactoryFromProtoTyped(proto, "whatever", dual_info_, - factory_context.serverFactoryContext()) - .status() - .IgnoreError(), - EnvoyException, "Success rate threshold cannot be less than 1.0%."); + auto status_or = admission_control_filter_factory.createFilterFactoryFromProtoTyped( + proto, "whatever", dual_info_, factory_context.serverFactoryContext()); + EXPECT_FALSE(status_or.ok()); + EXPECT_EQ("Success rate threshold cannot be less than 1.0%.", status_or.status().message()); } TEST_F(AdmissionControlConfigTest, SmallSuccessRateThreshold) { @@ -105,13 +104,10 @@ sampling_window: 1337s AdmissionControlProto proto; TestUtility::loadFromYamlAndValidate(yaml, proto); NiceMock factory_context; - EXPECT_THROW_WITH_MESSAGE( - admission_control_filter_factory - .createFilterFactoryFromProtoTyped(proto, "whatever", dual_info_, - factory_context.serverFactoryContext()) - .status() - .IgnoreError(), - EnvoyException, "Success rate threshold cannot be less than 1.0%."); + auto status_or = admission_control_filter_factory.createFilterFactoryFromProtoTyped( + proto, "whatever", dual_info_, factory_context.serverFactoryContext()); + EXPECT_FALSE(status_or.ok()); + EXPECT_EQ("Success rate threshold cannot be less than 1.0%.", status_or.status().message()); } // Verify the configuration when all fields are set. diff --git a/test/extensions/filters/http/admission_control/success_criteria_evaluator_test.cc b/test/extensions/filters/http/admission_control/success_criteria_evaluator_test.cc index 99c421710d7d..e33a95148c72 100644 --- a/test/extensions/filters/http/admission_control/success_criteria_evaluator_test.cc +++ b/test/extensions/filters/http/admission_control/success_criteria_evaluator_test.cc @@ -22,11 +22,19 @@ class SuccessCriteriaTest : public testing::Test { public: SuccessCriteriaTest() = default; - void makeEvaluator(const std::string& yaml) { + void makeEvaluator(const std::string& yaml, + absl::optional error_status_message = absl::nullopt) { AdmissionControlProto::SuccessCriteria proto; TestUtility::loadFromYamlAndValidate(yaml, proto); - evaluator_ = std::make_unique(proto); + absl::StatusOr> evaluator_or = + SuccessCriteriaEvaluator::create(proto); + if (error_status_message.has_value()) { + EXPECT_EQ(error_status_message.value(), evaluator_or.status().message()); + return; + } + EXPECT_TRUE(evaluator_or.ok()); + evaluator_ = std::move(evaluator_or.value()); } void expectHttpSuccess(int code) { EXPECT_TRUE(evaluator_->isHttpSuccess(code)); } @@ -139,35 +147,34 @@ TEST_F(SuccessCriteriaTest, GrpcRangeValidation) { grpc_success_status: - 17 )EOF"; - EXPECT_THROW_WITH_REGEX(makeEvaluator(yaml), EnvoyException, "invalid gRPC code*"); + makeEvaluator(yaml, "invalid gRPC code 17"); } // Verify correct HTTP range validation. TEST_F(SuccessCriteriaTest, HttpRangeValidation) { - auto check_ranges = [this](std::string&& yaml) { - EXPECT_THROW_WITH_REGEX(makeEvaluator(yaml), EnvoyException, "invalid HTTP range*"); - }; - - check_ranges(R"EOF( + makeEvaluator(R"EOF( http_criteria: http_success_status: - start: 300 end: 200 -)EOF"); +)EOF", + "invalid HTTP range: [300, 200)"); - check_ranges(R"EOF( + makeEvaluator(R"EOF( http_criteria: http_success_status: - start: 600 end: 600 -)EOF"); +)EOF", + "invalid HTTP range: [600, 600)"); - check_ranges(R"EOF( + makeEvaluator(R"EOF( http_criteria: http_success_status: - start: 99 end: 99 -)EOF"); +)EOF", + "invalid HTTP range: [99, 99)"); } } // namespace diff --git a/tools/code_format/config.yaml b/tools/code_format/config.yaml index 9498d53c3b0c..fac7e27b7c41 100644 --- a/tools/code_format/config.yaml +++ b/tools/code_format/config.yaml @@ -161,7 +161,6 @@ paths: - source/extensions/config_subscription - source/extensions/compression/zstd/common/dictionary_manager.h - source/extensions/filters/http/adaptive_concurrency/controller - - source/extensions/filters/http/admission_control - source/extensions/filters/http/aws_lambda - source/extensions/filters/http/basic_auth - source/extensions/filters/http/cache