Skip to content

coverage: more misc coverage #7042

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattklein123 merged 4 commits into from
May 23, 2019
Merged

coverage: more misc coverage #7042

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2e85312
coverage: more misc coverage
mattklein123 May 22, 2019
ca4070d
more
mattklein123 May 22, 2019
62db951
Merge remote-tracking branch 'origin/master' into more_coverage
mattklein123 May 23, 2019
bac2963
comment
mattklein123 May 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions source/common/http/async_client_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@
namespace Envoy {
namespace Http {

const std::list<std::string> AsyncStreamImpl::NullCorsPolicy::allow_origin_;
const std::list<std::regex> AsyncStreamImpl::NullCorsPolicy::allow_origin_regex_;
const absl::optional<bool> AsyncStreamImpl::NullCorsPolicy::allow_credentials_;
const std::vector<std::reference_wrapper<const Router::RateLimitPolicyEntry>>
AsyncStreamImpl::NullRateLimitPolicy::rate_limit_policy_entry_;
const AsyncStreamImpl::NullHedgePolicy AsyncStreamImpl::RouteEntryImpl::hedge_policy_;
Expand Down
21 changes: 2 additions & 19 deletions source/common/http/async_client_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,25 +90,6 @@ class AsyncStreamImpl : public AsyncClient::Stream,
AsyncClientImpl& parent_;

private:
struct NullCorsPolicy : public Router::CorsPolicy {
// Router::CorsPolicy
const std::list<std::string>& allowOrigins() const override { return allow_origin_; };
const std::list<std::regex>& allowOriginRegexes() const override {
return allow_origin_regex_;
};
const std::string& allowMethods() const override { return EMPTY_STRING; };
const std::string& allowHeaders() const override { return EMPTY_STRING; };
const std::string& exposeHeaders() const override { return EMPTY_STRING; };
const std::string& maxAge() const override { return EMPTY_STRING; };
const absl::optional<bool>& allowCredentials() const override { return allow_credentials_; };
bool enabled() const override { return false; };
bool shadowEnabled() const override { return false; };

static const std::list<std::string> allow_origin_;
static const std::list<std::regex> allow_origin_regex_;
static const absl::optional<bool> allow_credentials_;
};

struct NullHedgePolicy : public Router::HedgePolicy {
// Router::HedgePolicy
uint32_t initialRequests() const override { return 1; }
Expand Down Expand Up @@ -373,7 +354,9 @@ class AsyncStreamImpl : public AsyncClient::Stream,
bool is_grpc_request_{};
bool is_head_request_{false};
bool send_xff_{true};

friend class AsyncClientImpl;
friend class AsyncClientImplRouteTest;
};

class AsyncRequestImpl final : public AsyncClient::Request,
Expand Down
40 changes: 40 additions & 0 deletions test/common/http/async_client_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -909,5 +909,45 @@ TEST_F(AsyncClientImplTest, RdsGettersTest) {
}

} // namespace

// Must not be in anonymous namespace for friend to work.
class AsyncClientImplRouteTest : public testing::Test {
public:
AsyncStreamImpl::RouteImpl route_impl{"foo", absl::nullopt};
};

// Test the extended fake route that AsyncClient uses.
TEST_F(AsyncClientImplRouteTest, All) {
EXPECT_EQ(nullptr, route_impl.decorator());
EXPECT_EQ(nullptr, route_impl.perFilterConfig(""));
EXPECT_EQ(Code::InternalServerError, route_impl.routeEntry()->clusterNotFoundResponseCode());
EXPECT_EQ(nullptr, route_impl.routeEntry()->corsPolicy());
EXPECT_EQ(nullptr, route_impl.routeEntry()->hashPolicy());
EXPECT_EQ(1, route_impl.routeEntry()->hedgePolicy().initialRequests());
EXPECT_EQ(0, route_impl.routeEntry()->hedgePolicy().additionalRequestChance().numerator());
EXPECT_FALSE(route_impl.routeEntry()->hedgePolicy().hedgeOnPerTryTimeout());
EXPECT_EQ(nullptr, route_impl.routeEntry()->metadataMatchCriteria());
EXPECT_TRUE(route_impl.routeEntry()->rateLimitPolicy().empty());
EXPECT_TRUE(route_impl.routeEntry()->rateLimitPolicy().getApplicableRateLimit(0).empty());
EXPECT_EQ(absl::nullopt, route_impl.routeEntry()->idleTimeout());
EXPECT_EQ(absl::nullopt, route_impl.routeEntry()->grpcTimeoutOffset());
EXPECT_TRUE(route_impl.routeEntry()->opaqueConfig().empty());
EXPECT_TRUE(route_impl.routeEntry()->includeVirtualHostRateLimits());
EXPECT_TRUE(route_impl.routeEntry()->metadata().filter_metadata().empty());
EXPECT_EQ(nullptr,
route_impl.routeEntry()->typedMetadata().get<Config::TypedMetadata::Object>("bar"));
EXPECT_EQ(nullptr, route_impl.routeEntry()->perFilterConfig("bar"));
EXPECT_TRUE(route_impl.routeEntry()->upgradeMap().empty());
EXPECT_EQ(Router::InternalRedirectAction::PassThrough,
route_impl.routeEntry()->internalRedirectAction());
EXPECT_TRUE(route_impl.routeEntry()->shadowPolicy().runtimeKey().empty());
EXPECT_EQ(0, route_impl.routeEntry()->shadowPolicy().defaultValue().numerator());
EXPECT_TRUE(route_impl.routeEntry()->virtualHost().rateLimitPolicy().empty());
EXPECT_EQ(nullptr, route_impl.routeEntry()->virtualHost().corsPolicy());
EXPECT_EQ(nullptr, route_impl.routeEntry()->virtualHost().perFilterConfig("bar"));
EXPECT_FALSE(route_impl.routeEntry()->virtualHost().includeAttemptCount());
EXPECT_FALSE(route_impl.routeEntry()->virtualHost().routeConfig().usesVhds());
}

} // namespace Http
} // namespace Envoy
7 changes: 7 additions & 0 deletions test/common/router/config_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1834,6 +1834,7 @@ TEST_F(RouteMatcherTest, ClusterHeader) {
EXPECT_EQ("some_cluster", route->routeEntry()->clusterName());

// Make sure things forward and don't crash.
// TODO(mattklein123): Make this a real test of behavior.
EXPECT_EQ(std::chrono::milliseconds(0), route->routeEntry()->timeout());
route->routeEntry()->finalizeRequestHeaders(headers, stream_info, true);
route->routeEntry()->priority();
Expand All @@ -1844,6 +1845,11 @@ TEST_F(RouteMatcherTest, ClusterHeader) {
route->routeEntry()->virtualHost();
route->routeEntry()->virtualHost().rateLimitPolicy();
route->routeEntry()->pathMatchCriterion();
route->routeEntry()->hedgePolicy();
route->routeEntry()->maxGrpcTimeout();
route->routeEntry()->grpcTimeoutOffset();
route->routeEntry()->upgradeMap();
route->routeEntry()->internalRedirectAction();
}
}

