From 98197b86f1dffa0d574be2618cf1f6681b6d237e Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Thu, 18 Jan 2018 22:14:32 -0800 Subject: [PATCH 01/20] Support for external authroization tcp filter. Signed-off-by: Saurabh Mohan --- source/common/filter/BUILD | 18 ++ source/common/filter/ext_authz.cc | 109 ++++++++ source/common/filter/ext_authz.h | 118 +++++++++ source/server/config/network/BUILD | 13 + source/server/config/network/ext_authz.cc | 69 +++++ source/server/config/network/ext_authz.h | 42 ++++ test/common/filter/BUILD | 20 ++ test/common/filter/ext_authz_test.cc | 290 ++++++++++++++++++++++ 8 files changed, 679 insertions(+) create mode 100644 source/common/filter/ext_authz.cc create mode 100644 source/common/filter/ext_authz.h create mode 100644 source/server/config/network/ext_authz.cc create mode 100644 source/server/config/network/ext_authz.h create mode 100644 test/common/filter/ext_authz_test.cc diff --git a/source/common/filter/BUILD b/source/common/filter/BUILD index f1eddee693ad9..69d04ee634232 100644 --- a/source/common/filter/BUILD +++ b/source/common/filter/BUILD @@ -67,3 +67,21 @@ envoy_cc_library( "//source/common/request_info:request_info_lib", ], ) + +envoy_cc_library( + name = "ext_authz_lib", + srcs = ["ext_authz.cc"], + hdrs = ["ext_authz.h"], + external_deps = ["envoy_filter_network_ext_authz"], + deps = [ + "//include/envoy/network:connection_interface", + "//include/envoy/network:filter_interface", + "//include/envoy/ext_authz:ext_authz_interface", + "//include/envoy/runtime:runtime_interface", + "//include/envoy/stats:stats_macros", + "//include/envoy/upstream:cluster_manager_interface", + "//source/common/common:assert_lib", + "//source/common/tracing:http_tracer_lib", + "//source/common/ext_authz:ext_authz_lib", + ], +) diff --git a/source/common/filter/ext_authz.cc b/source/common/filter/ext_authz.cc new file mode 100644 index 0000000000000..64aacfa707e37 --- /dev/null +++ b/source/common/filter/ext_authz.cc @@ -0,0 +1,109 @@ +#include "common/filter/ext_authz.h" + +#include +#include + +#include "common/common/assert.h" +#include "common/tracing/http_tracer_impl.h" + +#include "fmt/format.h" + +namespace Envoy { +namespace ExtAuthz { +namespace TcpFilter { + +InstanceStats Config::generateStats(const std::string& name, Stats::Scope& scope) { + std::string final_prefix = fmt::format("ext_authz.{}.", name); + return {ALL_TCP_EXT_AUTHZ_STATS(POOL_COUNTER_PREFIX(scope, final_prefix), + POOL_GAUGE_PREFIX(scope, final_prefix))}; +} + +void Instance::setCheckReqGenerator(CheckRequestGenerator *crg) +{ + ASSERT(check_req_generator_ == nullptr); + check_req_generator_ = CheckRequestGeneratorPtr{std::move(crg)}; +} + +void Instance::callCheck() { + envoy::api::v2::auth::CheckRequest request; + if (check_req_generator_ == nullptr) { + setCheckReqGenerator(new ExtAuthzCheckRequestGenerator()); + } + check_req_generator_->createTcpCheck(filter_callbacks_, request); + + status_ = Status::Calling; + filter_callbacks_->connection().readDisable(true); + config_->stats().active_.inc(); + config_->stats().total_.inc(); + + calling_check_ = true; + client_->check(*this, request, Tracing::NullSpan::instance()); + calling_check_ = false; +} + +Network::FilterStatus Instance::onData(Buffer::Instance&) { + if (status_ == Status::NotStarted) { + // If the ssl handshake was not done and data is the next event! + callCheck(); + } + return status_ == Status::Calling ? Network::FilterStatus::StopIteration + : Network::FilterStatus::Continue; +} + +Network::FilterStatus Instance::onNewConnection() { + // Wait till the next event occurs. + return Network::FilterStatus::Continue; +} + +void Instance::onEvent(Network::ConnectionEvent event) { + // Make sure that any pending request in the client is cancelled. This will be NOP if the + // request already completed. + if (event == Network::ConnectionEvent::RemoteClose || + event == Network::ConnectionEvent::LocalClose) { + if (status_ == Status::Calling) { + client_->cancel(); + config_->stats().active_.dec(); + } + } else { + // SSL connection is post TCP newConnection. Therefore the ext_authz check in onEvent. + // if the ssl handshake was successful then it will invoke the + // Network::ConnectionEvent::Connected. + if (status_ == Status::NotStarted) { + callCheck(); + } + } +} + +void Instance::complete(CheckStatus status) { + status_ = Status::Complete; + filter_callbacks_->connection().readDisable(false); + config_->stats().active_.dec(); + + switch (status) { + case CheckStatus::OK: + config_->stats().ok_.inc(); + break; + case CheckStatus::Error: + config_->stats().error_.inc(); + break; + case CheckStatus::Denied: + config_->stats().unauthz_.inc(); + break; + } + + // We fail open if there is an error contacting the service. + if (status == CheckStatus::Denied || + (status == CheckStatus::Error && !config_->failOpen())) { + config_->stats().cx_closed_.inc(); + filter_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush); + } else { + // We can get completion inline, so only call continue if that isn't happening. + if (!calling_check_) { + filter_callbacks_->continueReading(); + } + } +} + +} // namespace TcpFilter +} // namespace ExtAuthz +} // namespace Envoy diff --git a/source/common/filter/ext_authz.h b/source/common/filter/ext_authz.h new file mode 100644 index 0000000000000..3249745630f0a --- /dev/null +++ b/source/common/filter/ext_authz.h @@ -0,0 +1,118 @@ +#pragma once + +#include +#include +#include +#include + +#include "envoy/network/connection.h" +#include "envoy/network/filter.h" +#include "envoy/ext_authz/ext_authz.h" +#include "envoy/runtime/runtime.h" +#include "envoy/stats/stats_macros.h" +#include "envoy/upstream/cluster_manager.h" + +#include "api/filter/network/ext_authz.pb.h" + +#include "common/ext_authz/ext_authz_impl.h" + +namespace Envoy { +namespace ExtAuthz { +namespace TcpFilter { + +/** + * All tcp external authorization stats. @see stats_macros.h + */ +// clang-format off +#define ALL_TCP_EXT_AUTHZ_STATS(COUNTER, GAUGE) \ + COUNTER(total) \ + COUNTER(error) \ + COUNTER(unauthz) \ + COUNTER(ok) \ + COUNTER(cx_closed) \ + GAUGE (active) +// clang-format on + +/** + * Struct definition for all external authorization stats. @see stats_macros.h + */ +struct InstanceStats { + ALL_TCP_EXT_AUTHZ_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT) +}; + +/** + * Global configuration for ExtAuthz filter. + */ +class Config { +public: + // @saumoh: TBD: Take care of grpc service != envoy_grpc() + Config(const envoy::api::v2::filter::network::ExtAuthz& config, Stats::Scope& scope, + Runtime::Loader& runtime, Upstream::ClusterManager& cm) + : stats_(generateStats(config.stat_prefix(), scope)), + runtime_(runtime), cm_(cm), cluster_name_(config.grpc_service().envoy_grpc().cluster_name()), failure_mode_allow_(config.failure_mode_allow()) {} + + Runtime::Loader& runtime() { return runtime_; } + std::string cluster() { return cluster_name_; } + Upstream::ClusterManager& cm() { return cm_; } + const InstanceStats& stats() { return stats_; } + bool failOpen() const { return failure_mode_allow_; } + +private: + static InstanceStats generateStats(const std::string& name, Stats::Scope& scope); + const InstanceStats stats_; + Runtime::Loader& runtime_; + Upstream::ClusterManager& cm_; + std::string cluster_name_; + bool failure_mode_allow_; +}; + +typedef std::shared_ptr ConfigSharedPtr; + +/** + * ExtAuthz filter instance. This filter will call the Authorization service with the given + * configuration parameters. If the authorization service returns an error or a deny the + * connection will be closed without any further filters being called. Otherwise all buffered + * data will be released to further filters. + */ +class Instance : public Network::ReadFilter, + public Network::ConnectionCallbacks, + public RequestCallbacks { +public: + Instance(ConfigSharedPtr config, ClientPtr&& client) + : config_(config), client_(std::move(client)) {} + ~Instance() {} + + // Network::ReadFilter + Network::FilterStatus onData(Buffer::Instance& data) override; + Network::FilterStatus onNewConnection() override; + void initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callbacks) override { + filter_callbacks_ = &callbacks; + filter_callbacks_->connection().addConnectionCallbacks(*this); + } + + // Network::ConnectionCallbacks + void onEvent(Network::ConnectionEvent event) override; + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} + + // ExtAuthz::RequestCallbacks + void complete(CheckStatus status) override; + + void setCheckReqGenerator(CheckRequestGenerator* crg); + +private: + enum class Status { NotStarted, Calling, Complete }; + void callCheck(); + + ConfigSharedPtr config_; + ClientPtr client_; + Network::ReadFilterCallbacks* filter_callbacks_{}; + Status status_{Status::NotStarted}; + bool calling_check_{}; + CheckRequestGeneratorPtr check_req_generator_{}; +}; + +} // TcpFilter +} // namespace ExtAuthz +} // namespace Envoy + diff --git a/source/server/config/network/BUILD b/source/server/config/network/BUILD index 949b589c4262b..5fe3ec2eac823 100644 --- a/source/server/config/network/BUILD +++ b/source/server/config/network/BUILD @@ -148,3 +148,16 @@ envoy_cc_library( "//source/common/ssl:connection_lib", ], ) + +envoy_cc_library( + name = "ext_authz_lib", + srcs = ["ext_authz.cc"], + hdrs = ["ext_authz.h"], + deps = [ + "//include/envoy/registry", + "//include/envoy/server:filter_config_interface", + "//source/common/config:well_known_names", + "//source/common/filter:ext_authz_lib", + "//source/common/protobuf:utility_lib", + ], +) diff --git a/source/server/config/network/ext_authz.cc b/source/server/config/network/ext_authz.cc new file mode 100644 index 0000000000000..ab2894db7472f --- /dev/null +++ b/source/server/config/network/ext_authz.cc @@ -0,0 +1,69 @@ +#include "server/config/network/ext_authz.h" + +#include +#include + +#include "envoy/network/connection.h" +#include "envoy/registry/registry.h" +#include "envoy/ext_authz/ext_authz.h" + +#include "common/ext_authz/ext_authz_impl.h" +#include "common/filter/ext_authz.h" +#include "common/protobuf/utility.h" + +#include "api/filter/network/ext_authz.pb.validate.h" + +namespace Envoy { +namespace Server { +namespace Configuration { + +NetworkFilterFactoryCb +ExtAuthzConfigFactory::createFilter(const envoy::api::v2::filter::network::ExtAuthz& proto_config, + FactoryContext& context) { + + ASSERT(!proto_config.stat_prefix().empty()); + ASSERT(proto_config.grpc_service().has_envoy_grpc()); + ASSERT(!proto_config.grpc_service().envoy_grpc().cluster_name().empty()); + + ExtAuthz::TcpFilter::ConfigSharedPtr ext_authz_config( + new ExtAuthz::TcpFilter::Config(proto_config, context.scope(), context.runtime(), context.clusterManager())); + const uint32_t timeout_ms = PROTOBUF_GET_MS_OR_DEFAULT(proto_config.grpc_service(), timeout, 20); + +/* @saumoh: could we just create one client_factory and use it the lambda? + ExtAuthz::ClientFactoryPtr client_factory( + new ExtAuthz::GrpcFactoryImpl(ext_authz_config->cluster(), ext_authz_config->cm())); +*/ + + return [ext_authz_config, timeout_ms](Network::FilterManager& filter_manager) -> void { + ExtAuthz::GrpcFactoryImpl client_factory(ext_authz_config->cluster(), ext_authz_config->cm()); + + filter_manager.addReadFilter(Network::ReadFilterSharedPtr{ + new ExtAuthz::TcpFilter::Instance(ext_authz_config, client_factory.create(std::chrono::milliseconds(timeout_ms)))}); + }; +} + +NetworkFilterFactoryCb ExtAuthzConfigFactory::createFilterFactory(const Json::Object& json_config, + FactoryContext& context) { + envoy::api::v2::filter::network::ExtAuthz proto_config; + MessageUtil::loadFromJson(json_config.asJsonString(), proto_config); + return createFilter(proto_config, context); +} + +NetworkFilterFactoryCb +ExtAuthzConfigFactory::createFilterFactoryFromProto(const Protobuf::Message& proto_config, + FactoryContext& context) { + return createFilter( + MessageUtil::downcastAndValidate( + proto_config), + context); +} + +/** + * Static registration for the external authorization filter. @see RegisterFactory. + */ +static Registry::RegisterFactory + registered_; + +} // namespace Configuration +} // namespace Server +} // namespace Envoy diff --git a/source/server/config/network/ext_authz.h b/source/server/config/network/ext_authz.h new file mode 100644 index 0000000000000..3419bc42b9a85 --- /dev/null +++ b/source/server/config/network/ext_authz.h @@ -0,0 +1,42 @@ +#pragma once + +#include + +#include "envoy/server/filter_config.h" + +#include "common/config/well_known_names.h" + +#include "api/filter/network/ext_authz.pb.h" + +namespace Envoy { +namespace Server { +namespace Configuration { + +/** + * Config registration for the external authorization filter. @see NamedNetworkFilterConfigFactory. + */ +class ExtAuthzConfigFactory : public NamedNetworkFilterConfigFactory { +public: + // NamedNetworkFilterConfigFactory + NetworkFilterFactoryCb createFilterFactory(const Json::Object& json_config, + FactoryContext& context) override; + + NetworkFilterFactoryCb createFilterFactoryFromProto(const Protobuf::Message& proto_config, + FactoryContext& context) override; + + ProtobufTypes::MessagePtr createEmptyConfigProto() override { + return ProtobufTypes::MessagePtr{new envoy::api::v2::filter::network::ExtAuthz()}; + } + + std::string name() override { return Config::NetworkFilterNames::get().EXT_AUTHORIZATION; } + +private: + NetworkFilterFactoryCb + createFilter(const envoy::api::v2::filter::network::ExtAuthz& proto_config, + FactoryContext& context); +}; + +} // namespace Configuration +} // namespace Server +} // namespace Envoy + diff --git a/test/common/filter/BUILD b/test/common/filter/BUILD index f850a54f36ab8..3e0485af74b60 100644 --- a/test/common/filter/BUILD +++ b/test/common/filter/BUILD @@ -46,3 +46,23 @@ envoy_cc_test( "//test/mocks/upstream:upstream_mocks", ], ) + +envoy_cc_test( + name = "ext_authz_test", + srcs = ["ext_authz_test.cc"], + external_deps = ["envoy_filter_network_ext_authz"], + deps = [ + "//source/common/buffer:buffer_lib", + "//source/common/config:filter_json_lib", + "//source/common/event:dispatcher_lib", + "//source/common/filter:ext_authz_lib", + "//source/common/json:json_loader_lib", + "//source/common/protobuf:utility_lib", + "//source/common/stats:stats_lib", + "//test/mocks/network:network_mocks", + "//test/mocks/ext_authz:ext_authz_mocks", + "//test/mocks/runtime:runtime_mocks", + "//test/mocks/upstream:upstream_mocks", + "//test/mocks/tracing:tracing_mocks", + ], +) diff --git a/test/common/filter/ext_authz_test.cc b/test/common/filter/ext_authz_test.cc new file mode 100644 index 0000000000000..d513f21de8281 --- /dev/null +++ b/test/common/filter/ext_authz_test.cc @@ -0,0 +1,290 @@ +#include +#include +#include + +#include "common/buffer/buffer_impl.h" +#include "common/filter/ext_authz.h" +#include "common/json/json_loader.h" +#include "common/protobuf/utility.h" +#include "common/stats/stats_impl.h" + +#include "test/mocks/network/mocks.h" +#include "test/mocks/ext_authz/mocks.h" +#include "test/mocks/runtime/mocks.h" +#include "test/mocks/tracing/mocks.h" +#include "test/mocks/upstream/mocks.h" +#include "test/test_common/printers.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +#include "api/filter/network/ext_authz.pb.validate.h" + +using testing::InSequence; +using testing::Invoke; +using testing::NiceMock; +using testing::Return; +using testing::WithArgs; +using testing::_; + +namespace Envoy { +namespace ExtAuthz { +namespace TcpFilter { + +class ExtAuthzFilterTest : public testing::Test { +public: + ExtAuthzFilterTest() { + std::string json = R"EOF( + { + "grpc_service": { + "envoy_grpc": { "cluster_name": "ext_authz_server" } + }, + "failure_mode_allow": true, + "stat_prefix": "name" + } + )EOF"; + + envoy::api::v2::filter::network::ExtAuthz proto_config{}; + MessageUtil::loadFromJson(json, proto_config); + config_.reset(new Config(proto_config, stats_store_, runtime_, cm_)); + client_ = new MockClient(); + filter_.reset(new Instance(config_, ClientPtr{client_})); + filter_->initializeReadFilterCallbacks(filter_callbacks_); + check_req_generator_ = new NiceMock(); + filter_->setCheckReqGenerator(check_req_generator_); + + // NOP currently. + filter_->onAboveWriteBufferHighWatermark(); + filter_->onBelowWriteBufferLowWatermark(); + } + + ~ExtAuthzFilterTest() { + for (const Stats::GaugeSharedPtr& gauge : stats_store_.gauges()) { + EXPECT_EQ(0U, gauge->value()); + } + } + + + Stats::IsolatedStoreImpl stats_store_; + NiceMock runtime_; + NiceMock cm_; + NiceMock* check_req_generator_; + ConfigSharedPtr config_; + MockClient* client_; + std::unique_ptr filter_; + NiceMock filter_callbacks_; + RequestCallbacks* request_callbacks_{}; +}; + +TEST_F(ExtAuthzFilterTest, BadExtAuthzConfig) { + std::string json_string = R"EOF( + { + "stat_prefix": "my_stat_prefix", + "grpc_service": {} + } + )EOF"; + + envoy::api::v2::filter::network::ExtAuthz proto_config{}; + MessageUtil::loadFromJson(json_string, proto_config); + + EXPECT_THROW(MessageUtil::downcastAndValidate(proto_config), + ProtoValidationException); +} + +TEST_F(ExtAuthzFilterTest, OK) { + InSequence s; + + EXPECT_CALL(*check_req_generator_, createTcpCheck(_, _)); + EXPECT_CALL(filter_callbacks_.connection_, readDisable(true)); + EXPECT_CALL(*client_, check(_, _, + testing::A())) + .WillOnce(WithArgs<0>( + Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); + Buffer::OwnedImpl data("hello"); + EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data)); + EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data)); + + EXPECT_CALL(filter_callbacks_.connection_, readDisable(false)); + EXPECT_CALL(filter_callbacks_, continueReading()); + request_callbacks_->complete(CheckStatus::OK); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data)); + + EXPECT_CALL(*client_, cancel()).Times(0); + filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::LocalClose); + + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.ok").value()); +} + +TEST_F(ExtAuthzFilterTest, Denied) { + InSequence s; + + EXPECT_CALL(*check_req_generator_, createTcpCheck(_, _)); + EXPECT_CALL(filter_callbacks_.connection_, readDisable(true)); + EXPECT_CALL(*client_, check(_, _, _)) + .WillOnce(WithArgs<0>( + Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); + Buffer::OwnedImpl data("hello"); + EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data)); + + EXPECT_CALL(filter_callbacks_.connection_, readDisable(false)); + EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush)); + EXPECT_CALL(*client_, cancel()).Times(0); + request_callbacks_->complete(CheckStatus::Denied); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data)); + + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.unauthz").value()); + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.cx_closed").value()); +} + +TEST_F(ExtAuthzFilterTest, OKWithSSLConnect) { + InSequence s; + + EXPECT_CALL(*check_req_generator_, createTcpCheck(_, _)); + EXPECT_CALL(filter_callbacks_.connection_, readDisable(true)); + EXPECT_CALL(*client_, check(_, _, + testing::A())) + .WillOnce(WithArgs<0>( + Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); + // Called by SSL when the handshake is done. + filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::Connected); + Buffer::OwnedImpl data("hello"); + EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data)); + EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data)); + + EXPECT_CALL(filter_callbacks_.connection_, readDisable(false)); + EXPECT_CALL(filter_callbacks_, continueReading()); + request_callbacks_->complete(CheckStatus::OK); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data)); + + EXPECT_CALL(*client_, cancel()).Times(0); + filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::LocalClose); + + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.ok").value()); +} + +TEST_F(ExtAuthzFilterTest, DeniedWithSSLConnect) { + InSequence s; + + EXPECT_CALL(*check_req_generator_, createTcpCheck(_, _)); + EXPECT_CALL(filter_callbacks_.connection_, readDisable(true)); + EXPECT_CALL(*client_, check(_, _, _)) + .WillOnce(WithArgs<0>( + Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); + // Called by SSL when the handshake is done. + filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::Connected); + Buffer::OwnedImpl data("hello"); + EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data)); + + EXPECT_CALL(filter_callbacks_.connection_, readDisable(false)); + EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush)); + EXPECT_CALL(*client_, cancel()).Times(0); + request_callbacks_->complete(CheckStatus::Denied); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data)); + + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.unauthz").value()); + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.cx_closed").value()); +} + +TEST_F(ExtAuthzFilterTest, FailOpen) { + InSequence s; + + EXPECT_CALL(*client_, check(_, _, _)) + .WillOnce(WithArgs<0>( + Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); + Buffer::OwnedImpl data("hello"); + EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data)); + + EXPECT_CALL(filter_callbacks_.connection_, close(_)).Times(0); + EXPECT_CALL(*client_, cancel()).Times(0); + EXPECT_CALL(filter_callbacks_, continueReading()); + request_callbacks_->complete(CheckStatus::Error); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data)); + + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.unauthz").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.cx_closed").value()); +} + +TEST_F(ExtAuthzFilterTest, Error) { + InSequence s; + + EXPECT_CALL(*client_, check(_, _, _)) + .WillOnce(WithArgs<0>( + Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); + Buffer::OwnedImpl data("hello"); + EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data)); + + EXPECT_CALL(filter_callbacks_, continueReading()); + request_callbacks_->complete(CheckStatus::Error); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data)); + + EXPECT_CALL(*client_, cancel()).Times(0); + filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); + + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.error").value()); +} + +TEST_F(ExtAuthzFilterTest, Disconnect) { + InSequence s; + + EXPECT_CALL(*client_, check(_, _, _)) + .WillOnce(WithArgs<0>( + Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); + Buffer::OwnedImpl data("hello"); + EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data)); + + EXPECT_CALL(*client_, cancel()); + filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); + + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); +} + +TEST_F(ExtAuthzFilterTest, ImmediateOK) { + InSequence s; + + EXPECT_CALL(filter_callbacks_, continueReading()).Times(0); + EXPECT_CALL(*client_, check(_, _, _)) + .WillOnce(WithArgs<0>(Invoke( + [&](RequestCallbacks& callbacks) -> void { callbacks.complete(CheckStatus::OK); }))); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); + Buffer::OwnedImpl data("hello"); + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data)); + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data)); + + EXPECT_CALL(*client_, cancel()).Times(0); + filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); + + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.ok").value()); +} + +} // namespace TcpFilter +} // namespace ExtAuthz +} // namespace Envoy From 981ae9dbaa3bb365ead76446f2de024d22aa46b6 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Tue, 23 Jan 2018 17:08:49 -0800 Subject: [PATCH 02/20] Merge with gRPC singleton manager. Signed-off-by: Saurabh Mohan --- source/server/config/network/ext_authz.cc | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/source/server/config/network/ext_authz.cc b/source/server/config/network/ext_authz.cc index ab2894db7472f..c8a1ba1c5d3b1 100644 --- a/source/server/config/network/ext_authz.cc +++ b/source/server/config/network/ext_authz.cc @@ -19,7 +19,7 @@ namespace Configuration { NetworkFilterFactoryCb ExtAuthzConfigFactory::createFilter(const envoy::api::v2::filter::network::ExtAuthz& proto_config, - FactoryContext& context) { + FactoryContext& context) { ASSERT(!proto_config.stat_prefix().empty()); ASSERT(proto_config.grpc_service().has_envoy_grpc()); @@ -29,13 +29,12 @@ ExtAuthzConfigFactory::createFilter(const envoy::api::v2::filter::network::ExtAu new ExtAuthz::TcpFilter::Config(proto_config, context.scope(), context.runtime(), context.clusterManager())); const uint32_t timeout_ms = PROTOBUF_GET_MS_OR_DEFAULT(proto_config.grpc_service(), timeout, 20); -/* @saumoh: could we just create one client_factory and use it the lambda? - ExtAuthz::ClientFactoryPtr client_factory( - new ExtAuthz::GrpcFactoryImpl(ext_authz_config->cluster(), ext_authz_config->cm())); -*/ + return [grpc_service = proto_config.grpc_service(), &context, ext_authz_config, timeout_ms]( + Network::FilterManager& filter_manager) -> void { - return [ext_authz_config, timeout_ms](Network::FilterManager& filter_manager) -> void { - ExtAuthz::GrpcFactoryImpl client_factory(ext_authz_config->cluster(), ext_authz_config->cm()); + ExtAuthz::GrpcFactoryImpl client_factory(grpc_service, + context.clusterManager().grpcAsyncClientManager(), + context.scope()); filter_manager.addReadFilter(Network::ReadFilterSharedPtr{ new ExtAuthz::TcpFilter::Instance(ext_authz_config, client_factory.create(std::chrono::milliseconds(timeout_ms)))}); From 3952f6879ebdd65c1749852e13d6063d3aed667f Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Wed, 24 Jan 2018 17:05:19 -0800 Subject: [PATCH 03/20] Format fixes for tcp files. Signed-off-by: Saurabh Mohan --- source/common/filter/BUILD | 4 ++-- source/common/filter/ext_authz.cc | 8 +++----- source/common/filter/ext_authz.h | 12 ++++++------ source/server/config/network/ext_authz.cc | 20 ++++++++++---------- source/server/config/network/ext_authz.h | 6 ++---- 5 files changed, 23 insertions(+), 27 deletions(-) diff --git a/source/common/filter/BUILD b/source/common/filter/BUILD index 69d04ee634232..6663269fd64f2 100644 --- a/source/common/filter/BUILD +++ b/source/common/filter/BUILD @@ -74,14 +74,14 @@ envoy_cc_library( hdrs = ["ext_authz.h"], external_deps = ["envoy_filter_network_ext_authz"], deps = [ + "//include/envoy/ext_authz:ext_authz_interface", "//include/envoy/network:connection_interface", "//include/envoy/network:filter_interface", - "//include/envoy/ext_authz:ext_authz_interface", "//include/envoy/runtime:runtime_interface", "//include/envoy/stats:stats_macros", "//include/envoy/upstream:cluster_manager_interface", "//source/common/common:assert_lib", - "//source/common/tracing:http_tracer_lib", "//source/common/ext_authz:ext_authz_lib", + "//source/common/tracing:http_tracer_lib", ], ) diff --git a/source/common/filter/ext_authz.cc b/source/common/filter/ext_authz.cc index 64aacfa707e37..2f3ff5067b38f 100644 --- a/source/common/filter/ext_authz.cc +++ b/source/common/filter/ext_authz.cc @@ -15,11 +15,10 @@ namespace TcpFilter { InstanceStats Config::generateStats(const std::string& name, Stats::Scope& scope) { std::string final_prefix = fmt::format("ext_authz.{}.", name); return {ALL_TCP_EXT_AUTHZ_STATS(POOL_COUNTER_PREFIX(scope, final_prefix), - POOL_GAUGE_PREFIX(scope, final_prefix))}; + POOL_GAUGE_PREFIX(scope, final_prefix))}; } -void Instance::setCheckReqGenerator(CheckRequestGenerator *crg) -{ +void Instance::setCheckReqGenerator(CheckRequestGenerator* crg) { ASSERT(check_req_generator_ == nullptr); check_req_generator_ = CheckRequestGeneratorPtr{std::move(crg)}; } @@ -92,8 +91,7 @@ void Instance::complete(CheckStatus status) { } // We fail open if there is an error contacting the service. - if (status == CheckStatus::Denied || - (status == CheckStatus::Error && !config_->failOpen())) { + if (status == CheckStatus::Denied || (status == CheckStatus::Error && !config_->failOpen())) { config_->stats().cx_closed_.inc(); filter_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush); } else { diff --git a/source/common/filter/ext_authz.h b/source/common/filter/ext_authz.h index 3249745630f0a..f5f3c0f95cf9a 100644 --- a/source/common/filter/ext_authz.h +++ b/source/common/filter/ext_authz.h @@ -5,17 +5,17 @@ #include #include +#include "envoy/ext_authz/ext_authz.h" #include "envoy/network/connection.h" #include "envoy/network/filter.h" -#include "envoy/ext_authz/ext_authz.h" #include "envoy/runtime/runtime.h" #include "envoy/stats/stats_macros.h" #include "envoy/upstream/cluster_manager.h" -#include "api/filter/network/ext_authz.pb.h" - #include "common/ext_authz/ext_authz_impl.h" +#include "api/filter/network/ext_authz.pb.h" + namespace Envoy { namespace ExtAuthz { namespace TcpFilter { @@ -48,8 +48,9 @@ class Config { // @saumoh: TBD: Take care of grpc service != envoy_grpc() Config(const envoy::api::v2::filter::network::ExtAuthz& config, Stats::Scope& scope, Runtime::Loader& runtime, Upstream::ClusterManager& cm) - : stats_(generateStats(config.stat_prefix(), scope)), - runtime_(runtime), cm_(cm), cluster_name_(config.grpc_service().envoy_grpc().cluster_name()), failure_mode_allow_(config.failure_mode_allow()) {} + : stats_(generateStats(config.stat_prefix(), scope)), runtime_(runtime), cm_(cm), + cluster_name_(config.grpc_service().envoy_grpc().cluster_name()), + failure_mode_allow_(config.failure_mode_allow()) {} Runtime::Loader& runtime() { return runtime_; } std::string cluster() { return cluster_name_; } @@ -115,4 +116,3 @@ class Instance : public Network::ReadFilter, } // TcpFilter } // namespace ExtAuthz } // namespace Envoy - diff --git a/source/server/config/network/ext_authz.cc b/source/server/config/network/ext_authz.cc index c8a1ba1c5d3b1..892988b90eff1 100644 --- a/source/server/config/network/ext_authz.cc +++ b/source/server/config/network/ext_authz.cc @@ -3,9 +3,9 @@ #include #include +#include "envoy/ext_authz/ext_authz.h" #include "envoy/network/connection.h" #include "envoy/registry/registry.h" -#include "envoy/ext_authz/ext_authz.h" #include "common/ext_authz/ext_authz_impl.h" #include "common/filter/ext_authz.h" @@ -25,19 +25,19 @@ ExtAuthzConfigFactory::createFilter(const envoy::api::v2::filter::network::ExtAu ASSERT(proto_config.grpc_service().has_envoy_grpc()); ASSERT(!proto_config.grpc_service().envoy_grpc().cluster_name().empty()); - ExtAuthz::TcpFilter::ConfigSharedPtr ext_authz_config( - new ExtAuthz::TcpFilter::Config(proto_config, context.scope(), context.runtime(), context.clusterManager())); + ExtAuthz::TcpFilter::ConfigSharedPtr ext_authz_config(new ExtAuthz::TcpFilter::Config( + proto_config, context.scope(), context.runtime(), context.clusterManager())); const uint32_t timeout_ms = PROTOBUF_GET_MS_OR_DEFAULT(proto_config.grpc_service(), timeout, 20); - return [grpc_service = proto_config.grpc_service(), &context, ext_authz_config, timeout_ms]( - Network::FilterManager& filter_manager) -> void { + return [ grpc_service = proto_config.grpc_service(), &context, ext_authz_config, + timeout_ms ](Network::FilterManager & filter_manager) + ->void { - ExtAuthz::GrpcFactoryImpl client_factory(grpc_service, - context.clusterManager().grpcAsyncClientManager(), - context.scope()); + ExtAuthz::GrpcFactoryImpl client_factory( + grpc_service, context.clusterManager().grpcAsyncClientManager(), context.scope()); - filter_manager.addReadFilter(Network::ReadFilterSharedPtr{ - new ExtAuthz::TcpFilter::Instance(ext_authz_config, client_factory.create(std::chrono::milliseconds(timeout_ms)))}); + filter_manager.addReadFilter(Network::ReadFilterSharedPtr{new ExtAuthz::TcpFilter::Instance( + ext_authz_config, client_factory.create(std::chrono::milliseconds(timeout_ms)))}); }; } diff --git a/source/server/config/network/ext_authz.h b/source/server/config/network/ext_authz.h index 3419bc42b9a85..12f3de1cb6131 100644 --- a/source/server/config/network/ext_authz.h +++ b/source/server/config/network/ext_authz.h @@ -31,12 +31,10 @@ class ExtAuthzConfigFactory : public NamedNetworkFilterConfigFactory { std::string name() override { return Config::NetworkFilterNames::get().EXT_AUTHORIZATION; } private: - NetworkFilterFactoryCb - createFilter(const envoy::api::v2::filter::network::ExtAuthz& proto_config, - FactoryContext& context); + NetworkFilterFactoryCb createFilter(const envoy::api::v2::filter::network::ExtAuthz& proto_config, + FactoryContext& context); }; } // namespace Configuration } // namespace Server } // namespace Envoy - From 9da8b85a770fdf844e96330e734ecdfd15691a41 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Thu, 25 Jan 2018 12:22:47 -0800 Subject: [PATCH 04/20] Fix test tcp filter format. Signed-off-by: Saurabh Mohan --- test/common/filter/BUILD | 4 ++-- test/common/filter/ext_authz_test.cc | 15 ++++++--------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/test/common/filter/BUILD b/test/common/filter/BUILD index 3e0485af74b60..b50846f99cc96 100644 --- a/test/common/filter/BUILD +++ b/test/common/filter/BUILD @@ -59,10 +59,10 @@ envoy_cc_test( "//source/common/json:json_loader_lib", "//source/common/protobuf:utility_lib", "//source/common/stats:stats_lib", - "//test/mocks/network:network_mocks", "//test/mocks/ext_authz:ext_authz_mocks", + "//test/mocks/network:network_mocks", "//test/mocks/runtime:runtime_mocks", - "//test/mocks/upstream:upstream_mocks", "//test/mocks/tracing:tracing_mocks", + "//test/mocks/upstream:upstream_mocks", ], ) diff --git a/test/common/filter/ext_authz_test.cc b/test/common/filter/ext_authz_test.cc index d513f21de8281..23d451e0d6776 100644 --- a/test/common/filter/ext_authz_test.cc +++ b/test/common/filter/ext_authz_test.cc @@ -8,18 +8,17 @@ #include "common/protobuf/utility.h" #include "common/stats/stats_impl.h" -#include "test/mocks/network/mocks.h" #include "test/mocks/ext_authz/mocks.h" +#include "test/mocks/network/mocks.h" #include "test/mocks/runtime/mocks.h" #include "test/mocks/tracing/mocks.h" #include "test/mocks/upstream/mocks.h" #include "test/test_common/printers.h" +#include "api/filter/network/ext_authz.pb.validate.h" #include "gmock/gmock.h" #include "gtest/gtest.h" -#include "api/filter/network/ext_authz.pb.validate.h" - using testing::InSequence; using testing::Invoke; using testing::NiceMock; @@ -64,7 +63,6 @@ class ExtAuthzFilterTest : public testing::Test { } } - Stats::IsolatedStoreImpl stats_store_; NiceMock runtime_; NiceMock cm_; @@ -87,7 +85,8 @@ TEST_F(ExtAuthzFilterTest, BadExtAuthzConfig) { envoy::api::v2::filter::network::ExtAuthz proto_config{}; MessageUtil::loadFromJson(json_string, proto_config); - EXPECT_THROW(MessageUtil::downcastAndValidate(proto_config), + EXPECT_THROW(MessageUtil::downcastAndValidate( + proto_config), ProtoValidationException); } @@ -96,8 +95,7 @@ TEST_F(ExtAuthzFilterTest, OK) { EXPECT_CALL(*check_req_generator_, createTcpCheck(_, _)); EXPECT_CALL(filter_callbacks_.connection_, readDisable(true)); - EXPECT_CALL(*client_, check(_, _, - testing::A())) + EXPECT_CALL(*client_, check(_, _, testing::A())) .WillOnce(WithArgs<0>( Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); @@ -149,8 +147,7 @@ TEST_F(ExtAuthzFilterTest, OKWithSSLConnect) { EXPECT_CALL(*check_req_generator_, createTcpCheck(_, _)); EXPECT_CALL(filter_callbacks_.connection_, readDisable(true)); - EXPECT_CALL(*client_, check(_, _, - testing::A())) + EXPECT_CALL(*client_, check(_, _, testing::A())) .WillOnce(WithArgs<0>( Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); From 9c02776d4cb410c00eb73bfe758d83d07f7fa5d1 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Fri, 26 Jan 2018 14:04:05 -0800 Subject: [PATCH 05/20] Merge with data-plane-api for tcp. Signed-off-by: Saurabh Mohan --- source/common/filter/BUILD | 2 +- source/common/filter/ext_authz.cc | 2 +- source/common/filter/ext_authz.h | 3 +-- source/server/config/network/BUILD | 1 + source/server/config/network/ext_authz.cc | 3 +-- source/server/config/network/ext_authz.h | 3 +-- test/common/filter/BUILD | 2 +- test/common/filter/ext_authz_test.cc | 3 ++- 8 files changed, 9 insertions(+), 10 deletions(-) diff --git a/source/common/filter/BUILD b/source/common/filter/BUILD index 659c91240a4de..942617dfeff88 100644 --- a/source/common/filter/BUILD +++ b/source/common/filter/BUILD @@ -70,7 +70,6 @@ envoy_cc_library( name = "ext_authz_lib", srcs = ["ext_authz.cc"], hdrs = ["ext_authz.h"], - external_deps = ["envoy_filter_network_ext_authz"], deps = [ "//include/envoy/ext_authz:ext_authz_interface", "//include/envoy/network:connection_interface", @@ -81,5 +80,6 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/ext_authz:ext_authz_lib", "//source/common/tracing:http_tracer_lib", + "@envoy_api//envoy/api/v2/filter/network:ext_authz_cc", ], ) diff --git a/source/common/filter/ext_authz.cc b/source/common/filter/ext_authz.cc index 2f3ff5067b38f..0bf48ea2bf654 100644 --- a/source/common/filter/ext_authz.cc +++ b/source/common/filter/ext_authz.cc @@ -24,7 +24,7 @@ void Instance::setCheckReqGenerator(CheckRequestGenerator* crg) { } void Instance::callCheck() { - envoy::api::v2::auth::CheckRequest request; + envoy::service::auth::v2::CheckRequest request; if (check_req_generator_ == nullptr) { setCheckReqGenerator(new ExtAuthzCheckRequestGenerator()); } diff --git a/source/common/filter/ext_authz.h b/source/common/filter/ext_authz.h index f5f3c0f95cf9a..a961f4c786bb4 100644 --- a/source/common/filter/ext_authz.h +++ b/source/common/filter/ext_authz.h @@ -5,6 +5,7 @@ #include #include +#include "envoy/api/v2/filter/network/ext_authz.pb.h" #include "envoy/ext_authz/ext_authz.h" #include "envoy/network/connection.h" #include "envoy/network/filter.h" @@ -14,8 +15,6 @@ #include "common/ext_authz/ext_authz_impl.h" -#include "api/filter/network/ext_authz.pb.h" - namespace Envoy { namespace ExtAuthz { namespace TcpFilter { diff --git a/source/server/config/network/BUILD b/source/server/config/network/BUILD index 43bdf85175cb8..6129a40961b68 100644 --- a/source/server/config/network/BUILD +++ b/source/server/config/network/BUILD @@ -155,5 +155,6 @@ envoy_cc_library( "//source/common/config:well_known_names", "//source/common/filter:ext_authz_lib", "//source/common/protobuf:utility_lib", + "@envoy_api//envoy/api/v2/filter/network:ext_authz_cc", ], ) diff --git a/source/server/config/network/ext_authz.cc b/source/server/config/network/ext_authz.cc index 892988b90eff1..fe0087d75865e 100644 --- a/source/server/config/network/ext_authz.cc +++ b/source/server/config/network/ext_authz.cc @@ -3,6 +3,7 @@ #include #include +#include "envoy/api/v2/filter/network/ext_authz.pb.validate.h" #include "envoy/ext_authz/ext_authz.h" #include "envoy/network/connection.h" #include "envoy/registry/registry.h" @@ -11,8 +12,6 @@ #include "common/filter/ext_authz.h" #include "common/protobuf/utility.h" -#include "api/filter/network/ext_authz.pb.validate.h" - namespace Envoy { namespace Server { namespace Configuration { diff --git a/source/server/config/network/ext_authz.h b/source/server/config/network/ext_authz.h index 12f3de1cb6131..609a8b03a1389 100644 --- a/source/server/config/network/ext_authz.h +++ b/source/server/config/network/ext_authz.h @@ -2,12 +2,11 @@ #include +#include "envoy/api/v2/filter/network/ext_authz.pb.h" #include "envoy/server/filter_config.h" #include "common/config/well_known_names.h" -#include "api/filter/network/ext_authz.pb.h" - namespace Envoy { namespace Server { namespace Configuration { diff --git a/test/common/filter/BUILD b/test/common/filter/BUILD index b50846f99cc96..b779bc3ad132f 100644 --- a/test/common/filter/BUILD +++ b/test/common/filter/BUILD @@ -50,7 +50,6 @@ envoy_cc_test( envoy_cc_test( name = "ext_authz_test", srcs = ["ext_authz_test.cc"], - external_deps = ["envoy_filter_network_ext_authz"], deps = [ "//source/common/buffer:buffer_lib", "//source/common/config:filter_json_lib", @@ -64,5 +63,6 @@ envoy_cc_test( "//test/mocks/runtime:runtime_mocks", "//test/mocks/tracing:tracing_mocks", "//test/mocks/upstream:upstream_mocks", + "@envoy_api//envoy/api/v2/filter/network:ext_authz_cc", ], ) diff --git a/test/common/filter/ext_authz_test.cc b/test/common/filter/ext_authz_test.cc index 23d451e0d6776..a72775b136e2d 100644 --- a/test/common/filter/ext_authz_test.cc +++ b/test/common/filter/ext_authz_test.cc @@ -2,6 +2,8 @@ #include #include +#include "envoy/api/v2/filter/network/ext_authz.pb.validate.h" + #include "common/buffer/buffer_impl.h" #include "common/filter/ext_authz.h" #include "common/json/json_loader.h" @@ -15,7 +17,6 @@ #include "test/mocks/upstream/mocks.h" #include "test/test_common/printers.h" -#include "api/filter/network/ext_authz.pb.validate.h" #include "gmock/gmock.h" #include "gtest/gtest.h" From 25d37c6966e92494324cc48842e2c8f1f00d10eb Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Mon, 29 Jan 2018 15:40:49 -0800 Subject: [PATCH 06/20] Review feedback for tcp files. Signed-off-by: Saurabh Mohan --- source/common/filter/ext_authz.cc | 2 +- source/common/filter/ext_authz.h | 2 +- source/server/config/network/ext_authz.cc | 10 +++++++--- test/common/filter/ext_authz_test.cc | 14 +++++++------- 4 files changed, 16 insertions(+), 12 deletions(-) diff --git a/source/common/filter/ext_authz.cc b/source/common/filter/ext_authz.cc index 0bf48ea2bf654..7f054b2277fb4 100644 --- a/source/common/filter/ext_authz.cc +++ b/source/common/filter/ext_authz.cc @@ -73,7 +73,7 @@ void Instance::onEvent(Network::ConnectionEvent event) { } } -void Instance::complete(CheckStatus status) { +void Instance::onComplete(CheckStatus status) { status_ = Status::Complete; filter_callbacks_->connection().readDisable(false); config_->stats().active_.dec(); diff --git a/source/common/filter/ext_authz.h b/source/common/filter/ext_authz.h index a961f4c786bb4..474255e6be8ca 100644 --- a/source/common/filter/ext_authz.h +++ b/source/common/filter/ext_authz.h @@ -96,7 +96,7 @@ class Instance : public Network::ReadFilter, void onBelowWriteBufferLowWatermark() override {} // ExtAuthz::RequestCallbacks - void complete(CheckStatus status) override; + void onComplete(CheckStatus status) override; void setCheckReqGenerator(CheckRequestGenerator* crg); diff --git a/source/server/config/network/ext_authz.cc b/source/server/config/network/ext_authz.cc index fe0087d75865e..26026010929fe 100644 --- a/source/server/config/network/ext_authz.cc +++ b/source/server/config/network/ext_authz.cc @@ -32,11 +32,15 @@ ExtAuthzConfigFactory::createFilter(const envoy::api::v2::filter::network::ExtAu timeout_ms ](Network::FilterManager & filter_manager) ->void { - ExtAuthz::GrpcFactoryImpl client_factory( - grpc_service, context.clusterManager().grpcAsyncClientManager(), context.scope()); + auto async_client_factory = + context.clusterManager().grpcAsyncClientManager().factoryForGrpcService(grpc_service, + context.scope()); + auto client = std::make_unique(async_client_factory->create(), + std::chrono::milliseconds(timeout_ms)); filter_manager.addReadFilter(Network::ReadFilterSharedPtr{new ExtAuthz::TcpFilter::Instance( - ext_authz_config, client_factory.create(std::chrono::milliseconds(timeout_ms)))}); + ext_authz_config, std::move(client), + timeoutclient_factory.create(std::chrono::milliseconds(timeout_ms)))}); }; } diff --git a/test/common/filter/ext_authz_test.cc b/test/common/filter/ext_authz_test.cc index a72775b136e2d..6574b3ad5bd1d 100644 --- a/test/common/filter/ext_authz_test.cc +++ b/test/common/filter/ext_authz_test.cc @@ -107,7 +107,7 @@ TEST_F(ExtAuthzFilterTest, OK) { EXPECT_CALL(filter_callbacks_.connection_, readDisable(false)); EXPECT_CALL(filter_callbacks_, continueReading()); - request_callbacks_->complete(CheckStatus::OK); + request_callbacks_->onComplete(CheckStatus::OK); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data)); @@ -134,7 +134,7 @@ TEST_F(ExtAuthzFilterTest, Denied) { EXPECT_CALL(filter_callbacks_.connection_, readDisable(false)); EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush)); EXPECT_CALL(*client_, cancel()).Times(0); - request_callbacks_->complete(CheckStatus::Denied); + request_callbacks_->onComplete(CheckStatus::Denied); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data)); @@ -161,7 +161,7 @@ TEST_F(ExtAuthzFilterTest, OKWithSSLConnect) { EXPECT_CALL(filter_callbacks_.connection_, readDisable(false)); EXPECT_CALL(filter_callbacks_, continueReading()); - request_callbacks_->complete(CheckStatus::OK); + request_callbacks_->onComplete(CheckStatus::OK); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data)); @@ -190,7 +190,7 @@ TEST_F(ExtAuthzFilterTest, DeniedWithSSLConnect) { EXPECT_CALL(filter_callbacks_.connection_, readDisable(false)); EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush)); EXPECT_CALL(*client_, cancel()).Times(0); - request_callbacks_->complete(CheckStatus::Denied); + request_callbacks_->onComplete(CheckStatus::Denied); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data)); @@ -213,7 +213,7 @@ TEST_F(ExtAuthzFilterTest, FailOpen) { EXPECT_CALL(filter_callbacks_.connection_, close(_)).Times(0); EXPECT_CALL(*client_, cancel()).Times(0); EXPECT_CALL(filter_callbacks_, continueReading()); - request_callbacks_->complete(CheckStatus::Error); + request_callbacks_->onComplete(CheckStatus::Error); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data)); @@ -235,7 +235,7 @@ TEST_F(ExtAuthzFilterTest, Error) { EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data)); EXPECT_CALL(filter_callbacks_, continueReading()); - request_callbacks_->complete(CheckStatus::Error); + request_callbacks_->onComplete(CheckStatus::Error); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data)); @@ -269,7 +269,7 @@ TEST_F(ExtAuthzFilterTest, ImmediateOK) { EXPECT_CALL(filter_callbacks_, continueReading()).Times(0); EXPECT_CALL(*client_, check(_, _, _)) .WillOnce(WithArgs<0>(Invoke( - [&](RequestCallbacks& callbacks) -> void { callbacks.complete(CheckStatus::OK); }))); + [&](RequestCallbacks& callbacks) -> void { callbacks.onComplete(CheckStatus::OK); }))); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); Buffer::OwnedImpl data("hello"); From 5dc54958e993d4e4f8a493b03b98b10a79897169 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Mon, 29 Jan 2018 16:33:26 -0800 Subject: [PATCH 07/20] Fix up network config build. Signed-off-by: Saurabh Mohan --- source/server/config/network/ext_authz.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/source/server/config/network/ext_authz.cc b/source/server/config/network/ext_authz.cc index 26026010929fe..d808becff50a6 100644 --- a/source/server/config/network/ext_authz.cc +++ b/source/server/config/network/ext_authz.cc @@ -36,11 +36,10 @@ ExtAuthzConfigFactory::createFilter(const envoy::api::v2::filter::network::ExtAu context.clusterManager().grpcAsyncClientManager().factoryForGrpcService(grpc_service, context.scope()); - auto client = std::make_unique(async_client_factory->create(), + auto client = std::make_unique(async_client_factory->create(), std::chrono::milliseconds(timeout_ms)); filter_manager.addReadFilter(Network::ReadFilterSharedPtr{new ExtAuthz::TcpFilter::Instance( - ext_authz_config, std::move(client), - timeoutclient_factory.create(std::chrono::milliseconds(timeout_ms)))}); + ext_authz_config, std::move(client))}); }; } From ce9c1e96783384c3a2f40ec531d042facb9b75af Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Tue, 30 Jan 2018 13:30:55 -0800 Subject: [PATCH 08/20] Remove mock for proto generation: tcp changes. Signed-off-by: Saurabh Mohan --- source/common/filter/ext_authz.cc | 10 +--------- source/common/filter/ext_authz.h | 4 +--- test/common/filter/BUILD | 1 + test/common/filter/ext_authz_test.cc | 27 ++++++++++++++++++++------- 4 files changed, 23 insertions(+), 19 deletions(-) diff --git a/source/common/filter/ext_authz.cc b/source/common/filter/ext_authz.cc index 7f054b2277fb4..6b7cb210adfeb 100644 --- a/source/common/filter/ext_authz.cc +++ b/source/common/filter/ext_authz.cc @@ -18,17 +18,9 @@ InstanceStats Config::generateStats(const std::string& name, Stats::Scope& scope POOL_GAUGE_PREFIX(scope, final_prefix))}; } -void Instance::setCheckReqGenerator(CheckRequestGenerator* crg) { - ASSERT(check_req_generator_ == nullptr); - check_req_generator_ = CheckRequestGeneratorPtr{std::move(crg)}; -} - void Instance::callCheck() { envoy::service::auth::v2::CheckRequest request; - if (check_req_generator_ == nullptr) { - setCheckReqGenerator(new ExtAuthzCheckRequestGenerator()); - } - check_req_generator_->createTcpCheck(filter_callbacks_, request); + check_req_generator_.createTcpCheck(filter_callbacks_, request); status_ = Status::Calling; filter_callbacks_->connection().readDisable(true); diff --git a/source/common/filter/ext_authz.h b/source/common/filter/ext_authz.h index 474255e6be8ca..3b9e9c03b6f94 100644 --- a/source/common/filter/ext_authz.h +++ b/source/common/filter/ext_authz.h @@ -98,8 +98,6 @@ class Instance : public Network::ReadFilter, // ExtAuthz::RequestCallbacks void onComplete(CheckStatus status) override; - void setCheckReqGenerator(CheckRequestGenerator* crg); - private: enum class Status { NotStarted, Calling, Complete }; void callCheck(); @@ -109,7 +107,7 @@ class Instance : public Network::ReadFilter, Network::ReadFilterCallbacks* filter_callbacks_{}; Status status_{Status::NotStarted}; bool calling_check_{}; - CheckRequestGeneratorPtr check_req_generator_{}; + ExtAuthzCheckRequestGenerator check_req_generator_; }; } // TcpFilter diff --git a/test/common/filter/BUILD b/test/common/filter/BUILD index b779bc3ad132f..9fdd7e0d4d9ed 100644 --- a/test/common/filter/BUILD +++ b/test/common/filter/BUILD @@ -56,6 +56,7 @@ envoy_cc_test( "//source/common/event:dispatcher_lib", "//source/common/filter:ext_authz_lib", "//source/common/json:json_loader_lib", + "//source/common/network:address_lib", "//source/common/protobuf:utility_lib", "//source/common/stats:stats_lib", "//test/mocks/ext_authz:ext_authz_mocks", diff --git a/test/common/filter/ext_authz_test.cc b/test/common/filter/ext_authz_test.cc index 6574b3ad5bd1d..1c73904ce262b 100644 --- a/test/common/filter/ext_authz_test.cc +++ b/test/common/filter/ext_authz_test.cc @@ -7,6 +7,7 @@ #include "common/buffer/buffer_impl.h" #include "common/filter/ext_authz.h" #include "common/json/json_loader.h" +#include "common/network/address_impl.h" #include "common/protobuf/utility.h" #include "common/stats/stats_impl.h" @@ -24,6 +25,7 @@ using testing::InSequence; using testing::Invoke; using testing::NiceMock; using testing::Return; +using testing::ReturnRef; using testing::WithArgs; using testing::_; @@ -50,8 +52,7 @@ class ExtAuthzFilterTest : public testing::Test { client_ = new MockClient(); filter_.reset(new Instance(config_, ClientPtr{client_})); filter_->initializeReadFilterCallbacks(filter_callbacks_); - check_req_generator_ = new NiceMock(); - filter_->setCheckReqGenerator(check_req_generator_); + addr_ = std::make_shared("/test/test.sock"); // NOP currently. filter_->onAboveWriteBufferHighWatermark(); @@ -67,11 +68,11 @@ class ExtAuthzFilterTest : public testing::Test { Stats::IsolatedStoreImpl stats_store_; NiceMock runtime_; NiceMock cm_; - NiceMock* check_req_generator_; ConfigSharedPtr config_; MockClient* client_; std::unique_ptr filter_; NiceMock filter_callbacks_; + Network::Address::InstanceConstSharedPtr addr_; RequestCallbacks* request_callbacks_{}; }; @@ -94,7 +95,8 @@ TEST_F(ExtAuthzFilterTest, BadExtAuthzConfig) { TEST_F(ExtAuthzFilterTest, OK) { InSequence s; - EXPECT_CALL(*check_req_generator_, createTcpCheck(_, _)); + EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(filter_callbacks_.connection_, readDisable(true)); EXPECT_CALL(*client_, check(_, _, testing::A())) .WillOnce(WithArgs<0>( @@ -121,7 +123,8 @@ TEST_F(ExtAuthzFilterTest, OK) { TEST_F(ExtAuthzFilterTest, Denied) { InSequence s; - EXPECT_CALL(*check_req_generator_, createTcpCheck(_, _)); + EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(filter_callbacks_.connection_, readDisable(true)); EXPECT_CALL(*client_, check(_, _, _)) .WillOnce(WithArgs<0>( @@ -146,7 +149,8 @@ TEST_F(ExtAuthzFilterTest, Denied) { TEST_F(ExtAuthzFilterTest, OKWithSSLConnect) { InSequence s; - EXPECT_CALL(*check_req_generator_, createTcpCheck(_, _)); + EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(filter_callbacks_.connection_, readDisable(true)); EXPECT_CALL(*client_, check(_, _, testing::A())) .WillOnce(WithArgs<0>( @@ -175,7 +179,8 @@ TEST_F(ExtAuthzFilterTest, OKWithSSLConnect) { TEST_F(ExtAuthzFilterTest, DeniedWithSSLConnect) { InSequence s; - EXPECT_CALL(*check_req_generator_, createTcpCheck(_, _)); + EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(filter_callbacks_.connection_, readDisable(true)); EXPECT_CALL(*client_, check(_, _, _)) .WillOnce(WithArgs<0>( @@ -202,6 +207,8 @@ TEST_F(ExtAuthzFilterTest, DeniedWithSSLConnect) { TEST_F(ExtAuthzFilterTest, FailOpen) { InSequence s; + EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(*client_, check(_, _, _)) .WillOnce(WithArgs<0>( Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); @@ -226,6 +233,8 @@ TEST_F(ExtAuthzFilterTest, FailOpen) { TEST_F(ExtAuthzFilterTest, Error) { InSequence s; + EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(*client_, check(_, _, _)) .WillOnce(WithArgs<0>( Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); @@ -249,6 +258,8 @@ TEST_F(ExtAuthzFilterTest, Error) { TEST_F(ExtAuthzFilterTest, Disconnect) { InSequence s; + EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(*client_, check(_, _, _)) .WillOnce(WithArgs<0>( Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); @@ -266,6 +277,8 @@ TEST_F(ExtAuthzFilterTest, Disconnect) { TEST_F(ExtAuthzFilterTest, ImmediateOK) { InSequence s; + EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(filter_callbacks_, continueReading()).Times(0); EXPECT_CALL(*client_, check(_, _, _)) .WillOnce(WithArgs<0>(Invoke( From 40746cd58978f9909a993f0339e433e2de9d9315 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Wed, 31 Jan 2018 14:46:47 -0800 Subject: [PATCH 09/20] Review comments tcp. Signed-off-by: Saurabh Mohan --- source/common/filter/ext_authz.cc | 2 +- source/common/filter/ext_authz.h | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/source/common/filter/ext_authz.cc b/source/common/filter/ext_authz.cc index 6b7cb210adfeb..f133cbdb5a92a 100644 --- a/source/common/filter/ext_authz.cc +++ b/source/common/filter/ext_authz.cc @@ -20,7 +20,7 @@ InstanceStats Config::generateStats(const std::string& name, Stats::Scope& scope void Instance::callCheck() { envoy::service::auth::v2::CheckRequest request; - check_req_generator_.createTcpCheck(filter_callbacks_, request); + CreateCheckRequest::createTcpCheck(filter_callbacks_, request); status_ = Status::Calling; filter_callbacks_->connection().readDisable(true); diff --git a/source/common/filter/ext_authz.h b/source/common/filter/ext_authz.h index 3b9e9c03b6f94..5d12ea307937c 100644 --- a/source/common/filter/ext_authz.h +++ b/source/common/filter/ext_authz.h @@ -107,7 +107,6 @@ class Instance : public Network::ReadFilter, Network::ReadFilterCallbacks* filter_callbacks_{}; Status status_{Status::NotStarted}; bool calling_check_{}; - ExtAuthzCheckRequestGenerator check_req_generator_; }; } // TcpFilter From a675ec2026a1923281ff2cd132f03c6959f400fb Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Mon, 5 Feb 2018 11:02:50 -0800 Subject: [PATCH 10/20] Merge tcp changes with data-plane-api reorg #2495. Signed-off-by: Saurabh Mohan --- source/common/filter/BUILD | 2 +- source/common/filter/ext_authz.h | 6 +++--- source/server/config/network/BUILD | 2 +- source/server/config/network/ext_authz.cc | 22 +++++++++++----------- source/server/config/network/ext_authz.h | 9 +++++---- test/common/filter/BUILD | 2 +- test/common/filter/ext_authz_test.cc | 10 +++++----- 7 files changed, 27 insertions(+), 26 deletions(-) diff --git a/source/common/filter/BUILD b/source/common/filter/BUILD index 18c1b55d3abe9..d32d54782ba47 100644 --- a/source/common/filter/BUILD +++ b/source/common/filter/BUILD @@ -80,6 +80,6 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/ext_authz:ext_authz_lib", "//source/common/tracing:http_tracer_lib", - "@envoy_api//envoy/api/v2/filter/network:ext_authz_cc", + "@envoy_api//envoy/config/filter/network/ext_authz/v2:ext_authz_cc", ], ) diff --git a/source/common/filter/ext_authz.h b/source/common/filter/ext_authz.h index 5d12ea307937c..6c4f6c981a336 100644 --- a/source/common/filter/ext_authz.h +++ b/source/common/filter/ext_authz.h @@ -5,7 +5,7 @@ #include #include -#include "envoy/api/v2/filter/network/ext_authz.pb.h" +#include "envoy/config/filter/network/ext_authz/v2/ext_authz.pb.h" #include "envoy/ext_authz/ext_authz.h" #include "envoy/network/connection.h" #include "envoy/network/filter.h" @@ -44,8 +44,8 @@ struct InstanceStats { */ class Config { public: - // @saumoh: TBD: Take care of grpc service != envoy_grpc() - Config(const envoy::api::v2::filter::network::ExtAuthz& config, Stats::Scope& scope, + // TBD(saumoh): Take care of grpc service != envoy_grpc() + Config(const envoy::config::filter::network::ext_authz::v2::ExtAuthz& config, Stats::Scope& scope, Runtime::Loader& runtime, Upstream::ClusterManager& cm) : stats_(generateStats(config.stat_prefix(), scope)), runtime_(runtime), cm_(cm), cluster_name_(config.grpc_service().envoy_grpc().cluster_name()), diff --git a/source/server/config/network/BUILD b/source/server/config/network/BUILD index 6163dab3f3b97..12fff4e293c28 100644 --- a/source/server/config/network/BUILD +++ b/source/server/config/network/BUILD @@ -157,6 +157,6 @@ envoy_cc_library( "//source/common/config:well_known_names", "//source/common/filter:ext_authz_lib", "//source/common/protobuf:utility_lib", - "@envoy_api//envoy/api/v2/filter/network:ext_authz_cc", + "@envoy_api//envoy/config/filter/network/ext_authz/v2:ext_authz_cc", ], ) diff --git a/source/server/config/network/ext_authz.cc b/source/server/config/network/ext_authz.cc index d808becff50a6..e671a1b9e65b8 100644 --- a/source/server/config/network/ext_authz.cc +++ b/source/server/config/network/ext_authz.cc @@ -3,7 +3,7 @@ #include #include -#include "envoy/api/v2/filter/network/ext_authz.pb.validate.h" +#include "envoy/config/filter/network/ext_authz/v2/ext_authz.pb.validate.h" #include "envoy/ext_authz/ext_authz.h" #include "envoy/network/connection.h" #include "envoy/registry/registry.h" @@ -16,9 +16,9 @@ namespace Envoy { namespace Server { namespace Configuration { -NetworkFilterFactoryCb -ExtAuthzConfigFactory::createFilter(const envoy::api::v2::filter::network::ExtAuthz& proto_config, - FactoryContext& context) { +NetworkFilterFactoryCb ExtAuthzConfigFactory::createFilter( + const envoy::config::filter::network::ext_authz::v2::ExtAuthz& proto_config, + FactoryContext& context) { ASSERT(!proto_config.stat_prefix().empty()); ASSERT(proto_config.grpc_service().has_envoy_grpc()); @@ -36,16 +36,16 @@ ExtAuthzConfigFactory::createFilter(const envoy::api::v2::filter::network::ExtAu context.clusterManager().grpcAsyncClientManager().factoryForGrpcService(grpc_service, context.scope()); - auto client = std::make_unique(async_client_factory->create(), - std::chrono::milliseconds(timeout_ms)); - filter_manager.addReadFilter(Network::ReadFilterSharedPtr{new ExtAuthz::TcpFilter::Instance( - ext_authz_config, std::move(client))}); + auto client = std::make_unique( + async_client_factory->create(), std::chrono::milliseconds(timeout_ms)); + filter_manager.addReadFilter(Network::ReadFilterSharedPtr{ + new ExtAuthz::TcpFilter::Instance(ext_authz_config, std::move(client))}); }; } NetworkFilterFactoryCb ExtAuthzConfigFactory::createFilterFactory(const Json::Object& json_config, FactoryContext& context) { - envoy::api::v2::filter::network::ExtAuthz proto_config; + envoy::config::filter::network::ext_authz::v2::ExtAuthz proto_config; MessageUtil::loadFromJson(json_config.asJsonString(), proto_config); return createFilter(proto_config, context); } @@ -54,8 +54,8 @@ NetworkFilterFactoryCb ExtAuthzConfigFactory::createFilterFactoryFromProto(const Protobuf::Message& proto_config, FactoryContext& context) { return createFilter( - MessageUtil::downcastAndValidate( - proto_config), + MessageUtil::downcastAndValidate< + const envoy::config::filter::network::ext_authz::v2::ExtAuthz&>(proto_config), context); } diff --git a/source/server/config/network/ext_authz.h b/source/server/config/network/ext_authz.h index 609a8b03a1389..42c981ef94e43 100644 --- a/source/server/config/network/ext_authz.h +++ b/source/server/config/network/ext_authz.h @@ -2,7 +2,7 @@ #include -#include "envoy/api/v2/filter/network/ext_authz.pb.h" +#include "envoy/config/filter/network/ext_authz/v2/ext_authz.pb.h" #include "envoy/server/filter_config.h" #include "common/config/well_known_names.h" @@ -24,14 +24,15 @@ class ExtAuthzConfigFactory : public NamedNetworkFilterConfigFactory { FactoryContext& context) override; ProtobufTypes::MessagePtr createEmptyConfigProto() override { - return ProtobufTypes::MessagePtr{new envoy::api::v2::filter::network::ExtAuthz()}; + return ProtobufTypes::MessagePtr{new envoy::config::filter::network::ext_authz::v2::ExtAuthz()}; } std::string name() override { return Config::NetworkFilterNames::get().EXT_AUTHORIZATION; } private: - NetworkFilterFactoryCb createFilter(const envoy::api::v2::filter::network::ExtAuthz& proto_config, - FactoryContext& context); + NetworkFilterFactoryCb + createFilter(const envoy::config::filter::network::ext_authz::v2::ExtAuthz& proto_config, + FactoryContext& context); }; } // namespace Configuration diff --git a/test/common/filter/BUILD b/test/common/filter/BUILD index 9fdd7e0d4d9ed..66bc5d48cf2e2 100644 --- a/test/common/filter/BUILD +++ b/test/common/filter/BUILD @@ -64,6 +64,6 @@ envoy_cc_test( "//test/mocks/runtime:runtime_mocks", "//test/mocks/tracing:tracing_mocks", "//test/mocks/upstream:upstream_mocks", - "@envoy_api//envoy/api/v2/filter/network:ext_authz_cc", + "@envoy_api//envoy/config/filter/network/ext_authz/v2:ext_authz_cc", ], ) diff --git a/test/common/filter/ext_authz_test.cc b/test/common/filter/ext_authz_test.cc index 1c73904ce262b..28bb543ce226f 100644 --- a/test/common/filter/ext_authz_test.cc +++ b/test/common/filter/ext_authz_test.cc @@ -2,7 +2,7 @@ #include #include -#include "envoy/api/v2/filter/network/ext_authz.pb.validate.h" +#include "envoy/config/filter/network/ext_authz/v2/ext_authz.pb.validate.h" #include "common/buffer/buffer_impl.h" #include "common/filter/ext_authz.h" @@ -46,7 +46,7 @@ class ExtAuthzFilterTest : public testing::Test { } )EOF"; - envoy::api::v2::filter::network::ExtAuthz proto_config{}; + envoy::config::filter::network::ext_authz::v2::ExtAuthz proto_config{}; MessageUtil::loadFromJson(json, proto_config); config_.reset(new Config(proto_config, stats_store_, runtime_, cm_)); client_ = new MockClient(); @@ -84,11 +84,11 @@ TEST_F(ExtAuthzFilterTest, BadExtAuthzConfig) { } )EOF"; - envoy::api::v2::filter::network::ExtAuthz proto_config{}; + envoy::config::filter::network::ext_authz::v2::ExtAuthz proto_config{}; MessageUtil::loadFromJson(json_string, proto_config); - EXPECT_THROW(MessageUtil::downcastAndValidate( - proto_config), + EXPECT_THROW(MessageUtil::downcastAndValidate< + const envoy::config::filter::network::ext_authz::v2::ExtAuthz&>(proto_config), ProtoValidationException); } From c8910adeed399960d611de83eaa9502523299d10 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Tue, 6 Feb 2018 21:33:12 -0800 Subject: [PATCH 11/20] Fixup default timeout. Signed-off-by: Saurabh Mohan --- source/server/config/network/ext_authz.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/server/config/network/ext_authz.cc b/source/server/config/network/ext_authz.cc index e671a1b9e65b8..b0f60dee7dc83 100644 --- a/source/server/config/network/ext_authz.cc +++ b/source/server/config/network/ext_authz.cc @@ -26,7 +26,7 @@ NetworkFilterFactoryCb ExtAuthzConfigFactory::createFilter( ExtAuthz::TcpFilter::ConfigSharedPtr ext_authz_config(new ExtAuthz::TcpFilter::Config( proto_config, context.scope(), context.runtime(), context.clusterManager())); - const uint32_t timeout_ms = PROTOBUF_GET_MS_OR_DEFAULT(proto_config.grpc_service(), timeout, 20); + const uint32_t timeout_ms = PROTOBUF_GET_MS_OR_DEFAULT(proto_config.grpc_service(), timeout, 200); return [ grpc_service = proto_config.grpc_service(), &context, ext_authz_config, timeout_ms ](Network::FilterManager & filter_manager) From 945ac1c55b1a0ea1ed1e3346e742c92e91af12c3 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Tue, 6 Feb 2018 21:40:15 -0800 Subject: [PATCH 12/20] Include network filter config in build. Signed-off-by: Saurabh Mohan --- source/exe/BUILD | 1 + 1 file changed, 1 insertion(+) diff --git a/source/exe/BUILD b/source/exe/BUILD index ba6db1d96a307..0ef764928f9dc 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -49,6 +49,7 @@ envoy_cc_library( "//source/server/config/listener:proxy_protocol_lib", "//source/server/config/network:client_ssl_auth_lib", "//source/server/config/network:echo_lib", + "//source/server/config/network:ext_authz_lib", "//source/server/config/network:http_connection_manager_lib", "//source/server/config/network:ratelimit_lib", "//source/server/config/network:raw_buffer_socket_lib", From 5b41b32fcf7358e05614e9e7bd71c3b4f8a6b53c Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Mon, 12 Feb 2018 15:24:27 -0800 Subject: [PATCH 13/20] Address review comments. Signed-off-by: Saurabh Mohan --- source/common/filter/ext_authz.cc | 13 +-- source/common/filter/ext_authz.h | 16 +--- source/server/config/network/ext_authz.cc | 5 +- test/common/filter/ext_authz_test.cc | 103 +++++++++++++++++++--- 4 files changed, 105 insertions(+), 32 deletions(-) diff --git a/source/common/filter/ext_authz.cc b/source/common/filter/ext_authz.cc index f133cbdb5a92a..c8530bfe489dc 100644 --- a/source/common/filter/ext_authz.cc +++ b/source/common/filter/ext_authz.cc @@ -34,7 +34,9 @@ void Instance::callCheck() { Network::FilterStatus Instance::onData(Buffer::Instance&) { if (status_ == Status::NotStarted) { - // If the ssl handshake was not done and data is the next event! + // If there was no ssl handshake there will be no event Network::ConnectionEvent::Connected + // and onData() will be the first event into the filter. + // Therefore, if the authorization service hasn not been invoked then we must do so now. callCheck(); } return status_ == Status::Calling ? Network::FilterStatus::StopIteration @@ -47,11 +49,11 @@ Network::FilterStatus Instance::onNewConnection() { } void Instance::onEvent(Network::ConnectionEvent event) { - // Make sure that any pending request in the client is cancelled. This will be NOP if the - // request already completed. if (event == Network::ConnectionEvent::RemoteClose || event == Network::ConnectionEvent::LocalClose) { if (status_ == Status::Calling) { + // Make sure that any pending request in the client is cancelled. This will be NOP if the + // request already completed. client_->cancel(); config_->stats().active_.dec(); } @@ -59,6 +61,7 @@ void Instance::onEvent(Network::ConnectionEvent event) { // SSL connection is post TCP newConnection. Therefore the ext_authz check in onEvent. // if the ssl handshake was successful then it will invoke the // Network::ConnectionEvent::Connected. + // Thus invoke the authorization service check if not done yet. if (status_ == Status::NotStarted) { callCheck(); } @@ -78,11 +81,11 @@ void Instance::onComplete(CheckStatus status) { config_->stats().error_.inc(); break; case CheckStatus::Denied: - config_->stats().unauthz_.inc(); + config_->stats().denied_.inc(); break; } - // We fail open if there is an error contacting the service. + // Fail open only if configured to do so and if the check status was a error. if (status == CheckStatus::Denied || (status == CheckStatus::Error && !config_->failOpen())) { config_->stats().cx_closed_.inc(); filter_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush); diff --git a/source/common/filter/ext_authz.h b/source/common/filter/ext_authz.h index 6c4f6c981a336..9c614d27f0da3 100644 --- a/source/common/filter/ext_authz.h +++ b/source/common/filter/ext_authz.h @@ -26,7 +26,7 @@ namespace TcpFilter { #define ALL_TCP_EXT_AUTHZ_STATS(COUNTER, GAUGE) \ COUNTER(total) \ COUNTER(error) \ - COUNTER(unauthz) \ + COUNTER(denied) \ COUNTER(ok) \ COUNTER(cx_closed) \ GAUGE (active) @@ -44,25 +44,17 @@ struct InstanceStats { */ class Config { public: - // TBD(saumoh): Take care of grpc service != envoy_grpc() - Config(const envoy::config::filter::network::ext_authz::v2::ExtAuthz& config, Stats::Scope& scope, - Runtime::Loader& runtime, Upstream::ClusterManager& cm) - : stats_(generateStats(config.stat_prefix(), scope)), runtime_(runtime), cm_(cm), - cluster_name_(config.grpc_service().envoy_grpc().cluster_name()), + Config(const envoy::config::filter::network::ext_authz::v2::ExtAuthz& config, Stats::Scope& scope) + : stats_(generateStats(config.stat_prefix(), scope)), failure_mode_allow_(config.failure_mode_allow()) {} - Runtime::Loader& runtime() { return runtime_; } - std::string cluster() { return cluster_name_; } - Upstream::ClusterManager& cm() { return cm_; } const InstanceStats& stats() { return stats_; } bool failOpen() const { return failure_mode_allow_; } + void setFailModeAllow(bool value) { failure_mode_allow_ = value; } private: static InstanceStats generateStats(const std::string& name, Stats::Scope& scope); const InstanceStats stats_; - Runtime::Loader& runtime_; - Upstream::ClusterManager& cm_; - std::string cluster_name_; bool failure_mode_allow_; }; diff --git a/source/server/config/network/ext_authz.cc b/source/server/config/network/ext_authz.cc index b0f60dee7dc83..583394a571996 100644 --- a/source/server/config/network/ext_authz.cc +++ b/source/server/config/network/ext_authz.cc @@ -21,11 +21,10 @@ NetworkFilterFactoryCb ExtAuthzConfigFactory::createFilter( FactoryContext& context) { ASSERT(!proto_config.stat_prefix().empty()); - ASSERT(proto_config.grpc_service().has_envoy_grpc()); ASSERT(!proto_config.grpc_service().envoy_grpc().cluster_name().empty()); - ExtAuthz::TcpFilter::ConfigSharedPtr ext_authz_config(new ExtAuthz::TcpFilter::Config( - proto_config, context.scope(), context.runtime(), context.clusterManager())); + ExtAuthz::TcpFilter::ConfigSharedPtr ext_authz_config( + new ExtAuthz::TcpFilter::Config(proto_config, context.scope())); const uint32_t timeout_ms = PROTOBUF_GET_MS_OR_DEFAULT(proto_config.grpc_service(), timeout, 200); return [ grpc_service = proto_config.grpc_service(), &context, ext_authz_config, diff --git a/test/common/filter/ext_authz_test.cc b/test/common/filter/ext_authz_test.cc index 28bb543ce226f..a1dc18d3015cb 100644 --- a/test/common/filter/ext_authz_test.cc +++ b/test/common/filter/ext_authz_test.cc @@ -48,7 +48,7 @@ class ExtAuthzFilterTest : public testing::Test { envoy::config::filter::network::ext_authz::v2::ExtAuthz proto_config{}; MessageUtil::loadFromJson(json, proto_config); - config_.reset(new Config(proto_config, stats_store_, runtime_, cm_)); + config_.reset(new Config(proto_config, stats_store_)); client_ = new MockClient(); filter_.reset(new Instance(config_, ClientPtr{client_})); filter_->initializeReadFilterCallbacks(filter_callbacks_); @@ -66,8 +66,6 @@ class ExtAuthzFilterTest : public testing::Test { } Stats::IsolatedStoreImpl stats_store_; - NiceMock runtime_; - NiceMock cm_; ConfigSharedPtr config_; MockClient* client_; std::unique_ptr filter_; @@ -92,7 +90,7 @@ TEST_F(ExtAuthzFilterTest, BadExtAuthzConfig) { ProtoValidationException); } -TEST_F(ExtAuthzFilterTest, OK) { +TEST_F(ExtAuthzFilterTest, OKWithOnData) { InSequence s; EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); @@ -103,9 +101,14 @@ TEST_F(ExtAuthzFilterTest, OK) { Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); + // Confirm that the invocation of onNewConnection did NOT increment the active or total count! + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(0U, stats_store_.gauge("ext_authz.name.active").value()); Buffer::OwnedImpl data("hello"); EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data)); - EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data)); + // Confirm that the invocation of onData does increment the active and total count! + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(1U, stats_store_.gauge("ext_authz.name.active").value()); EXPECT_CALL(filter_callbacks_.connection_, readDisable(false)); EXPECT_CALL(filter_callbacks_, continueReading()); @@ -117,10 +120,13 @@ TEST_F(ExtAuthzFilterTest, OK) { filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::LocalClose); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.denied").value()); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.ok").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.cx_closed").value()); } -TEST_F(ExtAuthzFilterTest, Denied) { +TEST_F(ExtAuthzFilterTest, DeniedWithOnData) { InSequence s; EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); @@ -131,8 +137,14 @@ TEST_F(ExtAuthzFilterTest, Denied) { Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); + // Confirm that the invocation of onNewConnection did NOT increment the active or total count! + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(0U, stats_store_.gauge("ext_authz.name.active").value()); Buffer::OwnedImpl data("hello"); EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data)); + // Confirm that the invocation of onData does increment the active and total count! + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(1U, stats_store_.gauge("ext_authz.name.active").value()); EXPECT_CALL(filter_callbacks_.connection_, readDisable(false)); EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush)); @@ -142,7 +154,9 @@ TEST_F(ExtAuthzFilterTest, Denied) { EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data)); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); - EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.unauthz").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.denied").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.ok").value()); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.cx_closed").value()); } @@ -157,11 +171,19 @@ TEST_F(ExtAuthzFilterTest, OKWithSSLConnect) { Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); + // Confirm that the invocation of onNewConnection did NOT increment the active or total count! + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(0U, stats_store_.gauge("ext_authz.name.active").value()); // Called by SSL when the handshake is done. filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::Connected); + // Confirm that the invocation of Connected event increment the active or total count! + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(1U, stats_store_.gauge("ext_authz.name.active").value()); Buffer::OwnedImpl data("hello"); EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data)); - EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data)); + // Confirm that the invocation of onData did NOT increment the active or total count! + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(1U, stats_store_.gauge("ext_authz.name.active").value()); EXPECT_CALL(filter_callbacks_.connection_, readDisable(false)); EXPECT_CALL(filter_callbacks_, continueReading()); @@ -173,7 +195,10 @@ TEST_F(ExtAuthzFilterTest, OKWithSSLConnect) { filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::LocalClose); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.denied").value()); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.ok").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.cx_closed").value()); } TEST_F(ExtAuthzFilterTest, DeniedWithSSLConnect) { @@ -187,10 +212,19 @@ TEST_F(ExtAuthzFilterTest, DeniedWithSSLConnect) { Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); + // Confirm that the invocation of onNewConnection did NOT increment the active or total count! + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(0U, stats_store_.gauge("ext_authz.name.active").value()); // Called by SSL when the handshake is done. filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::Connected); + // Confirm that the invocation of Connected event increment the active or total count! + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(1U, stats_store_.gauge("ext_authz.name.active").value()); Buffer::OwnedImpl data("hello"); EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data)); + // Confirm that the invocation of onData did NOT increment the active or total count! + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(1U, stats_store_.gauge("ext_authz.name.active").value()); EXPECT_CALL(filter_callbacks_.connection_, readDisable(false)); EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush)); @@ -200,7 +234,9 @@ TEST_F(ExtAuthzFilterTest, DeniedWithSSLConnect) { EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data)); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); - EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.unauthz").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.denied").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.ok").value()); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.cx_closed").value()); } @@ -226,11 +262,40 @@ TEST_F(ExtAuthzFilterTest, FailOpen) { EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.error").value()); - EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.unauthz").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.denied").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.ok").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.cx_closed").value()); } -TEST_F(ExtAuthzFilterTest, Error) { +TEST_F(ExtAuthzFilterTest, FailClose) { + InSequence s; + // Explicitily set the failure_mode_allow to false. + config_->setFailModeAllow(false); + + EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(*client_, check(_, _, _)) + .WillOnce(WithArgs<0>( + Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); + Buffer::OwnedImpl data("hello"); + EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data)); + + EXPECT_CALL(filter_callbacks_.connection_, close(_)).Times(1); + EXPECT_CALL(filter_callbacks_, continueReading()).Times(0); + request_callbacks_->onComplete(CheckStatus::Error); + + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.denied").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.ok").value()); + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.cx_closed").value()); +} + +// Test to verify that when callback to the authorization service is completed the filter +// does not invoke Cancel when the event RemoteClose occurs. +TEST_F(ExtAuthzFilterTest, DoNotCallCancelonRemoteClose) { InSequence s; EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); @@ -253,9 +318,14 @@ TEST_F(ExtAuthzFilterTest, Error) { EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.denied").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.ok").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.cx_closed").value()); } -TEST_F(ExtAuthzFilterTest, Disconnect) { +// Test to verify that Cancel is invoked when a RemoteClose event occurs and a call +// to the authorization service was in progress. +TEST_F(ExtAuthzFilterTest, VerifyCancelOnRemoteClose) { InSequence s; EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); @@ -272,8 +342,14 @@ TEST_F(ExtAuthzFilterTest, Disconnect) { filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.denied").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.ok").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.cx_closed").value()); } +// Test to verify that on stack response from the authorization service does NOT +// result in calling cancel. TEST_F(ExtAuthzFilterTest, ImmediateOK) { InSequence s; @@ -293,7 +369,10 @@ TEST_F(ExtAuthzFilterTest, ImmediateOK) { filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.denied").value()); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.ok").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.cx_closed").value()); } } // namespace TcpFilter From a649490e127f642563808c7f2168fc3c137abe81 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Wed, 14 Feb 2018 16:39:17 -0800 Subject: [PATCH 14/20] Review feedback. Signed-off-by: Saurabh Mohan --- source/common/ext_authz/ext_authz_impl.cc | 22 +++++++++++----------- source/common/ext_authz/ext_authz_impl.h | 4 ++-- source/common/filter/ext_authz.cc | 7 +++---- source/common/filter/ext_authz.h | 1 + 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/source/common/ext_authz/ext_authz_impl.cc b/source/common/ext_authz/ext_authz_impl.cc index d2b2a2eef3cc6..ac2f987eca224 100644 --- a/source/common/ext_authz/ext_authz_impl.cc +++ b/source/common/ext_authz/ext_authz_impl.cc @@ -66,9 +66,9 @@ void GrpcClientImpl::onFailure(Grpc::Status::GrpcStatus status, const std::strin callbacks_ = nullptr; } -void CreateCheckRequest::setAttrContextPeer(envoy::service::auth::v2::AttributeContext_Peer& peer, - const Network::Connection& connection, - const std::string& service, const bool local) { +void CheckRequestUtils::setAttrContextPeer(envoy::service::auth::v2::AttributeContext_Peer& peer, + const Network::Connection& connection, + const std::string& service, const bool local) { // Set the address auto addr = peer.mutable_address(); @@ -103,14 +103,14 @@ void CreateCheckRequest::setAttrContextPeer(envoy::service::auth::v2::AttributeC } } -std::string CreateCheckRequest::getHeaderStr(const Envoy::Http::HeaderEntry* entry) { +std::string CheckRequestUtils::getHeaderStr(const Envoy::Http::HeaderEntry* entry) { if (entry) { return entry->value().getString(); } return ""; } -void CreateCheckRequest::setHttpRequest( +void CheckRequestUtils::setHttpRequest( ::envoy::service::auth::v2::AttributeContext_HttpRequest& httpreq, const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, const Envoy::Http::HeaderMap& headers) { @@ -152,16 +152,16 @@ void CreateCheckRequest::setHttpRequest( mutable_headers); } -void CreateCheckRequest::setAttrContextRequest( +void CheckRequestUtils::setAttrContextRequest( ::envoy::service::auth::v2::AttributeContext_Request& req, const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, const Envoy::Http::HeaderMap& headers) { setHttpRequest(*req.mutable_http(), callbacks, headers); } -void CreateCheckRequest::createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, - const Envoy::Http::HeaderMap& headers, - envoy::service::auth::v2::CheckRequest& request) { +void CheckRequestUtils::createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, + const Envoy::Http::HeaderMap& headers, + envoy::service::auth::v2::CheckRequest& request) { auto attrs = request.mutable_attributes(); @@ -175,8 +175,8 @@ void CreateCheckRequest::createHttpCheck(const Envoy::Http::StreamDecoderFilterC setAttrContextRequest(*attrs->mutable_request(), callbacks, headers); } -void CreateCheckRequest::createTcpCheck(const Network::ReadFilterCallbacks* callbacks, - envoy::service::auth::v2::CheckRequest& request) { +void CheckRequestUtils::createTcpCheck(const Network::ReadFilterCallbacks* callbacks, + envoy::service::auth::v2::CheckRequest& request) { auto attrs = request.mutable_attributes(); diff --git a/source/common/ext_authz/ext_authz_impl.h b/source/common/ext_authz/ext_authz_impl.h index 862e0e61933f8..a7b79bf1a1569 100644 --- a/source/common/ext_authz/ext_authz_impl.h +++ b/source/common/ext_authz/ext_authz_impl.h @@ -63,13 +63,13 @@ class GrpcClientImpl : public Client, public ExtAuthzAsyncCallbacks { /** * For creating ext_authz.proto (authorization) request. - * CreateCheckRequest is used to extract attributes from the TCP/HTTP request + * CheckRequestUtils is used to extract attributes from the TCP/HTTP request * and fill out the details in the authorization protobuf that is sent to authorization * service. * The specific information in the request is as per the specification in the * data-plane-api. */ -class CreateCheckRequest { +class CheckRequestUtils { public: /** * createHttpCheck is used to extract the attributes from the stream and the http headers diff --git a/source/common/filter/ext_authz.cc b/source/common/filter/ext_authz.cc index c8530bfe489dc..f7e768dd3cd6b 100644 --- a/source/common/filter/ext_authz.cc +++ b/source/common/filter/ext_authz.cc @@ -13,14 +13,13 @@ namespace ExtAuthz { namespace TcpFilter { InstanceStats Config::generateStats(const std::string& name, Stats::Scope& scope) { - std::string final_prefix = fmt::format("ext_authz.{}.", name); + const std::string final_prefix = fmt::format("ext_authz.{}.", name); return {ALL_TCP_EXT_AUTHZ_STATS(POOL_COUNTER_PREFIX(scope, final_prefix), POOL_GAUGE_PREFIX(scope, final_prefix))}; } void Instance::callCheck() { - envoy::service::auth::v2::CheckRequest request; - CreateCheckRequest::createTcpCheck(filter_callbacks_, request); + CheckRequestUtils::createTcpCheck(filter_callbacks_, checkRequest_); status_ = Status::Calling; filter_callbacks_->connection().readDisable(true); @@ -28,7 +27,7 @@ void Instance::callCheck() { config_->stats().total_.inc(); calling_check_ = true; - client_->check(*this, request, Tracing::NullSpan::instance()); + client_->check(*this, checkRequest_, Tracing::NullSpan::instance()); calling_check_ = false; } diff --git a/source/common/filter/ext_authz.h b/source/common/filter/ext_authz.h index 9c614d27f0da3..dad3096876d87 100644 --- a/source/common/filter/ext_authz.h +++ b/source/common/filter/ext_authz.h @@ -99,6 +99,7 @@ class Instance : public Network::ReadFilter, Network::ReadFilterCallbacks* filter_callbacks_{}; Status status_{Status::NotStarted}; bool calling_check_{}; + envoy::service::auth::v2::CheckRequest checkRequest_{}; }; } // TcpFilter From d1392ff7030d59f74bc9b14635b6928adb257170 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Wed, 14 Feb 2018 21:16:01 -0800 Subject: [PATCH 15/20] Fix tests class name. Signed-off-by: Saurabh Mohan --- test/common/ext_authz/ext_authz_impl_test.cc | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/common/ext_authz/ext_authz_impl_test.cc b/test/common/ext_authz/ext_authz_impl_test.cc index 77a171c68e7f5..2c5ceb501c354 100644 --- a/test/common/ext_authz/ext_authz_impl_test.cc +++ b/test/common/ext_authz/ext_authz_impl_test.cc @@ -114,16 +114,16 @@ TEST_F(ExtAuthzGrpcClientTest, Cancel) { client_.cancel(); } -class CreateCheckRequestTest : public testing::Test { +class CheckRequestUtilsTest : public testing::Test { public: - CreateCheckRequestTest() { + CheckRequestUtilsTest() { addr_ = std::make_shared("1.2.3.4", 1111); protocol_ = Envoy::Http::Protocol::Http10; }; Network::Address::InstanceConstSharedPtr addr_; Optional protocol_; - CreateCheckRequest check_request_generator_; + CheckRequestUtils check_request_generator_; NiceMock callbacks_; NiceMock net_callbacks_; NiceMock connection_; @@ -131,7 +131,7 @@ class CreateCheckRequestTest : public testing::Test { NiceMock req_info_; }; -TEST_F(CreateCheckRequestTest, BasicTcp) { +TEST_F(CheckRequestUtilsTest, BasicTcp) { envoy::service::auth::v2::CheckRequest request; @@ -140,10 +140,10 @@ TEST_F(CreateCheckRequestTest, BasicTcp) { EXPECT_CALL(connection_, localAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(Const(connection_), ssl()).Times(2).WillRepeatedly(Return(&ssl_)); - CreateCheckRequest::createTcpCheck(&net_callbacks_, request); + CheckRequestUtils::createTcpCheck(&net_callbacks_, request); } -TEST_F(CreateCheckRequestTest, BasicHttp) { +TEST_F(CheckRequestUtilsTest, BasicHttp) { Http::HeaderMapImpl headers; envoy::service::auth::v2::CheckRequest request; @@ -155,10 +155,10 @@ TEST_F(CreateCheckRequestTest, BasicHttp) { EXPECT_CALL(callbacks_, streamId()).WillOnce(Return(0)); EXPECT_CALL(callbacks_, requestInfo()).Times(3).WillRepeatedly(ReturnRef(req_info_)); EXPECT_CALL(req_info_, protocol()).Times(2).WillRepeatedly(ReturnRef(protocol_)); - CreateCheckRequest::createHttpCheck(&callbacks_, headers, request); + CheckRequestUtils::createHttpCheck(&callbacks_, headers, request); } -TEST_F(CreateCheckRequestTest, CheckAttrContextPeer) { +TEST_F(CheckRequestUtilsTest, CheckAttrContextPeer) { Http::TestHeaderMapImpl request_headers{{"x-envoy-downstream-service-cluster", "foo"}, {":path", "/bar"}}; @@ -174,7 +174,7 @@ TEST_F(CreateCheckRequestTest, CheckAttrContextPeer) { EXPECT_CALL(ssl_, uriSanPeerCertificate()).WillOnce(Return("source")); EXPECT_CALL(ssl_, uriSanLocalCertificate()).WillOnce(Return("destination")); - CreateCheckRequest::createHttpCheck(&callbacks_, request_headers, request); + CheckRequestUtils::createHttpCheck(&callbacks_, request_headers, request); EXPECT_EQ("source", request.attributes().source().principal()); EXPECT_EQ("destination", request.attributes().destination().principal()); From 8f5db76a5c19085bf79be285132e9ade77e9f8f2 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Thu, 15 Feb 2018 22:04:16 -0800 Subject: [PATCH 16/20] Move callCheck to onData. Signed-off-by: Saurabh Mohan --- source/common/filter/ext_authz.cc | 17 +----- test/common/filter/ext_authz_test.cc | 90 +--------------------------- 2 files changed, 6 insertions(+), 101 deletions(-) diff --git a/source/common/filter/ext_authz.cc b/source/common/filter/ext_authz.cc index f7e768dd3cd6b..117e9d7a03ff9 100644 --- a/source/common/filter/ext_authz.cc +++ b/source/common/filter/ext_authz.cc @@ -22,7 +22,6 @@ void Instance::callCheck() { CheckRequestUtils::createTcpCheck(filter_callbacks_, checkRequest_); status_ = Status::Calling; - filter_callbacks_->connection().readDisable(true); config_->stats().active_.inc(); config_->stats().total_.inc(); @@ -33,9 +32,8 @@ void Instance::callCheck() { Network::FilterStatus Instance::onData(Buffer::Instance&) { if (status_ == Status::NotStarted) { - // If there was no ssl handshake there will be no event Network::ConnectionEvent::Connected - // and onData() will be the first event into the filter. - // Therefore, if the authorization service hasn not been invoked then we must do so now. + // By waiting to invoke the check at onData() the call to authorization service will have + // sufficient information to fillout the checkRequest_. callCheck(); } return status_ == Status::Calling ? Network::FilterStatus::StopIteration @@ -43,7 +41,7 @@ Network::FilterStatus Instance::onData(Buffer::Instance&) { } Network::FilterStatus Instance::onNewConnection() { - // Wait till the next event occurs. + // Wait till onData() happens. return Network::FilterStatus::Continue; } @@ -56,20 +54,11 @@ void Instance::onEvent(Network::ConnectionEvent event) { client_->cancel(); config_->stats().active_.dec(); } - } else { - // SSL connection is post TCP newConnection. Therefore the ext_authz check in onEvent. - // if the ssl handshake was successful then it will invoke the - // Network::ConnectionEvent::Connected. - // Thus invoke the authorization service check if not done yet. - if (status_ == Status::NotStarted) { - callCheck(); - } } } void Instance::onComplete(CheckStatus status) { status_ = Status::Complete; - filter_callbacks_->connection().readDisable(false); config_->stats().active_.dec(); switch (status) { diff --git a/test/common/filter/ext_authz_test.cc b/test/common/filter/ext_authz_test.cc index a1dc18d3015cb..8484cbffd890b 100644 --- a/test/common/filter/ext_authz_test.cc +++ b/test/common/filter/ext_authz_test.cc @@ -95,7 +95,6 @@ TEST_F(ExtAuthzFilterTest, OKWithOnData) { EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); - EXPECT_CALL(filter_callbacks_.connection_, readDisable(true)); EXPECT_CALL(*client_, check(_, _, testing::A())) .WillOnce(WithArgs<0>( Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); @@ -110,7 +109,6 @@ TEST_F(ExtAuthzFilterTest, OKWithOnData) { EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); EXPECT_EQ(1U, stats_store_.gauge("ext_authz.name.active").value()); - EXPECT_CALL(filter_callbacks_.connection_, readDisable(false)); EXPECT_CALL(filter_callbacks_, continueReading()); request_callbacks_->onComplete(CheckStatus::OK); @@ -131,7 +129,6 @@ TEST_F(ExtAuthzFilterTest, DeniedWithOnData) { EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); - EXPECT_CALL(filter_callbacks_.connection_, readDisable(true)); EXPECT_CALL(*client_, check(_, _, _)) .WillOnce(WithArgs<0>( Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); @@ -146,87 +143,6 @@ TEST_F(ExtAuthzFilterTest, DeniedWithOnData) { EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); EXPECT_EQ(1U, stats_store_.gauge("ext_authz.name.active").value()); - EXPECT_CALL(filter_callbacks_.connection_, readDisable(false)); - EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush)); - EXPECT_CALL(*client_, cancel()).Times(0); - request_callbacks_->onComplete(CheckStatus::Denied); - - EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data)); - - EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); - EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.error").value()); - EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.denied").value()); - EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.ok").value()); - EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.cx_closed").value()); -} - -TEST_F(ExtAuthzFilterTest, OKWithSSLConnect) { - InSequence s; - - EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); - EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); - EXPECT_CALL(filter_callbacks_.connection_, readDisable(true)); - EXPECT_CALL(*client_, check(_, _, testing::A())) - .WillOnce(WithArgs<0>( - Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); - - EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); - // Confirm that the invocation of onNewConnection did NOT increment the active or total count! - EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.total").value()); - EXPECT_EQ(0U, stats_store_.gauge("ext_authz.name.active").value()); - // Called by SSL when the handshake is done. - filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::Connected); - // Confirm that the invocation of Connected event increment the active or total count! - EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); - EXPECT_EQ(1U, stats_store_.gauge("ext_authz.name.active").value()); - Buffer::OwnedImpl data("hello"); - EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data)); - // Confirm that the invocation of onData did NOT increment the active or total count! - EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); - EXPECT_EQ(1U, stats_store_.gauge("ext_authz.name.active").value()); - - EXPECT_CALL(filter_callbacks_.connection_, readDisable(false)); - EXPECT_CALL(filter_callbacks_, continueReading()); - request_callbacks_->onComplete(CheckStatus::OK); - - EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data)); - - EXPECT_CALL(*client_, cancel()).Times(0); - filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::LocalClose); - - EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); - EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.error").value()); - EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.denied").value()); - EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.ok").value()); - EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.cx_closed").value()); -} - -TEST_F(ExtAuthzFilterTest, DeniedWithSSLConnect) { - InSequence s; - - EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); - EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); - EXPECT_CALL(filter_callbacks_.connection_, readDisable(true)); - EXPECT_CALL(*client_, check(_, _, _)) - .WillOnce(WithArgs<0>( - Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); - - EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); - // Confirm that the invocation of onNewConnection did NOT increment the active or total count! - EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.total").value()); - EXPECT_EQ(0U, stats_store_.gauge("ext_authz.name.active").value()); - // Called by SSL when the handshake is done. - filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::Connected); - // Confirm that the invocation of Connected event increment the active or total count! - EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); - EXPECT_EQ(1U, stats_store_.gauge("ext_authz.name.active").value()); - Buffer::OwnedImpl data("hello"); - EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data)); - // Confirm that the invocation of onData did NOT increment the active or total count! - EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); - EXPECT_EQ(1U, stats_store_.gauge("ext_authz.name.active").value()); - - EXPECT_CALL(filter_callbacks_.connection_, readDisable(false)); EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush)); EXPECT_CALL(*client_, cancel()).Times(0); request_callbacks_->onComplete(CheckStatus::Denied); @@ -293,8 +209,8 @@ TEST_F(ExtAuthzFilterTest, FailClose) { EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.cx_closed").value()); } -// Test to verify that when callback to the authorization service is completed the filter -// does not invoke Cancel when the event RemoteClose occurs. +// Test to verify that when callback from the authorization service has completed the filter +// does not invoke Cancel on RemoteClose event. TEST_F(ExtAuthzFilterTest, DoNotCallCancelonRemoteClose) { InSequence s; @@ -323,7 +239,7 @@ TEST_F(ExtAuthzFilterTest, DoNotCallCancelonRemoteClose) { EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.cx_closed").value()); } -// Test to verify that Cancel is invoked when a RemoteClose event occurs and a call +// Test to verify that Cancel is invoked when a RemoteClose event occurs while the call // to the authorization service was in progress. TEST_F(ExtAuthzFilterTest, VerifyCancelOnRemoteClose) { InSequence s; From b3dc7bf71955ec26e0131f8d35365bd4abe8feb3 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Fri, 16 Feb 2018 16:09:58 -0800 Subject: [PATCH 17/20] Add tcp filter server configuration tests. Signed-off-by: Saurabh Mohan --- source/server/config/network/ext_authz.cc | 3 +- test/server/config/network/BUILD | 3 + test/server/config/network/config_test.cc | 67 ++++++++++++++++++++++- 3 files changed, 69 insertions(+), 4 deletions(-) diff --git a/source/server/config/network/ext_authz.cc b/source/server/config/network/ext_authz.cc index 583394a571996..82940dcb3aba1 100644 --- a/source/server/config/network/ext_authz.cc +++ b/source/server/config/network/ext_authz.cc @@ -21,7 +21,8 @@ NetworkFilterFactoryCb ExtAuthzConfigFactory::createFilter( FactoryContext& context) { ASSERT(!proto_config.stat_prefix().empty()); - ASSERT(!proto_config.grpc_service().envoy_grpc().cluster_name().empty()); + ASSERT(!proto_config.grpc_service().envoy_grpc().cluster_name().empty() || + !proto_config.grpc_service().google_grpc().target_uri().empty()); ExtAuthz::TcpFilter::ConfigSharedPtr ext_authz_config( new ExtAuthz::TcpFilter::Config(proto_config, context.scope())); diff --git a/test/server/config/network/BUILD b/test/server/config/network/BUILD index 015b5ee748180..c51d869bb03ae 100644 --- a/test/server/config/network/BUILD +++ b/test/server/config/network/BUILD @@ -15,13 +15,16 @@ envoy_cc_test( "//source/common/access_log:access_log_lib", "//source/common/config:well_known_names", "//source/common/dynamo:dynamo_filter_lib", + "//source/common/protobuf:utility_lib", "//source/server/config/access_log:file_access_log_lib", "//source/server/config/network:client_ssl_auth_lib", + "//source/server/config/network:ext_authz_lib", "//source/server/config/network:http_connection_manager_lib", "//source/server/config/network:mongo_proxy_lib", "//source/server/config/network:ratelimit_lib", "//source/server/config/network:redis_proxy_lib", "//source/server/config/network:tcp_proxy_lib", + "//test/mocks/grpc:grpc_mocks", "//test/mocks/server:server_mocks", "//test/test_common:utility_lib", ], diff --git a/test/server/config/network/config_test.cc b/test/server/config/network/config_test.cc index 4cbba58826b45..076b7e5899008 100644 --- a/test/server/config/network/config_test.cc +++ b/test/server/config/network/config_test.cc @@ -6,15 +6,18 @@ #include "common/config/filter_json.h" #include "common/config/well_known_names.h" #include "common/dynamo/dynamo_filter.h" +#include "common/protobuf/utility.h" #include "server/config/access_log/file_access_log.h" #include "server/config/network/client_ssl_auth.h" +#include "server/config/network/ext_authz.h" #include "server/config/network/http_connection_manager.h" #include "server/config/network/mongo_proxy.h" #include "server/config/network/ratelimit.h" #include "server/config/network/redis_proxy.h" #include "server/config/network/tcp_proxy.h" +#include "test/mocks/grpc/mocks.h" #include "test/mocks/server/mocks.h" #include "test/test_common/utility.h" @@ -22,6 +25,7 @@ #include "gtest/gtest.h" using testing::NiceMock; +using testing::Invoke; using testing::_; namespace Envoy { @@ -44,9 +48,12 @@ TEST(NetworkFilterConfigTest, ValidateFail) { envoy::config::filter::network::redis_proxy::v2::RedisProxy redis_proto; TcpProxyConfigFactory tcp_proxy_factory; envoy::config::filter::network::tcp_proxy::v2::TcpProxy tcp_proxy_proto; + ExtAuthzConfigFactory ext_authz_factory; + envoy::config::filter::network::ext_authz::v2::ExtAuthz ext_authz_proto; const std::vector> filter_cases = { {client_ssl_auth_factory, client_ssl_auth_proto}, + {ext_authz_factory, ext_authz_proto}, {hcm_factory, hcm_proto}, {mongo_factory, mongo_proto}, {rate_limit_factory, rate_limit_proto}, @@ -489,9 +496,8 @@ TEST(NetworkFilterConfigTest, BadAccessLogNestedTypes) { TEST(NetworkFilterConfigTest, DoubleRegistrationTest) { EXPECT_THROW_WITH_MESSAGE( (Registry::RegisterFactory()), - EnvoyException, - fmt::format("Double registration for name: '{}'", - Config::NetworkFilterNames::get().CLIENT_SSL_AUTH)); + EnvoyException, fmt::format("Double registration for name: '{}'", + Config::NetworkFilterNames::get().CLIENT_SSL_AUTH)); } TEST(AccessLogConfigTest, FileAccessLogTest) { @@ -532,6 +538,61 @@ TEST(TcpProxyConfigTest, TcpProxyConfigTest) { cb(connection); } +TEST(NetworkFilterConfigTest, ExtAuthzCorrectJson) { + std::string json = R"EOF( + { + "grpc_service": { + "envoy_grpc": { "cluster_name": "ext_authz_server" } + }, + "failure_mode_allow": true, + "stat_prefix": "name" + } + )EOF"; + + Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json); + NiceMock context; + ExtAuthzConfigFactory factory; + + EXPECT_CALL(context.cluster_manager_.async_client_manager_, factoryForGrpcService(_, _)) + .WillOnce(Invoke([](const envoy::api::v2::core::GrpcService&, Stats::Scope&) { + return std::make_unique>(); + })); + NetworkFilterFactoryCb cb = factory.createFilterFactory(*json_config, context); + Network::MockConnection connection; + EXPECT_CALL(connection, addReadFilter(_)); + cb(connection); +} + +TEST(NetworkFilterConfigTest, ExtAuthzCorrectProto) { + std::string json = R"EOF( + { + "grpc_service": { + "google_grpc": { + "target_uri": "ext_authz_server", + "stat_prefix": "google" + } + }, + "failure_mode_allow": false, + "stat_prefix": "name" + } + )EOF"; + + envoy::config::filter::network::ext_authz::v2::ExtAuthz proto_config{}; + MessageUtil::loadFromJson(json, proto_config); + + NiceMock context; + ExtAuthzConfigFactory factory; + + EXPECT_CALL(context.cluster_manager_.async_client_manager_, factoryForGrpcService(_, _)) + .WillOnce(Invoke([](const envoy::api::v2::core::GrpcService&, Stats::Scope&) { + return std::make_unique>(); + })); + NetworkFilterFactoryCb cb = factory.createFilterFactoryFromProto(proto_config, context); + Network::MockConnection connection; + EXPECT_CALL(connection, addReadFilter(_)); + cb(connection); +} + } // namespace Configuration } // namespace Server } // namespace Envoy From 4143a3cd4b440053743a7849346a48261afcf797 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Fri, 16 Feb 2018 16:47:33 -0800 Subject: [PATCH 18/20] Fix formatting issue. Signed-off-by: Saurabh Mohan --- test/server/config/network/config_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/server/config/network/config_test.cc b/test/server/config/network/config_test.cc index 076b7e5899008..fb4e150f082ac 100644 --- a/test/server/config/network/config_test.cc +++ b/test/server/config/network/config_test.cc @@ -24,8 +24,8 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" -using testing::NiceMock; using testing::Invoke; +using testing::NiceMock; using testing::_; namespace Envoy { From a02d94983d326a72846157384270f7b409248418 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Fri, 16 Feb 2018 16:58:04 -0800 Subject: [PATCH 19/20] Formatting fix up with clang-5 not 3.8 Signed-off-by: Saurabh Mohan --- test/server/config/network/config_test.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/server/config/network/config_test.cc b/test/server/config/network/config_test.cc index fb4e150f082ac..2ca2545086735 100644 --- a/test/server/config/network/config_test.cc +++ b/test/server/config/network/config_test.cc @@ -496,8 +496,9 @@ TEST(NetworkFilterConfigTest, BadAccessLogNestedTypes) { TEST(NetworkFilterConfigTest, DoubleRegistrationTest) { EXPECT_THROW_WITH_MESSAGE( (Registry::RegisterFactory()), - EnvoyException, fmt::format("Double registration for name: '{}'", - Config::NetworkFilterNames::get().CLIENT_SSL_AUTH)); + EnvoyException, + fmt::format("Double registration for name: '{}'", + Config::NetworkFilterNames::get().CLIENT_SSL_AUTH)); } TEST(AccessLogConfigTest, FileAccessLogTest) { From 4d13223802dad0648c115a877de689626debd6c2 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Mon, 19 Feb 2018 20:29:13 -0800 Subject: [PATCH 20/20] Remove createFilterFactor implementation. Signed-off-by: Saurabh Mohan --- source/server/config/network/ext_authz.cc | 8 +++----- test/server/config/network/config_test.cc | 25 ----------------------- 2 files changed, 3 insertions(+), 30 deletions(-) diff --git a/source/server/config/network/ext_authz.cc b/source/server/config/network/ext_authz.cc index 82940dcb3aba1..b2c7887702c80 100644 --- a/source/server/config/network/ext_authz.cc +++ b/source/server/config/network/ext_authz.cc @@ -43,11 +43,9 @@ NetworkFilterFactoryCb ExtAuthzConfigFactory::createFilter( }; } -NetworkFilterFactoryCb ExtAuthzConfigFactory::createFilterFactory(const Json::Object& json_config, - FactoryContext& context) { - envoy::config::filter::network::ext_authz::v2::ExtAuthz proto_config; - MessageUtil::loadFromJson(json_config.asJsonString(), proto_config); - return createFilter(proto_config, context); +NetworkFilterFactoryCb ExtAuthzConfigFactory::createFilterFactory(const Json::Object&, + FactoryContext&) { + NOT_IMPLEMENTED; } NetworkFilterFactoryCb diff --git a/test/server/config/network/config_test.cc b/test/server/config/network/config_test.cc index 2ca2545086735..2a3bad76b5db3 100644 --- a/test/server/config/network/config_test.cc +++ b/test/server/config/network/config_test.cc @@ -539,31 +539,6 @@ TEST(TcpProxyConfigTest, TcpProxyConfigTest) { cb(connection); } -TEST(NetworkFilterConfigTest, ExtAuthzCorrectJson) { - std::string json = R"EOF( - { - "grpc_service": { - "envoy_grpc": { "cluster_name": "ext_authz_server" } - }, - "failure_mode_allow": true, - "stat_prefix": "name" - } - )EOF"; - - Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json); - NiceMock context; - ExtAuthzConfigFactory factory; - - EXPECT_CALL(context.cluster_manager_.async_client_manager_, factoryForGrpcService(_, _)) - .WillOnce(Invoke([](const envoy::api::v2::core::GrpcService&, Stats::Scope&) { - return std::make_unique>(); - })); - NetworkFilterFactoryCb cb = factory.createFilterFactory(*json_config, context); - Network::MockConnection connection; - EXPECT_CALL(connection, addReadFilter(_)); - cb(connection); -} - TEST(NetworkFilterConfigTest, ExtAuthzCorrectProto) { std::string json = R"EOF( {