From c4de56ef80fcf0572a325b9fcee2e59ae140121e Mon Sep 17 00:00:00 2001 From: Kyle Larose Date: Mon, 29 Apr 2019 15:56:04 -0400 Subject: [PATCH 01/19] refactor original_src_socket_option into common We'll want to use this option for both the http and the listener filter. Bring it out into a common library. Signed-off-by: Kyle Larose --- .../filters/common/original_src/BUILD | 24 +++++++++++++++++++ .../original_src_socket_option.cc | 8 ++++--- .../original_src/original_src_socket_option.h | 6 +++-- .../filters/listener/original_src/BUILD | 15 +----------- .../listener/original_src/original_src.cc | 5 ++-- .../listener/original_src/original_src.h | 1 - .../filters/common/original_src/BUILD | 22 +++++++++++++++++ .../original_src_socket_option_test.cc | 8 ++++--- .../filters/listener/original_src/BUILD | 13 ---------- .../original_src/original_src_test.cc | 1 - 10 files changed, 64 insertions(+), 39 deletions(-) create mode 100644 source/extensions/filters/common/original_src/BUILD rename source/extensions/filters/{listener => common}/original_src/original_src_socket_option.cc (92%) rename source/extensions/filters/{listener => common}/original_src/original_src_socket_option.h (94%) create mode 100644 test/extensions/filters/common/original_src/BUILD rename test/extensions/filters/{listener => common}/original_src/original_src_socket_option_test.cc (95%) diff --git a/source/extensions/filters/common/original_src/BUILD b/source/extensions/filters/common/original_src/BUILD new file mode 100644 index 0000000000000..a3d93c5b80933 --- /dev/null +++ b/source/extensions/filters/common/original_src/BUILD @@ -0,0 +1,24 @@ +licenses(["notice"]) # Apache 2 + +# Helprs for filters for mirroring the downstream remote address on the upstream's source. + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_library", + "envoy_package", +) + +envoy_package() + +envoy_cc_library( + name = "original_src_socket_option_lib", + srcs = ["original_src_socket_option.cc"], + hdrs = ["original_src_socket_option.h"], + deps = [ + "//include/envoy/network:listen_socket_interface", + "//source/common/common:assert_lib", + "//source/common/common:minimal_logger_lib", + "//source/common/network:address_lib", + "//source/common/network:utility_lib", + ], +) diff --git a/source/extensions/filters/listener/original_src/original_src_socket_option.cc b/source/extensions/filters/common/original_src/original_src_socket_option.cc similarity index 92% rename from source/extensions/filters/listener/original_src/original_src_socket_option.cc rename to source/extensions/filters/common/original_src/original_src_socket_option.cc index 913f629d695c8..0a15c216bf92e 100644 --- a/source/extensions/filters/listener/original_src/original_src_socket_option.cc +++ b/source/extensions/filters/common/original_src/original_src_socket_option.cc @@ -1,10 +1,11 @@ -#include "extensions/filters/listener/original_src/original_src_socket_option.h" +#include "extensions/filters/common/original_src/original_src_socket_option.h" #include "common/common/assert.h" namespace Envoy { namespace Extensions { -namespace ListenerFilters { +namespace Filters { +namespace Common { namespace OriginalSrc { OriginalSrcSocketOption::OriginalSrcSocketOption( @@ -57,6 +58,7 @@ OriginalSrcSocketOption::getOptionDetails(const Network::Socket&, } } // namespace OriginalSrc -} // namespace ListenerFilters +} // namespace Common +} // namespace Filters } // namespace Extensions } // namespace Envoy diff --git a/source/extensions/filters/listener/original_src/original_src_socket_option.h b/source/extensions/filters/common/original_src/original_src_socket_option.h similarity index 94% rename from source/extensions/filters/listener/original_src/original_src_socket_option.h rename to source/extensions/filters/common/original_src/original_src_socket_option.h index 6f2c1c000a85a..2659354d85984 100644 --- a/source/extensions/filters/listener/original_src/original_src_socket_option.h +++ b/source/extensions/filters/common/original_src/original_src_socket_option.h @@ -5,7 +5,8 @@ namespace Envoy { namespace Extensions { -namespace ListenerFilters { +namespace Filters { +namespace Common { namespace OriginalSrc { /** * A socket option implementation which allows a connection to spoof its source IP/port using @@ -40,6 +41,7 @@ class OriginalSrcSocketOption : public Network::Socket::Option { }; } // namespace OriginalSrc -} // namespace ListenerFilters +} // namespace Common +} // namespace Filters } // namespace Extensions } // namespace Envoy diff --git a/source/extensions/filters/listener/original_src/BUILD b/source/extensions/filters/listener/original_src/BUILD index e0af9e6742164..d530f2801a0d8 100644 --- a/source/extensions/filters/listener/original_src/BUILD +++ b/source/extensions/filters/listener/original_src/BUILD @@ -19,26 +19,12 @@ envoy_cc_library( ], ) -envoy_cc_library( - name = "original_src_socket_option_lib", - srcs = ["original_src_socket_option.cc"], - hdrs = ["original_src_socket_option.h"], - deps = [ - "//include/envoy/network:listen_socket_interface", - "//source/common/common:assert_lib", - "//source/common/common:minimal_logger_lib", - "//source/common/network:address_lib", - "//source/common/network:utility_lib", - ], -) - envoy_cc_library( name = "original_src_lib", srcs = ["original_src.cc"], hdrs = ["original_src.h"], deps = [ ":config_lib", - ":original_src_socket_option_lib", "//include/envoy/buffer:buffer_interface", "//include/envoy/network:address_interface", "//include/envoy/network:connection_interface", @@ -46,6 +32,7 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/common:minimal_logger_lib", "//source/common/network:socket_option_factory_lib", + "//source/extensions/filters/common/original_src:original_src_socket_option_lib", ], ) diff --git a/source/extensions/filters/listener/original_src/original_src.cc b/source/extensions/filters/listener/original_src/original_src.cc index 2f6f745fda240..e7e2d5e7fdb5b 100644 --- a/source/extensions/filters/listener/original_src/original_src.cc +++ b/source/extensions/filters/listener/original_src/original_src.cc @@ -7,7 +7,7 @@ #include "common/network/socket_option_factory.h" #include "common/network/utility.h" -#include "extensions/filters/listener/original_src/original_src_socket_option.h" +#include "extensions/filters/common/original_src/original_src_socket_option.h" namespace Envoy { namespace Extensions { @@ -36,7 +36,8 @@ Network::FilterStatus OriginalSrcFilter::onAccept(Network::ListenerFilterCallbac // into the upstream connection later. auto options_to_add = std::make_shared(); options_to_add->emplace_back( - std::make_shared(std::move(address_without_port))); + std::make_shared( + std::move(address_without_port))); if (config_.mark() != 0) { auto mark_options = Network::SocketOptionFactory::buildSocketMarkOptions(config_.mark()); diff --git a/source/extensions/filters/listener/original_src/original_src.h b/source/extensions/filters/listener/original_src/original_src.h index 180e04ecf5c67..f201644f88106 100644 --- a/source/extensions/filters/listener/original_src/original_src.h +++ b/source/extensions/filters/listener/original_src/original_src.h @@ -6,7 +6,6 @@ #include "common/common/logger.h" #include "extensions/filters/listener/original_src/config.h" -#include "extensions/filters/listener/original_src/original_src_socket_option.h" namespace Envoy { namespace Extensions { diff --git a/test/extensions/filters/common/original_src/BUILD b/test/extensions/filters/common/original_src/BUILD new file mode 100644 index 0000000000000..2556dde34cc6c --- /dev/null +++ b/test/extensions/filters/common/original_src/BUILD @@ -0,0 +1,22 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_test", + "envoy_cc_test_library", + "envoy_package", +) + +envoy_package() + +envoy_cc_test( + name = "original_src_socket_option_test", + srcs = ["original_src_socket_option_test.cc"], + deps = [ + "//source/common/network:address_lib", + "//source/extensions/filters/common/original_src:original_src_socket_option_lib", + "//test/mocks:common_lib", + "//test/mocks/network:network_mocks", + "//test/test_common:printers_lib", + ], +) diff --git a/test/extensions/filters/listener/original_src/original_src_socket_option_test.cc b/test/extensions/filters/common/original_src/original_src_socket_option_test.cc similarity index 95% rename from test/extensions/filters/listener/original_src/original_src_socket_option_test.cc rename to test/extensions/filters/common/original_src/original_src_socket_option_test.cc index 1ebdc096af754..9fc29b0aa5263 100644 --- a/test/extensions/filters/listener/original_src/original_src_socket_option_test.cc +++ b/test/extensions/filters/common/original_src/original_src_socket_option_test.cc @@ -2,7 +2,7 @@ #include "common/network/utility.h" -#include "extensions/filters/listener/original_src/original_src_socket_option.h" +#include "extensions/filters/common/original_src/original_src_socket_option.h" #include "test/mocks/common.h" #include "test/mocks/network/mocks.h" @@ -16,7 +16,8 @@ using testing::Eq; namespace Envoy { namespace Extensions { -namespace ListenerFilters { +namespace Filters { +namespace Common { namespace OriginalSrc { namespace { @@ -105,6 +106,7 @@ TEST_F(OriginalSrcSocketOptionTest, TestOptionDetailsNotSupported) { } // namespace } // namespace OriginalSrc -} // namespace ListenerFilters +} // namespace Common +} // namespace Filters } // namespace Extensions } // namespace Envoy diff --git a/test/extensions/filters/listener/original_src/BUILD b/test/extensions/filters/listener/original_src/BUILD index 2b522a9f33d76..9a43087b8efa3 100644 --- a/test/extensions/filters/listener/original_src/BUILD +++ b/test/extensions/filters/listener/original_src/BUILD @@ -49,16 +49,3 @@ envoy_extension_cc_test( "@envoy_api//envoy/config/filter/listener/original_src/v2alpha1:original_src_cc", ], ) - -envoy_extension_cc_test( - name = "original_src_socket_option_test", - srcs = ["original_src_socket_option_test.cc"], - extension_name = "envoy.filters.listener.original_src", - deps = [ - "//source/common/network:address_lib", - "//source/extensions/filters/listener/original_src:original_src_socket_option_lib", - "//test/mocks:common_lib", - "//test/mocks/network:network_mocks", - "//test/test_common:printers_lib", - ], -) diff --git a/test/extensions/filters/listener/original_src/original_src_test.cc b/test/extensions/filters/listener/original_src/original_src_test.cc index 2da95ec8685db..ffed37e4855ca 100644 --- a/test/extensions/filters/listener/original_src/original_src_test.cc +++ b/test/extensions/filters/listener/original_src/original_src_test.cc @@ -4,7 +4,6 @@ #include "common/network/utility.h" #include "extensions/filters/listener/original_src/original_src.h" -#include "extensions/filters/listener/original_src/original_src_socket_option.h" #include "test/mocks/buffer/mocks.h" #include "test/mocks/common.h" From 03100a4171d9b50a58ade72d44735a7029cf9a55 Mon Sep 17 00:00:00 2001 From: Kyle Larose Date: Mon, 29 Apr 2019 17:11:25 -0400 Subject: [PATCH 02/19] refactor creation of socket options to helper Signed-off-by: Kyle Larose --- .../filters/common/original_src/BUILD | 13 ++++++ .../original_src/socket_option_factory.cc | 40 +++++++++++++++++++ .../original_src/socket_option_factory.h | 19 +++++++++ .../filters/listener/original_src/BUILD | 3 +- .../listener/original_src/original_src.cc | 27 ++----------- 5 files changed, 77 insertions(+), 25 deletions(-) create mode 100644 source/extensions/filters/common/original_src/socket_option_factory.cc create mode 100644 source/extensions/filters/common/original_src/socket_option_factory.h diff --git a/source/extensions/filters/common/original_src/BUILD b/source/extensions/filters/common/original_src/BUILD index a3d93c5b80933..1f39f83548558 100644 --- a/source/extensions/filters/common/original_src/BUILD +++ b/source/extensions/filters/common/original_src/BUILD @@ -22,3 +22,16 @@ envoy_cc_library( "//source/common/network:utility_lib", ], ) + +envoy_cc_library( + name = "socket_option_factory_lib", + srcs = ["socket_option_factory.cc"], + hdrs = ["socket_option_factory.h"], + deps = [ + ":original_src_socket_option_lib", + "//include/envoy/network:listen_socket_interface", + "//source/common/network:address_lib", + "//source/common/network:socket_option_factory_lib", + "//source/common/network:utility_lib", + ], +) diff --git a/source/extensions/filters/common/original_src/socket_option_factory.cc b/source/extensions/filters/common/original_src/socket_option_factory.cc new file mode 100644 index 0000000000000..e3e12baf6f237 --- /dev/null +++ b/source/extensions/filters/common/original_src/socket_option_factory.cc @@ -0,0 +1,40 @@ +#include "extensions/filters/common/original_src/socket_option_factory.h" + +#include "common/network/socket_option_factory.h" +#include "common/network/utility.h" + +#include "extensions/filters/common/original_src/original_src_socket_option.h" + +namespace Envoy { +namespace Extensions { +namespace Filters { +namespace Common { +namespace OriginalSrc { + +Network::Socket::OptionsSharedPtr +buildOriginalSrcOptions(Network::Address::InstanceConstSharedPtr source, uint32_t mark) { + auto address_without_port = Network::Utility::getAddressWithPort(*source, 0); + + // note: we don't expect this to change the behaviour of the socket. We expect it to be copied + // into the upstream connection later. + auto options_to_add = std::make_shared(); + options_to_add->emplace_back( + std::make_shared( + std::move(address_without_port))); + + if (mark != 0) { + auto mark_options = Network::SocketOptionFactory::buildSocketMarkOptions(mark); + options_to_add->insert(options_to_add->end(), mark_options->begin(), mark_options->end()); + } + + auto transparent_options = Network::SocketOptionFactory::buildIpTransparentOptions(); + options_to_add->insert(options_to_add->end(), transparent_options->begin(), + transparent_options->end()); + return options_to_add; +} + +} // namespace OriginalSrc +} // namespace Common +} // namespace Filters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/common/original_src/socket_option_factory.h b/source/extensions/filters/common/original_src/socket_option_factory.h new file mode 100644 index 0000000000000..6e10df6bc08b0 --- /dev/null +++ b/source/extensions/filters/common/original_src/socket_option_factory.h @@ -0,0 +1,19 @@ +#pragma once + +#include "envoy/network/address.h" +#include "envoy/network/listen_socket.h" + +namespace Envoy { +namespace Extensions { +namespace Filters { +namespace Common { +namespace OriginalSrc { + +Network::Socket::OptionsSharedPtr +buildOriginalSrcOptions(Network::Address::InstanceConstSharedPtr source, uint32_t mark); + +} // namespace OriginalSrc +} // namespace Common +} // namespace Filters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/listener/original_src/BUILD b/source/extensions/filters/listener/original_src/BUILD index d530f2801a0d8..e6a9eb6a3f034 100644 --- a/source/extensions/filters/listener/original_src/BUILD +++ b/source/extensions/filters/listener/original_src/BUILD @@ -31,8 +31,7 @@ envoy_cc_library( "//include/envoy/network:filter_interface", "//source/common/common:assert_lib", "//source/common/common:minimal_logger_lib", - "//source/common/network:socket_option_factory_lib", - "//source/extensions/filters/common/original_src:original_src_socket_option_lib", + "//source/extensions/filters/common/original_src:socket_option_factory_lib", ], ) diff --git a/source/extensions/filters/listener/original_src/original_src.cc b/source/extensions/filters/listener/original_src/original_src.cc index e7e2d5e7fdb5b..684867d3c39c7 100644 --- a/source/extensions/filters/listener/original_src/original_src.cc +++ b/source/extensions/filters/listener/original_src/original_src.cc @@ -4,10 +4,8 @@ #include "envoy/network/connection.h" #include "common/common/assert.h" -#include "common/network/socket_option_factory.h" -#include "common/network/utility.h" -#include "extensions/filters/common/original_src/original_src_socket_option.h" +#include "extensions/filters/common/original_src/socket_option_factory.h" namespace Envoy { namespace Extensions { @@ -29,26 +27,9 @@ Network::FilterStatus OriginalSrcFilter::onAccept(Network::ListenerFilterCallbac // nothing we can do with this. return Network::FilterStatus::Continue; } - - auto address_without_port = Network::Utility::getAddressWithPort(*address, 0); - - // note: we don't expect this to change the behaviour of the socket. We expect it to be copied - // into the upstream connection later. - auto options_to_add = std::make_shared(); - options_to_add->emplace_back( - std::make_shared( - std::move(address_without_port))); - - if (config_.mark() != 0) { - auto mark_options = Network::SocketOptionFactory::buildSocketMarkOptions(config_.mark()); - options_to_add->insert(options_to_add->end(), mark_options->begin(), mark_options->end()); - } - - auto transparent_options = Network::SocketOptionFactory::buildIpTransparentOptions(); - options_to_add->insert(options_to_add->end(), transparent_options->begin(), - transparent_options->end()); - - socket.addOptions(options_to_add); + auto options_to_add = + Filters::Common::OriginalSrc::buildOriginalSrcOptions(std::move(address), config_.mark()); + socket.addOptions(std::move(options_to_add)); return Network::FilterStatus::Continue; } From d13f9056d3b8915f69e413b640cf2362383e301e Mon Sep 17 00:00:00 2001 From: Kyle Larose Date: Wed, 1 May 2019 09:37:54 -0400 Subject: [PATCH 03/19] finish up skeleton of original source filter The original source filter is now all hooked up, but it has some problems -- namely, it sets options on the downstream connection's socket, which is wrong. Signed-off-by: Kyle Larose --- .../filter/http/original_src/v2alpha1/BUILD | 8 +++ .../original_src/v2alpha1/original_src.proto | 26 +++++++++ source/extensions/extensions_build_config.bzl | 1 + .../filters/http/original_src/BUILD | 47 +++++++++++++++ .../filters/http/original_src/config.cc | 14 +++++ .../filters/http/original_src/config.h | 22 +++++++ .../filters/http/original_src/original_src.cc | 58 +++++++++++++++++++ .../filters/http/original_src/original_src.h | 41 +++++++++++++ .../original_src_config_factory.cc | 31 ++++++++++ .../original_src_config_factory.h | 29 ++++++++++ .../filters/http/well_known_names.h | 2 + 11 files changed, 279 insertions(+) create mode 100644 api/envoy/config/filter/http/original_src/v2alpha1/BUILD create mode 100644 api/envoy/config/filter/http/original_src/v2alpha1/original_src.proto create mode 100644 source/extensions/filters/http/original_src/BUILD create mode 100644 source/extensions/filters/http/original_src/config.cc create mode 100644 source/extensions/filters/http/original_src/config.h create mode 100644 source/extensions/filters/http/original_src/original_src.cc create mode 100644 source/extensions/filters/http/original_src/original_src.h create mode 100644 source/extensions/filters/http/original_src/original_src_config_factory.cc create mode 100644 source/extensions/filters/http/original_src/original_src_config_factory.h diff --git a/api/envoy/config/filter/http/original_src/v2alpha1/BUILD b/api/envoy/config/filter/http/original_src/v2alpha1/BUILD new file mode 100644 index 0000000000000..e064545b21cde --- /dev/null +++ b/api/envoy/config/filter/http/original_src/v2alpha1/BUILD @@ -0,0 +1,8 @@ +load("@envoy_api//bazel:api_build_system.bzl", "api_proto_library_internal") + +licenses(["notice"]) # Apache 2 + +api_proto_library_internal( + name = "original_src", + srcs = ["original_src.proto"], +) diff --git a/api/envoy/config/filter/http/original_src/v2alpha1/original_src.proto b/api/envoy/config/filter/http/original_src/v2alpha1/original_src.proto new file mode 100644 index 0000000000000..32f37a8c48f0c --- /dev/null +++ b/api/envoy/config/filter/http/original_src/v2alpha1/original_src.proto @@ -0,0 +1,26 @@ +syntax = "proto3"; + +package envoy.config.filter.http.original_src.v2alpha1; + +option java_outer_classname = "OriginalSrcProto"; +option java_multiple_files = true; +option java_package = "io.envoyproxy.envoy.config.filter.http.original_src.v2alpha1"; + +option go_package = "v2alpha1"; + +import "validate/validate.proto"; + +// [#protodoc-title: Original Src Filter] +// Use the Original source address on upstream connections. + +// The Original Src filter binds upstream connections to the original source address determined +// for the request. This address could come from something like the Proxy Protocol filter, or it +// could come from trusted http headers. +message OriginalSrc { + + // Sets the SO_MARK option on the upstream connection's socket to the provided value. Used to + // ensure that non-local addresses may be routed back through envoy when binding to the original + // source address. The option will not be applied if the mark is 0. + // [#proto-status: experimental] + uint32 mark = 1; +} diff --git a/source/extensions/extensions_build_config.bzl b/source/extensions/extensions_build_config.bzl index 5b08a48e1f091..3116447c138fd 100644 --- a/source/extensions/extensions_build_config.bzl +++ b/source/extensions/extensions_build_config.bzl @@ -39,6 +39,7 @@ EXTENSIONS = { "envoy.filters.http.ip_tagging": "//source/extensions/filters/http/ip_tagging:config", "envoy.filters.http.jwt_authn": "//source/extensions/filters/http/jwt_authn:config", "envoy.filters.http.lua": "//source/extensions/filters/http/lua:config", + "envoy.filters.http.original_src": "//source/extensions/filters/http/original_src:config", "envoy.filters.http.ratelimit": "//source/extensions/filters/http/ratelimit:config", "envoy.filters.http.rbac": "//source/extensions/filters/http/rbac:config", "envoy.filters.http.router": "//source/extensions/filters/http/router:config", diff --git a/source/extensions/filters/http/original_src/BUILD b/source/extensions/filters/http/original_src/BUILD new file mode 100644 index 0000000000000..04e89334f85f0 --- /dev/null +++ b/source/extensions/filters/http/original_src/BUILD @@ -0,0 +1,47 @@ +licenses(["notice"]) # Apache 2 + +# A filter for mirroring the downstream remote address on the upstream's source. + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_library", + "envoy_package", +) + +envoy_package() + +envoy_cc_library( + name = "config_lib", + srcs = ["config.cc"], + hdrs = ["config.h"], + deps = [ + "@envoy_api//envoy/config/filter/http/original_src/v2alpha1:original_src_cc", + ], +) + +envoy_cc_library( + name = "original_src_lib", + srcs = ["original_src.cc"], + hdrs = ["original_src.h"], + deps = [ + ":config_lib", + "//include/envoy/http:filter_interface", + "//source/common/common:assert_lib", + "//source/common/common:minimal_logger_lib", + "//source/extensions/filters/common/original_src:socket_option_factory_lib", + ], +) + +envoy_cc_library( + name = "config", # The extension build system requires a library named config + srcs = ["original_src_config_factory.cc"], + hdrs = ["original_src_config_factory.h"], + deps = [ + ":config_lib", + ":original_src_lib", + "//include/envoy/registry", + "//source/extensions/filters/http:well_known_names", + "//source/extensions/filters/http/common:factory_base_lib", + "@envoy_api//envoy/config/filter/http/original_src/v2alpha1:original_src_cc", + ], +) diff --git a/source/extensions/filters/http/original_src/config.cc b/source/extensions/filters/http/original_src/config.cc new file mode 100644 index 0000000000000..ae4f0e193b534 --- /dev/null +++ b/source/extensions/filters/http/original_src/config.cc @@ -0,0 +1,14 @@ +#include "extensions/filters/http/original_src/config.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace OriginalSrc { + +Config::Config(const envoy::config::filter::http::original_src::v2alpha1::OriginalSrc& config) + : mark_(config.mark()) {} + +} // namespace OriginalSrc +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/original_src/config.h b/source/extensions/filters/http/original_src/config.h new file mode 100644 index 0000000000000..1de3d5bc4eecf --- /dev/null +++ b/source/extensions/filters/http/original_src/config.h @@ -0,0 +1,22 @@ +#pragma once + +#include "envoy/config/filter/http/original_src/v2alpha1/original_src.pb.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace OriginalSrc { +class Config { +public: + Config() = default; + Config(const envoy::config::filter::http::original_src::v2alpha1::OriginalSrc& config); + + uint32_t mark() const { return mark_; } + +private: + uint32_t mark_ = 0; +}; +} // namespace OriginalSrc +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/original_src/original_src.cc b/source/extensions/filters/http/original_src/original_src.cc new file mode 100644 index 0000000000000..65a4b54a83ebb --- /dev/null +++ b/source/extensions/filters/http/original_src/original_src.cc @@ -0,0 +1,58 @@ +#include "extensions/filters/http/original_src/original_src.h" + +#include "common/common/assert.h" + +#include "extensions/filters/common/original_src/socket_option_factory.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace OriginalSrc { + +OriginalSrcFilter::OriginalSrcFilter(const Config& config) : config_(config) {} + +void OriginalSrcFilter::onDestroy() {} + +Http::FilterHeadersStatus OriginalSrcFilter::decodeHeaders(Http::HeaderMap&, bool end_stream) { + + // We wait until the end of all the headers to ensure any that affect that downstream remote + // address have been parsed. Further, if there's no downstream connection, we can't do anything + // so just continue. + if (!end_stream || !callbacks_->connection()) { + return Http::FilterHeadersStatus::Continue; + } + + auto connection_options = callbacks_->connection()->socketOptions(); + auto address = callbacks_->streamInfo().downstreamRemoteAddress(); + ASSERT(address); + + ENVOY_LOG(debug, + "Got a new connection in the original_src filter for address {}. Marking with {}", + address->asString(), config_.mark()); + + if (address->type() != Network::Address::Type::Ip) { + // Nothing we can do with this. + return Http::FilterHeadersStatus::Continue; + } + auto options_to_add = + Filters::Common::OriginalSrc::buildOriginalSrcOptions(std::move(address), config_.mark()); + Network::Socket::appendOptions(connection_options, options_to_add); + return Http::FilterHeadersStatus::Continue; +} + +Http::FilterDataStatus OriginalSrcFilter::decodeData(Buffer::Instance&, bool) { + return Http::FilterDataStatus::Continue; +} + +Http::FilterTrailersStatus OriginalSrcFilter::decodeTrailers(Http::HeaderMap&) { + return Http::FilterTrailersStatus::Continue; +} + +void OriginalSrcFilter::setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) { + callbacks_ = &callbacks; +} + +} // namespace OriginalSrc +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/original_src/original_src.h b/source/extensions/filters/http/original_src/original_src.h new file mode 100644 index 0000000000000..f0ca34ddaad21 --- /dev/null +++ b/source/extensions/filters/http/original_src/original_src.h @@ -0,0 +1,41 @@ +#pragma once + +#include "envoy/http/filter.h" +#include "envoy/network/address.h" + +#include "common/common/logger.h" + +#include "extensions/filters/http/original_src/config.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace OriginalSrc { + +/** + * Implements the Original Src network filter. This filter places the source address of the socket + * into an option which will alter be used to partition upstream connections. + * This does not support non-ip (e.g. AF_UNIX) connections, which will be failed and counted. + */ +class OriginalSrcFilter : public Http::StreamDecoderFilter, Logger::Loggable { +public: + OriginalSrcFilter(const Config& config); + + // Http::StreamFilterBase + void onDestroy() override; + + // Http::StreamDecoderFilter + Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap& headers, bool end_stream) override; + Http::FilterDataStatus decodeData(Buffer::Instance& data, bool end_stream) override; + Http::FilterTrailersStatus decodeTrailers(Http::HeaderMap& trailers) override; + void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override; + +private: + Config config_; + Http::StreamDecoderFilterCallbacks* callbacks_{}; +}; + +} // namespace OriginalSrc +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/original_src/original_src_config_factory.cc b/source/extensions/filters/http/original_src/original_src_config_factory.cc new file mode 100644 index 0000000000000..58c7752be2aa5 --- /dev/null +++ b/source/extensions/filters/http/original_src/original_src_config_factory.cc @@ -0,0 +1,31 @@ +#include "extensions/filters/http/original_src/original_src_config_factory.h" + +#include "envoy/registry/registry.h" + +#include "extensions/filters/http/original_src/config.h" +#include "extensions/filters/http/original_src/original_src.h" +#include "extensions/filters/http/well_known_names.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace OriginalSrc { + +Http::FilterFactoryCb OriginalSrcConfigFactory::createFilterFactoryFromProtoTyped( + const envoy::config::filter::http::original_src::v2alpha1::OriginalSrc& proto_config, + const std::string&, Server::Configuration::FactoryContext&) { + Config config(proto_config); + return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamDecoderFilter(std::make_unique(config)); + }; +} + +/** + * Static registration for the original_src filter. @see RegisterFactory. + */ +REGISTER_FACTORY(OriginalSrcConfigFactory, Server::Configuration::NamedHttpFilterConfigFactory); + +} // namespace OriginalSrc +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/original_src/original_src_config_factory.h b/source/extensions/filters/http/original_src/original_src_config_factory.h new file mode 100644 index 0000000000000..76f8375a41bc4 --- /dev/null +++ b/source/extensions/filters/http/original_src/original_src_config_factory.h @@ -0,0 +1,29 @@ +#pragma once + +#include "envoy/config/filter/http/original_src/v2alpha1/original_src.pb.h" +#include "envoy/config/filter/http/original_src/v2alpha1/original_src.pb.validate.h" + +#include "extensions/filters/http/common/factory_base.h" +#include "extensions/filters/http/well_known_names.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace OriginalSrc { +/** + * Config registration for the original_src filter. + */ +class OriginalSrcConfigFactory + : public Common::FactoryBase { +public: + OriginalSrcConfigFactory() : FactoryBase(HttpFilterNames::get().OriginalSrc) {} + + Http::FilterFactoryCb createFilterFactoryFromProtoTyped( + const envoy::config::filter::http::original_src::v2alpha1::OriginalSrc& proto_config, + const std::string& stat_prefix, Server::Configuration::FactoryContext& context) override; +}; + +} // namespace OriginalSrc +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/well_known_names.h b/source/extensions/filters/http/well_known_names.h index 0352c5bc2c061..ef9e140acc767 100644 --- a/source/extensions/filters/http/well_known_names.h +++ b/source/extensions/filters/http/well_known_names.h @@ -54,6 +54,8 @@ class HttpFilterNameValues { const std::string HeaderToMetadata = "envoy.filters.http.header_to_metadata"; // Tap filter const std::string Tap = "envoy.filters.http.tap"; + // Original Src Filter + const std::string OriginalSrc = "envoy.filters.http.original_src"; // Converts names from v1 to v2 const Config::V1Converter v1_converter_; From ae0a630b202521e024a6f3f05adcd4dc613d1407 Mon Sep 17 00:00:00 2001 From: Kyle Larose Date: Wed, 1 May 2019 10:54:40 -0400 Subject: [PATCH 04/19] add upstreamSocketOptions We need these to configure per-request socket options from the http filters. Signed-off-by: Kyle Larose --- include/envoy/http/filter.h | 14 ++++++++++++++ include/envoy/upstream/load_balancer.h | 5 +++++ source/common/http/async_client_impl.h | 2 ++ source/common/http/conn_manager_impl.h | 9 +++++++++ source/common/router/router.h | 4 ++++ source/common/upstream/load_balancer_impl.h | 2 ++ test/common/router/router_test.cc | 20 ++++++++++++++++++++ test/mocks/http/mocks.h | 2 ++ test/mocks/upstream/load_balancer_context.h | 1 + 9 files changed, 59 insertions(+) diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index eec28670591ed..b73fe3eeeb025 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -404,6 +404,20 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks { // Note that HttpConnectionManager sanitization will *not* be performed on the // recreated stream, as it is assumed that sanitization has already been done. virtual bool recreateStream() PURE; + + /** + * Adds socket options to be applied to the upstream request. Note that unique values for the + * options will likely lead to many connection pools being created. The added options are appended + * to any previously added. + * + * @param options The options to be added. + */ + virtual void addUpstreamSocketOptions(const Network::Socket::OptionsSharedPtr& options) PURE; + + /** + * @return The socket options to be applied to the upstream request. + */ + virtual Network::Socket::OptionsSharedPtr getUpstreamSocketOptions() const PURE; }; /** diff --git a/include/envoy/upstream/load_balancer.h b/include/envoy/upstream/load_balancer.h index 6906990074e73..a62d2ac3e309a 100644 --- a/include/envoy/upstream/load_balancer.h +++ b/include/envoy/upstream/load_balancer.h @@ -71,6 +71,11 @@ class LoadBalancerContext { * ignored. */ virtual uint32_t hostSelectionRetryCount() const PURE; + + /** + * Returns the set of socket options which should be applied on upstream connections + */ + virtual Network::Socket::OptionsSharedPtr upstreamSocketOptions() const PURE; }; /** diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index b288664db86a9..b53a94b59a664 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -355,6 +355,8 @@ class AsyncStreamImpl : public AsyncClient::Stream, void setDecoderBufferLimit(uint32_t) override {} uint32_t decoderBufferLimit() override { return 0; } bool recreateStream() override { return false; } + void addUpstreamSocketOptions(const Network::Socket::OptionsSharedPtr&) override {} + Network::Socket::OptionsSharedPtr getUpstreamSocketOptions() const override { return {}; } AsyncClient::StreamCallbacks& stream_callbacks_; const uint64_t stream_id_; diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 3d61de6622912..60d233b0e456c 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -242,6 +242,14 @@ class ConnectionManagerImpl : Logger::Loggable, uint32_t decoderBufferLimit() override { return parent_.buffer_limit_; } bool recreateStream() override; + void addUpstreamSocketOptions(const Network::Socket::OptionsSharedPtr& options) override { + Network::Socket::appendOptions(upstream_options_, options); + } + + Network::Socket::OptionsSharedPtr getUpstreamSocketOptions() const override { + return upstream_options_; + } + // Each decoder filter instance checks if the request passed to the filter is gRPC // so that we can issue gRPC local responses to gRPC requests. Filter's decodeHeaders() // called here may change the content type, so we must check it before the call. @@ -259,6 +267,7 @@ class ConnectionManagerImpl : Logger::Loggable, StreamDecoderFilterSharedPtr handle_; bool is_grpc_request_{}; + Network::Socket::OptionsSharedPtr upstream_options_; }; typedef std::unique_ptr ActiveStreamDecoderFilterPtr; diff --git a/source/common/router/router.h b/source/common/router/router.h index 0c560ba8fe80c..d56ded0e455e8 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -239,6 +239,10 @@ class Filter : Logger::Loggable, return retry_state_->hostSelectionMaxAttempts(); } + Network::Socket::OptionsSharedPtr upstreamSocketOptions() const override { + return callbacks_->getUpstreamSocketOptions(); + } + /** * Set a computed cookie to be sent with the downstream headers. * @param key supplies the size of the cookie diff --git a/source/common/upstream/load_balancer_impl.h b/source/common/upstream/load_balancer_impl.h index 7ba7377cfe999..3f0dca76df6a4 100644 --- a/source/common/upstream/load_balancer_impl.h +++ b/source/common/upstream/load_balancer_impl.h @@ -148,6 +148,8 @@ class LoadBalancerContextBase : public LoadBalancerContext { bool shouldSelectAnotherHost(const Host&) override { return false; } uint32_t hostSelectionRetryCount() const override { return 1; } + + Network::Socket::OptionsSharedPtr upstreamSocketOptions() const override { return {}; } }; /** diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index e083c5e587ade..47fca71a689de 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -7,6 +7,7 @@ #include "common/config/metadata.h" #include "common/config/well_known_names.h" #include "common/http/context_impl.h" +#include "common/network/socket_option_factory.h" #include "common/network/utility.h" #include "common/router/config_impl.h" #include "common/router/router.h" @@ -2961,6 +2962,25 @@ TEST_F(RouterTest, AutoHostRewriteDisabled) { router_.decodeHeaders(incoming_headers, true); } +TEST_F(RouterTest, upstreamSocketOptionsReturnedEmpty) { + EXPECT_CALL(callbacks_, getUpstreamSocketOptions()) + .WillOnce(Return(Network::Socket::OptionsSharedPtr())); + + auto options = router_.upstreamSocketOptions(); + + EXPECT_EQ(options.get(), nullptr); +} + +TEST_F(RouterTest, upstreamSocketOptionsReturnedNonEmpty) { + Network::Socket::OptionsSharedPtr to_return = + Network::SocketOptionFactory::buildIpTransparentOptions(); + EXPECT_CALL(callbacks_, getUpstreamSocketOptions()).WillOnce(Return(to_return)); + + auto options = router_.upstreamSocketOptions(); + + EXPECT_EQ(to_return, options); +} + class WatermarkTest : public RouterTest { public: void sendRequest(bool header_only_request = true, bool pool_ready = true) { diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index a5f2971743c2b..735f4ca1d9330 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -148,6 +148,8 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, MOCK_METHOD1(setDecoderBufferLimit, void(uint32_t)); MOCK_METHOD0(decoderBufferLimit, uint32_t()); MOCK_METHOD0(recreateStream, bool()); + MOCK_METHOD1(addUpstreamSocketOptions, void(const Network::Socket::OptionsSharedPtr& options)); + MOCK_CONST_METHOD0(getUpstreamSocketOptions, Network::Socket::OptionsSharedPtr()); // Http::StreamDecoderFilterCallbacks void sendLocalReply(Code code, absl::string_view body, diff --git a/test/mocks/upstream/load_balancer_context.h b/test/mocks/upstream/load_balancer_context.h index 578813991fbe1..13221f30e41c1 100644 --- a/test/mocks/upstream/load_balancer_context.h +++ b/test/mocks/upstream/load_balancer_context.h @@ -18,6 +18,7 @@ class MockLoadBalancerContext : public LoadBalancerContext { const HealthyAndDegradedLoad&(const PrioritySet&, const HealthyAndDegradedLoad&)); MOCK_METHOD1(shouldSelectAnotherHost, bool(const Host&)); MOCK_CONST_METHOD0(hostSelectionRetryCount, uint32_t()); + MOCK_CONST_METHOD0(usptreamSocketOptions, Network::Socket::OptionsSharedPtr()); }; } // namespace Upstream From 78cb8e857a02b0debffc4eb0721783d88a6d9008 Mon Sep 17 00:00:00 2001 From: Kyle Larose Date: Wed, 1 May 2019 14:38:41 -0400 Subject: [PATCH 05/19] finish up http original src filter - Hook in to new callback options mechanism - add ut Signed-off-by: Kyle Larose --- .../filters/http/original_src/original_src.cc | 14 +- .../filters/http/original_src/BUILD | 53 +++++ .../filters/http/original_src/config_test.cc | 54 +++++ .../original_src_config_factory_test.cc | 52 +++++ .../http/original_src/original_src_test.cc | 216 ++++++++++++++++++ 5 files changed, 378 insertions(+), 11 deletions(-) create mode 100644 test/extensions/filters/http/original_src/BUILD create mode 100644 test/extensions/filters/http/original_src/config_test.cc create mode 100644 test/extensions/filters/http/original_src/original_src_config_factory_test.cc create mode 100644 test/extensions/filters/http/original_src/original_src_test.cc diff --git a/source/extensions/filters/http/original_src/original_src.cc b/source/extensions/filters/http/original_src/original_src.cc index 65a4b54a83ebb..5c339d3904f50 100644 --- a/source/extensions/filters/http/original_src/original_src.cc +++ b/source/extensions/filters/http/original_src/original_src.cc @@ -13,16 +13,7 @@ OriginalSrcFilter::OriginalSrcFilter(const Config& config) : config_(config) {} void OriginalSrcFilter::onDestroy() {} -Http::FilterHeadersStatus OriginalSrcFilter::decodeHeaders(Http::HeaderMap&, bool end_stream) { - - // We wait until the end of all the headers to ensure any that affect that downstream remote - // address have been parsed. Further, if there's no downstream connection, we can't do anything - // so just continue. - if (!end_stream || !callbacks_->connection()) { - return Http::FilterHeadersStatus::Continue; - } - - auto connection_options = callbacks_->connection()->socketOptions(); +Http::FilterHeadersStatus OriginalSrcFilter::decodeHeaders(Http::HeaderMap&, bool) { auto address = callbacks_->streamInfo().downstreamRemoteAddress(); ASSERT(address); @@ -34,9 +25,10 @@ Http::FilterHeadersStatus OriginalSrcFilter::decodeHeaders(Http::HeaderMap&, boo // Nothing we can do with this. return Http::FilterHeadersStatus::Continue; } + auto options_to_add = Filters::Common::OriginalSrc::buildOriginalSrcOptions(std::move(address), config_.mark()); - Network::Socket::appendOptions(connection_options, options_to_add); + callbacks_->addUpstreamSocketOptions(options_to_add); return Http::FilterHeadersStatus::Continue; } diff --git a/test/extensions/filters/http/original_src/BUILD b/test/extensions/filters/http/original_src/BUILD new file mode 100644 index 0000000000000..7360ac19db62b --- /dev/null +++ b/test/extensions/filters/http/original_src/BUILD @@ -0,0 +1,53 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_test", + "envoy_package", +) +load( + "//test/extensions:extensions_build_system.bzl", + "envoy_extension_cc_test", +) + +envoy_package() + +envoy_extension_cc_test( + name = "config_test", + srcs = ["config_test.cc"], + extension_name = "envoy.filters.http.original_src", + deps = [ + "//source/extensions/filters/http/original_src:config_lib", + "@envoy_api//envoy/config/filter/http/original_src/v2alpha1:original_src_cc", + ], +) + +envoy_extension_cc_test( + name = "original_src_config_factory_test", + srcs = ["original_src_config_factory_test.cc"], + extension_name = "envoy.filters.http.original_src", + deps = [ + "//source/extensions/filters/http/original_src:config", + "//source/extensions/filters/http/original_src:config_lib", + "//source/extensions/filters/http/original_src:original_src_lib", + "//test/mocks/server:server_mocks", + "@envoy_api//envoy/config/filter/http/original_src/v2alpha1:original_src_cc", + ], +) + +envoy_extension_cc_test( + name = "original_src_test", + srcs = ["original_src_test.cc"], + extension_name = "envoy.filters.http.original_src", + deps = [ + "//source/common/network:socket_option_lib", + "//source/extensions/filters/http/original_src:original_src_lib", + "//test/mocks:common_lib", + "//test/mocks/buffer:buffer_mocks", + "//test/mocks/network:network_mocks", + "//test/mocks/http:http_mocks", + "//test/test_common:printers_lib", + "//test/test_common:utility_lib", + "@envoy_api//envoy/config/filter/http/original_src/v2alpha1:original_src_cc", + ], +) diff --git a/test/extensions/filters/http/original_src/config_test.cc b/test/extensions/filters/http/original_src/config_test.cc new file mode 100644 index 0000000000000..a55bd19619922 --- /dev/null +++ b/test/extensions/filters/http/original_src/config_test.cc @@ -0,0 +1,54 @@ +#include + +#include "envoy/config/filter/http/original_src/v2alpha1/original_src.pb.h" +#include "envoy/config/filter/http/original_src/v2alpha1/original_src.pb.validate.h" + +#include "extensions/filters/http/original_src/config.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace OriginalSrc { +namespace { + +// In keeping with the class under test, it would have made sense to call this ConfigTest. However, +// when running coverage tests, that conflicts with tests elsewhere in the codebase. +class OriginalSrcConfigTest : public testing::Test { +public: + Config makeConfigFromProto( + const envoy::config::filter::http::original_src::v2alpha1::OriginalSrc& proto_config) { + return Config(proto_config); + } +}; + +TEST_F(OriginalSrcConfigTest, TestUseMark0) { + envoy::config::filter::http::original_src::v2alpha1::OriginalSrc config_proto; + config_proto.set_mark(0); + auto config = makeConfigFromProto(config_proto); + + EXPECT_EQ(config.mark(), 0); +} + +TEST_F(OriginalSrcConfigTest, TestUseMark1234) { + envoy::config::filter::http::original_src::v2alpha1::OriginalSrc config_proto; + config_proto.set_mark(1234); + auto config = makeConfigFromProto(config_proto); + + EXPECT_EQ(config.mark(), 1234); +} + +TEST_F(OriginalSrcConfigTest, TestUseMarkMax) { + envoy::config::filter::http::original_src::v2alpha1::OriginalSrc config_proto; + config_proto.set_mark(std::numeric_limits::max()); + auto config = makeConfigFromProto(config_proto); + + EXPECT_EQ(config.mark(), std::numeric_limits::max()); +} + +} // namespace +} // namespace OriginalSrc +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/filters/http/original_src/original_src_config_factory_test.cc b/test/extensions/filters/http/original_src/original_src_config_factory_test.cc new file mode 100644 index 0000000000000..262d45ea97dc7 --- /dev/null +++ b/test/extensions/filters/http/original_src/original_src_config_factory_test.cc @@ -0,0 +1,52 @@ +#include "envoy/config/filter/http/original_src/v2alpha1/original_src.pb.h" +#include "envoy/config/filter/http/original_src/v2alpha1/original_src.pb.validate.h" + +#include "extensions/filters/http/original_src/config.h" +#include "extensions/filters/http/original_src/original_src.h" +#include "extensions/filters/http/original_src/original_src_config_factory.h" + +#include "test/mocks/server/mocks.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::Invoke; + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace OriginalSrc { +namespace { + +TEST(OriginalSrcConfigFactoryTest, TestCreateFactory) { + std::string yaml = R"EOF( + mark: 5 +)EOF"; + + OriginalSrcConfigFactory factory; + ProtobufTypes::MessagePtr proto_config = factory.createEmptyConfigProto(); + MessageUtil::loadFromYaml(yaml, *proto_config); + + Server::Configuration::MockFactoryContext context; + + Http::FilterFactoryCb cb = + factory.createFilterFactoryFromProto(*proto_config, "", context); + + Http::MockFilterChainFactoryCallbacks filter_callback; + Http::StreamDecoderFilterSharedPtr added_filter; + EXPECT_CALL(filter_callback, addStreamDecoderFilter(_)) + .WillOnce(Invoke([&added_filter](Http::StreamDecoderFilterSharedPtr filter) { + added_filter = std::move(filter); + })); + + cb(filter_callback); + + // Make sure we actually create the correct type! + EXPECT_NE(dynamic_cast(added_filter.get()), nullptr); +} + +} // namespace +} // namespace OriginalSrc +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/filters/http/original_src/original_src_test.cc b/test/extensions/filters/http/original_src/original_src_test.cc new file mode 100644 index 0000000000000..bc82fa510c8af --- /dev/null +++ b/test/extensions/filters/http/original_src/original_src_test.cc @@ -0,0 +1,216 @@ +#include "envoy/config/filter/http/original_src/v2alpha1/original_src.pb.h" + +#include "common/network/socket_option_impl.h" +#include "common/network/utility.h" + +#include "extensions/filters/http/original_src/original_src.h" + +#include "test/mocks/buffer/mocks.h" +#include "test/mocks/common.h" +#include "test/mocks/network/mocks.h" +#include "test/mocks/http/mocks.h" +#include "test/test_common/printers.h" +#include "test/test_common/utility.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::_; +using testing::Exactly; +using testing::SaveArg; +using testing::StrictMock; + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace OriginalSrc { +namespace { + +class OriginalSrcTest : public testing::Test { +public: + std::unique_ptr makeDefaultFilter() { + return makeFilterWithCallbacks(callbacks_); + } + + std::unique_ptr makeFilterWithCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) { + Config default_config; + auto filter = std::make_unique(default_config); + filter->setDecoderFilterCallbacks(callbacks); + return filter; + } + + std::unique_ptr makeMarkingFilter(uint32_t mark) { + envoy::config::filter::http::original_src::v2alpha1::OriginalSrc proto_config; + proto_config.set_mark(mark); + + Config config(proto_config); + auto filter = std::make_unique(config); + filter->setDecoderFilterCallbacks(callbacks_); + return filter; + } + + void setAddressToReturn(const std::string& address) { + callbacks_.stream_info_.downstream_remote_address_ = Network::Utility::resolveUrl(address); + } + +protected: + StrictMock buffer_; + NiceMock callbacks_; + NiceMock socket_; + Http::TestHeaderMapImpl headers_; + + + absl::optional + findOptionDetails(const Network::Socket::Options& options, Network::SocketOptionName name, + envoy::api::v2::core::SocketOption::SocketState state) { + for (const auto& option : options) { + auto details = option->getOptionDetails(socket_, state); + if (details.has_value() && details->name_ == name) { + return details; + } + } + + return absl::nullopt; + } +}; + +TEST_F(OriginalSrcTest, onNonIpAddressDecodeSkips) { + auto filter = makeDefaultFilter(); + setAddressToReturn("unix://domain.socket"); + EXPECT_CALL(callbacks_, addUpstreamSocketOptions(_)).Times(0); + EXPECT_EQ(filter->decodeHeaders(headers_, false), Http::FilterHeadersStatus::Continue); +} + +TEST_F(OriginalSrcTest, decodeHeadersIpv4AddressAddsOption) { + auto filter = makeDefaultFilter(); + + Network::Socket::OptionsSharedPtr options; + setAddressToReturn("tcp://1.2.3.4:0"); + EXPECT_CALL(callbacks_, addUpstreamSocketOptions(_)).WillOnce(SaveArg<0>(&options)); + + EXPECT_EQ(filter->decodeHeaders(headers_, false), Http::FilterHeadersStatus::Continue); + + // not ideal -- we're assuming that the original_src option is first, but it's a fair assumption + // for now. + ASSERT_NE(options->at(0), nullptr); + + NiceMock socket; + EXPECT_CALL(socket, setLocalAddress(PointeesEq(callbacks_.stream_info_.downstream_remote_address_))); + options->at(0)->setOption(socket, envoy::api::v2::core::SocketOption::STATE_PREBIND); +} + +TEST_F(OriginalSrcTest, decodeHeadersIpv4AddressUsesCorrectAddress) { + auto filter = makeDefaultFilter(); + Network::Socket::OptionsSharedPtr options; + setAddressToReturn("tcp://1.2.3.4:0"); + EXPECT_CALL(callbacks_, addUpstreamSocketOptions(_)).WillOnce(SaveArg<0>(&options)); + + filter->decodeHeaders(headers_, false); + std::vector key; + // not ideal -- we're assuming that the original_src option is first, but it's a fair assumption + // for now. + options->at(0)->hashKey(key); + std::vector expected_key = {1, 2, 3, 4}; + + EXPECT_EQ(key, expected_key); +} + +TEST_F(OriginalSrcTest, decodeHeadersIpv4AddressBleachesPort) { + auto filter = makeDefaultFilter(); + Network::Socket::OptionsSharedPtr options; + setAddressToReturn("tcp://1.2.3.4:80"); + EXPECT_CALL(callbacks_, addUpstreamSocketOptions(_)).WillOnce(SaveArg<0>(&options)); + + filter->decodeHeaders(headers_, false); + + NiceMock socket; + const auto expected_address = Network::Utility::parseInternetAddress("1.2.3.4"); + EXPECT_CALL(socket, setLocalAddress(PointeesEq(expected_address))); + + // not ideal -- we're assuming that the original_src option is first, but it's a fair assumption + // for now. + options->at(0)->setOption(socket, envoy::api::v2::core::SocketOption::STATE_PREBIND); +} + +TEST_F(OriginalSrcTest, filterAddsTransparentOption) { + if (!ENVOY_SOCKET_IP_TRANSPARENT.has_value()) { + // The option isn't supported on this platform. Just skip the test. + return; + } + + auto filter = makeDefaultFilter(); + Network::Socket::OptionsSharedPtr options; + setAddressToReturn("tcp://1.2.3.4:80"); + EXPECT_CALL(callbacks_, addUpstreamSocketOptions(_)).WillOnce(SaveArg<0>(&options)); + + filter->decodeHeaders(headers_, false); + + auto transparent_option = findOptionDetails(*options, ENVOY_SOCKET_IP_TRANSPARENT, + envoy::api::v2::core::SocketOption::STATE_PREBIND); + + EXPECT_TRUE(transparent_option.has_value()); +} + +TEST_F(OriginalSrcTest, filterAddsMarkOption) { + if (!ENVOY_SOCKET_SO_MARK.has_value()) { + // The option isn't supported on this platform. Just skip the test. + return; + } + + auto filter = makeMarkingFilter(1234); + Network::Socket::OptionsSharedPtr options; + setAddressToReturn("tcp://1.2.3.4:80"); + EXPECT_CALL(callbacks_, addUpstreamSocketOptions(_)).WillOnce(SaveArg<0>(&options)); + + filter->decodeHeaders(headers_, false); + + auto mark_option = findOptionDetails(*options, ENVOY_SOCKET_SO_MARK, + envoy::api::v2::core::SocketOption::STATE_PREBIND); + + ASSERT_TRUE(mark_option.has_value()); + uint32_t value = 1234; + absl::string_view value_as_bstr(reinterpret_cast(&value), sizeof(value)); + EXPECT_EQ(value_as_bstr, mark_option->value_); +} + +TEST_F(OriginalSrcTest, Mark0NotAdded) { + if (!ENVOY_SOCKET_SO_MARK.has_value()) { + // The option isn't supported on this platform. Just skip the test. + return; + } + + auto filter = makeMarkingFilter(0); + Network::Socket::OptionsSharedPtr options; + setAddressToReturn("tcp://1.2.3.4:80"); + EXPECT_CALL(callbacks_, addUpstreamSocketOptions(_)).WillOnce(SaveArg<0>(&options)); + + filter->decodeHeaders(headers_, false); + + auto mark_option = findOptionDetails(*options, ENVOY_SOCKET_SO_MARK, + envoy::api::v2::core::SocketOption::STATE_PREBIND); + + ASSERT_FALSE(mark_option.has_value()); +} + +TEST_F(OriginalSrcTest, decodeDataDoesNothing) { + StrictMock callbacks; + auto filter = makeFilterWithCallbacks(callbacks); + + EXPECT_EQ(Http::FilterDataStatus::Continue, filter->decodeData(buffer_, true)); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter->decodeData(buffer_, false)); +} + +TEST_F(OriginalSrcTest, decodeTrailersDoesNothing) { + StrictMock callbacks; + auto filter = makeFilterWithCallbacks(callbacks); + + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter->decodeTrailers(headers_)); + + // make sure the headers aren't changed at all by comparing them to the default. + EXPECT_EQ(headers_, Http::TestHeaderMapImpl()); +} +} // namespace +} // namespace OriginalSrc +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy From 3ce2987a5181a206606b0440726b17fd984bb707 Mon Sep 17 00:00:00 2001 From: Kyle Larose Date: Thu, 2 May 2019 10:51:30 -0400 Subject: [PATCH 06/19] fixup rebase previous original_src_filter Signed-off-by: Kyle Larose --- test/extensions/filters/http/original_src/BUILD | 2 +- .../original_src/original_src_config_factory_test.cc | 3 +-- .../filters/http/original_src/original_src_test.cc | 9 +++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/test/extensions/filters/http/original_src/BUILD b/test/extensions/filters/http/original_src/BUILD index 7360ac19db62b..851df2edd9fe5 100644 --- a/test/extensions/filters/http/original_src/BUILD +++ b/test/extensions/filters/http/original_src/BUILD @@ -44,8 +44,8 @@ envoy_extension_cc_test( "//source/extensions/filters/http/original_src:original_src_lib", "//test/mocks:common_lib", "//test/mocks/buffer:buffer_mocks", - "//test/mocks/network:network_mocks", "//test/mocks/http:http_mocks", + "//test/mocks/network:network_mocks", "//test/test_common:printers_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/filter/http/original_src/v2alpha1:original_src_cc", diff --git a/test/extensions/filters/http/original_src/original_src_config_factory_test.cc b/test/extensions/filters/http/original_src/original_src_config_factory_test.cc index 262d45ea97dc7..3912a96efe623 100644 --- a/test/extensions/filters/http/original_src/original_src_config_factory_test.cc +++ b/test/extensions/filters/http/original_src/original_src_config_factory_test.cc @@ -29,8 +29,7 @@ TEST(OriginalSrcConfigFactoryTest, TestCreateFactory) { Server::Configuration::MockFactoryContext context; - Http::FilterFactoryCb cb = - factory.createFilterFactoryFromProto(*proto_config, "", context); + Http::FilterFactoryCb cb = factory.createFilterFactoryFromProto(*proto_config, "", context); Http::MockFilterChainFactoryCallbacks filter_callback; Http::StreamDecoderFilterSharedPtr added_filter; diff --git a/test/extensions/filters/http/original_src/original_src_test.cc b/test/extensions/filters/http/original_src/original_src_test.cc index bc82fa510c8af..abd3398f742f5 100644 --- a/test/extensions/filters/http/original_src/original_src_test.cc +++ b/test/extensions/filters/http/original_src/original_src_test.cc @@ -7,8 +7,8 @@ #include "test/mocks/buffer/mocks.h" #include "test/mocks/common.h" -#include "test/mocks/network/mocks.h" #include "test/mocks/http/mocks.h" +#include "test/mocks/network/mocks.h" #include "test/test_common/printers.h" #include "test/test_common/utility.h" @@ -32,7 +32,8 @@ class OriginalSrcTest : public testing::Test { return makeFilterWithCallbacks(callbacks_); } - std::unique_ptr makeFilterWithCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) { + std::unique_ptr + makeFilterWithCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) { Config default_config; auto filter = std::make_unique(default_config); filter->setDecoderFilterCallbacks(callbacks); @@ -59,7 +60,6 @@ class OriginalSrcTest : public testing::Test { NiceMock socket_; Http::TestHeaderMapImpl headers_; - absl::optional findOptionDetails(const Network::Socket::Options& options, Network::SocketOptionName name, envoy::api::v2::core::SocketOption::SocketState state) { @@ -95,7 +95,8 @@ TEST_F(OriginalSrcTest, decodeHeadersIpv4AddressAddsOption) { ASSERT_NE(options->at(0), nullptr); NiceMock socket; - EXPECT_CALL(socket, setLocalAddress(PointeesEq(callbacks_.stream_info_.downstream_remote_address_))); + EXPECT_CALL(socket, + setLocalAddress(PointeesEq(callbacks_.stream_info_.downstream_remote_address_))); options->at(0)->setOption(socket, envoy::api::v2::core::SocketOption::STATE_PREBIND); } From 682536d3e0333fad431f04ae6ac35af0dbb0c1eb Mon Sep 17 00:00:00 2001 From: Kyle Larose Date: Thu, 2 May 2019 10:51:40 -0400 Subject: [PATCH 07/19] hook upstreamSocketOptions into httpConnPool We use the existing mechanism used to pass socket options to the connection pool creation logic to pass the upstream socket options. We do this by creating a new socket options object, then placing both sets of socket options into it. Signed-off-by: Kyle Larose --- .../common/upstream/cluster_manager_impl.cc | 36 +++++--- .../upstream/cluster_manager_impl_test.cc | 92 +++++++++++++++++-- test/mocks/upstream/load_balancer_context.cc | 10 +- test/mocks/upstream/load_balancer_context.h | 5 +- 4 files changed, 118 insertions(+), 25 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index c73bf5b639c92..44e1eb33e1b1d 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -40,6 +40,16 @@ namespace Envoy { namespace Upstream { +namespace { + +void addOptionsIfNotNull(Network::Socket::OptionsSharedPtr& options, + const Network::Socket::OptionsSharedPtr& to_add) { + if (to_add) { + Network::Socket::appendOptions(options, to_add); + } +} + +} // namespace void ClusterManagerInitHelper::addCluster(Cluster& cluster) { // See comments in ClusterManagerImpl::addOrUpdateCluster() for why this is only called during @@ -1133,22 +1143,22 @@ ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::connPool( return nullptr; } - // Inherit socket options from downstream connection, if set. std::vector hash_key = {uint8_t(protocol)}; - // Use downstream connection socket options for computing connection pool hash key, if any. + Network::Socket::OptionsSharedPtr upstream_options(std::make_shared()); + if (context) { + // Inherit socket options from downstream connection, if set. + if (context->downstreamConnection()) { + addOptionsIfNotNull(upstream_options, context->downstreamConnection()->socketOptions()); + } + addOptionsIfNotNull(upstream_options, context->upstreamSocketOptions()); + } + + // Use the socket options for computing connection pool hash key, if any. // This allows socket options to control connection pooling so that connections with // different options are not pooled together. - bool have_options = false; - if (context && context->downstreamConnection()) { - const Network::ConnectionSocket::OptionsSharedPtr& options = - context->downstreamConnection()->socketOptions(); - if (options) { - for (const auto& option : *options) { - have_options = true; - option->hashKey(hash_key); - } - } + for (const auto& option : *upstream_options) { + option->hashKey(hash_key); } ConnPoolsContainer& container = *parent_.getHttpConnPoolsContainer(host, true); @@ -1159,7 +1169,7 @@ ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::connPool( container.pools_->getPool(priority, hash_key, [&]() { return parent_.parent_.factory_.allocateConnPool( parent_.thread_local_dispatcher_, host, priority, protocol, - have_options ? context->downstreamConnection()->socketOptions() : nullptr); + !upstream_options->empty() ? upstream_options : nullptr); }); if (pool.has_value()) { diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index e60a51b00ed02..4eec09eeb2668 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -9,6 +9,7 @@ #include "common/config/bootstrap_json.h" #include "common/config/utility.h" #include "common/http/context_impl.h" +#include "common/network/socket_option_factory.h" #include "common/network/socket_option_impl.h" #include "common/network/transport_socket_options_impl.h" #include "common/network/utility.h" @@ -71,8 +72,8 @@ class TestClusterManagerFactory : public ClusterManagerFactory { Http::ConnectionPool::InstancePtr allocateConnPool(Event::Dispatcher&, HostConstSharedPtr host, ResourcePriority, Http::Protocol, - const Network::ConnectionSocket::OptionsSharedPtr&) override { - return Http::ConnectionPool::InstancePtr{allocateConnPool_(host)}; + const Network::ConnectionSocket::OptionsSharedPtr& options) override { + return Http::ConnectionPool::InstancePtr{allocateConnPool_(host, options)}; } Tcp::ConnectionPool::InstancePtr @@ -101,7 +102,9 @@ class TestClusterManagerFactory : public ClusterManagerFactory { MOCK_METHOD1(clusterManagerFromProto_, ClusterManager*(const envoy::config::bootstrap::v2::Bootstrap& bootstrap)); - MOCK_METHOD1(allocateConnPool_, Http::ConnectionPool::Instance*(HostConstSharedPtr host)); + MOCK_METHOD2(allocateConnPool_, + Http::ConnectionPool::Instance*(HostConstSharedPtr host, + Network::ConnectionSocket::OptionsSharedPtr)); MOCK_METHOD1(allocateTcpConnPool_, Tcp::ConnectionPool::Instance*(HostConstSharedPtr host)); MOCK_METHOD4(clusterFromProto_, ClusterSharedPtr(const envoy::api::v2::Cluster& cluster, ClusterManager& cm, @@ -1204,7 +1207,7 @@ TEST_F(ClusterManagerImplTest, DynamicAddRemove) { EXPECT_EQ(cluster2->info_, cluster_manager_->get("fake_cluster")->info()); EXPECT_EQ(1UL, cluster_manager_->clusters().size()); Http::ConnectionPool::MockInstance* cp = new Http::ConnectionPool::MockInstance(); - EXPECT_CALL(factory_, allocateConnPool_(_)).WillOnce(Return(cp)); + EXPECT_CALL(factory_, allocateConnPool_(_, _)).WillOnce(Return(cp)); EXPECT_EQ(cp, cluster_manager_->httpConnPoolForCluster("fake_cluster", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); @@ -1359,7 +1362,7 @@ TEST_F(ClusterManagerImplTest, CloseHttpConnectionsOnHealthFailure) { })); create(parseBootstrapFromJson(json)); - EXPECT_CALL(factory_, allocateConnPool_(_)).WillOnce(Return(cp1)); + EXPECT_CALL(factory_, allocateConnPool_(_, _)).WillOnce(Return(cp1)); cluster_manager_->httpConnPoolForCluster("some_cluster", ResourcePriority::Default, Http::Protocol::Http11, nullptr); @@ -1370,7 +1373,7 @@ TEST_F(ClusterManagerImplTest, CloseHttpConnectionsOnHealthFailure) { test_host->healthFlagSet(Host::HealthFlag::FAILED_OUTLIER_CHECK); outlier_detector.runCallbacks(test_host); - EXPECT_CALL(factory_, allocateConnPool_(_)).WillOnce(Return(cp2)); + EXPECT_CALL(factory_, allocateConnPool_(_, _)).WillOnce(Return(cp2)); cluster_manager_->httpConnPoolForCluster("some_cluster", ResourcePriority::High, Http::Protocol::Http11, nullptr); } @@ -1626,7 +1629,7 @@ TEST_F(ClusterManagerImplTest, DynamicHostRemove) { EXPECT_CALL(initialized, ready()); cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); }); - EXPECT_CALL(factory_, allocateConnPool_(_)) + EXPECT_CALL(factory_, allocateConnPool_(_, _)) .Times(4) .WillRepeatedly(ReturnNew()); @@ -1790,7 +1793,7 @@ TEST_F(ClusterManagerImplTest, DynamicHostRemoveWithTls) { EXPECT_CALL(initialized, ready()); cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); }); - EXPECT_CALL(factory_, allocateConnPool_(_)) + EXPECT_CALL(factory_, allocateConnPool_(_, _)) .Times(4) .WillRepeatedly(ReturnNew()); @@ -1980,7 +1983,7 @@ TEST_F(ClusterManagerImplTest, DynamicHostRemoveDefaultPriority) { dns_callback(TestUtility::makeDnsResponse({"127.0.0.2"})); - EXPECT_CALL(factory_, allocateConnPool_(_)) + EXPECT_CALL(factory_, allocateConnPool_(_, _)) .WillOnce(ReturnNew()); EXPECT_CALL(factory_, allocateTcpConnPool_(_)) @@ -2057,7 +2060,7 @@ TEST_F(ClusterManagerImplTest, ConnPoolDestroyWithDraining) { dns_callback(TestUtility::makeDnsResponse({"127.0.0.2"})); MockConnPoolWithDestroy* mock_cp = new MockConnPoolWithDestroy(); - EXPECT_CALL(factory_, allocateConnPool_(_)).WillOnce(Return(mock_cp)); + EXPECT_CALL(factory_, allocateConnPool_(_, _)).WillOnce(Return(mock_cp)); MockTcpConnPoolWithDestroy* mock_tcp = new MockTcpConnPoolWithDestroy(); EXPECT_CALL(factory_, allocateTcpConnPool_(_)).WillOnce(Return(mock_tcp)); @@ -2488,6 +2491,75 @@ TEST_F(ClusterManagerImplTest, MergedUpdatesDestroyedOnUpdate) { EXPECT_EQ(0, factory_.stats_.gauge("cluster_manager.warming_clusters").value()); } +TEST_F(ClusterManagerImplTest, UpstreamSocketOptionsPassedToConnPool) { + createWithLocalClusterUpdate(); + NiceMock context; + + Http::ConnectionPool::MockInstance* to_create = new Http::ConnectionPool::MockInstance(); + Network::Socket::OptionsSharedPtr options_to_return = + std::move(Network::SocketOptionFactory::buildIpTransparentOptions()); + + EXPECT_CALL(context, upstreamSocketOptions()).WillOnce(Return(options_to_return)); + EXPECT_CALL(factory_, allocateConnPool_(_, _)).WillOnce(Return(to_create)); + + Http::ConnectionPool::Instance* cp = cluster_manager_->httpConnPoolForCluster( + "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, &context); + + EXPECT_NE(nullptr, cp); +} + +TEST_F(ClusterManagerImplTest, UpstreamSocketOptionsUsedInConnPoolHash) { + createWithLocalClusterUpdate(); + NiceMock context1; + NiceMock context2; + + Http::ConnectionPool::MockInstance* to_create1 = new Http::ConnectionPool::MockInstance(); + Http::ConnectionPool::MockInstance* to_create2 = new Http::ConnectionPool::MockInstance(); + Network::Socket::OptionsSharedPtr options1 = + std::move(Network::SocketOptionFactory::buildIpTransparentOptions()); + Network::Socket::OptionsSharedPtr options2 = + std::move(Network::SocketOptionFactory::buildSocketMarkOptions(3)); + + EXPECT_CALL(context1, upstreamSocketOptions()).WillRepeatedly(Return(options1)); + EXPECT_CALL(context2, upstreamSocketOptions()).WillRepeatedly(Return(options2)); + EXPECT_CALL(factory_, allocateConnPool_(_, _)).WillOnce(Return(to_create1)); + + Http::ConnectionPool::Instance* cp1 = cluster_manager_->httpConnPoolForCluster( + "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, &context1); + + EXPECT_CALL(factory_, allocateConnPool_(_, _)).WillOnce(Return(to_create2)); + Http::ConnectionPool::Instance* cp2 = cluster_manager_->httpConnPoolForCluster( + "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, &context2); + + Http::ConnectionPool::Instance* should_be_cp1 = cluster_manager_->httpConnPoolForCluster( + "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, &context1); + Http::ConnectionPool::Instance* should_be_cp2 = cluster_manager_->httpConnPoolForCluster( + "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, &context2); + + // The different upstream options should lead to different hashKeys, thus different pools + EXPECT_NE(cp1, cp2); + + // reusing the same options should lead to the same connection pools. + EXPECT_EQ(cp1, should_be_cp1); + EXPECT_EQ(cp2, should_be_cp2); +} + +TEST_F(ClusterManagerImplTest, UpstreamSocketOptionsNullIsOkay) { + createWithLocalClusterUpdate(); + NiceMock context; + + Http::ConnectionPool::MockInstance* to_create = new Http::ConnectionPool::MockInstance(); + Network::Socket::OptionsSharedPtr options_to_return = nullptr; + + EXPECT_CALL(context, upstreamSocketOptions()).WillOnce(Return(options_to_return)); + EXPECT_CALL(factory_, allocateConnPool_(_, _)).WillOnce(Return(to_create)); + + Http::ConnectionPool::Instance* cp = cluster_manager_->httpConnPoolForCluster( + "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, &context); + + EXPECT_NE(nullptr, cp); +} + class ClusterManagerInitHelperTest : public testing::Test { public: MOCK_METHOD1(onClusterInit, void(Cluster& cluster)); diff --git a/test/mocks/upstream/load_balancer_context.cc b/test/mocks/upstream/load_balancer_context.cc index 5a07100bc6642..5a424b07b8672 100644 --- a/test/mocks/upstream/load_balancer_context.cc +++ b/test/mocks/upstream/load_balancer_context.cc @@ -1,9 +1,17 @@ #include "test/mocks/upstream/load_balancer_context.h" +using testing::_; +using testing::ReturnRef; + namespace Envoy { namespace Upstream { -MockLoadBalancerContext::MockLoadBalancerContext() = default; +MockLoadBalancerContext::MockLoadBalancerContext() { + // By default, set loads which treat everything as healthy in the first priority. + priority_load_.healthy_priority_load_ = HealthyLoad({100}); + priority_load_.degraded_priority_load_ = DegradedLoad({0}); + ON_CALL(*this, determinePriorityLoad(_, _)).WillByDefault(ReturnRef(priority_load_)); +} MockLoadBalancerContext::~MockLoadBalancerContext() = default; diff --git a/test/mocks/upstream/load_balancer_context.h b/test/mocks/upstream/load_balancer_context.h index 13221f30e41c1..495da74935a4a 100644 --- a/test/mocks/upstream/load_balancer_context.h +++ b/test/mocks/upstream/load_balancer_context.h @@ -18,7 +18,10 @@ class MockLoadBalancerContext : public LoadBalancerContext { const HealthyAndDegradedLoad&(const PrioritySet&, const HealthyAndDegradedLoad&)); MOCK_METHOD1(shouldSelectAnotherHost, bool(const Host&)); MOCK_CONST_METHOD0(hostSelectionRetryCount, uint32_t()); - MOCK_CONST_METHOD0(usptreamSocketOptions, Network::Socket::OptionsSharedPtr()); + MOCK_CONST_METHOD0(upstreamSocketOptions, Network::Socket::OptionsSharedPtr()); + +private: + HealthyAndDegradedLoad priority_load_; }; } // namespace Upstream From 818e307203e3da4afdfa36a4cb923b42c41f2905 Mon Sep 17 00:00:00 2001 From: Kyle Larose Date: Thu, 2 May 2019 13:56:12 -0400 Subject: [PATCH 08/19] Move upstream options to ActiveStream The active stream is what's shared between the various filters. Push the options up to it so they're actually shared! Signed-off-by: Kyle Larose --- source/common/http/conn_manager_impl.cc | 3 ++- source/common/http/conn_manager_impl.h | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 286ee7855f67b..f57081456981b 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -403,7 +403,8 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect stream_id_(connection_manager.random_generator_.random()), request_response_timespan_(new Stats::Timespan( connection_manager_.stats_.named_.downstream_rq_time_, connection_manager_.timeSource())), - stream_info_(connection_manager_.codec_->protocol(), connection_manager_.timeSource()) { + stream_info_(connection_manager_.codec_->protocol(), connection_manager_.timeSource()), + upstream_options_(std::make_shared()) { connection_manager_.stats_.named_.downstream_rq_total_.inc(); connection_manager_.stats_.named_.downstream_rq_active_.inc(); if (connection_manager_.codec_->protocol() == Protocol::Http2) { diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 60d233b0e456c..e87f4846b2c55 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -243,11 +243,11 @@ class ConnectionManagerImpl : Logger::Loggable, bool recreateStream() override; void addUpstreamSocketOptions(const Network::Socket::OptionsSharedPtr& options) override { - Network::Socket::appendOptions(upstream_options_, options); + Network::Socket::appendOptions(parent_.upstream_options_, options); } Network::Socket::OptionsSharedPtr getUpstreamSocketOptions() const override { - return upstream_options_; + return parent_.upstream_options_; } // Each decoder filter instance checks if the request passed to the filter is gRPC @@ -267,7 +267,6 @@ class ConnectionManagerImpl : Logger::Loggable, StreamDecoderFilterSharedPtr handle_; bool is_grpc_request_{}; - Network::Socket::OptionsSharedPtr upstream_options_; }; typedef std::unique_ptr ActiveStreamDecoderFilterPtr; @@ -525,6 +524,7 @@ class ConnectionManagerImpl : Logger::Loggable, // Whether a filter has indicated that the response should be treated as a headers only // response. bool encoding_headers_only_{}; + Network::Socket::OptionsSharedPtr upstream_options_; }; typedef std::unique_ptr ActiveStreamPtr; From 1be9292d6d266a07f34b92baefec9a4106e2c339 Mon Sep 17 00:00:00 2001 From: Kyle Larose Date: Thu, 2 May 2019 15:22:13 -0400 Subject: [PATCH 09/19] document original_src http filter Signed-off-by: Kyle Larose --- api/docs/BUILD | 1 + docs/build.sh | 1 + docs/root/api-v2/config/filter/http/http.rst | 1 + .../http_filters/http_filters.rst | 1 + .../http_filters/original_src_filter.rst | 65 +++++++++++++++++++ .../intro/arch_overview/ip_transparency.rst | 26 +++++++- 6 files changed, 94 insertions(+), 1 deletion(-) create mode 100644 docs/root/configuration/http_filters/original_src_filter.rst diff --git a/api/docs/BUILD b/api/docs/BUILD index b6840bd844a5c..2b0ded7c5126b 100644 --- a/api/docs/BUILD +++ b/api/docs/BUILD @@ -49,6 +49,7 @@ proto_library( "//envoy/config/filter/http/rate_limit/v2:rate_limit", "//envoy/config/filter/http/rbac/v2:rbac", "//envoy/config/filter/http/router/v2:router", + "//envoy/config/filter/http/original_src/v2alpha1:original_src", "//envoy/config/filter/http/squash/v2:squash", "//envoy/config/filter/http/tap/v2alpha:tap", "//envoy/config/filter/http/transcoder/v2:transcoder", diff --git a/docs/build.sh b/docs/build.sh index 3ebbb94f4e51a..638bd4b35f789 100755 --- a/docs/build.sh +++ b/docs/build.sh @@ -100,6 +100,7 @@ PROTO_RST=" /envoy/config/filter/http/ip_tagging/v2/ip_tagging/envoy/config/filter/http/ip_tagging/v2/ip_tagging.proto.rst /envoy/config/filter/http/jwt_authn/v2alpha/jwt_authn/envoy/config/filter/http/jwt_authn/v2alpha/config.proto.rst /envoy/config/filter/http/lua/v2/lua/envoy/config/filter/http/lua/v2/lua.proto.rst + /envoy/config/filter/http/original_src/v2alpha1/original_src/envoy/config/filter/http/original_src/v2alpha1/original_src.proto.rst /envoy/config/filter/http/rate_limit/v2/rate_limit/envoy/config/filter/http/rate_limit/v2/rate_limit.proto.rst /envoy/config/filter/http/rbac/v2/rbac/envoy/config/filter/http/rbac/v2/rbac.proto.rst /envoy/config/filter/http/router/v2/router/envoy/config/filter/http/router/v2/router.proto.rst diff --git a/docs/root/api-v2/config/filter/http/http.rst b/docs/root/api-v2/config/filter/http/http.rst index 86e82f875a2fa..0aff5791bf6aa 100644 --- a/docs/root/api-v2/config/filter/http/http.rst +++ b/docs/root/api-v2/config/filter/http/http.rst @@ -7,3 +7,4 @@ HTTP filters */v2/* */v2alpha/* + */v2alpha1/* diff --git a/docs/root/configuration/http_filters/http_filters.rst b/docs/root/configuration/http_filters/http_filters.rst index 5f0c00275d80b..5fcbedd731ed8 100644 --- a/docs/root/configuration/http_filters/http_filters.rst +++ b/docs/root/configuration/http_filters/http_filters.rst @@ -22,6 +22,7 @@ HTTP filters ip_tagging_filter jwt_authn_filter lua_filter + original_src_filter rate_limit_filter rbac_filter router_filter diff --git a/docs/root/configuration/http_filters/original_src_filter.rst b/docs/root/configuration/http_filters/original_src_filter.rst new file mode 100644 index 0000000000000..46a6b1b7ff743 --- /dev/null +++ b/docs/root/configuration/http_filters/original_src_filter.rst @@ -0,0 +1,65 @@ +.. _config_http_filters_original_src: + +Original Source +=============== + +* :ref:`HTTP filter v2 API reference ` +* This filter should be configured with the name *envoy.original_src*. + +The original source http filter replicates the downstream remote address of the connection on +the upstream side of Envoy. For example, if a downstream connection connects to Envoy with IP +address ``10.1.2.3``, then Envoy will connect to the upstream with source IP ``10.1.2.3``. The +downstream remote address is determined based on the logic for the "trusted client address" +outlined in :ref:`XFF `. + + +IP Version Support +------------------ +The filter supports both IPv4 and IPv6 as addresses. Note that the upstream connection must support +the version used. + +Extra Setup +----------- + +The downstream remote address used will likely be globally routable. By default, packets returning +from the upstream host to that address will not route through Envoy. The network must be configured +to forcefully route any traffic whose IP was replicated by Envoy back through the Envoy host. + +If Envoy and the upstream are on the same host -- e.g. in an sidecar deployment --, then iptables +and routing rules can be used to ensure correct behaviour. The filter has an unsigned integer +configuration, +:ref:`mark `. Setting +this to *X* causes Envoy to *mark* all upstream packets originating from this http with value +*X*. Note that if +:ref:`mark ` is set +to 0, Envoy will not mark upstream packets. + +We can use the following set of commands to ensure that all ipv4 and ipv6 traffic marked with *X* +(assumed to be 123 in the example) routes correctly. Note that this example assumes that *eth0* is +the default outbound interface. + +.. code-block:: text + + iptables -t mangle -I PREROUTING -m mark --mark 123 -j CONNMARK --save-mark + iptables -t mangle -I OUTPUT -m connmark --mark 123 -j CONNMARK --restore-mark + ip6tables -t mangle -I PREROUTING -m mark --mark 123 -j CONNMARK --save-mark + ip6tables -t mangle -I OUTPUT -m connmark --mark 123 -j CONNMARK --restore-mark + ip rule add fwmark 123 lookup 100 + ip route add local 0.0.0.0/0 dev lo table 100 + ip -6 rule add fwmark 123 lookup 100 + ip -6 route add local ::/0 dev lo table 100 + echo 1 > /proc/sys/net/ipv4/conf/eth0/route_localnet + + +Example HTTP configuration +------------------------------ + +The following example configures Envoy to use the original source for all connections made on port +8888. All upstream packets are marked with 123. + +.. code-block:: yaml + + http_filters: + - name: envoy..original_src + config: + mark: 123 diff --git a/docs/root/intro/arch_overview/ip_transparency.rst b/docs/root/intro/arch_overview/ip_transparency.rst index 48b442eca7dda..58a9c35126884 100644 --- a/docs/root/intro/arch_overview/ip_transparency.rst +++ b/docs/root/intro/arch_overview/ip_transparency.rst @@ -25,7 +25,9 @@ HTTP Headers HTTP headers may carry the original IP address of the request in the :ref:`x-forwarded-for ` header. The upstream server -can use this header to determine the downstream remote address. +can use this header to determine the downstream remote address. Envoy may also use this header to +choose the IP address used by the +:ref:`Original Src HTTP Filter `. The HTTP header approach has a few downsides: @@ -69,3 +71,25 @@ Some drawbacks to the Original Source filter: * It requires that Envoy have access to the downstream remote address. * Its configuration is relatively complex. * It may introduce a slight performance hit due to restrictions on connection pooling. + +.. _arch_overview_ip_transparency_original_src_http: + +Original Source HTTP Filter +--------------------------- + +In controlled deployments, it may be possible to replicate the downstream remote address on the +upstream connection by using a +:ref:`Original Source HTTP filter `. This filter operates much like +the :ref:`Original Src Listener Filter `. The +main difference is that it can infer the original source address from HTTP headers, which is important +for cases where a single downstream connection carries multiple HTTP requests from different original +source addresses. +This filter will work with any upstream HTTP host. However, it requires fairly complex configuration, +and it may not be supported in all deployments due to routing constraints. + +Some drawbacks to the Original Source filter: + +* It requires that Envoy be properly configured to extract the downstream remote address from the + :ref:`x-forwarded-for ` header. +* Its configuration is relatively complex. +* It may introduce a slight performance hit due to restrictions on connection pooling. From 918a5c6051fed3a18c3a32578fff17356e0c77f6 Mon Sep 17 00:00:00 2001 From: Kyle Larose Date: Thu, 2 May 2019 15:24:25 -0400 Subject: [PATCH 10/19] Various fixes - build - docs Signed-off-by: Kyle Larose --- api/docs/BUILD | 2 +- docs/root/configuration/http_filters/original_src_filter.rst | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/docs/BUILD b/api/docs/BUILD index 2b0ded7c5126b..6d962ecbce9bf 100644 --- a/api/docs/BUILD +++ b/api/docs/BUILD @@ -46,10 +46,10 @@ proto_library( "//envoy/config/filter/http/ip_tagging/v2:ip_tagging", "//envoy/config/filter/http/jwt_authn/v2alpha:jwt_authn", "//envoy/config/filter/http/lua/v2:lua", + "//envoy/config/filter/http/original_src/v2alpha1:original_src", "//envoy/config/filter/http/rate_limit/v2:rate_limit", "//envoy/config/filter/http/rbac/v2:rbac", "//envoy/config/filter/http/router/v2:router", - "//envoy/config/filter/http/original_src/v2alpha1:original_src", "//envoy/config/filter/http/squash/v2:squash", "//envoy/config/filter/http/tap/v2alpha:tap", "//envoy/config/filter/http/transcoder/v2:transcoder", diff --git a/docs/root/configuration/http_filters/original_src_filter.rst b/docs/root/configuration/http_filters/original_src_filter.rst index 46a6b1b7ff743..29171918bfb6b 100644 --- a/docs/root/configuration/http_filters/original_src_filter.rst +++ b/docs/root/configuration/http_filters/original_src_filter.rst @@ -4,7 +4,7 @@ Original Source =============== * :ref:`HTTP filter v2 API reference ` -* This filter should be configured with the name *envoy.original_src*. +* This filter should be configured with the name *envoy.filters.http.original_src*. The original source http filter replicates the downstream remote address of the connection on the upstream side of Envoy. For example, if a downstream connection connects to Envoy with IP @@ -60,6 +60,6 @@ The following example configures Envoy to use the original source for all connec .. code-block:: yaml http_filters: - - name: envoy..original_src + - name: envoy.filters.http.original_src config: mark: 123 From 04480a09de3731451937384301d549bc50e4675e Mon Sep 17 00:00:00 2001 From: Kyle Larose Date: Thu, 2 May 2019 16:50:27 -0400 Subject: [PATCH 11/19] add release note Signed-off-by: Kyle Larose --- docs/root/intro/version_history.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 6fe432f0139f2..0e1960d04e560 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -12,6 +12,7 @@ Version history * hot restart: stats are no longer shared between hot restart parent/child via shared memory, but rather by RPC. Hot restart version incremented to 11. * http: mitigated a race condition with the :ref:`delayed_close_timeout` where it could trigger while actively flushing a pending write buffer for a downstream connection. * jwt_authn: make filter's parsing of JWT more flexible, allowing syntax like ``jwt=eyJhbGciOiJS...ZFnFIw,extra=7,realm=123`` +* original_src filter: added the :ref:`filter`. * redis: added :ref:`prefix routing ` to enable routing commands based on their key's prefix to different upstream. * redis: add support for zpopmax and zpopmin commands. * redis: added From b289bb6a25f4440c2ab30ceb08acae1ea55c192d Mon Sep 17 00:00:00 2001 From: Kyle Larose Date: Thu, 2 May 2019 17:28:13 -0400 Subject: [PATCH 12/19] fix compile issue not found by gcc Signed-off-by: Kyle Larose --- test/common/upstream/cluster_manager_impl_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index d93c2f54c14b2..cc5e9baf33914 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -2123,7 +2123,7 @@ TEST_F(ClusterManagerImplTest, OriginalDstInitialization) { // Tests that all the HC/weight/metadata changes are delivered in one go, as long as // there's no hosts changes in between. // Also tests that if hosts are added/removed between mergeable updates, delivery will -// happen and the scheduled update will be canceled. +// happen and the scheduled update will be cancelled. TEST_F(ClusterManagerImplTest, MergedUpdates) { createWithLocalClusterUpdate(); @@ -2495,7 +2495,7 @@ TEST_F(ClusterManagerImplTest, UpstreamSocketOptionsPassedToConnPool) { Http::ConnectionPool::MockInstance* to_create = new Http::ConnectionPool::MockInstance(); Network::Socket::OptionsSharedPtr options_to_return = - std::move(Network::SocketOptionFactory::buildIpTransparentOptions()); + Network::SocketOptionFactory::buildIpTransparentOptions(); EXPECT_CALL(context, upstreamSocketOptions()).WillOnce(Return(options_to_return)); EXPECT_CALL(factory_, allocateConnPool_(_, _)).WillOnce(Return(to_create)); @@ -2514,9 +2514,9 @@ TEST_F(ClusterManagerImplTest, UpstreamSocketOptionsUsedInConnPoolHash) { Http::ConnectionPool::MockInstance* to_create1 = new Http::ConnectionPool::MockInstance(); Http::ConnectionPool::MockInstance* to_create2 = new Http::ConnectionPool::MockInstance(); Network::Socket::OptionsSharedPtr options1 = - std::move(Network::SocketOptionFactory::buildIpTransparentOptions()); + Network::SocketOptionFactory::buildIpTransparentOptions(); Network::Socket::OptionsSharedPtr options2 = - std::move(Network::SocketOptionFactory::buildSocketMarkOptions(3)); + Network::SocketOptionFactory::buildSocketMarkOptions(3); EXPECT_CALL(context1, upstreamSocketOptions()).WillRepeatedly(Return(options1)); EXPECT_CALL(context2, upstreamSocketOptions()).WillRepeatedly(Return(options2)); From 0c294dd8d2c05d8f68be3f1106a603639b81ed39 Mon Sep 17 00:00:00 2001 From: Kyle Larose Date: Wed, 8 May 2019 16:55:20 -0400 Subject: [PATCH 13/19] review fixes Cleaned up docs and a comment Signed-off-by: Kyle Larose --- .../configuration/http_filters/original_src_filter.rst | 6 ++++++ docs/root/intro/arch_overview/ip_transparency.rst | 4 +++- source/extensions/filters/http/original_src/original_src.h | 7 ++++--- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/docs/root/configuration/http_filters/original_src_filter.rst b/docs/root/configuration/http_filters/original_src_filter.rst index 29171918bfb6b..939088d4ca4e4 100644 --- a/docs/root/configuration/http_filters/original_src_filter.rst +++ b/docs/root/configuration/http_filters/original_src_filter.rst @@ -13,6 +13,10 @@ downstream remote address is determined based on the logic for the "trusted clie outlined in :ref:`XFF `. +Note that the filter is intended to be used in conjuction with the +:ref:`Router ` filter. In particular, it must run prior to the router +filter so that it may add the desired source IP to the state of the filter chain. + IP Version Support ------------------ The filter supports both IPv4 and IPv6 as addresses. Note that the upstream connection must support @@ -63,3 +67,5 @@ The following example configures Envoy to use the original source for all connec - name: envoy.filters.http.original_src config: mark: 123 + - name: envoy.router + config: {} diff --git a/docs/root/intro/arch_overview/ip_transparency.rst b/docs/root/intro/arch_overview/ip_transparency.rst index 58a9c35126884..095ec54bc124c 100644 --- a/docs/root/intro/arch_overview/ip_transparency.rst +++ b/docs/root/intro/arch_overview/ip_transparency.rst @@ -83,7 +83,9 @@ upstream connection by using a the :ref:`Original Src Listener Filter `. The main difference is that it can infer the original source address from HTTP headers, which is important for cases where a single downstream connection carries multiple HTTP requests from different original -source addresses. +source addresses. Deployments with a front proxy forwarding to sidecar proxies are examples where case +applies. + This filter will work with any upstream HTTP host. However, it requires fairly complex configuration, and it may not be supported in all deployments due to routing constraints. diff --git a/source/extensions/filters/http/original_src/original_src.h b/source/extensions/filters/http/original_src/original_src.h index f0ca34ddaad21..871f4b48957ee 100644 --- a/source/extensions/filters/http/original_src/original_src.h +++ b/source/extensions/filters/http/original_src/original_src.h @@ -13,9 +13,10 @@ namespace HttpFilters { namespace OriginalSrc { /** - * Implements the Original Src network filter. This filter places the source address of the socket - * into an option which will alter be used to partition upstream connections. - * This does not support non-ip (e.g. AF_UNIX) connections, which will be failed and counted. + * Implements the Original Src http filter. This filter places the source address of the requested, + * as determined by the stream's `downstreamRemoteAddress()`, into an option which will be used to + * partition upstream connections. This does not support non-ip (e.g. AF_UNIX) connections; they + * will use the same address they would have had this filter not been installed. */ class OriginalSrcFilter : public Http::StreamDecoderFilter, Logger::Loggable { public: From 2df6143cc5b2c690893583142c1cd7c05da1c78c Mon Sep 17 00:00:00 2001 From: Kyle Larose Date: Wed, 8 May 2019 17:08:15 -0400 Subject: [PATCH 14/19] fix spelling Signed-off-by: Kyle Larose --- docs/root/configuration/http_filters/original_src_filter.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/configuration/http_filters/original_src_filter.rst b/docs/root/configuration/http_filters/original_src_filter.rst index 939088d4ca4e4..32120d4898f20 100644 --- a/docs/root/configuration/http_filters/original_src_filter.rst +++ b/docs/root/configuration/http_filters/original_src_filter.rst @@ -13,7 +13,7 @@ downstream remote address is determined based on the logic for the "trusted clie outlined in :ref:`XFF `. -Note that the filter is intended to be used in conjuction with the +Note that the filter is intended to be used in conjunction with the :ref:`Router ` filter. In particular, it must run prior to the router filter so that it may add the desired source IP to the state of the filter chain. From b48b79a3fac288ef89467771495b46753ab3c918 Mon Sep 17 00:00:00 2001 From: Kyle Larose Date: Thu, 9 May 2019 09:37:49 -0400 Subject: [PATCH 15/19] fix failing coverage test The test didn't like tests being named the same across modules. Signed-off-by: Kyle Larose --- .../filters/http/original_src/config_test.cc | 8 ++++---- .../original_src_config_factory_test.cc | 2 +- .../http/original_src/original_src_test.cc | 20 +++++++++---------- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/test/extensions/filters/http/original_src/config_test.cc b/test/extensions/filters/http/original_src/config_test.cc index a55bd19619922..dcfc34f3eeabe 100644 --- a/test/extensions/filters/http/original_src/config_test.cc +++ b/test/extensions/filters/http/original_src/config_test.cc @@ -15,7 +15,7 @@ namespace { // In keeping with the class under test, it would have made sense to call this ConfigTest. However, // when running coverage tests, that conflicts with tests elsewhere in the codebase. -class OriginalSrcConfigTest : public testing::Test { +class OriginalSrcHttpConfigTest : public testing::Test { public: Config makeConfigFromProto( const envoy::config::filter::http::original_src::v2alpha1::OriginalSrc& proto_config) { @@ -23,7 +23,7 @@ class OriginalSrcConfigTest : public testing::Test { } }; -TEST_F(OriginalSrcConfigTest, TestUseMark0) { +TEST_F(OriginalSrcHttpConfigTest, TestUseMark0) { envoy::config::filter::http::original_src::v2alpha1::OriginalSrc config_proto; config_proto.set_mark(0); auto config = makeConfigFromProto(config_proto); @@ -31,7 +31,7 @@ TEST_F(OriginalSrcConfigTest, TestUseMark0) { EXPECT_EQ(config.mark(), 0); } -TEST_F(OriginalSrcConfigTest, TestUseMark1234) { +TEST_F(OriginalSrcHttpConfigTest, TestUseMark1234) { envoy::config::filter::http::original_src::v2alpha1::OriginalSrc config_proto; config_proto.set_mark(1234); auto config = makeConfigFromProto(config_proto); @@ -39,7 +39,7 @@ TEST_F(OriginalSrcConfigTest, TestUseMark1234) { EXPECT_EQ(config.mark(), 1234); } -TEST_F(OriginalSrcConfigTest, TestUseMarkMax) { +TEST_F(OriginalSrcHttpConfigTest, TestUseMarkMax) { envoy::config::filter::http::original_src::v2alpha1::OriginalSrc config_proto; config_proto.set_mark(std::numeric_limits::max()); auto config = makeConfigFromProto(config_proto); diff --git a/test/extensions/filters/http/original_src/original_src_config_factory_test.cc b/test/extensions/filters/http/original_src/original_src_config_factory_test.cc index 3912a96efe623..33fcc0a33890e 100644 --- a/test/extensions/filters/http/original_src/original_src_config_factory_test.cc +++ b/test/extensions/filters/http/original_src/original_src_config_factory_test.cc @@ -18,7 +18,7 @@ namespace HttpFilters { namespace OriginalSrc { namespace { -TEST(OriginalSrcConfigFactoryTest, TestCreateFactory) { +TEST(OriginalSrcHttpConfigFactoryTest, TestCreateFactory) { std::string yaml = R"EOF( mark: 5 )EOF"; diff --git a/test/extensions/filters/http/original_src/original_src_test.cc b/test/extensions/filters/http/original_src/original_src_test.cc index abd3398f742f5..a727fcb6c1e4d 100644 --- a/test/extensions/filters/http/original_src/original_src_test.cc +++ b/test/extensions/filters/http/original_src/original_src_test.cc @@ -26,7 +26,7 @@ namespace HttpFilters { namespace OriginalSrc { namespace { -class OriginalSrcTest : public testing::Test { +class OriginalSrcHttpTest : public testing::Test { public: std::unique_ptr makeDefaultFilter() { return makeFilterWithCallbacks(callbacks_); @@ -74,14 +74,14 @@ class OriginalSrcTest : public testing::Test { } }; -TEST_F(OriginalSrcTest, onNonIpAddressDecodeSkips) { +TEST_F(OriginalSrcHttpTest, onNonIpAddressDecodeSkips) { auto filter = makeDefaultFilter(); setAddressToReturn("unix://domain.socket"); EXPECT_CALL(callbacks_, addUpstreamSocketOptions(_)).Times(0); EXPECT_EQ(filter->decodeHeaders(headers_, false), Http::FilterHeadersStatus::Continue); } -TEST_F(OriginalSrcTest, decodeHeadersIpv4AddressAddsOption) { +TEST_F(OriginalSrcHttpTest, decodeHeadersIpv4AddressAddsOption) { auto filter = makeDefaultFilter(); Network::Socket::OptionsSharedPtr options; @@ -100,7 +100,7 @@ TEST_F(OriginalSrcTest, decodeHeadersIpv4AddressAddsOption) { options->at(0)->setOption(socket, envoy::api::v2::core::SocketOption::STATE_PREBIND); } -TEST_F(OriginalSrcTest, decodeHeadersIpv4AddressUsesCorrectAddress) { +TEST_F(OriginalSrcHttpTest, decodeHeadersIpv4AddressUsesCorrectAddress) { auto filter = makeDefaultFilter(); Network::Socket::OptionsSharedPtr options; setAddressToReturn("tcp://1.2.3.4:0"); @@ -116,7 +116,7 @@ TEST_F(OriginalSrcTest, decodeHeadersIpv4AddressUsesCorrectAddress) { EXPECT_EQ(key, expected_key); } -TEST_F(OriginalSrcTest, decodeHeadersIpv4AddressBleachesPort) { +TEST_F(OriginalSrcHttpTest, decodeHeadersIpv4AddressBleachesPort) { auto filter = makeDefaultFilter(); Network::Socket::OptionsSharedPtr options; setAddressToReturn("tcp://1.2.3.4:80"); @@ -133,7 +133,7 @@ TEST_F(OriginalSrcTest, decodeHeadersIpv4AddressBleachesPort) { options->at(0)->setOption(socket, envoy::api::v2::core::SocketOption::STATE_PREBIND); } -TEST_F(OriginalSrcTest, filterAddsTransparentOption) { +TEST_F(OriginalSrcHttpTest, filterAddsTransparentOption) { if (!ENVOY_SOCKET_IP_TRANSPARENT.has_value()) { // The option isn't supported on this platform. Just skip the test. return; @@ -152,7 +152,7 @@ TEST_F(OriginalSrcTest, filterAddsTransparentOption) { EXPECT_TRUE(transparent_option.has_value()); } -TEST_F(OriginalSrcTest, filterAddsMarkOption) { +TEST_F(OriginalSrcHttpTest, filterAddsMarkOption) { if (!ENVOY_SOCKET_SO_MARK.has_value()) { // The option isn't supported on this platform. Just skip the test. return; @@ -174,7 +174,7 @@ TEST_F(OriginalSrcTest, filterAddsMarkOption) { EXPECT_EQ(value_as_bstr, mark_option->value_); } -TEST_F(OriginalSrcTest, Mark0NotAdded) { +TEST_F(OriginalSrcHttpTest, Mark0NotAdded) { if (!ENVOY_SOCKET_SO_MARK.has_value()) { // The option isn't supported on this platform. Just skip the test. return; @@ -193,7 +193,7 @@ TEST_F(OriginalSrcTest, Mark0NotAdded) { ASSERT_FALSE(mark_option.has_value()); } -TEST_F(OriginalSrcTest, decodeDataDoesNothing) { +TEST_F(OriginalSrcHttpTest, decodeDataDoesNothing) { StrictMock callbacks; auto filter = makeFilterWithCallbacks(callbacks); @@ -201,7 +201,7 @@ TEST_F(OriginalSrcTest, decodeDataDoesNothing) { EXPECT_EQ(Http::FilterDataStatus::Continue, filter->decodeData(buffer_, false)); } -TEST_F(OriginalSrcTest, decodeTrailersDoesNothing) { +TEST_F(OriginalSrcHttpTest, decodeTrailersDoesNothing) { StrictMock callbacks; auto filter = makeFilterWithCallbacks(callbacks); From cecf5e3c2a01443ab8cebd55faa493d29ee13494 Mon Sep 17 00:00:00 2001 From: Kyle Larose Date: Thu, 9 May 2019 14:20:05 -0400 Subject: [PATCH 16/19] code review fixes Comments + style issues Signed-off-by: Kyle Larose --- .../filters/http/original_src/original_src.cc | 12 +++++----- .../filters/http/original_src/original_src.h | 10 ++++---- .../http/original_src/original_src_test.cc | 24 +++++++++---------- 3 files changed, 23 insertions(+), 23 deletions(-) diff --git a/source/extensions/filters/http/original_src/original_src.cc b/source/extensions/filters/http/original_src/original_src.cc index 5c339d3904f50..ccbcded7b1138 100644 --- a/source/extensions/filters/http/original_src/original_src.cc +++ b/source/extensions/filters/http/original_src/original_src.cc @@ -14,20 +14,20 @@ OriginalSrcFilter::OriginalSrcFilter(const Config& config) : config_(config) {} void OriginalSrcFilter::onDestroy() {} Http::FilterHeadersStatus OriginalSrcFilter::decodeHeaders(Http::HeaderMap&, bool) { - auto address = callbacks_->streamInfo().downstreamRemoteAddress(); - ASSERT(address); + const auto downstream_address = callbacks_->streamInfo().downstreamRemoteAddress(); + ASSERT(downstream_address); ENVOY_LOG(debug, "Got a new connection in the original_src filter for address {}. Marking with {}", - address->asString(), config_.mark()); + downstream_address->asString(), config_.mark()); - if (address->type() != Network::Address::Type::Ip) { + if (downstream_address->type() != Network::Address::Type::Ip) { // Nothing we can do with this. return Http::FilterHeadersStatus::Continue; } - auto options_to_add = - Filters::Common::OriginalSrc::buildOriginalSrcOptions(std::move(address), config_.mark()); + const auto options_to_add = Filters::Common::OriginalSrc::buildOriginalSrcOptions( + std::move(downstream_address), config_.mark()); callbacks_->addUpstreamSocketOptions(options_to_add); return Http::FilterHeadersStatus::Continue; } diff --git a/source/extensions/filters/http/original_src/original_src.h b/source/extensions/filters/http/original_src/original_src.h index 871f4b48957ee..1754a3bb00903 100644 --- a/source/extensions/filters/http/original_src/original_src.h +++ b/source/extensions/filters/http/original_src/original_src.h @@ -13,14 +13,14 @@ namespace HttpFilters { namespace OriginalSrc { /** - * Implements the Original Src http filter. This filter places the source address of the requested, - * as determined by the stream's `downstreamRemoteAddress()`, into an option which will be used to - * partition upstream connections. This does not support non-ip (e.g. AF_UNIX) connections; they - * will use the same address they would have had this filter not been installed. + * Implements the Original Src http filter. This filter places the downstream source address of the + * request, as determined by the stream's `downstreamRemoteAddress()`, into an option which will be + * used to partition upstream connections. This does not support non-ip (e.g. AF_UNIX) connections; + * they will use the same address they would have had this filter not been installed. */ class OriginalSrcFilter : public Http::StreamDecoderFilter, Logger::Loggable { public: - OriginalSrcFilter(const Config& config); + explicit OriginalSrcFilter(const Config& config); // Http::StreamFilterBase void onDestroy() override; diff --git a/test/extensions/filters/http/original_src/original_src_test.cc b/test/extensions/filters/http/original_src/original_src_test.cc index a727fcb6c1e4d..2ef4ce9d96654 100644 --- a/test/extensions/filters/http/original_src/original_src_test.cc +++ b/test/extensions/filters/http/original_src/original_src_test.cc @@ -74,14 +74,14 @@ class OriginalSrcHttpTest : public testing::Test { } }; -TEST_F(OriginalSrcHttpTest, onNonIpAddressDecodeSkips) { +TEST_F(OriginalSrcHttpTest, OnNonIpAddressDecodeSkips) { auto filter = makeDefaultFilter(); setAddressToReturn("unix://domain.socket"); EXPECT_CALL(callbacks_, addUpstreamSocketOptions(_)).Times(0); EXPECT_EQ(filter->decodeHeaders(headers_, false), Http::FilterHeadersStatus::Continue); } -TEST_F(OriginalSrcHttpTest, decodeHeadersIpv4AddressAddsOption) { +TEST_F(OriginalSrcHttpTest, DecodeHeadersIpv4AddressAddsOption) { auto filter = makeDefaultFilter(); Network::Socket::OptionsSharedPtr options; @@ -90,7 +90,7 @@ TEST_F(OriginalSrcHttpTest, decodeHeadersIpv4AddressAddsOption) { EXPECT_EQ(filter->decodeHeaders(headers_, false), Http::FilterHeadersStatus::Continue); - // not ideal -- we're assuming that the original_src option is first, but it's a fair assumption + // Not ideal -- we're assuming that the original_src option is first, but it's a fair assumption // for now. ASSERT_NE(options->at(0), nullptr); @@ -100,7 +100,7 @@ TEST_F(OriginalSrcHttpTest, decodeHeadersIpv4AddressAddsOption) { options->at(0)->setOption(socket, envoy::api::v2::core::SocketOption::STATE_PREBIND); } -TEST_F(OriginalSrcHttpTest, decodeHeadersIpv4AddressUsesCorrectAddress) { +TEST_F(OriginalSrcHttpTest, DecodeHeadersIpv4AddressUsesCorrectAddress) { auto filter = makeDefaultFilter(); Network::Socket::OptionsSharedPtr options; setAddressToReturn("tcp://1.2.3.4:0"); @@ -108,7 +108,7 @@ TEST_F(OriginalSrcHttpTest, decodeHeadersIpv4AddressUsesCorrectAddress) { filter->decodeHeaders(headers_, false); std::vector key; - // not ideal -- we're assuming that the original_src option is first, but it's a fair assumption + // Not ideal -- we're assuming that the original_src option is first, but it's a fair assumption // for now. options->at(0)->hashKey(key); std::vector expected_key = {1, 2, 3, 4}; @@ -116,7 +116,7 @@ TEST_F(OriginalSrcHttpTest, decodeHeadersIpv4AddressUsesCorrectAddress) { EXPECT_EQ(key, expected_key); } -TEST_F(OriginalSrcHttpTest, decodeHeadersIpv4AddressBleachesPort) { +TEST_F(OriginalSrcHttpTest, DecodeHeadersIpv4AddressBleachesPort) { auto filter = makeDefaultFilter(); Network::Socket::OptionsSharedPtr options; setAddressToReturn("tcp://1.2.3.4:80"); @@ -128,12 +128,12 @@ TEST_F(OriginalSrcHttpTest, decodeHeadersIpv4AddressBleachesPort) { const auto expected_address = Network::Utility::parseInternetAddress("1.2.3.4"); EXPECT_CALL(socket, setLocalAddress(PointeesEq(expected_address))); - // not ideal -- we're assuming that the original_src option is first, but it's a fair assumption + // Not ideal -- we're assuming that the original_src option is first, but it's a fair assumption // for now. options->at(0)->setOption(socket, envoy::api::v2::core::SocketOption::STATE_PREBIND); } -TEST_F(OriginalSrcHttpTest, filterAddsTransparentOption) { +TEST_F(OriginalSrcHttpTest, FilterAddsTransparentOption) { if (!ENVOY_SOCKET_IP_TRANSPARENT.has_value()) { // The option isn't supported on this platform. Just skip the test. return; @@ -152,7 +152,7 @@ TEST_F(OriginalSrcHttpTest, filterAddsTransparentOption) { EXPECT_TRUE(transparent_option.has_value()); } -TEST_F(OriginalSrcHttpTest, filterAddsMarkOption) { +TEST_F(OriginalSrcHttpTest, FilterAddsMarkOption) { if (!ENVOY_SOCKET_SO_MARK.has_value()) { // The option isn't supported on this platform. Just skip the test. return; @@ -193,7 +193,7 @@ TEST_F(OriginalSrcHttpTest, Mark0NotAdded) { ASSERT_FALSE(mark_option.has_value()); } -TEST_F(OriginalSrcHttpTest, decodeDataDoesNothing) { +TEST_F(OriginalSrcHttpTest, DecodeDataDoesNothing) { StrictMock callbacks; auto filter = makeFilterWithCallbacks(callbacks); @@ -201,13 +201,13 @@ TEST_F(OriginalSrcHttpTest, decodeDataDoesNothing) { EXPECT_EQ(Http::FilterDataStatus::Continue, filter->decodeData(buffer_, false)); } -TEST_F(OriginalSrcHttpTest, decodeTrailersDoesNothing) { +TEST_F(OriginalSrcHttpTest, DecodeTrailersDoesNothing) { StrictMock callbacks; auto filter = makeFilterWithCallbacks(callbacks); EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter->decodeTrailers(headers_)); - // make sure the headers aren't changed at all by comparing them to the default. + // Make sure the headers haven't changed at all by comparing them to the default. EXPECT_EQ(headers_, Http::TestHeaderMapImpl()); } } // namespace From db489b3418924ac5692a4fea4d728e3fc85e32bf Mon Sep 17 00:00:00 2001 From: Kyle Larose Date: Mon, 13 May 2019 13:36:59 -0400 Subject: [PATCH 17/19] add snowp as the original_src sponsor. Signed-off-by: Kyle Larose --- CODEOWNERS | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CODEOWNERS b/CODEOWNERS index 4916bead4c8d0..15038305d4570 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -4,6 +4,10 @@ # csrf extension /*/extensions/filters/http/csrf @dschaller @mattklein123 +# original_src http filter extension +/*/extensions/filters/http/original_src @snowp +# original_src listener filter extension +/*/extensions/filters/listener/original_src @snowp # dubbo_proxy extension /*/extensions/filters/network/dubbo_proxy @zyfjeff @lizan # thrift_proxy extension From af1c9bcb0a4cb8a5ce6bf367b182c3ff5b9cacab Mon Sep 17 00:00:00 2001 From: Kyle Larose Date: Wed, 15 May 2019 08:32:57 -0400 Subject: [PATCH 18/19] Code review fixes - const nits - explicit constructor - capitalization - simplified a few tests Also added myself as a reviewer. Signed-off-by: Kyle Larose --- CODEOWNERS | 4 +- .../original_src/socket_option_factory.cc | 8 ++-- .../filters/http/original_src/config.h | 2 +- .../filters/http/original_src/original_src.cc | 8 ++-- .../original_src_config_factory_test.cc | 2 +- .../http/original_src/original_src_test.cc | 41 +++++++++---------- 6 files changed, 32 insertions(+), 33 deletions(-) diff --git a/CODEOWNERS b/CODEOWNERS index 15038305d4570..c9e496f9fceb6 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -5,9 +5,9 @@ # csrf extension /*/extensions/filters/http/csrf @dschaller @mattklein123 # original_src http filter extension -/*/extensions/filters/http/original_src @snowp +/*/extensions/filters/http/original_src @snowp @klarose # original_src listener filter extension -/*/extensions/filters/listener/original_src @snowp +/*/extensions/filters/listener/original_src @snowp @klarose # dubbo_proxy extension /*/extensions/filters/network/dubbo_proxy @zyfjeff @lizan # thrift_proxy extension diff --git a/source/extensions/filters/common/original_src/socket_option_factory.cc b/source/extensions/filters/common/original_src/socket_option_factory.cc index e3e12baf6f237..cf84b9d26596c 100644 --- a/source/extensions/filters/common/original_src/socket_option_factory.cc +++ b/source/extensions/filters/common/original_src/socket_option_factory.cc @@ -13,9 +13,9 @@ namespace OriginalSrc { Network::Socket::OptionsSharedPtr buildOriginalSrcOptions(Network::Address::InstanceConstSharedPtr source, uint32_t mark) { - auto address_without_port = Network::Utility::getAddressWithPort(*source, 0); + const auto address_without_port = Network::Utility::getAddressWithPort(*source, 0); - // note: we don't expect this to change the behaviour of the socket. We expect it to be copied + // Note: we don't expect this to change the behaviour of the socket. We expect it to be copied // into the upstream connection later. auto options_to_add = std::make_shared(); options_to_add->emplace_back( @@ -23,11 +23,11 @@ buildOriginalSrcOptions(Network::Address::InstanceConstSharedPtr source, uint32_ std::move(address_without_port))); if (mark != 0) { - auto mark_options = Network::SocketOptionFactory::buildSocketMarkOptions(mark); + const auto mark_options = Network::SocketOptionFactory::buildSocketMarkOptions(mark); options_to_add->insert(options_to_add->end(), mark_options->begin(), mark_options->end()); } - auto transparent_options = Network::SocketOptionFactory::buildIpTransparentOptions(); + const auto transparent_options = Network::SocketOptionFactory::buildIpTransparentOptions(); options_to_add->insert(options_to_add->end(), transparent_options->begin(), transparent_options->end()); return options_to_add; diff --git a/source/extensions/filters/http/original_src/config.h b/source/extensions/filters/http/original_src/config.h index 1de3d5bc4eecf..3cbe17d8a75a6 100644 --- a/source/extensions/filters/http/original_src/config.h +++ b/source/extensions/filters/http/original_src/config.h @@ -9,7 +9,7 @@ namespace OriginalSrc { class Config { public: Config() = default; - Config(const envoy::config::filter::http::original_src::v2alpha1::OriginalSrc& config); + explicit Config(const envoy::config::filter::http::original_src::v2alpha1::OriginalSrc& config); uint32_t mark() const { return mark_; } diff --git a/source/extensions/filters/http/original_src/original_src.cc b/source/extensions/filters/http/original_src/original_src.cc index ccbcded7b1138..18d71275fc655 100644 --- a/source/extensions/filters/http/original_src/original_src.cc +++ b/source/extensions/filters/http/original_src/original_src.cc @@ -17,15 +17,15 @@ Http::FilterHeadersStatus OriginalSrcFilter::decodeHeaders(Http::HeaderMap&, boo const auto downstream_address = callbacks_->streamInfo().downstreamRemoteAddress(); ASSERT(downstream_address); - ENVOY_LOG(debug, - "Got a new connection in the original_src filter for address {}. Marking with {}", - downstream_address->asString(), config_.mark()); - if (downstream_address->type() != Network::Address::Type::Ip) { // Nothing we can do with this. return Http::FilterHeadersStatus::Continue; } + ENVOY_LOG(debug, + "Got a new connection in the original_src filter for address {}. Marking with {}", + downstream_address->asString(), config_.mark()); + const auto options_to_add = Filters::Common::OriginalSrc::buildOriginalSrcOptions( std::move(downstream_address), config_.mark()); callbacks_->addUpstreamSocketOptions(options_to_add); diff --git a/test/extensions/filters/http/original_src/original_src_config_factory_test.cc b/test/extensions/filters/http/original_src/original_src_config_factory_test.cc index 33fcc0a33890e..108fcfaa8357f 100644 --- a/test/extensions/filters/http/original_src/original_src_config_factory_test.cc +++ b/test/extensions/filters/http/original_src/original_src_config_factory_test.cc @@ -19,7 +19,7 @@ namespace OriginalSrc { namespace { TEST(OriginalSrcHttpConfigFactoryTest, TestCreateFactory) { - std::string yaml = R"EOF( + const std::string yaml = R"EOF( mark: 5 )EOF"; diff --git a/test/extensions/filters/http/original_src/original_src_test.cc b/test/extensions/filters/http/original_src/original_src_test.cc index 2ef4ce9d96654..b4efb4f71cc6e 100644 --- a/test/extensions/filters/http/original_src/original_src_test.cc +++ b/test/extensions/filters/http/original_src/original_src_test.cc @@ -34,7 +34,7 @@ class OriginalSrcHttpTest : public testing::Test { std::unique_ptr makeFilterWithCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) { - Config default_config; + const Config default_config; auto filter = std::make_unique(default_config); filter->setDecoderFilterCallbacks(callbacks); return filter; @@ -44,7 +44,7 @@ class OriginalSrcHttpTest : public testing::Test { envoy::config::filter::http::original_src::v2alpha1::OriginalSrc proto_config; proto_config.set_mark(mark); - Config config(proto_config); + const Config config(proto_config); auto filter = std::make_unique(config); filter->setDecoderFilterCallbacks(callbacks_); return filter; @@ -64,7 +64,7 @@ class OriginalSrcHttpTest : public testing::Test { findOptionDetails(const Network::Socket::Options& options, Network::SocketOptionName name, envoy::api::v2::core::SocketOption::SocketState state) { for (const auto& option : options) { - auto details = option->getOptionDetails(socket_, state); + const auto details = option->getOptionDetails(socket_, state); if (details.has_value() && details->name_ == name) { return details; } @@ -90,14 +90,12 @@ TEST_F(OriginalSrcHttpTest, DecodeHeadersIpv4AddressAddsOption) { EXPECT_EQ(filter->decodeHeaders(headers_, false), Http::FilterHeadersStatus::Continue); - // Not ideal -- we're assuming that the original_src option is first, but it's a fair assumption - // for now. - ASSERT_NE(options->at(0), nullptr); - NiceMock socket; EXPECT_CALL(socket, setLocalAddress(PointeesEq(callbacks_.stream_info_.downstream_remote_address_))); - options->at(0)->setOption(socket, envoy::api::v2::core::SocketOption::STATE_PREBIND); + for (const auto& option : *options) { + option->setOption(socket, envoy::api::v2::core::SocketOption::STATE_PREBIND); + } } TEST_F(OriginalSrcHttpTest, DecodeHeadersIpv4AddressUsesCorrectAddress) { @@ -108,9 +106,10 @@ TEST_F(OriginalSrcHttpTest, DecodeHeadersIpv4AddressUsesCorrectAddress) { filter->decodeHeaders(headers_, false); std::vector key; - // Not ideal -- we're assuming that the original_src option is first, but it's a fair assumption - // for now. - options->at(0)->hashKey(key); + for (const auto& option : *options) { + option->hashKey(key); + } + std::vector expected_key = {1, 2, 3, 4}; EXPECT_EQ(key, expected_key); @@ -126,11 +125,11 @@ TEST_F(OriginalSrcHttpTest, DecodeHeadersIpv4AddressBleachesPort) { NiceMock socket; const auto expected_address = Network::Utility::parseInternetAddress("1.2.3.4"); - EXPECT_CALL(socket, setLocalAddress(PointeesEq(expected_address))); - // Not ideal -- we're assuming that the original_src option is first, but it's a fair assumption - // for now. - options->at(0)->setOption(socket, envoy::api::v2::core::SocketOption::STATE_PREBIND); + EXPECT_CALL(socket, setLocalAddress(PointeesEq(expected_address))); + for (const auto& option : *options) { + option->setOption(socket, envoy::api::v2::core::SocketOption::STATE_PREBIND); + } } TEST_F(OriginalSrcHttpTest, FilterAddsTransparentOption) { @@ -146,8 +145,8 @@ TEST_F(OriginalSrcHttpTest, FilterAddsTransparentOption) { filter->decodeHeaders(headers_, false); - auto transparent_option = findOptionDetails(*options, ENVOY_SOCKET_IP_TRANSPARENT, - envoy::api::v2::core::SocketOption::STATE_PREBIND); + const auto transparent_option = findOptionDetails( + *options, ENVOY_SOCKET_IP_TRANSPARENT, envoy::api::v2::core::SocketOption::STATE_PREBIND); EXPECT_TRUE(transparent_option.has_value()); } @@ -165,8 +164,8 @@ TEST_F(OriginalSrcHttpTest, FilterAddsMarkOption) { filter->decodeHeaders(headers_, false); - auto mark_option = findOptionDetails(*options, ENVOY_SOCKET_SO_MARK, - envoy::api::v2::core::SocketOption::STATE_PREBIND); + const auto mark_option = findOptionDetails(*options, ENVOY_SOCKET_SO_MARK, + envoy::api::v2::core::SocketOption::STATE_PREBIND); ASSERT_TRUE(mark_option.has_value()); uint32_t value = 1234; @@ -187,8 +186,8 @@ TEST_F(OriginalSrcHttpTest, Mark0NotAdded) { filter->decodeHeaders(headers_, false); - auto mark_option = findOptionDetails(*options, ENVOY_SOCKET_SO_MARK, - envoy::api::v2::core::SocketOption::STATE_PREBIND); + const auto mark_option = findOptionDetails(*options, ENVOY_SOCKET_SO_MARK, + envoy::api::v2::core::SocketOption::STATE_PREBIND); ASSERT_FALSE(mark_option.has_value()); } From befc157e3b87397a82ad3a523fc78357894a8330 Mon Sep 17 00:00:00 2001 From: Kyle Larose Date: Tue, 21 May 2019 13:42:55 -0400 Subject: [PATCH 19/19] Rework UT a bit For the trailers and decodedata tests, run through the normal flow taken by an http filter. Signed-off-by: Kyle Larose --- .../http/original_src/original_src_test.cc | 28 +++++++++++++++---- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/test/extensions/filters/http/original_src/original_src_test.cc b/test/extensions/filters/http/original_src/original_src_test.cc index b4efb4f71cc6e..c166824bfab3c 100644 --- a/test/extensions/filters/http/original_src/original_src_test.cc +++ b/test/extensions/filters/http/original_src/original_src_test.cc @@ -192,22 +192,38 @@ TEST_F(OriginalSrcHttpTest, Mark0NotAdded) { ASSERT_FALSE(mark_option.has_value()); } -TEST_F(OriginalSrcHttpTest, DecodeDataDoesNothing) { +TEST_F(OriginalSrcHttpTest, TrailersAndDataEndStreamDoNothing) { + // Use a strict mock to show that decodeData and decodeTrailers do nothing to the callback. StrictMock callbacks; auto filter = makeFilterWithCallbacks(callbacks); + // This will be invoked in decodeHeaders. + EXPECT_CALL(callbacks, addUpstreamSocketOptions(_)); + EXPECT_CALL(callbacks, streamInfo()); + callbacks.stream_info_.downstream_remote_address_ = + Network::Utility::parseInternetAddress("1.2.3.4"); + filter->decodeHeaders(headers_, true); + + // No new expectations => no side effects from calling these. EXPECT_EQ(Http::FilterDataStatus::Continue, filter->decodeData(buffer_, true)); - EXPECT_EQ(Http::FilterDataStatus::Continue, filter->decodeData(buffer_, false)); + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter->decodeTrailers(headers_)); } -TEST_F(OriginalSrcHttpTest, DecodeTrailersDoesNothing) { +TEST_F(OriginalSrcHttpTest, TrailersAndDataNotEndStreamDoNothing) { + // Use a strict mock to show that decodeData and decodeTrailers do nothing to the callback. StrictMock callbacks; auto filter = makeFilterWithCallbacks(callbacks); - EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter->decodeTrailers(headers_)); + // This will be invoked in decodeHeaders. + EXPECT_CALL(callbacks, addUpstreamSocketOptions(_)); + EXPECT_CALL(callbacks, streamInfo()); + callbacks.stream_info_.downstream_remote_address_ = + Network::Utility::parseInternetAddress("1.2.3.4"); + filter->decodeHeaders(headers_, false); - // Make sure the headers haven't changed at all by comparing them to the default. - EXPECT_EQ(headers_, Http::TestHeaderMapImpl()); + // No new expectations => no side effects from calling these. + EXPECT_EQ(Http::FilterDataStatus::Continue, filter->decodeData(buffer_, false)); + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter->decodeTrailers(headers_)); } } // namespace } // namespace OriginalSrc