Expand Down Expand Up @@ -3165,6 +3171,7 @@ name: foo
genRedirectHeaders("redirect.lyft.com", "/https", false, false);
EXPECT_EQ("https://redirect.lyft.com/https",
config.route(headers, 0)->directResponseEntry()->newPath(headers));
EXPECT_EQ(nullptr, config.route(headers, 0)->perFilterConfig("bar"));
}
{
Http::TestHeaderMapImpl headers =
Expand Down
109 changes: 72 additions & 37 deletions test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,48 +344,77 @@ class SslTapIntegrationTest : public SslIntegrationTest {
void initialize() override {
// TODO(mattklein123): Merge/use the code in ConfigHelper::setTapTransportSocket().
config_helper_.addConfigModifier([this](envoy::config::bootstrap::v2::Bootstrap& bootstrap) {
auto* filter_chain =
bootstrap.mutable_static_resources()->mutable_listeners(0)->mutable_filter_chains(0);
// Configure inner SSL transport socket based on existing config.
envoy::api::v2::core::TransportSocket ssl_transport_socket;
ssl_transport_socket.set_name("tls");
MessageUtil::jsonConvert(filter_chain->tls_context(), *ssl_transport_socket.mutable_config());
// Configure outer tap transport socket.
auto* transport_socket = filter_chain->mutable_transport_socket();
transport_socket->set_name("envoy.transport_sockets.tap");
envoy::config::transport_socket::tap::v2alpha::Tap tap_config;
tap_config.mutable_common_config()
->mutable_static_config()
->mutable_match_config()
->set_any_match(true);
auto* output_config =
tap_config.mutable_common_config()->mutable_static_config()->mutable_output_config();
if (max_rx_bytes_.has_value()) {
output_config->mutable_max_buffered_rx_bytes()->set_value(max_rx_bytes_.value());
// The test supports tapping either the downstream or upstream connection, but not both.
if (upstream_tap_) {
setupUpstreamTap(bootstrap);
} else {
setupDownstreamTap(bootstrap);
}
if (max_tx_bytes_.has_value()) {
output_config->mutable_max_buffered_tx_bytes()->set_value(max_tx_bytes_.value());
}

auto* output_sink = output_config->mutable_sinks()->Add();
output_sink->set_format(format_);
output_sink->mutable_file_per_tap()->set_path_prefix(path_prefix_);
tap_config.mutable_transport_socket()->MergeFrom(ssl_transport_socket);
MessageUtil::jsonConvert(tap_config, *transport_socket->mutable_config());
// Nuke TLS context from legacy location.
filter_chain->clear_tls_context();
// Rest of TLS initialization.
});
SslIntegrationTest::initialize();
// This confuses our socket counting.
debug_with_s_client_ = false;
}

void setupUpstreamTap(envoy::config::bootstrap::v2::Bootstrap& bootstrap) {
auto* transport_socket =
bootstrap.mutable_static_resources()->mutable_clusters(0)->mutable_transport_socket();
transport_socket->set_name("envoy.transport_sockets.tap");
envoy::api::v2::core::TransportSocket raw_transport_socket;
raw_transport_socket.set_name("raw_buffer");
envoy::config::transport_socket::tap::v2alpha::Tap tap_config =
createTapConfig(raw_transport_socket);
tap_config.mutable_transport_socket()->MergeFrom(raw_transport_socket);
MessageUtil::jsonConvert(tap_config, *transport_socket->mutable_config());
}

void setupDownstreamTap(envoy::config::bootstrap::v2::Bootstrap& bootstrap) {
auto* filter_chain =
bootstrap.mutable_static_resources()->mutable_listeners(0)->mutable_filter_chains(0);
// Configure inner SSL transport socket based on existing config.
envoy::api::v2::core::TransportSocket ssl_transport_socket;
ssl_transport_socket.set_name("tls");
MessageUtil::jsonConvert(filter_chain->tls_context(), *ssl_transport_socket.mutable_config());
// Configure outer tap transport socket.
auto* transport_socket = filter_chain->mutable_transport_socket();
transport_socket->set_name("envoy.transport_sockets.tap");
envoy::config::transport_socket::tap::v2alpha::Tap tap_config =
createTapConfig(ssl_transport_socket);
tap_config.mutable_transport_socket()->MergeFrom(ssl_transport_socket);
MessageUtil::jsonConvert(tap_config, *transport_socket->mutable_config());
// Nuke TLS context from legacy location.
filter_chain->clear_tls_context();
}

envoy::config::transport_socket::tap::v2alpha::Tap
createTapConfig(const envoy::api::v2::core::TransportSocket& inner_transport) {
envoy::config::transport_socket::tap::v2alpha::Tap tap_config;
tap_config.mutable_common_config()
->mutable_static_config()
->mutable_match_config()
->set_any_match(true);
auto* output_config =
tap_config.mutable_common_config()->mutable_static_config()->mutable_output_config();
if (max_rx_bytes_.has_value()) {
output_config->mutable_max_buffered_rx_bytes()->set_value(max_rx_bytes_.value());
}
if (max_tx_bytes_.has_value()) {
output_config->mutable_max_buffered_tx_bytes()->set_value(max_tx_bytes_.value());
}

auto* output_sink = output_config->mutable_sinks()->Add();
output_sink->set_format(format_);
output_sink->mutable_file_per_tap()->set_path_prefix(path_prefix_);
tap_config.mutable_transport_socket()->MergeFrom(inner_transport);
return tap_config;
}

std::string path_prefix_ = TestEnvironment::temporaryPath("ssl_trace");
envoy::service::tap::v2alpha::OutputSink::Format format_{
envoy::service::tap::v2alpha::OutputSink::PROTO_BINARY};
absl::optional<uint64_t> max_rx_bytes_;
absl::optional<uint64_t> max_tx_bytes_;
bool upstream_tap_{};
};

INSTANTIATE_TEST_SUITE_P(IpVersions, SslTapIntegrationTest,
Expand Down Expand Up @@ -531,25 +560,31 @@ TEST_P(SslTapIntegrationTest, RequestWithTextProto) {
EXPECT_FALSE(trace.socket_buffered_trace().write_truncated());
}

// Validate a single request with JSON (body as string) output.
TEST_P(SslTapIntegrationTest, RequestWithJsonBodyAsString) {
max_rx_bytes_ = 4;
max_tx_bytes_ = 5;
// Validate a single request with JSON (body as string) output. This test uses an upstream tap.
TEST_P(SslTapIntegrationTest, RequestWithJsonBodyAsStringUpstreamTap) {
upstream_tap_ = true;
max_rx_bytes_ = 5;
max_tx_bytes_ = 4;

format_ = envoy::service::tap::v2alpha::OutputSink::JSON_BODY_AS_STRING;
ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr {
return makeSslClientConnection({});
};
const uint64_t id = Network::ConnectionImpl::nextGlobalIdForTest() + 1;
const uint64_t id = Network::ConnectionImpl::nextGlobalIdForTest() + 2;
testRouterRequestAndResponseWithBody(512, 1024, false, &creator);
checkStats();
codec_client_->close();
test_server_->waitForCounterGe("http.config_test.downstream_cx_destroy", 1);
test_server_.reset();

// This must be done after server shutdown so that connection pool connections are closed and
// the tap written.
envoy::data::tap::v2alpha::TraceWrapper trace;
MessageUtil::loadFromFile(fmt::format("{}_{}.json", path_prefix_, id), trace, *api_);

// Test some obvious properties.
EXPECT_EQ(trace.socket_buffered_trace().events(0).read().data().as_string(), "POST");
EXPECT_EQ(trace.socket_buffered_trace().events(1).write().data().as_string(), "HTTP/");
EXPECT_EQ(trace.socket_buffered_trace().events(0).write().data().as_string(), "POST");
EXPECT_EQ(trace.socket_buffered_trace().events(1).read().data().as_string(), "HTTP/");
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz May 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like you modified a previous test to do something different. TBH I'm not fully understanding the semantic difference. But I do wonder whether it might have made sense to keep the old function and add a new one. Was the old functionality of the RequestWithJsonBodyAsString test not needed?

Copy link
Copy Markdown
Member Author

@mattklein123 mattklein123 May 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is the tap filter is installed on the upstream connection vs. the downstream. This test was primarily testing a different output type, but now we cover two different things. I can bring back the old test but I'm not sure it adds much extra value.

EXPECT_TRUE(trace.socket_buffered_trace().read_truncated());
EXPECT_TRUE(trace.socket_buffered_trace().write_truncated());
}
Expand Down