Skip to content
16 changes: 14 additions & 2 deletions test/server/BUILD
Original file line number Diff line number Diff line change
@@ -1,13 +1,25 @@
load(
"@envoy//bazel:envoy_build_system.bzl",
"envoy_cc_test",
"envoy_cc_test_library",
"envoy_package",
)

licenses(["notice"]) # Apache 2

envoy_package()

envoy_cc_test_library(
name = "http_filter_integration_test_base_lib",
srcs = ["http_filter_integration_test_base.cc"],
hdrs = ["http_filter_integration_test_base.h"],
repository = "@envoy",
deps = [
"//source/server:well_known_headers_lib",
"@envoy//test/integration:http_integration_lib",
],
)

envoy_cc_test(
name = "http_test_server_filter_integration_test",
srcs = ["http_test_server_filter_integration_test.cc"],
Expand All @@ -25,9 +37,9 @@ envoy_cc_test(
srcs = ["http_dynamic_delay_filter_integration_test.cc"],
repository = "@envoy",
deps = [
":http_filter_integration_test_base_lib",
"//source/server:http_dynamic_delay_filter_config",
"@envoy//source/common/api:api_lib_with_external_headers",
"@envoy//test/integration:http_integration_lib",
],
)

Expand All @@ -36,10 +48,10 @@ envoy_cc_test(
srcs = ["http_time_tracking_filter_integration_test.cc"],
repository = "@envoy",
deps = [
":http_filter_integration_test_base_lib",
"//source/server:http_time_tracking_filter_config",
"@envoy//include/envoy/upstream:cluster_manager_interface_with_external_headers",
"@envoy//source/common/api:api_lib_with_external_headers",
"@envoy//test/integration:http_integration_lib",
"@envoy//test/test_common:simulated_time_system_lib",
],
)
63 changes: 63 additions & 0 deletions test/server/http_filter_integration_test_base.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
#include "test/server/http_filter_integration_test_base.h"

#include "server/well_known_headers.h"

#include "gtest/gtest.h"

namespace Nighthawk {

HttpFilterIntegrationTestBase::HttpFilterIntegrationTestBase(
Envoy::Network::Address::IpVersion ip_version)
: HttpIntegrationTest(Envoy::Http::CodecClient::Type::HTTP1, ip_version),
request_headers_({{":method", "GET"}, {":path", "/"}, {":authority", "host"}}) {}

void HttpFilterIntegrationTestBase::initializeFilterConfiguration(absl::string_view config) {
config_helper_.addFilter(std::string(config));
HttpIntegrationTest::initialize();
}

void HttpFilterIntegrationTestBase::setRequestLevelConfiguration(
absl::string_view request_level_config) {
setRequestHeader(Server::TestServer::HeaderNames::get().TestServerConfig, request_level_config);
}

void HttpFilterIntegrationTestBase::switchToPostWithEntityBody() {
setRequestHeader(Envoy::Http::Headers::get().Method,
Envoy::Http::Headers::get().MethodValues.Post);
}

void HttpFilterIntegrationTestBase::setRequestHeader(
const Envoy::Http::LowerCaseString& header_name, absl::string_view header_value) {
request_headers_.setCopy(header_name, header_value);
}

Envoy::IntegrationStreamDecoderPtr
HttpFilterIntegrationTestBase::getResponse(ResponseOrigin expected_origin) {
cleanupUpstreamAndDownstream();
codec_client_ = makeHttpConnection(lookupPort("http"));
Envoy::IntegrationStreamDecoderPtr response;
const bool is_post = request_headers_.Method()->value().getStringView() ==
Envoy::Http::Headers::get().MethodValues.Post;
// Upon observing a POST request method, we inject a content body, as promised in the header file.
// This is useful, because emitting an entity body will hit distinct code in extensions. Hence we
// facilitate that.
const uint64_t request_body_size = is_post ? 1024 : 0;
// An extension can either act as an origin and synthesize a reply, or delegate that
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking through again, these comments are very helpful. Thank you for adding them

// responsibility to an upstream. This behavior may change from request to request. For example,
// an extension is designed to transform input from an upstream, may start acting as an origin on
// misconfiguration.
if (expected_origin == ResponseOrigin::UPSTREAM) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm having trouble understanding this function's full intent. There are a lot of conditionals that seem to make the behavior very different. Is this actually common behavior across filters?

Maybe some comments in this section about why things are happening would be helpful.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added some comments in ca17f41. Does that provide the type of context you'd consider helpful when reading this code?
Basically, this support call is an extract of the what we need to cover the stream codec code for all our extensions. We'll later see that this allows us deprecate much more complex code (which was the way to do it a while back), as wel as deduplicate some similar code.

Copy link
Copy Markdown
Member Author

@oschaaf oschaaf Sep 15, 2020

Choose a reason for hiding this comment

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

Additionally, for some more concrete context, #533 is the direct follow up to this (currently in draft, as it depends on this PR, and will lean up once we merge this one and merge the result into that). It brings in a new test file which is using this extensively.
That test file adds tests for ensuring basic behavioural guarantees across all of our extensions.

The behaviours we are looking for involve certain configuration (error) handling, as well as handling default/empty configuration, both static from disk as well as dynamically delivered via response headers. These tests are parameterised over GET and POST requests.

Once we land that, we can in turn start purging / enhancing the other test files, and make them more about just testing the functionality that the extension brings in. Where applicable, it will also use the functionality we add in this PR, and we can purge their own variant(s).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

(NOTE: the changes from the last paragraph above, are already staged over at #512)

response = sendRequestAndWaitForResponse(request_headers_, request_body_size,
default_response_headers_, /*response_body_size*/ 0);
} else {
if (is_post) {
response = codec_client_->makeRequestWithBody(request_headers_, request_body_size);
} else {
response = codec_client_->makeHeaderOnlyRequest(request_headers_);
}
response->waitForEndStream();
}
return response;
}

} // namespace Nighthawk
85 changes: 85 additions & 0 deletions test/server/http_filter_integration_test_base.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
#pragma once

#include <string>

#include "external/envoy/test/integration/http_integration.h"

namespace Nighthawk {

/**
* Base class with shared functionality for testing Nighthawk test server http filter extensions.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we add two notes/clarifications here: 1. This class is stateful. 2. Unless I'm mistaken I don't think this class is threadsafe

* The class is stateful, and not safe to use from multiple threads.
*/
class HttpFilterIntegrationTestBase : public Envoy::HttpIntegrationTest {
protected:
/**
* Indicate the expected response origin. A test failure will occur upon mismatch.
*/
enum class ResponseOrigin {
/**
* The upstream will supply the response, and not the extension under test.
*/
UPSTREAM,
/**
* The extension under test will suplly a response, and no upstream will be set up to do that.
*/
EXTENSION
};
/**
* Construct a new HttpFilterIntegrationTestBase instance.
*
* @param ip_version Specify the ip version that the integration test server will use to listen
* for connections.
*/
HttpFilterIntegrationTestBase(Envoy::Network::Address::IpVersion ip_version);

/**
* We don't override SetUp(): tests using this fixture must call initializeFilterConfiguration()
* instead. This is to avoid imposing the need to create a fixture per filter configuration.
*
* @param filter_configuration Pass configuration for the filter under test. Will be handed off to
* Envoy::HttpIntegrationTest::config_helper_.addFilter.
*/
void initializeFilterConfiguration(absl::string_view filter_configuration);

/**
* Make getResponse send request-level configuration. Test server extensions read that
* configuration and merge it with their static configuration to determine a final effective
* configuration. See TestServerConfig in well_known_headers.h for the up to date header name.
*
* @param request_level_config Configuration to be delivered by request-header in future calls to
* getResponse(). For example: "{response_body_size:1024}".
*/
void setRequestLevelConfiguration(absl::string_view request_level_config);

/**
* Switch getResponse() to use the POST request method with an entity body.
* Doing so will make tests hit a different code paths in extensions.
*/
void switchToPostWithEntityBody();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a thought, curious what you think - it might be more self-documenting here, if we take in a parameter for the post body size. Or even then, rename this to setPostBodySize, with an indication that unless this is called, the requests into the filter will be GET requests.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I would be more up for doing this, but rather do it once a functional need for it arises (which is very possible). Right now, in the context of this PR and the changes coming after this, I'm on a mission to minimize and contain complexity that has organically grown while we added the test server and the two new extensions. Part of effort is making everything go through this hopefully well defined but minimal functionality. It should be trivial to refactor this later on according to what you suggest, once the need arises in new tests?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That makes sense to me. I'm happy to push it through as is, and if it comes up, do the refactor as you suggest.


/**
* Set a request header value. Overwrites any existing value.
*
* @param header_name Name of the request header to set.
* @param header_value Value to set for the request header.
*/
void setRequestHeader(const Envoy::Http::LowerCaseString& header_name,
absl::string_view header_value);

/**
* Fetch a response, according to the options specified by the class methods. By default,
* simulates a GET request with minimal headers.
* @param expected_origin Indicate which component will be expected to reply: the extension or
* a fake upstream. Will cause a test failure upon mismatch. Can be used to verify that an
* extension short circuits and directly responds when expected.
* @return Envoy::IntegrationStreamDecoderPtr Pointer to the integration stream decoder, which can
* be used to inspect the response.
*/
Envoy::IntegrationStreamDecoderPtr getResponse(ResponseOrigin expected_origin);

private:
Envoy::Http::TestRequestHeaderMapImpl request_headers_;
};

} // namespace Nighthawk