Skip to content

Commit

Permalink
ext_proc: Improve test coverage in matching_utils (envoyproxy#34947)
Browse files Browse the repository at this point in the history
Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>
  • Loading branch information
jbohanon authored Jun 27, 2024
1 parent c213589 commit cb91c46
Show file tree
Hide file tree
Showing 4 changed files with 140 additions and 8 deletions.
27 changes: 23 additions & 4 deletions source/extensions/filters/http/ext_proc/matching_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,35 @@ ExpressionManager::evaluateAttributes(const Filters::Common::Expr::Activation& a
continue;
}

// Possible types from source/extensions/filters/common/expr/context.cc:
// - String
// - StringView
// - Map
// - Timestamp
// - Int64
// - Duration
// - Bool
// - Uint64
// - Bytes
//
// Of these, we need to handle
// - bool as bool
// - int64, uint64 as number
// - everything else as string via print
//
// Handling all value types here would be graceful but is not currently
// testable and drives down coverage %. This is not a _great_ reason to
// not do it; will get feedback from reviewers.
ProtobufWkt::Value value;
switch (result.value().type()) {
case google::api::expr::runtime::CelValue::Type::kBool:
value.set_bool_value(result.value().BoolOrDie());
break;
case google::api::expr::runtime::CelValue::Type::kNullType:
value.set_null_value(ProtobufWkt::NullValue{});
case google::api::expr::runtime::CelValue::Type::kInt64:
value.set_number_value(result.value().Int64OrDie());
break;
case google::api::expr::runtime::CelValue::Type::kDouble:
value.set_number_value(result.value().DoubleOrDie());
case google::api::expr::runtime::CelValue::Type::kUint64:
value.set_number_value(result.value().Uint64OrDie());
break;
default:
value.set_string_value(Filters::Common::Expr::print(result.value()));
Expand Down
22 changes: 22 additions & 0 deletions test/extensions/filters/http/ext_proc/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,28 @@ envoy_extension_cc_test(
],
)

envoy_extension_cc_test(
name = "matching_utils_test",
size = "small",
srcs = ["matching_utils_test.cc"],
copts = select({
"//bazel:windows_x86_64": [],
"//conditions:default": [
"-DUSE_CEL_PARSER",
],
}),
extension_names = ["envoy.filters.http.ext_proc"],
tags = ["skip_on_windows"],
deps = [
":utils_lib",
"//source/extensions/filters/http/ext_proc:matching_utils_lib",
"//test/mocks/local_info:local_info_mocks",
"//test/mocks/server:server_factory_context_mocks",
"//test/mocks/stream_info:stream_info_mocks",
"//test/test_common:utility_lib",
],
)

envoy_extension_cc_test(
name = "mutation_utils_test",
size = "small",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3830,9 +3830,11 @@ TEST_P(ExtProcIntegrationTest, RequestResponseAttributes) {
proto_config_.mutable_request_attributes()->Add("request.path");
proto_config_.mutable_request_attributes()->Add("request.method");
proto_config_.mutable_request_attributes()->Add("request.scheme");
proto_config_.mutable_request_attributes()->Add("connection.mtls");
proto_config_.mutable_request_attributes()->Add("request.size"); // tests int64
proto_config_.mutable_request_attributes()->Add("connection.mtls"); // tests bool
proto_config_.mutable_request_attributes()->Add("connection.id"); // tests uint64
proto_config_.mutable_request_attributes()->Add("response.code");
proto_config_.mutable_response_attributes()->Add("response.code");
proto_config_.mutable_response_attributes()->Add("response.code"); // tests int64
proto_config_.mutable_response_attributes()->Add("response.code_details");

initializeConfig();
Expand All @@ -3852,9 +3854,11 @@ TEST_P(ExtProcIntegrationTest, RequestResponseAttributes) {
EXPECT_EQ(proto_struct.fields().at("request.path").string_value(), "/");
EXPECT_EQ(proto_struct.fields().at("request.method").string_value(), "GET");
EXPECT_EQ(proto_struct.fields().at("request.scheme").string_value(), "http");
EXPECT_EQ(proto_struct.fields().at("request.size").number_value(), 0);
EXPECT_EQ(proto_struct.fields().at("connection.mtls").bool_value(), false);
EXPECT_TRUE(proto_struct.fields().at("connection.id").has_number_value());
// Make sure we did not include the attribute which was not yet available.
EXPECT_EQ(proto_struct.fields().size(), 4);
EXPECT_EQ(proto_struct.fields().size(), 6);
EXPECT_FALSE(proto_struct.fields().contains("response.code"));

// Make sure we are not including any data in the deprecated HttpHeaders.attributes.
Expand All @@ -3874,7 +3878,7 @@ TEST_P(ExtProcIntegrationTest, RequestResponseAttributes) {
EXPECT_TRUE(req.has_response_headers());
EXPECT_EQ(req.attributes().size(), 1);
auto proto_struct = req.attributes().at("envoy.filters.http.ext_proc");
EXPECT_EQ(proto_struct.fields().at("response.code").string_value(), "200");
EXPECT_EQ(proto_struct.fields().at("response.code").number_value(), 200);
EXPECT_EQ(proto_struct.fields().at("response.code_details").string_value(),
StreamInfo::ResponseCodeDetails::get().ViaUpstream);

Expand Down
87 changes: 87 additions & 0 deletions test/extensions/filters/http/ext_proc/matching_utils_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
#include "source/common/protobuf/protobuf.h"
#include "source/extensions/filters/common/expr/evaluator.h"
#include "source/extensions/filters/http/ext_proc/matching_utils.h"

#include "test/mocks/local_info/mocks.h"
#include "test/mocks/server/server_factory_context.h"
#include "test/mocks/stream_info/mocks.h"
#include "test/test_common/utility.h"

#include "gmock/gmock.h"
#include "gtest/gtest.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace ExternalProcessing {
namespace {

using ::Envoy::Http::TestRequestHeaderMapImpl;
using ::Envoy::Http::TestRequestTrailerMapImpl;
using ::Envoy::Http::TestResponseHeaderMapImpl;
using ::Envoy::Http::TestResponseTrailerMapImpl;

class ExpressionManagerTest : public testing::Test {
public:
void initialize() { builder_ = Envoy::Extensions::Filters::Common::Expr::getBuilder(context_); }

std::shared_ptr<Filters::Common::Expr::BuilderInstance> builder_;
NiceMock<Server::Configuration::MockServerFactoryContext> context_;
testing::NiceMock<StreamInfo::MockStreamInfo> stream_info_;
testing::NiceMock<LocalInfo::MockLocalInfo> local_info_;
TestRequestHeaderMapImpl request_headers_;
TestResponseHeaderMapImpl response_headers_;
TestRequestTrailerMapImpl request_trailers_;
TestResponseTrailerMapImpl response_trailers_;
Protobuf::RepeatedPtrField<std::string> req_matchers_;
Protobuf::RepeatedPtrField<std::string> resp_matchers_;
};

#if defined(USE_CEL_PARSER)
TEST_F(ExpressionManagerTest, DuplicateAttributesIgnored) {
initialize();
req_matchers_.Add("request.path");
req_matchers_.Add("request.method");
req_matchers_.Add("request.method");
req_matchers_.Add("request.path");
const auto expr_mgr = ExpressionManager(builder_, local_info_, req_matchers_, resp_matchers_);

request_headers_.setMethod("GET");
request_headers_.setPath("/foo");
const auto activation_ptr = Filters::Common::Expr::createActivation(
&expr_mgr.localInfo(), stream_info_, &request_headers_, &response_headers_,
&response_trailers_);

auto result = expr_mgr.evaluateRequestAttributes(*activation_ptr);
EXPECT_EQ(2, result.fields_size());
EXPECT_NE(result.fields().end(), result.fields().find("request.path"));
EXPECT_NE(result.fields().end(), result.fields().find("request.method"));
}

TEST_F(ExpressionManagerTest, UnparsableExpressionThrowsException) {
initialize();
req_matchers_.Add("++");
EXPECT_THROW_WITH_REGEX(ExpressionManager(builder_, local_info_, req_matchers_, resp_matchers_),
EnvoyException, "Unable to parse descriptor expression.*");
}

TEST_F(ExpressionManagerTest, EmptyExpressionReturnsEmptyStruct) {
initialize();
const auto expr_mgr = ExpressionManager(builder_, local_info_, req_matchers_, resp_matchers_);

request_headers_ = TestRequestHeaderMapImpl();
request_headers_.setMethod("GET");
request_headers_.setPath("/foo");
const auto activation_ptr = Filters::Common::Expr::createActivation(
&expr_mgr.localInfo(), stream_info_, &request_headers_, &response_headers_,
&response_trailers_);

EXPECT_EQ(0, expr_mgr.evaluateRequestAttributes(*activation_ptr).fields_size());
}
#endif

} // namespace
} // namespace ExternalProcessing
} // namespace HttpFilters
} // namespace Extensions
} // namespace Envoy

0 comments on commit cb91c46

Please sign in to comment.