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",
],
)
56 changes: 56 additions & 0 deletions test/server/http_filter_integration_test_base.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
#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::setup(const std::string& config) {

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 to check - this can't be an absl::string_view?

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.

Done -- this was slightly more convenient in the implementation as we need to copy, but I agree consistency outweighs that. Thanks for catching

config_helper_.addFilter(config);
HttpIntegrationTest::initialize();
}

void HttpFilterIntegrationTestBase::updateRequestLevelConfiguration(
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;
const uint64_t request_body_size = is_post ? 1024 : 0;
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.

@oschaaf oschaaf Sep 15, 2020

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.

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
75 changes: 75 additions & 0 deletions test/server/http_filter_integration_test_base.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
#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

*/
class HttpFilterIntegrationTestBase : public Envoy::HttpIntegrationTest {
protected:
/**
* Indicate the expected response origin.
*/
enum class ResponseOrigin { UPSTREAM, EXTENSION };

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 a one line summary here for each type of ResponseOrigin? This may also allow you to remove a comment elsewhere, if it's duplication at this point.

/**
* 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 setup() instead.

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.

nit: reference to setup() here is no longer accurate

* This is to avoid imposing the need to create a fixture per filter configuration.
*
* @param config configuration to pass to Envoy::HttpIntegrationTest::config_helper_.addFilter.

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.

is this configuration for the filter being tested? If so, can we clarify "filter configuration"

*/
void setup(const std::string& config);

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 think having a setup and a SetUp is confusing, especially since it's not really consistent with our capitalization scheme. Can we rename this to something like initializeConfig?

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.

Done


/**
* 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 updateRequestLevelConfiguration(absl::string_view request_level_config);

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.

nit/optional: How do you feel about changing this to setRequestLevelConfiguration? I think it'd then be more consistent with setRequestHeader


/**
* 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. The request headers default to a minimal GET, but this may be changed

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 rephrase this comment a little (correct anything here that is inaccurate):
Fetch a response, according to the options specified by the class methods. By default, simulates a GET request with minimal headers.

* via other methods in this class.
*
* @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