From 79b97f0de0720c706be6b974c112fbaa44934653 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Thu, 18 Jan 2018 22:12:12 -0800 Subject: [PATCH 01/17] Support for external authroization grpc service. Signed-off-by: Saurabh Mohan --- bazel/repositories.bzl | 10 + include/envoy/ext_authz/BUILD | 23 ++ include/envoy/ext_authz/ext_authz.h | 102 +++++++ include/envoy/request_info/request_info.h | 4 +- .../common/access_log/grpc_access_log_impl.cc | 9 +- source/common/config/well_known_names.h | 9 +- source/common/ext_authz/BUILD | 30 +++ source/common/ext_authz/ext_authz_impl.cc | 248 ++++++++++++++++++ source/common/ext_authz/ext_authz_impl.h | 126 +++++++++ source/common/request_info/utility.cc | 7 +- source/common/request_info/utility.h | 1 + source/server/BUILD | 1 + test/common/ext_authz/BUILD | 22 ++ test/common/ext_authz/ext_authz_impl_test.cc | 154 +++++++++++ test/mocks/ext_authz/BUILD | 18 ++ test/mocks/ext_authz/mocks.cc | 13 + test/mocks/ext_authz/mocks.h | 40 +++ 17 files changed, 812 insertions(+), 5 deletions(-) create mode 100644 include/envoy/ext_authz/BUILD create mode 100644 include/envoy/ext_authz/ext_authz.h create mode 100644 source/common/ext_authz/BUILD create mode 100644 source/common/ext_authz/ext_authz_impl.cc create mode 100644 source/common/ext_authz/ext_authz_impl.h create mode 100644 test/common/ext_authz/BUILD create mode 100644 test/common/ext_authz/ext_authz_impl_test.cc create mode 100644 test/mocks/ext_authz/BUILD create mode 100644 test/mocks/ext_authz/mocks.cc create mode 100644 test/mocks/ext_authz/mocks.h diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index eae8fd4a28448..61fcdd10d28a5 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -168,6 +168,7 @@ def _envoy_api_deps(): "rate_limit", "router", "transcoder", + "ext_authz", ] for t in http_filter_bind_targets: native.bind( @@ -181,6 +182,7 @@ def _envoy_api_deps(): "redis_proxy", "rate_limit", "client_ssl_auth", + "ext_authz", ] for t in network_filter_bind_targets: native.bind( @@ -195,6 +197,14 @@ def _envoy_api_deps(): name = "http_api_protos", actual = "@googleapis//:http_api_protos", ) + auth_bind_targets = [ + "auth" + ] + for t in auth_bind_targets: + native.bind( + name = "envoy_api_auth_" + t, + actual = "@envoy_api//api/auth:" + t + "_cc", + ) def envoy_dependencies(path = "@envoy_deps//", skip_targets = []): envoy_repository = repository_rule( diff --git a/include/envoy/ext_authz/BUILD b/include/envoy/ext_authz/BUILD new file mode 100644 index 0000000000000..236006520a0bc --- /dev/null +++ b/include/envoy/ext_authz/BUILD @@ -0,0 +1,23 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_library", + "envoy_package", +) + +envoy_package() + +envoy_cc_library( + name = "ext_authz_interface", + hdrs = ["ext_authz.h"], + external_deps = ["envoy_api_auth_auth"], + deps = [ + "//include/envoy/common:optional", + "//include/envoy/http:filter_interface", + "//include/envoy/http:header_map_interface", + "//include/envoy/network:filter_interface", + "//include/envoy/tracing:http_tracer_interface", + ], +) + diff --git a/include/envoy/ext_authz/ext_authz.h b/include/envoy/ext_authz/ext_authz.h new file mode 100644 index 0000000000000..2727ff698eb7c --- /dev/null +++ b/include/envoy/ext_authz/ext_authz.h @@ -0,0 +1,102 @@ +#pragma once + +#include +#include +#include +#include + +#include "envoy/common/optional.h" +#include "envoy/common/pure.h" +#include "envoy/http/filter.h" +#include "envoy/http/header_map.h" +#include "envoy/network/filter.h" +#include "envoy/tracing/http_tracer.h" + +#include "api/auth/external_auth.pb.h" + +namespace Envoy { +namespace ExtAuthz { + +/** + * Possible async results for a check call. + */ +enum class CheckStatus { + // The request is authorized. + OK, + // The authz service could not be queried. + Error, + // The request is denied. + Denied +}; + +/** + * Async callbacks used during check() calls. + */ +class RequestCallbacks { +public: + virtual ~RequestCallbacks() {} + + /** + * Called when a check request is complete. The resulting status is supplied. + */ + virtual void complete(CheckStatus status) PURE; +}; + + +class Client { + public: + // Destructor + virtual ~Client() {} + + /** + * Cancel an inflight Check request. + */ + virtual void cancel() PURE; + + // A check call. + virtual void check(RequestCallbacks &callback, const envoy::api::v2::auth::CheckRequest& request, + Tracing::Span& parent_span) PURE; + +}; + +typedef std::unique_ptr ClientPtr; + +/** + * An interface for creating a external authorization client. + */ +class ClientFactory { +public: + virtual ~ClientFactory() {} + + /** + * Return a new authz client. + */ + virtual ClientPtr create(const Optional& timeout) PURE; +}; + +typedef std::unique_ptr ClientFactoryPtr; + + +/** + * An interface for creating ext_authz.proto (authorization) request. + * CheckRequestGenerator 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. + */ +class CheckRequestGenerator { +public: + // Destructor + virtual ~CheckRequestGenerator() {} + + virtual void createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, + const Envoy::Http::HeaderMap &headers, + envoy::api::v2::auth::CheckRequest& request) PURE; + virtual void createTcpCheck(const Network::ReadFilterCallbacks* callbacks, + envoy::api::v2::auth::CheckRequest& request) PURE; +}; + +typedef std::unique_ptr CheckRequestGeneratorPtr; + +} // namespace ExtAuthz +} // namespace Envoy + diff --git a/include/envoy/request_info/request_info.h b/include/envoy/request_info/request_info.h index ad36706ab192a..ccee61596d99d 100644 --- a/include/envoy/request_info/request_info.h +++ b/include/envoy/request_info/request_info.h @@ -38,8 +38,10 @@ enum ResponseFlag { FaultInjected = 0x400, // Request was ratelimited locally by rate limit filter. RateLimited = 0x800, + // Request was unauthorized. + Unauthorized = 0x1000, // ATTENTION: MAKE SURE THIS REMAINS EQUAL TO THE LAST FLAG. - LastFlag = RateLimited + LastFlag = Unauthorized }; /** diff --git a/source/common/access_log/grpc_access_log_impl.cc b/source/common/access_log/grpc_access_log_impl.cc index cbc383850c620..76c4e3e142895 100644 --- a/source/common/access_log/grpc_access_log_impl.cc +++ b/source/common/access_log/grpc_access_log_impl.cc @@ -82,7 +82,7 @@ void HttpGrpcAccessLog::responseFlagsToAccessLogResponseFlags( envoy::api::v2::filter::accesslog::AccessLogCommon& common_access_log, const RequestInfo::RequestInfo& request_info) { - static_assert(RequestInfo::ResponseFlag::LastFlag == 0x800, + static_assert(RequestInfo::ResponseFlag::LastFlag == 0x1000, "A flag has been added. Fix this code."); if (request_info.getResponseFlag(RequestInfo::ResponseFlag::FailedLocalHealthCheck)) { @@ -132,6 +132,13 @@ void HttpGrpcAccessLog::responseFlagsToAccessLogResponseFlags( if (request_info.getResponseFlag(RequestInfo::ResponseFlag::RateLimited)) { common_access_log.mutable_response_flags()->set_rate_limited(true); } + + // TODO(saumoh): Uncomment once unauthorization has been added to accesslog.proto + // + // if (request_info.getResponseFlag(RequestInfo::ResponseFlag::Unauthorized)) { + // common_access_log.mutable_response_flags()->set_unauthorized(true); + // } + } void HttpGrpcAccessLog::log(const Http::HeaderMap* request_headers, diff --git a/source/common/config/well_known_names.h b/source/common/config/well_known_names.h index 40756b131972c..43fa371c0cf29 100644 --- a/source/common/config/well_known_names.h +++ b/source/common/config/well_known_names.h @@ -66,13 +66,15 @@ class NetworkFilterNameValues { const std::string REDIS_PROXY = "envoy.redis_proxy"; // IP tagging filter const std::string TCP_PROXY = "envoy.tcp_proxy"; + // Authorization filter + const std::string EXT_AUTHORIZATION = "envoy.ext_authz"; // Converts names from v1 to v2 const V1Converter v1_converter_; NetworkFilterNameValues() : v1_converter_({CLIENT_SSL_AUTH, ECHO, HTTP_CONNECTION_MANAGER, MONGO_PROXY, RATE_LIMIT, - REDIS_PROXY, TCP_PROXY}) {} + REDIS_PROXY, TCP_PROXY, EXT_AUTHORIZATION}) {} }; typedef ConstSingleton NetworkFilterNames; @@ -117,13 +119,16 @@ class HttpFilterNameValues { const std::string HEALTH_CHECK = "envoy.health_check"; // Lua filter const std::string LUA = "envoy.lua"; + // External Authorization filter + const std::string EXT_AUTHORIZATION = "envoy.ext_authz"; + // Converts names from v1 to v2 const V1Converter v1_converter_; HttpFilterNameValues() : v1_converter_({BUFFER, CORS, DYNAMO, FAULT, GRPC_HTTP1_BRIDGE, GRPC_JSON_TRANSCODER, - GRPC_WEB, HEALTH_CHECK, IP_TAGGING, RATE_LIMIT, ROUTER, LUA}) {} + GRPC_WEB, HEALTH_CHECK, IP_TAGGING, RATE_LIMIT, ROUTER, LUA, EXT_AUTHORIZATION}) {} }; typedef ConstSingleton HttpFilterNames; diff --git a/source/common/ext_authz/BUILD b/source/common/ext_authz/BUILD new file mode 100644 index 0000000000000..61714cb4a3257 --- /dev/null +++ b/source/common/ext_authz/BUILD @@ -0,0 +1,30 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_library", + "envoy_package", +) + +envoy_package() + +envoy_cc_library( + name = "ext_authz_lib", + srcs = ["ext_authz_impl.cc"], + hdrs = ["ext_authz_impl.h"], + external_deps = ["envoy_bootstrap"], + deps = [ + "//include/envoy/ext_authz:ext_authz_interface", + "//include/envoy/grpc:async_client_interface", + "//include/envoy/http:protocol_interface", + "//include/envoy/network:address_interface", + "//include/envoy/network:connection_interface", + "//include/envoy/upstream:cluster_manager_interface", + "//include/envoy/ssl:connection_interface", + "//source/common/common:assert_lib", + "//source/common/grpc:async_client_lib", + "//source/common/http:headers_lib", + "//source/common/tracing:http_tracer_lib", + ], +) + diff --git a/source/common/ext_authz/ext_authz_impl.cc b/source/common/ext_authz/ext_authz_impl.cc new file mode 100644 index 0000000000000..d508599f7a9d2 --- /dev/null +++ b/source/common/ext_authz/ext_authz_impl.cc @@ -0,0 +1,248 @@ +#include "common/ext_authz/ext_authz_impl.h" + +#include +#include +#include +#include + +#include "envoy/access_log/access_log.h" +#include "envoy/ssl/connection.h" + +#include "common/common/assert.h" +#include "common/grpc/async_client_impl.h" +#include "common/http/headers.h" + +#include "fmt/format.h" + +namespace Envoy { +namespace ExtAuthz { + +using ::envoy::api::v2::auth::AttributeContext; +using ::envoy::api::v2::auth::AttributeContext_HTTPRequest; +using ::envoy::api::v2::auth::AttributeContext_Peer; +using ::envoy::api::v2::auth::AttributeContext_Request; + +GrpcClientImpl::GrpcClientImpl(ExtAuthzAsyncClientPtr&& async_client, + const Optional& timeout) + : service_method_(*Protobuf::DescriptorPool::generated_pool()->FindMethodByName( + "envoy.api.v2.auth.Authorization.Check")), + async_client_(std::move(async_client)), timeout_(timeout) {} + +GrpcClientImpl::~GrpcClientImpl() { ASSERT(!callbacks_); } + +void GrpcClientImpl::cancel() { + ASSERT(callbacks_ != nullptr); + request_->cancel(); + callbacks_ = nullptr; +} + +void GrpcClientImpl::check(RequestCallbacks& callbacks, const envoy::api::v2::auth::CheckRequest& request, + Tracing::Span& parent_span) { + ASSERT(callbacks_ == nullptr); + callbacks_ = &callbacks; + + request_ = async_client_->send(service_method_, request, *this, parent_span, timeout_); +} + +void GrpcClientImpl::onSuccess(std::unique_ptr&& response, + Tracing::Span& span) { + CheckStatus status = CheckStatus::OK; + ASSERT(response->status().code() != Grpc::Status::GrpcStatus::Unknown); + if (response->status().code() != Grpc::Status::GrpcStatus::Ok) { + status = CheckStatus::Denied; + span.setTag(Constants::get().TraceStatus, Constants::get().TraceUnauthz); + } else { + span.setTag(Constants::get().TraceStatus, Constants::get().TraceOk); + } + + callbacks_->complete(status); + callbacks_ = nullptr; +} + +void GrpcClientImpl::onFailure(Grpc::Status::GrpcStatus status, const std::string&, + Tracing::Span&) { + ASSERT(status != Grpc::Status::GrpcStatus::Ok); + UNREFERENCED_PARAMETER(status); + callbacks_->complete(CheckStatus::Error); + callbacks_ = nullptr; +} + +GrpcFactoryImpl::GrpcFactoryImpl(const std::string& cluster_name, + Upstream::ClusterManager& cm) + : cluster_name_(cluster_name), cm_(cm) { + if (!cm_.get(cluster_name_)) { + throw EnvoyException(fmt::format("unknown external authorization service cluster '{}'", cluster_name_)); + } +} + +ClientPtr GrpcFactoryImpl::create(const Optional& timeout) { + return ClientPtr{new GrpcClientImpl( + ExtAuthzAsyncClientPtr{ + new Grpc::AsyncClientImpl(cm_, cluster_name_)}, + timeout)}; +} + +std::unique_ptr<::envoy::api::v2::Address> ExtAuthzCheckRequestGenerator::getProtobufAddress(const Network::Address::InstanceConstSharedPtr& instance) { + + std::unique_ptr<::envoy::api::v2::Address> addr = std::unique_ptr<::envoy::api::v2::Address>{new ::envoy::api::v2::Address()}; + ASSERT(addr); + + if (instance->type() == Network::Address::Type::Ip) { + addr->mutable_socket_address()->set_address(instance->ip()->addressAsString()); + addr->mutable_socket_address()->set_port_value(instance->ip()->port()); + } else { + ASSERT(instance->type() == Network::Address::Type::Pipe); + addr->mutable_pipe()->set_path(instance->asString()); + } + return addr; +} + +std::unique_ptr ExtAuthzCheckRequestGenerator::getConnectionPeer(const Network::Connection *connection, + const std::string& service, + const bool local) { + + std::unique_ptr peer = std::unique_ptr{new AttributeContext_Peer()}; + ASSERT(peer); + + // Set the address + if (!local) { + peer->set_allocated_address(getProtobufAddress(connection->remoteAddress()).release()); + } else { + peer->set_allocated_address(getProtobufAddress(connection->localAddress()).release()); + } + + // Set the principal + // Preferably the SAN from the peer's cert or + // Subject from the peer's cert. + std::string principal; + Ssl::Connection* ssl = + const_cast(connection->ssl()); + if (ssl != nullptr) { + if (!local) { + principal = ssl->uriSanPeerCertificate(); + + if (principal.empty()) { + principal = ssl->subjectPeerCertificate(); + } + } else { + principal = ssl->uriSanLocalCertificate(); + + if (principal.empty()) { + principal = ssl->subjectLocalCertificate(); + } + } + } + peer->set_principal(principal); + + if (!service.empty()) { + peer->set_service(service); + } + + return peer; +} + +std::unique_ptr ExtAuthzCheckRequestGenerator::getConnectionPeer(const Network::Connection& connection, + const std::string& service, + const bool local) { + return getConnectionPeer(&connection, service, local); +} + + +const std::string ExtAuthzCheckRequestGenerator::getProtocolStr(const Envoy::Http::Protocol& p) { + + switch(p) { + case Envoy::Http::Protocol::Http10: + return std::string("Http1.0"); + case Envoy::Http::Protocol::Http11: + return std::string("Http1.1"); + case Envoy::Http::Protocol::Http2: + return std::string("Http2"); + default: + break; + } + return std::string("unknown"); +} + +Envoy::Http::HeaderMap::Iterate ExtAuthzCheckRequestGenerator::fillHttpHeaders(const Envoy::Http::HeaderEntry &e, + void *ctx) { + ::google::protobuf::Map< ::std::string, ::std::string >* mhdrs = static_cast<::google::protobuf::Map< ::std::string, ::std::string >*>(ctx); + (*mhdrs)[std::string(e.key().c_str(), e.key().size())] = std::string(e.value().c_str(), e.value().size()); + return Envoy::Http::HeaderMap::Iterate::Continue; +} + +std::string ExtAuthzCheckRequestGenerator::getHeaderStr(const Envoy::Http::HeaderEntry *entry) { + if (entry) { + return std::string(entry->value().c_str(), entry->value().size()); + } + return ""; +} + +std::unique_ptr ExtAuthzCheckRequestGenerator::getHttpRequest(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, + const Envoy::Http::HeaderMap &headers) { + + + AttributeContext_HTTPRequest *httpreq = new AttributeContext_HTTPRequest(); + ASSERT(httpreq); + + // Set id + // The streamId is not qualified as a const. Although it is as it does not modify the object. + Envoy::Http::StreamDecoderFilterCallbacks *sdfc = const_cast(callbacks); + httpreq->set_id(std::to_string(sdfc->streamId())); + + // Set method + httpreq->set_method(std::move(getHeaderStr(headers.Method()))); + // Set path + httpreq->set_path(std::move(getHeaderStr(headers.Path()))); + // Set host + httpreq->set_host(std::move(getHeaderStr(headers.Host()))); + // Set scheme + httpreq->set_scheme(std::move(getHeaderStr(headers.Scheme()))); + + // Set size + // need to convert to google buffer 64t; + httpreq->set_size(sdfc->requestInfo().bytesReceived()); + + // Set protocol + httpreq->set_protocol(getProtocolStr(sdfc->requestInfo().protocol().value())); + + // Fill in the headers + auto* mhdrs = httpreq->mutable_headers(); + headers.iterate(fillHttpHeaders, mhdrs); + + std::unique_ptr req = std::unique_ptr{new AttributeContext_Request()}; + ASSERT(req); + req->set_allocated_http(httpreq); + + return req; +} + +void ExtAuthzCheckRequestGenerator::createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, + const Envoy::Http::HeaderMap &headers, + envoy::api::v2::auth::CheckRequest& request) { + + AttributeContext* attrs = request.mutable_attributes(); + ASSERT(attrs); + + Envoy::Http::StreamDecoderFilterCallbacks* cb = const_cast(callbacks); + + std::string service = getHeaderStr(headers.EnvoyDownstreamServiceCluster()); + attrs->set_allocated_source(getConnectionPeer(cb->connection(), service, false).release()); + attrs->set_allocated_destination(getConnectionPeer(cb->connection(), "", true).release()); + attrs->set_allocated_request(getHttpRequest(callbacks, headers).release()); +} + +void ExtAuthzCheckRequestGenerator::createTcpCheck(const Network::ReadFilterCallbacks* callbacks, + envoy::api::v2::auth::CheckRequest& request) { + + AttributeContext* attrs = request.mutable_attributes(); + ASSERT(attrs); + + Network::ReadFilterCallbacks* cb = const_cast(callbacks); + attrs->set_allocated_source(getConnectionPeer(cb->connection(), "", false).release()); + attrs->set_allocated_destination(getConnectionPeer(cb->connection(), "", true).release()); +} + +} // namespace ExtAuthz +} // namespace Envoy + diff --git a/source/common/ext_authz/ext_authz_impl.h b/source/common/ext_authz/ext_authz_impl.h new file mode 100644 index 0000000000000..54c3738718df3 --- /dev/null +++ b/source/common/ext_authz/ext_authz_impl.h @@ -0,0 +1,126 @@ +#pragma once + +#include +#include +#include +#include + +#include "envoy/ext_authz/ext_authz.h" +#include "envoy/grpc/async_client.h" +#include "envoy/http/protocol.h" +#include "envoy/network/address.h" +#include "envoy/network/connection.h" + +#include "envoy/tracing/http_tracer.h" +#include "envoy/upstream/cluster_manager.h" + +#include "common/singleton/const_singleton.h" + +#include "api/bootstrap.pb.h" + +namespace Envoy { +namespace ExtAuthz { + +typedef Grpc::AsyncClient + ExtAuthzAsyncClient; +typedef std::unique_ptr ExtAuthzAsyncClientPtr; + +typedef Grpc::AsyncRequestCallbacks ExtAuthzAsyncCallbacks; + +struct ConstantValues { + const std::string TraceStatus = "ext_authz_status"; + const std::string TraceUnauthz = "ext_authz_unauthorized"; + const std::string TraceOk = "ext_authz_ok"; +}; + +typedef ConstSingleton Constants; + +// TODO(htuch): We should have only one client per thread, but today we create one per filter stack. +// This will require support for more than one outstanding request per client. +class GrpcClientImpl : public Client, public ExtAuthzAsyncCallbacks { +public: + GrpcClientImpl(ExtAuthzAsyncClientPtr&& async_client, + const Optional& timeout); + ~GrpcClientImpl(); + + // ExtAuthz::Client + void cancel() override; + void check(RequestCallbacks& callbacks, const envoy::api::v2::auth::CheckRequest& request, + Tracing::Span& parent_span) override; + + // Grpc::AsyncRequestCallbacks + void onCreateInitialMetadata(Http::HeaderMap&) override {} + void onSuccess(std::unique_ptr&& response, + Tracing::Span& span) override; + void onFailure(Grpc::Status::GrpcStatus status, const std::string& message, + Tracing::Span& span) override; + +private: + const Protobuf::MethodDescriptor& service_method_; + ExtAuthzAsyncClientPtr async_client_; + Grpc::AsyncRequest* request_{}; + Optional timeout_; + RequestCallbacks* callbacks_{}; +}; + +class GrpcFactoryImpl : public ClientFactory { +public: + GrpcFactoryImpl(const std::string& cluster_name, + Upstream::ClusterManager& cm); + + // ExtAuthz::ClientFactory + ClientPtr create(const Optional& timeout) override; + +private: + const std::string cluster_name_; + Upstream::ClusterManager& cm_; +}; + +class NullClientImpl : public Client { +public: + // ExtAuthz::Client + void cancel() override {} + void check(RequestCallbacks& callbacks, const envoy::api::v2::auth::CheckRequest&, + Tracing::Span&) override { + callbacks.complete(CheckStatus::OK); + } +}; + +class NullFactoryImpl : public ClientFactory { +public: + // ExtAuthz::ClientFactory + ClientPtr create(const Optional&) override { + return ClientPtr{new NullClientImpl()}; + } +}; + +class ExtAuthzCheckRequestGenerator: public CheckRequestGenerator { +public: + ExtAuthzCheckRequestGenerator() {} + ~ExtAuthzCheckRequestGenerator() {} + + // ExtAuthz::CheckRequestGenIntf + void createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, + const Envoy::Http::HeaderMap &headers, + envoy::api::v2::auth::CheckRequest& request); + void createTcpCheck(const Network::ReadFilterCallbacks* callbacks, + envoy::api::v2::auth::CheckRequest& request); + +private: + std::unique_ptr<::envoy::api::v2::Address> getProtobufAddress(const Network::Address::InstanceConstSharedPtr&); + std::unique_ptr<::envoy::api::v2::auth::AttributeContext_Peer> getConnectionPeer(const Network::Connection *, + const std::string&, + const bool); + std::unique_ptr<::envoy::api::v2::auth::AttributeContext_Peer> getConnectionPeer(const Network::Connection&, + const std::string&, + const bool); + std::unique_ptr<::envoy::api::v2::auth::AttributeContext_Request> getHttpRequest(const Envoy::Http::StreamDecoderFilterCallbacks*, + const Envoy::Http::HeaderMap &); + const std::string getProtocolStr(const Envoy::Http::Protocol&); + std::string getHeaderStr(const Envoy::Http::HeaderEntry *entry); + static Envoy::Http::HeaderMap::Iterate fillHttpHeaders(const Envoy::Http::HeaderEntry&, void *); +}; + +} // namespace ExtAuthz +} // namespace Envoy diff --git a/source/common/request_info/utility.cc b/source/common/request_info/utility.cc index 433c6f1966b70..fb2858017c52a 100644 --- a/source/common/request_info/utility.cc +++ b/source/common/request_info/utility.cc @@ -20,6 +20,7 @@ const std::string ResponseFlagUtils::NO_ROUTE_FOUND = "NR"; const std::string ResponseFlagUtils::DELAY_INJECTED = "DI"; const std::string ResponseFlagUtils::FAULT_INJECTED = "FI"; const std::string ResponseFlagUtils::RATE_LIMITED = "RL"; +const std::string ResponseFlagUtils::UNAUTHORIZED = "UA"; void ResponseFlagUtils::appendString(std::string& result, const std::string& append) { if (result.empty()) { @@ -32,7 +33,7 @@ void ResponseFlagUtils::appendString(std::string& result, const std::string& app const std::string ResponseFlagUtils::toShortString(const RequestInfo& request_info) { std::string result; - static_assert(ResponseFlag::LastFlag == 0x800, "A flag has been added. Fix this code."); + static_assert(ResponseFlag::LastFlag == 0x1000, "A flag has been added. Fix this code."); if (request_info.getResponseFlag(ResponseFlag::FailedLocalHealthCheck)) { appendString(result, FAILED_LOCAL_HEALTH_CHECK); @@ -82,6 +83,10 @@ const std::string ResponseFlagUtils::toShortString(const RequestInfo& request_in appendString(result, RATE_LIMITED); } + if (request_info.getResponseFlag(ResponseFlag::Unauthorized)) { + appendString(result, UNAUTHORIZED); + } + return result.empty() ? NONE : result; } diff --git a/source/common/request_info/utility.h b/source/common/request_info/utility.h index 1f4614a297659..a9288151b1baa 100644 --- a/source/common/request_info/utility.h +++ b/source/common/request_info/utility.h @@ -32,6 +32,7 @@ class ResponseFlagUtils { const static std::string DELAY_INJECTED; const static std::string FAULT_INJECTED; const static std::string RATE_LIMITED; + const static std::string UNAUTHORIZED; }; /** diff --git a/source/server/BUILD b/source/server/BUILD index d2bea25d768aa..2c6b8c2e01f93 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -36,6 +36,7 @@ envoy_cc_library( "//include/envoy/server:filter_config_interface", "//include/envoy/server:instance_interface", "//include/envoy/ssl:context_manager_interface", + "//source/common/ext_authz:ext_authz_lib", "//source/common/common:assert_lib", "//source/common/common:logger_lib", "//source/common/common:utility_lib", diff --git a/test/common/ext_authz/BUILD b/test/common/ext_authz/BUILD new file mode 100644 index 0000000000000..7486369149a29 --- /dev/null +++ b/test/common/ext_authz/BUILD @@ -0,0 +1,22 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_test", + "envoy_package", +) + +envoy_package() + +envoy_cc_test( + name = "ext_authz_impl_test", + srcs = ["ext_authz_impl_test.cc"], + deps = [ + "//source/common/http:header_map_lib", + "//source/common/http:headers_lib", + "//source/common/ext_authz:ext_authz_lib", + "//test/mocks/grpc:grpc_mocks", + "//test/mocks/upstream:upstream_mocks", + "//test/test_common:utility_lib", + ], +) diff --git a/test/common/ext_authz/ext_authz_impl_test.cc b/test/common/ext_authz/ext_authz_impl_test.cc new file mode 100644 index 0000000000000..a51637618c0d4 --- /dev/null +++ b/test/common/ext_authz/ext_authz_impl_test.cc @@ -0,0 +1,154 @@ +#include +#include +#include + +#include "common/http/header_map_impl.h" +#include "common/http/headers.h" +#include "common/ext_authz/ext_authz_impl.h" + +#include "test/mocks/grpc/mocks.h" +#include "test/mocks/upstream/mocks.h" +#include "test/test_common/printers.h" +#include "test/test_common/utility.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::AtLeast; +using testing::Invoke; +using testing::Ref; +using testing::Return; +using testing::WithArg; +using testing::_; + +namespace Envoy { +namespace ExtAuthz { + +class MockRequestCallbacks : public RequestCallbacks { +public: + MOCK_METHOD1(complete, void(CheckStatus status)); +}; + +class ExtAuthzGrpcClientTest : public testing::Test { +public: + ExtAuthzGrpcClientTest() + : async_client_(new Grpc::MockAsyncClient()), + client_(ExtAuthzAsyncClientPtr{async_client_}, Optional()) {} + + Grpc::MockAsyncClient* async_client_; + Grpc::MockAsyncRequest async_request_; + GrpcClientImpl client_; + MockRequestCallbacks request_callbacks_; + Tracing::MockSpan span_; +}; + +TEST_F(ExtAuthzGrpcClientTest, Basic) { + std::unique_ptr response; + + { + envoy::api::v2::auth::CheckRequest request; + Http::HeaderMapImpl headers; + EXPECT_CALL(*async_client_, send(_, ProtoEq(request), Ref(client_), _, _)) + .WillOnce(Invoke([this]( + const Protobuf::MethodDescriptor& service_method, + const envoy::api::v2::auth::CheckRequest&, + Grpc::AsyncRequestCallbacks&, + Tracing::Span&, + const Optional&) -> Grpc::AsyncRequest* { + EXPECT_EQ("envoy.api.v2.auth.Authorization", service_method.service()->full_name()); + EXPECT_EQ("Check", service_method.name()); + return &async_request_; + })); + + client_.check(request_callbacks_, request, Tracing::NullSpan::instance()); + + client_.onCreateInitialMetadata(headers); + EXPECT_EQ(nullptr, headers.RequestId()); + + response.reset(new envoy::api::v2::auth::CheckResponse()); + ::google::rpc::Status *status = new ::google::rpc::Status(); + status->set_code(Grpc::Status::GrpcStatus::PermissionDenied); + response->set_allocated_status(status); + EXPECT_CALL(span_, setTag("ext_authz_status", "ext_authz_unauthorized")); + EXPECT_CALL(request_callbacks_, complete(CheckStatus::Denied)); + client_.onSuccess(std::move(response), span_); + } + + { + envoy::api::v2::auth::CheckRequest request; + Http::HeaderMapImpl headers; + EXPECT_CALL(*async_client_, send(_, ProtoEq(request), _, _, _)) + .WillOnce(Return(&async_request_)); + + client_.check(request_callbacks_, request, + Tracing::NullSpan::instance()); + + client_.onCreateInitialMetadata(headers); + + response.reset(new envoy::api::v2::auth::CheckResponse()); + ::google::rpc::Status *status = new ::google::rpc::Status(); + status->set_code(Grpc::Status::GrpcStatus::Ok); + response->set_allocated_status(status); + EXPECT_CALL(span_, setTag("ext_authz_status", "ext_authz_ok")); + EXPECT_CALL(request_callbacks_, complete(CheckStatus::OK)); + client_.onSuccess(std::move(response), span_); + } + + + { + envoy::api::v2::auth::CheckRequest request; + EXPECT_CALL(*async_client_, send(_, ProtoEq(request), _, _, _)) + .WillOnce(Return(&async_request_)); + + client_.check(request_callbacks_, request, + Tracing::NullSpan::instance()); + + response.reset(new envoy::api::v2::auth::CheckResponse()); + EXPECT_CALL(request_callbacks_, complete(CheckStatus::Error)); + client_.onFailure(Grpc::Status::Unknown, "", span_); + } +} + +TEST_F(ExtAuthzGrpcClientTest, Cancel) { + std::unique_ptr response; + envoy::api::v2::auth::CheckRequest request; + + EXPECT_CALL(*async_client_, send(_, _, _, _, _)).WillOnce(Return(&async_request_)); + + client_.check(request_callbacks_, request, Tracing::NullSpan::instance()); + + EXPECT_CALL(async_request_, cancel()); + client_.cancel(); +} + +TEST(ExtAuthzGrpcFactoryTest, NoCluster) { + Upstream::MockClusterManager cm; + + EXPECT_CALL(cm, get("foo")).WillOnce(Return(nullptr)); + EXPECT_THROW(GrpcFactoryImpl("foo", cm), EnvoyException); +} + +TEST(ExtAuthzGrpcFactoryTest, Create) { + Upstream::MockClusterManager cm; + + EXPECT_CALL(cm, get("foo")).Times(AtLeast(1)); + GrpcFactoryImpl factory("foo", cm); + factory.create(Optional()); +} + +TEST(ExtAuthzNullFactoryTest, Basic) { + NullFactoryImpl factory; + ClientPtr client = factory.create(Optional()); + MockRequestCallbacks request_callbacks; + envoy::api::v2::auth::CheckRequest request; + EXPECT_CALL(request_callbacks, complete(CheckStatus::OK)); + client->check(request_callbacks, request, Tracing::NullSpan::instance()); + client->cancel(); +} + +// TODO(saumoh): Add TEST for createHttpCheck and createTcpCheck + +} // namespace ExtAuthz +} // namespace Envoy diff --git a/test/mocks/ext_authz/BUILD b/test/mocks/ext_authz/BUILD new file mode 100644 index 0000000000000..0a1c65bd37e2d --- /dev/null +++ b/test/mocks/ext_authz/BUILD @@ -0,0 +1,18 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_mock", + "envoy_package", +) + +envoy_package() + +envoy_cc_mock( + name = "ext_authz_mocks", + srcs = ["mocks.cc"], + hdrs = ["mocks.h"], + deps = [ + "//include/envoy/ext_authz:ext_authz_interface" + ], +) diff --git a/test/mocks/ext_authz/mocks.cc b/test/mocks/ext_authz/mocks.cc new file mode 100644 index 0000000000000..6365eb1b90dac --- /dev/null +++ b/test/mocks/ext_authz/mocks.cc @@ -0,0 +1,13 @@ +#include "mocks.h" + +namespace Envoy { +namespace ExtAuthz { + +MockClient::MockClient() {} +MockClient::~MockClient() {} + +MockCheckRequestGen::MockCheckRequestGen() {} +MockCheckRequestGen::~MockCheckRequestGen() {} + +} // namespace ExtAuthz +} // namespace Envoy diff --git a/test/mocks/ext_authz/mocks.h b/test/mocks/ext_authz/mocks.h new file mode 100644 index 0000000000000..8db925e8391c9 --- /dev/null +++ b/test/mocks/ext_authz/mocks.h @@ -0,0 +1,40 @@ +#pragma once + +#include +#include + +#include "envoy/ext_authz/ext_authz.h" + +#include "gmock/gmock.h" + +namespace Envoy { +namespace ExtAuthz { + +class MockClient : public Client { +public: + MockClient(); + ~MockClient(); + + // ExtAuthz::Client + MOCK_METHOD0(cancel, void()); + MOCK_METHOD3(check, void(RequestCallbacks& callbacks, + const envoy::api::v2::auth::CheckRequest& request, + Tracing::Span& parent_span)); +}; + +class MockCheckRequestGen : public CheckRequestGenerator { +public: + MockCheckRequestGen(); + ~MockCheckRequestGen(); + + // ExtAuthz::CheckRequestGenerator + MOCK_METHOD3(createHttpCheck, void(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, + + const Envoy::Http::HeaderMap &headers, + envoy::api::v2::auth::CheckRequest& request)); + MOCK_METHOD2(createTcpCheck, void(const Network::ReadFilterCallbacks* callbacks, + envoy::api::v2::auth::CheckRequest& request)); +}; + +} // namespace ExtAuthz +} // namespace Envoy From bf7d7be05306e16916886f8dc1a4c144acf03136 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Tue, 23 Jan 2018 17:07:39 -0800 Subject: [PATCH 02/17] Merge with gRPC singleton manager. Signed-off-by: Saurabh Mohan --- source/common/ext_authz/BUILD | 2 +- source/common/ext_authz/ext_authz_impl.cc | 22 +++++-------- source/common/ext_authz/ext_authz_impl.h | 23 ++++++-------- test/common/ext_authz/ext_authz_impl_test.cc | 33 +++++++++----------- 4 files changed, 33 insertions(+), 47 deletions(-) diff --git a/source/common/ext_authz/BUILD b/source/common/ext_authz/BUILD index 61714cb4a3257..60077617e5483 100644 --- a/source/common/ext_authz/BUILD +++ b/source/common/ext_authz/BUILD @@ -12,10 +12,10 @@ envoy_cc_library( name = "ext_authz_lib", srcs = ["ext_authz_impl.cc"], hdrs = ["ext_authz_impl.h"], - external_deps = ["envoy_bootstrap"], deps = [ "//include/envoy/ext_authz:ext_authz_interface", "//include/envoy/grpc:async_client_interface", + "//include/envoy/grpc:async_client_manager_interface", "//include/envoy/http:protocol_interface", "//include/envoy/network:address_interface", "//include/envoy/network:connection_interface", diff --git a/source/common/ext_authz/ext_authz_impl.cc b/source/common/ext_authz/ext_authz_impl.cc index d508599f7a9d2..ce163f1cb2df8 100644 --- a/source/common/ext_authz/ext_authz_impl.cc +++ b/source/common/ext_authz/ext_authz_impl.cc @@ -18,11 +18,11 @@ namespace Envoy { namespace ExtAuthz { using ::envoy::api::v2::auth::AttributeContext; -using ::envoy::api::v2::auth::AttributeContext_HTTPRequest; +using ::envoy::api::v2::auth::AttributeContext_HttpRequest; using ::envoy::api::v2::auth::AttributeContext_Peer; using ::envoy::api::v2::auth::AttributeContext_Request; -GrpcClientImpl::GrpcClientImpl(ExtAuthzAsyncClientPtr&& async_client, +GrpcClientImpl::GrpcClientImpl(Grpc::AsyncClientPtr&& async_client, const Optional& timeout) : service_method_(*Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.api.v2.auth.Authorization.Check")), @@ -67,20 +67,14 @@ void GrpcClientImpl::onFailure(Grpc::Status::GrpcStatus status, const std::strin callbacks_ = nullptr; } -GrpcFactoryImpl::GrpcFactoryImpl(const std::string& cluster_name, - Upstream::ClusterManager& cm) - : cluster_name_(cluster_name), cm_(cm) { - if (!cm_.get(cluster_name_)) { - throw EnvoyException(fmt::format("unknown external authorization service cluster '{}'", cluster_name_)); - } +GrpcFactoryImpl::GrpcFactoryImpl(const envoy::api::v2::GrpcService &grpc_service, + Grpc::AsyncClientManager& async_client_manager, + Stats::Scope& scope) { + async_client_factory_ = async_client_manager.factoryForGrpcService(grpc_service, scope); } ClientPtr GrpcFactoryImpl::create(const Optional& timeout) { - return ClientPtr{new GrpcClientImpl( - ExtAuthzAsyncClientPtr{ - new Grpc::AsyncClientImpl(cm_, cluster_name_)}, - timeout)}; + return std::make_unique(async_client_factory_->create(), timeout); } std::unique_ptr<::envoy::api::v2::Address> ExtAuthzCheckRequestGenerator::getProtobufAddress(const Network::Address::InstanceConstSharedPtr& instance) { @@ -182,7 +176,7 @@ std::unique_ptr ExtAuthzCheckRequestGenerator::getHttp const Envoy::Http::HeaderMap &headers) { - AttributeContext_HTTPRequest *httpreq = new AttributeContext_HTTPRequest(); + AttributeContext_HttpRequest *httpreq = new AttributeContext_HttpRequest(); ASSERT(httpreq); // Set id diff --git a/source/common/ext_authz/ext_authz_impl.h b/source/common/ext_authz/ext_authz_impl.h index 54c3738718df3..168485b182b7f 100644 --- a/source/common/ext_authz/ext_authz_impl.h +++ b/source/common/ext_authz/ext_authz_impl.h @@ -7,6 +7,7 @@ #include "envoy/ext_authz/ext_authz.h" #include "envoy/grpc/async_client.h" +#include "envoy/grpc/async_client_manager.h" #include "envoy/http/protocol.h" #include "envoy/network/address.h" #include "envoy/network/connection.h" @@ -16,17 +17,11 @@ #include "common/singleton/const_singleton.h" -#include "api/bootstrap.pb.h" - namespace Envoy { namespace ExtAuthz { -typedef Grpc::AsyncClient - ExtAuthzAsyncClient; -typedef std::unique_ptr ExtAuthzAsyncClientPtr; - -typedef Grpc::AsyncRequestCallbacks ExtAuthzAsyncCallbacks; +typedef Grpc::TypedAsyncRequestCallbacks + ExtAuthzAsyncCallbacks; struct ConstantValues { const std::string TraceStatus = "ext_authz_status"; @@ -40,7 +35,7 @@ typedef ConstSingleton Constants; // This will require support for more than one outstanding request per client. class GrpcClientImpl : public Client, public ExtAuthzAsyncCallbacks { public: - GrpcClientImpl(ExtAuthzAsyncClientPtr&& async_client, + GrpcClientImpl(Grpc::AsyncClientPtr&& async_client, const Optional& timeout); ~GrpcClientImpl(); @@ -58,7 +53,7 @@ class GrpcClientImpl : public Client, public ExtAuthzAsyncCallbacks { private: const Protobuf::MethodDescriptor& service_method_; - ExtAuthzAsyncClientPtr async_client_; + Grpc::AsyncClientPtr async_client_; Grpc::AsyncRequest* request_{}; Optional timeout_; RequestCallbacks* callbacks_{}; @@ -66,15 +61,15 @@ class GrpcClientImpl : public Client, public ExtAuthzAsyncCallbacks { class GrpcFactoryImpl : public ClientFactory { public: - GrpcFactoryImpl(const std::string& cluster_name, - Upstream::ClusterManager& cm); + GrpcFactoryImpl(const envoy::api::v2::GrpcService &grpc_service, + Grpc::AsyncClientManager& async_client_manager, Stats::Scope& scope); // ExtAuthz::ClientFactory ClientPtr create(const Optional& timeout) override; private: - const std::string cluster_name_; - Upstream::ClusterManager& cm_; + Grpc::AsyncClientFactoryPtr async_client_factory_; + std::string cluster_name_; }; class NullClientImpl : public Client { diff --git a/test/common/ext_authz/ext_authz_impl_test.cc b/test/common/ext_authz/ext_authz_impl_test.cc index a51637618c0d4..941a9d753b640 100644 --- a/test/common/ext_authz/ext_authz_impl_test.cc +++ b/test/common/ext_authz/ext_authz_impl_test.cc @@ -32,12 +32,10 @@ class MockRequestCallbacks : public RequestCallbacks { class ExtAuthzGrpcClientTest : public testing::Test { public: ExtAuthzGrpcClientTest() - : async_client_(new Grpc::MockAsyncClient()), - client_(ExtAuthzAsyncClientPtr{async_client_}, Optional()) {} + : async_client_(new Grpc::MockAsyncClient()), + client_(Grpc::AsyncClientPtr{async_client_}, Optional()) {} - Grpc::MockAsyncClient* async_client_; + Grpc::MockAsyncClient* async_client_; Grpc::MockAsyncRequest async_request_; GrpcClientImpl client_; MockRequestCallbacks request_callbacks_; @@ -53,8 +51,8 @@ TEST_F(ExtAuthzGrpcClientTest, Basic) { EXPECT_CALL(*async_client_, send(_, ProtoEq(request), Ref(client_), _, _)) .WillOnce(Invoke([this]( const Protobuf::MethodDescriptor& service_method, - const envoy::api::v2::auth::CheckRequest&, - Grpc::AsyncRequestCallbacks&, + const Protobuf::Message&, + Grpc::AsyncRequestCallbacks&, Tracing::Span&, const Optional&) -> Grpc::AsyncRequest* { EXPECT_EQ("envoy.api.v2.auth.Authorization", service_method.service()->full_name()); @@ -123,18 +121,17 @@ TEST_F(ExtAuthzGrpcClientTest, Cancel) { client_.cancel(); } -TEST(ExtAuthzGrpcFactoryTest, NoCluster) { - Upstream::MockClusterManager cm; - - EXPECT_CALL(cm, get("foo")).WillOnce(Return(nullptr)); - EXPECT_THROW(GrpcFactoryImpl("foo", cm), EnvoyException); -} - TEST(ExtAuthzGrpcFactoryTest, Create) { - Upstream::MockClusterManager cm; - - EXPECT_CALL(cm, get("foo")).Times(AtLeast(1)); - GrpcFactoryImpl factory("foo", cm); + envoy::api::v2::GrpcService config; + config.mutable_envoy_grpc()->set_cluster_name("foo"); + Grpc::MockAsyncClientManager async_client_manager; + Stats::MockStore scope; + EXPECT_CALL(async_client_manager, + factoryForGrpcService(ProtoEq(config), Ref(scope))) + .WillOnce(Invoke([](const envoy::api::v2::GrpcService&, Stats::Scope&) { + return std::make_unique>(); + })); + GrpcFactoryImpl factory(config, async_client_manager, scope); factory.create(Optional()); } From 78347052056f374d1cf6bc0ab9367a7c32719aa6 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Wed, 24 Jan 2018 16:27:54 -0800 Subject: [PATCH 03/17] Fix build with clang and test of request_info. Signed-off-by: Saurabh Mohan --- source/common/ext_authz/ext_authz_impl.cc | 8 ++++---- test/common/request_info/utility_test.cc | 6 ++++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/source/common/ext_authz/ext_authz_impl.cc b/source/common/ext_authz/ext_authz_impl.cc index ce163f1cb2df8..fae273b402913 100644 --- a/source/common/ext_authz/ext_authz_impl.cc +++ b/source/common/ext_authz/ext_authz_impl.cc @@ -185,13 +185,13 @@ std::unique_ptr ExtAuthzCheckRequestGenerator::getHttp httpreq->set_id(std::to_string(sdfc->streamId())); // Set method - httpreq->set_method(std::move(getHeaderStr(headers.Method()))); + httpreq->set_method(getHeaderStr(headers.Method())); // Set path - httpreq->set_path(std::move(getHeaderStr(headers.Path()))); + httpreq->set_path(getHeaderStr(headers.Path())); // Set host - httpreq->set_host(std::move(getHeaderStr(headers.Host()))); + httpreq->set_host(getHeaderStr(headers.Host())); // Set scheme - httpreq->set_scheme(std::move(getHeaderStr(headers.Scheme()))); + httpreq->set_scheme(getHeaderStr(headers.Scheme())); // Set size // need to convert to google buffer 64t; diff --git a/test/common/request_info/utility_test.cc b/test/common/request_info/utility_test.cc index 583d26fe2a336..34191b7226690 100644 --- a/test/common/request_info/utility_test.cc +++ b/test/common/request_info/utility_test.cc @@ -14,7 +14,7 @@ namespace Envoy { namespace RequestInfo { TEST(ResponseFlagUtilsTest, toShortStringConversion) { - static_assert(ResponseFlag::LastFlag == 0x800, "A flag has been added. Fix this code."); + static_assert(ResponseFlag::LastFlag == 0x1000, "A flag has been added. Fix this code."); std::vector> expected = { std::make_pair(ResponseFlag::FailedLocalHealthCheck, "LH"), @@ -28,7 +28,9 @@ TEST(ResponseFlagUtilsTest, toShortStringConversion) { std::make_pair(ResponseFlag::NoRouteFound, "NR"), std::make_pair(ResponseFlag::DelayInjected, "DI"), std::make_pair(ResponseFlag::FaultInjected, "FI"), - std::make_pair(ResponseFlag::RateLimited, "RL")}; + std::make_pair(ResponseFlag::RateLimited, "RL"), + std::make_pair(ResponseFlag::Unauthorized, "UA"), + }; for (const auto& test_case : expected) { NiceMock request_info; From 48edb729e100fd21980c9cbb2adfc9892b2e0362 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Wed, 24 Jan 2018 17:03:38 -0800 Subject: [PATCH 04/17] Format fixes for common files. Signed-off-by: Saurabh Mohan --- .../common/access_log/grpc_access_log_impl.cc | 1 - source/common/ext_authz/BUILD | 3 +- source/common/ext_authz/ext_authz_impl.cc | 86 ++++++++++--------- source/common/ext_authz/ext_authz_impl.h | 28 +++--- 4 files changed, 60 insertions(+), 58 deletions(-) diff --git a/source/common/access_log/grpc_access_log_impl.cc b/source/common/access_log/grpc_access_log_impl.cc index f29f3b4e6b5cd..234dcf96ca7d6 100644 --- a/source/common/access_log/grpc_access_log_impl.cc +++ b/source/common/access_log/grpc_access_log_impl.cc @@ -137,7 +137,6 @@ void HttpGrpcAccessLog::responseFlagsToAccessLogResponseFlags( // if (request_info.getResponseFlag(RequestInfo::ResponseFlag::Unauthorized)) { // common_access_log.mutable_response_flags()->set_unauthorized(true); // } - } void HttpGrpcAccessLog::log(const Http::HeaderMap* request_headers, diff --git a/source/common/ext_authz/BUILD b/source/common/ext_authz/BUILD index 60077617e5483..dd3d076a76d93 100644 --- a/source/common/ext_authz/BUILD +++ b/source/common/ext_authz/BUILD @@ -19,12 +19,11 @@ envoy_cc_library( "//include/envoy/http:protocol_interface", "//include/envoy/network:address_interface", "//include/envoy/network:connection_interface", - "//include/envoy/upstream:cluster_manager_interface", "//include/envoy/ssl:connection_interface", + "//include/envoy/upstream:cluster_manager_interface", "//source/common/common:assert_lib", "//source/common/grpc:async_client_lib", "//source/common/http:headers_lib", "//source/common/tracing:http_tracer_lib", ], ) - diff --git a/source/common/ext_authz/ext_authz_impl.cc b/source/common/ext_authz/ext_authz_impl.cc index fae273b402913..83ec4d283b620 100644 --- a/source/common/ext_authz/ext_authz_impl.cc +++ b/source/common/ext_authz/ext_authz_impl.cc @@ -36,7 +36,8 @@ void GrpcClientImpl::cancel() { callbacks_ = nullptr; } -void GrpcClientImpl::check(RequestCallbacks& callbacks, const envoy::api::v2::auth::CheckRequest& request, +void GrpcClientImpl::check(RequestCallbacks& callbacks, + const envoy::api::v2::auth::CheckRequest& request, Tracing::Span& parent_span) { ASSERT(callbacks_ == nullptr); callbacks_ = &callbacks; @@ -67,7 +68,7 @@ void GrpcClientImpl::onFailure(Grpc::Status::GrpcStatus status, const std::strin callbacks_ = nullptr; } -GrpcFactoryImpl::GrpcFactoryImpl(const envoy::api::v2::GrpcService &grpc_service, +GrpcFactoryImpl::GrpcFactoryImpl(const envoy::api::v2::GrpcService& grpc_service, Grpc::AsyncClientManager& async_client_manager, Stats::Scope& scope) { async_client_factory_ = async_client_manager.factoryForGrpcService(grpc_service, scope); @@ -77,9 +78,11 @@ ClientPtr GrpcFactoryImpl::create(const Optional& tim return std::make_unique(async_client_factory_->create(), timeout); } -std::unique_ptr<::envoy::api::v2::Address> ExtAuthzCheckRequestGenerator::getProtobufAddress(const Network::Address::InstanceConstSharedPtr& instance) { +std::unique_ptr<::envoy::api::v2::Address> ExtAuthzCheckRequestGenerator::getProtobufAddress( + const Network::Address::InstanceConstSharedPtr& instance) { - std::unique_ptr<::envoy::api::v2::Address> addr = std::unique_ptr<::envoy::api::v2::Address>{new ::envoy::api::v2::Address()}; + std::unique_ptr<::envoy::api::v2::Address> addr = + std::unique_ptr<::envoy::api::v2::Address>{new ::envoy::api::v2::Address()}; ASSERT(addr); if (instance->type() == Network::Address::Type::Ip) { @@ -92,11 +95,12 @@ std::unique_ptr<::envoy::api::v2::Address> ExtAuthzCheckRequestGenerator::getPro return addr; } -std::unique_ptr ExtAuthzCheckRequestGenerator::getConnectionPeer(const Network::Connection *connection, - const std::string& service, - const bool local) { +std::unique_ptr +ExtAuthzCheckRequestGenerator::getConnectionPeer(const Network::Connection* connection, + const std::string& service, const bool local) { - std::unique_ptr peer = std::unique_ptr{new AttributeContext_Peer()}; + std::unique_ptr peer = + std::unique_ptr{new AttributeContext_Peer()}; ASSERT(peer); // Set the address @@ -110,8 +114,7 @@ std::unique_ptr ExtAuthzCheckRequestGenerator::getConnect // Preferably the SAN from the peer's cert or // Subject from the peer's cert. std::string principal; - Ssl::Connection* ssl = - const_cast(connection->ssl()); + Ssl::Connection* ssl = const_cast(connection->ssl()); if (ssl != nullptr) { if (!local) { principal = ssl->uriSanPeerCertificate(); @@ -136,52 +139,54 @@ std::unique_ptr ExtAuthzCheckRequestGenerator::getConnect return peer; } -std::unique_ptr ExtAuthzCheckRequestGenerator::getConnectionPeer(const Network::Connection& connection, - const std::string& service, - const bool local) { +std::unique_ptr +ExtAuthzCheckRequestGenerator::getConnectionPeer(const Network::Connection& connection, + const std::string& service, const bool local) { return getConnectionPeer(&connection, service, local); } - const std::string ExtAuthzCheckRequestGenerator::getProtocolStr(const Envoy::Http::Protocol& p) { - switch(p) { - case Envoy::Http::Protocol::Http10: - return std::string("Http1.0"); - case Envoy::Http::Protocol::Http11: - return std::string("Http1.1"); - case Envoy::Http::Protocol::Http2: - return std::string("Http2"); - default: - break; + switch (p) { + case Envoy::Http::Protocol::Http10: + return std::string("Http1.0"); + case Envoy::Http::Protocol::Http11: + return std::string("Http1.1"); + case Envoy::Http::Protocol::Http2: + return std::string("Http2"); + default: + break; } return std::string("unknown"); } -Envoy::Http::HeaderMap::Iterate ExtAuthzCheckRequestGenerator::fillHttpHeaders(const Envoy::Http::HeaderEntry &e, - void *ctx) { - ::google::protobuf::Map< ::std::string, ::std::string >* mhdrs = static_cast<::google::protobuf::Map< ::std::string, ::std::string >*>(ctx); - (*mhdrs)[std::string(e.key().c_str(), e.key().size())] = std::string(e.value().c_str(), e.value().size()); +Envoy::Http::HeaderMap::Iterate +ExtAuthzCheckRequestGenerator::fillHttpHeaders(const Envoy::Http::HeaderEntry& e, void* ctx) { + ::google::protobuf::Map<::std::string, ::std::string>* mhdrs = + static_cast<::google::protobuf::Map<::std::string, ::std::string>*>(ctx); + (*mhdrs)[std::string(e.key().c_str(), e.key().size())] = + std::string(e.value().c_str(), e.value().size()); return Envoy::Http::HeaderMap::Iterate::Continue; } -std::string ExtAuthzCheckRequestGenerator::getHeaderStr(const Envoy::Http::HeaderEntry *entry) { +std::string ExtAuthzCheckRequestGenerator::getHeaderStr(const Envoy::Http::HeaderEntry* entry) { if (entry) { return std::string(entry->value().c_str(), entry->value().size()); } return ""; } -std::unique_ptr ExtAuthzCheckRequestGenerator::getHttpRequest(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, - const Envoy::Http::HeaderMap &headers) { - +std::unique_ptr ExtAuthzCheckRequestGenerator::getHttpRequest( + const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, + const Envoy::Http::HeaderMap& headers) { - AttributeContext_HttpRequest *httpreq = new AttributeContext_HttpRequest(); + AttributeContext_HttpRequest* httpreq = new AttributeContext_HttpRequest(); ASSERT(httpreq); // Set id // The streamId is not qualified as a const. Although it is as it does not modify the object. - Envoy::Http::StreamDecoderFilterCallbacks *sdfc = const_cast(callbacks); + Envoy::Http::StreamDecoderFilterCallbacks* sdfc = + const_cast(callbacks); httpreq->set_id(std::to_string(sdfc->streamId())); // Set method @@ -204,21 +209,23 @@ std::unique_ptr ExtAuthzCheckRequestGenerator::getHttp auto* mhdrs = httpreq->mutable_headers(); headers.iterate(fillHttpHeaders, mhdrs); - std::unique_ptr req = std::unique_ptr{new AttributeContext_Request()}; + std::unique_ptr req = + std::unique_ptr{new AttributeContext_Request()}; ASSERT(req); req->set_allocated_http(httpreq); return req; } -void ExtAuthzCheckRequestGenerator::createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, - const Envoy::Http::HeaderMap &headers, - envoy::api::v2::auth::CheckRequest& request) { +void ExtAuthzCheckRequestGenerator::createHttpCheck( + const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, + const Envoy::Http::HeaderMap& headers, envoy::api::v2::auth::CheckRequest& request) { AttributeContext* attrs = request.mutable_attributes(); ASSERT(attrs); - Envoy::Http::StreamDecoderFilterCallbacks* cb = const_cast(callbacks); + Envoy::Http::StreamDecoderFilterCallbacks* cb = + const_cast(callbacks); std::string service = getHeaderStr(headers.EnvoyDownstreamServiceCluster()); attrs->set_allocated_source(getConnectionPeer(cb->connection(), service, false).release()); @@ -232,11 +239,10 @@ void ExtAuthzCheckRequestGenerator::createTcpCheck(const Network::ReadFilterCall AttributeContext* attrs = request.mutable_attributes(); ASSERT(attrs); - Network::ReadFilterCallbacks* cb = const_cast(callbacks); + Network::ReadFilterCallbacks* cb = const_cast(callbacks); attrs->set_allocated_source(getConnectionPeer(cb->connection(), "", false).release()); attrs->set_allocated_destination(getConnectionPeer(cb->connection(), "", true).release()); } } // namespace ExtAuthz } // namespace Envoy - diff --git a/source/common/ext_authz/ext_authz_impl.h b/source/common/ext_authz/ext_authz_impl.h index 168485b182b7f..30204470f4a61 100644 --- a/source/common/ext_authz/ext_authz_impl.h +++ b/source/common/ext_authz/ext_authz_impl.h @@ -11,7 +11,6 @@ #include "envoy/http/protocol.h" #include "envoy/network/address.h" #include "envoy/network/connection.h" - #include "envoy/tracing/http_tracer.h" #include "envoy/upstream/cluster_manager.h" @@ -61,7 +60,7 @@ class GrpcClientImpl : public Client, public ExtAuthzAsyncCallbacks { class GrpcFactoryImpl : public ClientFactory { public: - GrpcFactoryImpl(const envoy::api::v2::GrpcService &grpc_service, + GrpcFactoryImpl(const envoy::api::v2::GrpcService& grpc_service, Grpc::AsyncClientManager& async_client_manager, Stats::Scope& scope); // ExtAuthz::ClientFactory @@ -90,31 +89,30 @@ class NullFactoryImpl : public ClientFactory { } }; -class ExtAuthzCheckRequestGenerator: public CheckRequestGenerator { +class ExtAuthzCheckRequestGenerator : public CheckRequestGenerator { public: ExtAuthzCheckRequestGenerator() {} ~ExtAuthzCheckRequestGenerator() {} // ExtAuthz::CheckRequestGenIntf void createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, - const Envoy::Http::HeaderMap &headers, + const Envoy::Http::HeaderMap& headers, envoy::api::v2::auth::CheckRequest& request); void createTcpCheck(const Network::ReadFilterCallbacks* callbacks, envoy::api::v2::auth::CheckRequest& request); private: - std::unique_ptr<::envoy::api::v2::Address> getProtobufAddress(const Network::Address::InstanceConstSharedPtr&); - std::unique_ptr<::envoy::api::v2::auth::AttributeContext_Peer> getConnectionPeer(const Network::Connection *, - const std::string&, - const bool); - std::unique_ptr<::envoy::api::v2::auth::AttributeContext_Peer> getConnectionPeer(const Network::Connection&, - const std::string&, - const bool); - std::unique_ptr<::envoy::api::v2::auth::AttributeContext_Request> getHttpRequest(const Envoy::Http::StreamDecoderFilterCallbacks*, - const Envoy::Http::HeaderMap &); + std::unique_ptr<::envoy::api::v2::Address> + getProtobufAddress(const Network::Address::InstanceConstSharedPtr&); + std::unique_ptr<::envoy::api::v2::auth::AttributeContext_Peer> + getConnectionPeer(const Network::Connection*, const std::string&, const bool); + std::unique_ptr<::envoy::api::v2::auth::AttributeContext_Peer> + getConnectionPeer(const Network::Connection&, const std::string&, const bool); + std::unique_ptr<::envoy::api::v2::auth::AttributeContext_Request> + getHttpRequest(const Envoy::Http::StreamDecoderFilterCallbacks*, const Envoy::Http::HeaderMap&); const std::string getProtocolStr(const Envoy::Http::Protocol&); - std::string getHeaderStr(const Envoy::Http::HeaderEntry *entry); - static Envoy::Http::HeaderMap::Iterate fillHttpHeaders(const Envoy::Http::HeaderEntry&, void *); + std::string getHeaderStr(const Envoy::Http::HeaderEntry* entry); + static Envoy::Http::HeaderMap::Iterate fillHttpHeaders(const Envoy::Http::HeaderEntry&, void*); }; } // namespace ExtAuthz From 1612396ee84ab5850a480165dbeaddbf0cddf096 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Wed, 24 Jan 2018 22:05:24 -0800 Subject: [PATCH 05/17] Use auto and make_unique. Signed-off-by: Saurabh Mohan --- source/common/ext_authz/BUILD | 1 + source/common/ext_authz/ext_authz_impl.cc | 29 +++++++++-------------- source/common/ext_authz/ext_authz_impl.h | 3 --- 3 files changed, 12 insertions(+), 21 deletions(-) diff --git a/source/common/ext_authz/BUILD b/source/common/ext_authz/BUILD index dd3d076a76d93..0fc1c60a16d46 100644 --- a/source/common/ext_authz/BUILD +++ b/source/common/ext_authz/BUILD @@ -24,6 +24,7 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/grpc:async_client_lib", "//source/common/http:headers_lib", + "//source/common/protobuf", "//source/common/tracing:http_tracer_lib", ], ) diff --git a/source/common/ext_authz/ext_authz_impl.cc b/source/common/ext_authz/ext_authz_impl.cc index 83ec4d283b620..355ea0735f03b 100644 --- a/source/common/ext_authz/ext_authz_impl.cc +++ b/source/common/ext_authz/ext_authz_impl.cc @@ -11,6 +11,7 @@ #include "common/common/assert.h" #include "common/grpc/async_client_impl.h" #include "common/http/headers.h" +#include "common/protobuf/protobuf.h" #include "fmt/format.h" @@ -81,9 +82,7 @@ ClientPtr GrpcFactoryImpl::create(const Optional& tim std::unique_ptr<::envoy::api::v2::Address> ExtAuthzCheckRequestGenerator::getProtobufAddress( const Network::Address::InstanceConstSharedPtr& instance) { - std::unique_ptr<::envoy::api::v2::Address> addr = - std::unique_ptr<::envoy::api::v2::Address>{new ::envoy::api::v2::Address()}; - ASSERT(addr); + auto addr = std::make_unique<::envoy::api::v2::Address>(); if (instance->type() == Network::Address::Type::Ip) { addr->mutable_socket_address()->set_address(instance->ip()->addressAsString()); @@ -99,9 +98,7 @@ std::unique_ptr ExtAuthzCheckRequestGenerator::getConnectionPeer(const Network::Connection* connection, const std::string& service, const bool local) { - std::unique_ptr peer = - std::unique_ptr{new AttributeContext_Peer()}; - ASSERT(peer); + auto peer = std::make_unique(); // Set the address if (!local) { @@ -162,8 +159,8 @@ const std::string ExtAuthzCheckRequestGenerator::getProtocolStr(const Envoy::Htt Envoy::Http::HeaderMap::Iterate ExtAuthzCheckRequestGenerator::fillHttpHeaders(const Envoy::Http::HeaderEntry& e, void* ctx) { - ::google::protobuf::Map<::std::string, ::std::string>* mhdrs = - static_cast<::google::protobuf::Map<::std::string, ::std::string>*>(ctx); + Envoy::Protobuf::Map<::std::string, ::std::string>* mhdrs = + static_cast*>(ctx); (*mhdrs)[std::string(e.key().c_str(), e.key().size())] = std::string(e.value().c_str(), e.value().size()); return Envoy::Http::HeaderMap::Iterate::Continue; @@ -180,8 +177,7 @@ std::unique_ptr ExtAuthzCheckRequestGenerator::getHttp const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, const Envoy::Http::HeaderMap& headers) { - AttributeContext_HttpRequest* httpreq = new AttributeContext_HttpRequest(); - ASSERT(httpreq); + auto httpreq = new AttributeContext_HttpRequest(); // Set id // The streamId is not qualified as a const. Although it is as it does not modify the object. @@ -206,12 +202,11 @@ std::unique_ptr ExtAuthzCheckRequestGenerator::getHttp httpreq->set_protocol(getProtocolStr(sdfc->requestInfo().protocol().value())); // Fill in the headers - auto* mhdrs = httpreq->mutable_headers(); + auto mhdrs = httpreq->mutable_headers(); headers.iterate(fillHttpHeaders, mhdrs); - std::unique_ptr req = - std::unique_ptr{new AttributeContext_Request()}; - ASSERT(req); + auto req = std::make_unique(); + req->set_allocated_http(httpreq); return req; @@ -221,8 +216,7 @@ void ExtAuthzCheckRequestGenerator::createHttpCheck( const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, const Envoy::Http::HeaderMap& headers, envoy::api::v2::auth::CheckRequest& request) { - AttributeContext* attrs = request.mutable_attributes(); - ASSERT(attrs); + auto attrs = request.mutable_attributes(); Envoy::Http::StreamDecoderFilterCallbacks* cb = const_cast(callbacks); @@ -236,8 +230,7 @@ void ExtAuthzCheckRequestGenerator::createHttpCheck( void ExtAuthzCheckRequestGenerator::createTcpCheck(const Network::ReadFilterCallbacks* callbacks, envoy::api::v2::auth::CheckRequest& request) { - AttributeContext* attrs = request.mutable_attributes(); - ASSERT(attrs); + auto attrs = request.mutable_attributes(); Network::ReadFilterCallbacks* cb = const_cast(callbacks); attrs->set_allocated_source(getConnectionPeer(cb->connection(), "", false).release()); diff --git a/source/common/ext_authz/ext_authz_impl.h b/source/common/ext_authz/ext_authz_impl.h index 30204470f4a61..1915204dc48f3 100644 --- a/source/common/ext_authz/ext_authz_impl.h +++ b/source/common/ext_authz/ext_authz_impl.h @@ -91,9 +91,6 @@ class NullFactoryImpl : public ClientFactory { class ExtAuthzCheckRequestGenerator : public CheckRequestGenerator { public: - ExtAuthzCheckRequestGenerator() {} - ~ExtAuthzCheckRequestGenerator() {} - // ExtAuthz::CheckRequestGenIntf void createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, const Envoy::Http::HeaderMap& headers, From 4a197fb91f1d397afe64ac35c59ad4f8616fd0c0 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Thu, 25 Jan 2018 12:22:22 -0800 Subject: [PATCH 06/17] Fix test common format. Signed-off-by: Saurabh Mohan --- test/common/ext_authz/BUILD | 2 +- test/common/ext_authz/ext_authz_impl_test.cc | 34 ++++++++------------ test/mocks/ext_authz/BUILD | 2 +- test/mocks/ext_authz/mocks.h | 22 ++++++------- 4 files changed, 27 insertions(+), 33 deletions(-) diff --git a/test/common/ext_authz/BUILD b/test/common/ext_authz/BUILD index 7486369149a29..a5ff1d09c1358 100644 --- a/test/common/ext_authz/BUILD +++ b/test/common/ext_authz/BUILD @@ -12,9 +12,9 @@ envoy_cc_test( name = "ext_authz_impl_test", srcs = ["ext_authz_impl_test.cc"], deps = [ + "//source/common/ext_authz:ext_authz_lib", "//source/common/http:header_map_lib", "//source/common/http:headers_lib", - "//source/common/ext_authz:ext_authz_lib", "//test/mocks/grpc:grpc_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:utility_lib", diff --git a/test/common/ext_authz/ext_authz_impl_test.cc b/test/common/ext_authz/ext_authz_impl_test.cc index 941a9d753b640..a47da3a12a4d7 100644 --- a/test/common/ext_authz/ext_authz_impl_test.cc +++ b/test/common/ext_authz/ext_authz_impl_test.cc @@ -2,9 +2,9 @@ #include #include +#include "common/ext_authz/ext_authz_impl.h" #include "common/http/header_map_impl.h" #include "common/http/headers.h" -#include "common/ext_authz/ext_authz_impl.h" #include "test/mocks/grpc/mocks.h" #include "test/mocks/upstream/mocks.h" @@ -49,16 +49,14 @@ TEST_F(ExtAuthzGrpcClientTest, Basic) { envoy::api::v2::auth::CheckRequest request; Http::HeaderMapImpl headers; EXPECT_CALL(*async_client_, send(_, ProtoEq(request), Ref(client_), _, _)) - .WillOnce(Invoke([this]( - const Protobuf::MethodDescriptor& service_method, - const Protobuf::Message&, - Grpc::AsyncRequestCallbacks&, - Tracing::Span&, - const Optional&) -> Grpc::AsyncRequest* { - EXPECT_EQ("envoy.api.v2.auth.Authorization", service_method.service()->full_name()); - EXPECT_EQ("Check", service_method.name()); - return &async_request_; - })); + .WillOnce( + Invoke([this](const Protobuf::MethodDescriptor& service_method, + const Protobuf::Message&, Grpc::AsyncRequestCallbacks&, Tracing::Span&, + const Optional&) -> Grpc::AsyncRequest* { + EXPECT_EQ("envoy.api.v2.auth.Authorization", service_method.service()->full_name()); + EXPECT_EQ("Check", service_method.name()); + return &async_request_; + })); client_.check(request_callbacks_, request, Tracing::NullSpan::instance()); @@ -66,7 +64,7 @@ TEST_F(ExtAuthzGrpcClientTest, Basic) { EXPECT_EQ(nullptr, headers.RequestId()); response.reset(new envoy::api::v2::auth::CheckResponse()); - ::google::rpc::Status *status = new ::google::rpc::Status(); + ::google::rpc::Status* status = new ::google::rpc::Status(); status->set_code(Grpc::Status::GrpcStatus::PermissionDenied); response->set_allocated_status(status); EXPECT_CALL(span_, setTag("ext_authz_status", "ext_authz_unauthorized")); @@ -80,13 +78,12 @@ TEST_F(ExtAuthzGrpcClientTest, Basic) { EXPECT_CALL(*async_client_, send(_, ProtoEq(request), _, _, _)) .WillOnce(Return(&async_request_)); - client_.check(request_callbacks_, request, - Tracing::NullSpan::instance()); + client_.check(request_callbacks_, request, Tracing::NullSpan::instance()); client_.onCreateInitialMetadata(headers); response.reset(new envoy::api::v2::auth::CheckResponse()); - ::google::rpc::Status *status = new ::google::rpc::Status(); + ::google::rpc::Status* status = new ::google::rpc::Status(); status->set_code(Grpc::Status::GrpcStatus::Ok); response->set_allocated_status(status); EXPECT_CALL(span_, setTag("ext_authz_status", "ext_authz_ok")); @@ -94,14 +91,12 @@ TEST_F(ExtAuthzGrpcClientTest, Basic) { client_.onSuccess(std::move(response), span_); } - { envoy::api::v2::auth::CheckRequest request; EXPECT_CALL(*async_client_, send(_, ProtoEq(request), _, _, _)) .WillOnce(Return(&async_request_)); - client_.check(request_callbacks_, request, - Tracing::NullSpan::instance()); + client_.check(request_callbacks_, request, Tracing::NullSpan::instance()); response.reset(new envoy::api::v2::auth::CheckResponse()); EXPECT_CALL(request_callbacks_, complete(CheckStatus::Error)); @@ -126,8 +121,7 @@ TEST(ExtAuthzGrpcFactoryTest, Create) { config.mutable_envoy_grpc()->set_cluster_name("foo"); Grpc::MockAsyncClientManager async_client_manager; Stats::MockStore scope; - EXPECT_CALL(async_client_manager, - factoryForGrpcService(ProtoEq(config), Ref(scope))) + EXPECT_CALL(async_client_manager, factoryForGrpcService(ProtoEq(config), Ref(scope))) .WillOnce(Invoke([](const envoy::api::v2::GrpcService&, Stats::Scope&) { return std::make_unique>(); })); diff --git a/test/mocks/ext_authz/BUILD b/test/mocks/ext_authz/BUILD index 0a1c65bd37e2d..725c43b303014 100644 --- a/test/mocks/ext_authz/BUILD +++ b/test/mocks/ext_authz/BUILD @@ -13,6 +13,6 @@ envoy_cc_mock( srcs = ["mocks.cc"], hdrs = ["mocks.h"], deps = [ - "//include/envoy/ext_authz:ext_authz_interface" + "//include/envoy/ext_authz:ext_authz_interface", ], ) diff --git a/test/mocks/ext_authz/mocks.h b/test/mocks/ext_authz/mocks.h index 8db925e8391c9..59ca9fbf95598 100644 --- a/test/mocks/ext_authz/mocks.h +++ b/test/mocks/ext_authz/mocks.h @@ -17,23 +17,23 @@ class MockClient : public Client { // ExtAuthz::Client MOCK_METHOD0(cancel, void()); - MOCK_METHOD3(check, void(RequestCallbacks& callbacks, - const envoy::api::v2::auth::CheckRequest& request, - Tracing::Span& parent_span)); + MOCK_METHOD3(check, + void(RequestCallbacks& callbacks, const envoy::api::v2::auth::CheckRequest& request, + Tracing::Span& parent_span)); }; class MockCheckRequestGen : public CheckRequestGenerator { public: - MockCheckRequestGen(); - ~MockCheckRequestGen(); + MockCheckRequestGen(); + ~MockCheckRequestGen(); - // ExtAuthz::CheckRequestGenerator - MOCK_METHOD3(createHttpCheck, void(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, + // ExtAuthz::CheckRequestGenerator + MOCK_METHOD3(createHttpCheck, void(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, - const Envoy::Http::HeaderMap &headers, - envoy::api::v2::auth::CheckRequest& request)); - MOCK_METHOD2(createTcpCheck, void(const Network::ReadFilterCallbacks* callbacks, - envoy::api::v2::auth::CheckRequest& request)); + const Envoy::Http::HeaderMap& headers, + envoy::api::v2::auth::CheckRequest& request)); + MOCK_METHOD2(createTcpCheck, void(const Network::ReadFilterCallbacks* callbacks, + envoy::api::v2::auth::CheckRequest& request)); }; } // namespace ExtAuthz From 141a203e47df1ba097e056b9395d8d47a0864f3b Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Thu, 25 Jan 2018 12:30:00 -0800 Subject: [PATCH 07/17] More format issues in common. Signed-off-by: Saurabh Mohan --- include/envoy/ext_authz/BUILD | 1 - include/envoy/ext_authz/ext_authz.h | 12 ++++-------- source/common/config/well_known_names.h | 4 ++-- source/server/BUILD | 2 +- 4 files changed, 7 insertions(+), 12 deletions(-) diff --git a/include/envoy/ext_authz/BUILD b/include/envoy/ext_authz/BUILD index 236006520a0bc..f7b2f326c713b 100644 --- a/include/envoy/ext_authz/BUILD +++ b/include/envoy/ext_authz/BUILD @@ -20,4 +20,3 @@ envoy_cc_library( "//include/envoy/tracing:http_tracer_interface", ], ) - diff --git a/include/envoy/ext_authz/ext_authz.h b/include/envoy/ext_authz/ext_authz.h index 2727ff698eb7c..78167325cfed5 100644 --- a/include/envoy/ext_authz/ext_authz.h +++ b/include/envoy/ext_authz/ext_authz.h @@ -42,9 +42,8 @@ class RequestCallbacks { virtual void complete(CheckStatus status) PURE; }; - class Client { - public: +public: // Destructor virtual ~Client() {} @@ -54,9 +53,8 @@ class Client { virtual void cancel() PURE; // A check call. - virtual void check(RequestCallbacks &callback, const envoy::api::v2::auth::CheckRequest& request, + virtual void check(RequestCallbacks& callback, const envoy::api::v2::auth::CheckRequest& request, Tracing::Span& parent_span) PURE; - }; typedef std::unique_ptr ClientPtr; @@ -76,7 +74,6 @@ class ClientFactory { typedef std::unique_ptr ClientFactoryPtr; - /** * An interface for creating ext_authz.proto (authorization) request. * CheckRequestGenerator is used to extract attributes from the TCP/HTTP request @@ -85,11 +82,11 @@ typedef std::unique_ptr ClientFactoryPtr; */ class CheckRequestGenerator { public: - // Destructor + // Destructor virtual ~CheckRequestGenerator() {} virtual void createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, - const Envoy::Http::HeaderMap &headers, + const Envoy::Http::HeaderMap& headers, envoy::api::v2::auth::CheckRequest& request) PURE; virtual void createTcpCheck(const Network::ReadFilterCallbacks* callbacks, envoy::api::v2::auth::CheckRequest& request) PURE; @@ -99,4 +96,3 @@ typedef std::unique_ptr CheckRequestGeneratorPtr; } // namespace ExtAuthz } // namespace Envoy - diff --git a/source/common/config/well_known_names.h b/source/common/config/well_known_names.h index 0b0ffcacc3fa4..1fcbd3b70e530 100644 --- a/source/common/config/well_known_names.h +++ b/source/common/config/well_known_names.h @@ -122,13 +122,13 @@ class HttpFilterNameValues { // External Authorization filter const std::string EXT_AUTHORIZATION = "envoy.ext_authz"; - // Converts names from v1 to v2 const V1Converter v1_converter_; HttpFilterNameValues() : v1_converter_({BUFFER, CORS, DYNAMO, FAULT, GRPC_HTTP1_BRIDGE, GRPC_JSON_TRANSCODER, - GRPC_WEB, HEALTH_CHECK, IP_TAGGING, RATE_LIMIT, ROUTER, LUA, EXT_AUTHORIZATION}) {} + GRPC_WEB, HEALTH_CHECK, IP_TAGGING, RATE_LIMIT, ROUTER, LUA, + EXT_AUTHORIZATION}) {} }; typedef ConstSingleton HttpFilterNames; diff --git a/source/server/BUILD b/source/server/BUILD index f68780803e9ce..750c8a7ebe118 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -36,12 +36,12 @@ envoy_cc_library( "//include/envoy/server:filter_config_interface", "//include/envoy/server:instance_interface", "//include/envoy/ssl:context_manager_interface", - "//source/common/ext_authz:ext_authz_lib", "//source/common/common:assert_lib", "//source/common/common:logger_lib", "//source/common/common:utility_lib", "//source/common/config:lds_json_lib", "//source/common/config:utility_lib", + "//source/common/ext_authz:ext_authz_lib", "//source/common/network:resolver_lib", "//source/common/network:utility_lib", "//source/common/protobuf:utility_lib", From 0df4b9ae5dac4b83e47ea7856128021c9a528152 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Fri, 26 Jan 2018 11:15:44 -0800 Subject: [PATCH 08/17] Check generator class testing code. Signed-off-by: Saurabh Mohan --- test/common/ext_authz/BUILD | 5 ++ test/common/ext_authz/ext_authz_impl_test.cc | 53 +++++++++++++++++++- 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/test/common/ext_authz/BUILD b/test/common/ext_authz/BUILD index a5ff1d09c1358..a0cfd0d21f2ff 100644 --- a/test/common/ext_authz/BUILD +++ b/test/common/ext_authz/BUILD @@ -15,7 +15,12 @@ envoy_cc_test( "//source/common/ext_authz:ext_authz_lib", "//source/common/http:header_map_lib", "//source/common/http:headers_lib", + "//source/common/network:address_lib", "//test/mocks/grpc:grpc_mocks", + "//test/mocks/http:http_mocks", + "//test/mocks/network:network_mocks", + "//test/mocks/request_info:request_info_mocks", + "//test/mocks/ssl:ssl_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:utility_lib", ], diff --git a/test/common/ext_authz/ext_authz_impl_test.cc b/test/common/ext_authz/ext_authz_impl_test.cc index a47da3a12a4d7..30e8dad43419f 100644 --- a/test/common/ext_authz/ext_authz_impl_test.cc +++ b/test/common/ext_authz/ext_authz_impl_test.cc @@ -5,8 +5,13 @@ #include "common/ext_authz/ext_authz_impl.h" #include "common/http/header_map_impl.h" #include "common/http/headers.h" +#include "common/network/address_impl.h" #include "test/mocks/grpc/mocks.h" +#include "test/mocks/http/mocks.h" +#include "test/mocks/network/mocks.h" +#include "test/mocks/request_info/mocks.h" +#include "test/mocks/ssl/mocks.h" #include "test/mocks/upstream/mocks.h" #include "test/test_common/printers.h" #include "test/test_common/utility.h" @@ -18,6 +23,7 @@ using testing::AtLeast; using testing::Invoke; using testing::Ref; using testing::Return; +using testing::ReturnRef; using testing::WithArg; using testing::_; @@ -139,7 +145,52 @@ TEST(ExtAuthzNullFactoryTest, Basic) { client->cancel(); } -// TODO(saumoh): Add TEST for createHttpCheck and createTcpCheck +class ExtAuthzCheckRequestGeneratorTest : public testing::Test { +public: + ExtAuthzCheckRequestGeneratorTest() { + addr_ = std::make_shared("1.2.3.4", 1111); + protocol_ = Envoy::Http::Protocol::Http10; + // ssl_ = new Envoy::Ssl::MockConnection(); + // Network::Address::InstanceConstSharedPtr{new Network::Address::Ipv4Instance("1.2.3.4", 111)}; + }; + + Network::Address::InstanceConstSharedPtr addr_; + Optional protocol_; + ExtAuthzCheckRequestGenerator check_request_generator_; + NiceMock callbacks_; + NiceMock net_callbacks_; + NiceMock connection_; + NiceMock ssl_; + NiceMock req_info_; +}; + +TEST_F(ExtAuthzCheckRequestGeneratorTest, BasicTcp) { + + envoy::api::v2::auth::CheckRequest request; + + EXPECT_CALL(net_callbacks_, connection()).Times(2).WillRepeatedly(ReturnRef(connection_)); + EXPECT_CALL(connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(connection_, localAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(Const(connection_), ssl()).Times(2).WillRepeatedly(Return(&ssl_)); + + check_request_generator_.createTcpCheck(&net_callbacks_, request); +} + +TEST_F(ExtAuthzCheckRequestGeneratorTest, BasicHttp) { + + Http::HeaderMapImpl headers; + envoy::api::v2::auth::CheckRequest request; + + EXPECT_CALL(callbacks_, connection()).Times(2).WillRepeatedly(Return(&connection_)); + EXPECT_CALL(connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(connection_, localAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(Const(connection_), ssl()).Times(2).WillRepeatedly(Return(&ssl_)); + + EXPECT_CALL(callbacks_, streamId()).WillOnce(Return(0)); + EXPECT_CALL(callbacks_, requestInfo()).Times(2).WillRepeatedly(ReturnRef(req_info_)); + EXPECT_CALL(req_info_, protocol()).WillOnce(ReturnRef(protocol_)); + check_request_generator_.createHttpCheck(&callbacks_, headers, request); +} } // namespace ExtAuthz } // namespace Envoy From b4c7fea56a9e97dab6e736e44e5241ff98228fef Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Fri, 26 Jan 2018 14:03:17 -0800 Subject: [PATCH 09/17] Merge with data-plane-api for common. Signed-off-by: Saurabh Mohan --- include/envoy/ext_authz/BUILD | 2 +- include/envoy/ext_authz/ext_authz.h | 10 ++--- source/common/ext_authz/ext_authz_impl.cc | 21 +++++----- source/common/ext_authz/ext_authz_impl.h | 18 ++++----- test/common/ext_authz/ext_authz_impl_test.cc | 40 ++++++++++---------- test/mocks/ext_authz/mocks.h | 11 +++--- 6 files changed, 51 insertions(+), 51 deletions(-) diff --git a/include/envoy/ext_authz/BUILD b/include/envoy/ext_authz/BUILD index f7b2f326c713b..7e02622743025 100644 --- a/include/envoy/ext_authz/BUILD +++ b/include/envoy/ext_authz/BUILD @@ -11,12 +11,12 @@ envoy_package() envoy_cc_library( name = "ext_authz_interface", hdrs = ["ext_authz.h"], - external_deps = ["envoy_api_auth_auth"], deps = [ "//include/envoy/common:optional", "//include/envoy/http:filter_interface", "//include/envoy/http:header_map_interface", "//include/envoy/network:filter_interface", "//include/envoy/tracing:http_tracer_interface", + "@envoy_api//envoy/service/auth/v2:external_auth_cc", ], ) diff --git a/include/envoy/ext_authz/ext_authz.h b/include/envoy/ext_authz/ext_authz.h index 78167325cfed5..b84fbf4704ecc 100644 --- a/include/envoy/ext_authz/ext_authz.h +++ b/include/envoy/ext_authz/ext_authz.h @@ -10,10 +10,9 @@ #include "envoy/http/filter.h" #include "envoy/http/header_map.h" #include "envoy/network/filter.h" +#include "envoy/service/auth/v2/external_auth.pb.h" #include "envoy/tracing/http_tracer.h" -#include "api/auth/external_auth.pb.h" - namespace Envoy { namespace ExtAuthz { @@ -53,7 +52,8 @@ class Client { virtual void cancel() PURE; // A check call. - virtual void check(RequestCallbacks& callback, const envoy::api::v2::auth::CheckRequest& request, + virtual void check(RequestCallbacks& callback, + const envoy::service::auth::v2::CheckRequest& request, Tracing::Span& parent_span) PURE; }; @@ -87,9 +87,9 @@ class CheckRequestGenerator { virtual void createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, const Envoy::Http::HeaderMap& headers, - envoy::api::v2::auth::CheckRequest& request) PURE; + envoy::service::auth::v2::CheckRequest& request) PURE; virtual void createTcpCheck(const Network::ReadFilterCallbacks* callbacks, - envoy::api::v2::auth::CheckRequest& request) PURE; + envoy::service::auth::v2::CheckRequest& request) PURE; }; typedef std::unique_ptr CheckRequestGeneratorPtr; diff --git a/source/common/ext_authz/ext_authz_impl.cc b/source/common/ext_authz/ext_authz_impl.cc index 355ea0735f03b..98dec8ed1ada4 100644 --- a/source/common/ext_authz/ext_authz_impl.cc +++ b/source/common/ext_authz/ext_authz_impl.cc @@ -18,15 +18,15 @@ namespace Envoy { namespace ExtAuthz { -using ::envoy::api::v2::auth::AttributeContext; -using ::envoy::api::v2::auth::AttributeContext_HttpRequest; -using ::envoy::api::v2::auth::AttributeContext_Peer; -using ::envoy::api::v2::auth::AttributeContext_Request; +using ::envoy::service::auth::v2::AttributeContext; +using ::envoy::service::auth::v2::AttributeContext_HttpRequest; +using ::envoy::service::auth::v2::AttributeContext_Peer; +using ::envoy::service::auth::v2::AttributeContext_Request; GrpcClientImpl::GrpcClientImpl(Grpc::AsyncClientPtr&& async_client, const Optional& timeout) : service_method_(*Protobuf::DescriptorPool::generated_pool()->FindMethodByName( - "envoy.api.v2.auth.Authorization.Check")), + "envoy.service.auth.v2.Authorization.Check")), async_client_(std::move(async_client)), timeout_(timeout) {} GrpcClientImpl::~GrpcClientImpl() { ASSERT(!callbacks_); } @@ -38,7 +38,7 @@ void GrpcClientImpl::cancel() { } void GrpcClientImpl::check(RequestCallbacks& callbacks, - const envoy::api::v2::auth::CheckRequest& request, + const envoy::service::auth::v2::CheckRequest& request, Tracing::Span& parent_span) { ASSERT(callbacks_ == nullptr); callbacks_ = &callbacks; @@ -46,7 +46,7 @@ void GrpcClientImpl::check(RequestCallbacks& callbacks, request_ = async_client_->send(service_method_, request, *this, parent_span, timeout_); } -void GrpcClientImpl::onSuccess(std::unique_ptr&& response, +void GrpcClientImpl::onSuccess(std::unique_ptr&& response, Tracing::Span& span) { CheckStatus status = CheckStatus::OK; ASSERT(response->status().code() != Grpc::Status::GrpcStatus::Unknown); @@ -214,7 +214,7 @@ std::unique_ptr ExtAuthzCheckRequestGenerator::getHttp void ExtAuthzCheckRequestGenerator::createHttpCheck( const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, - const Envoy::Http::HeaderMap& headers, envoy::api::v2::auth::CheckRequest& request) { + const Envoy::Http::HeaderMap& headers, envoy::service::auth::v2::CheckRequest& request) { auto attrs = request.mutable_attributes(); @@ -227,8 +227,9 @@ void ExtAuthzCheckRequestGenerator::createHttpCheck( attrs->set_allocated_request(getHttpRequest(callbacks, headers).release()); } -void ExtAuthzCheckRequestGenerator::createTcpCheck(const Network::ReadFilterCallbacks* callbacks, - envoy::api::v2::auth::CheckRequest& request) { +void ExtAuthzCheckRequestGenerator::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 1915204dc48f3..9912f7b854cbd 100644 --- a/source/common/ext_authz/ext_authz_impl.h +++ b/source/common/ext_authz/ext_authz_impl.h @@ -19,7 +19,7 @@ namespace Envoy { namespace ExtAuthz { -typedef Grpc::TypedAsyncRequestCallbacks +typedef Grpc::TypedAsyncRequestCallbacks ExtAuthzAsyncCallbacks; struct ConstantValues { @@ -40,12 +40,12 @@ class GrpcClientImpl : public Client, public ExtAuthzAsyncCallbacks { // ExtAuthz::Client void cancel() override; - void check(RequestCallbacks& callbacks, const envoy::api::v2::auth::CheckRequest& request, + void check(RequestCallbacks& callbacks, const envoy::service::auth::v2::CheckRequest& request, Tracing::Span& parent_span) override; // Grpc::AsyncRequestCallbacks void onCreateInitialMetadata(Http::HeaderMap&) override {} - void onSuccess(std::unique_ptr&& response, + void onSuccess(std::unique_ptr&& response, Tracing::Span& span) override; void onFailure(Grpc::Status::GrpcStatus status, const std::string& message, Tracing::Span& span) override; @@ -75,7 +75,7 @@ class NullClientImpl : public Client { public: // ExtAuthz::Client void cancel() override {} - void check(RequestCallbacks& callbacks, const envoy::api::v2::auth::CheckRequest&, + void check(RequestCallbacks& callbacks, const envoy::service::auth::v2::CheckRequest&, Tracing::Span&) override { callbacks.complete(CheckStatus::OK); } @@ -94,18 +94,18 @@ class ExtAuthzCheckRequestGenerator : public CheckRequestGenerator { // ExtAuthz::CheckRequestGenIntf void createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, const Envoy::Http::HeaderMap& headers, - envoy::api::v2::auth::CheckRequest& request); + envoy::service::auth::v2::CheckRequest& request); void createTcpCheck(const Network::ReadFilterCallbacks* callbacks, - envoy::api::v2::auth::CheckRequest& request); + envoy::service::auth::v2::CheckRequest& request); private: std::unique_ptr<::envoy::api::v2::Address> getProtobufAddress(const Network::Address::InstanceConstSharedPtr&); - std::unique_ptr<::envoy::api::v2::auth::AttributeContext_Peer> + std::unique_ptr<::envoy::service::auth::v2::AttributeContext_Peer> getConnectionPeer(const Network::Connection*, const std::string&, const bool); - std::unique_ptr<::envoy::api::v2::auth::AttributeContext_Peer> + std::unique_ptr<::envoy::service::auth::v2::AttributeContext_Peer> getConnectionPeer(const Network::Connection&, const std::string&, const bool); - std::unique_ptr<::envoy::api::v2::auth::AttributeContext_Request> + std::unique_ptr<::envoy::service::auth::v2::AttributeContext_Request> getHttpRequest(const Envoy::Http::StreamDecoderFilterCallbacks*, const Envoy::Http::HeaderMap&); const std::string getProtocolStr(const Envoy::Http::Protocol&); std::string getHeaderStr(const Envoy::Http::HeaderEntry* entry); diff --git a/test/common/ext_authz/ext_authz_impl_test.cc b/test/common/ext_authz/ext_authz_impl_test.cc index 30e8dad43419f..20d694f547c53 100644 --- a/test/common/ext_authz/ext_authz_impl_test.cc +++ b/test/common/ext_authz/ext_authz_impl_test.cc @@ -49,27 +49,27 @@ class ExtAuthzGrpcClientTest : public testing::Test { }; TEST_F(ExtAuthzGrpcClientTest, Basic) { - std::unique_ptr response; + std::unique_ptr response; { - envoy::api::v2::auth::CheckRequest request; + envoy::service::auth::v2::CheckRequest request; Http::HeaderMapImpl headers; EXPECT_CALL(*async_client_, send(_, ProtoEq(request), Ref(client_), _, _)) - .WillOnce( - Invoke([this](const Protobuf::MethodDescriptor& service_method, - const Protobuf::Message&, Grpc::AsyncRequestCallbacks&, Tracing::Span&, - const Optional&) -> Grpc::AsyncRequest* { - EXPECT_EQ("envoy.api.v2.auth.Authorization", service_method.service()->full_name()); - EXPECT_EQ("Check", service_method.name()); - return &async_request_; - })); + .WillOnce(Invoke([this](const Protobuf::MethodDescriptor& service_method, + const Protobuf::Message&, Grpc::AsyncRequestCallbacks&, + Tracing::Span&, + const Optional&) -> Grpc::AsyncRequest* { + EXPECT_EQ("envoy.service.auth.v2.Authorization", service_method.service()->full_name()); + EXPECT_EQ("Check", service_method.name()); + return &async_request_; + })); client_.check(request_callbacks_, request, Tracing::NullSpan::instance()); client_.onCreateInitialMetadata(headers); EXPECT_EQ(nullptr, headers.RequestId()); - response.reset(new envoy::api::v2::auth::CheckResponse()); + response.reset(new envoy::service::auth::v2::CheckResponse()); ::google::rpc::Status* status = new ::google::rpc::Status(); status->set_code(Grpc::Status::GrpcStatus::PermissionDenied); response->set_allocated_status(status); @@ -79,7 +79,7 @@ TEST_F(ExtAuthzGrpcClientTest, Basic) { } { - envoy::api::v2::auth::CheckRequest request; + envoy::service::auth::v2::CheckRequest request; Http::HeaderMapImpl headers; EXPECT_CALL(*async_client_, send(_, ProtoEq(request), _, _, _)) .WillOnce(Return(&async_request_)); @@ -88,7 +88,7 @@ TEST_F(ExtAuthzGrpcClientTest, Basic) { client_.onCreateInitialMetadata(headers); - response.reset(new envoy::api::v2::auth::CheckResponse()); + response.reset(new envoy::service::auth::v2::CheckResponse()); ::google::rpc::Status* status = new ::google::rpc::Status(); status->set_code(Grpc::Status::GrpcStatus::Ok); response->set_allocated_status(status); @@ -98,21 +98,21 @@ TEST_F(ExtAuthzGrpcClientTest, Basic) { } { - envoy::api::v2::auth::CheckRequest request; + envoy::service::auth::v2::CheckRequest request; EXPECT_CALL(*async_client_, send(_, ProtoEq(request), _, _, _)) .WillOnce(Return(&async_request_)); client_.check(request_callbacks_, request, Tracing::NullSpan::instance()); - response.reset(new envoy::api::v2::auth::CheckResponse()); + response.reset(new envoy::service::auth::v2::CheckResponse()); EXPECT_CALL(request_callbacks_, complete(CheckStatus::Error)); client_.onFailure(Grpc::Status::Unknown, "", span_); } } TEST_F(ExtAuthzGrpcClientTest, Cancel) { - std::unique_ptr response; - envoy::api::v2::auth::CheckRequest request; + std::unique_ptr response; + envoy::service::auth::v2::CheckRequest request; EXPECT_CALL(*async_client_, send(_, _, _, _, _)).WillOnce(Return(&async_request_)); @@ -139,7 +139,7 @@ TEST(ExtAuthzNullFactoryTest, Basic) { NullFactoryImpl factory; ClientPtr client = factory.create(Optional()); MockRequestCallbacks request_callbacks; - envoy::api::v2::auth::CheckRequest request; + envoy::service::auth::v2::CheckRequest request; EXPECT_CALL(request_callbacks, complete(CheckStatus::OK)); client->check(request_callbacks, request, Tracing::NullSpan::instance()); client->cancel(); @@ -166,7 +166,7 @@ class ExtAuthzCheckRequestGeneratorTest : public testing::Test { TEST_F(ExtAuthzCheckRequestGeneratorTest, BasicTcp) { - envoy::api::v2::auth::CheckRequest request; + envoy::service::auth::v2::CheckRequest request; EXPECT_CALL(net_callbacks_, connection()).Times(2).WillRepeatedly(ReturnRef(connection_)); EXPECT_CALL(connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); @@ -179,7 +179,7 @@ TEST_F(ExtAuthzCheckRequestGeneratorTest, BasicTcp) { TEST_F(ExtAuthzCheckRequestGeneratorTest, BasicHttp) { Http::HeaderMapImpl headers; - envoy::api::v2::auth::CheckRequest request; + envoy::service::auth::v2::CheckRequest request; EXPECT_CALL(callbacks_, connection()).Times(2).WillRepeatedly(Return(&connection_)); EXPECT_CALL(connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); diff --git a/test/mocks/ext_authz/mocks.h b/test/mocks/ext_authz/mocks.h index 59ca9fbf95598..ca973efd63c19 100644 --- a/test/mocks/ext_authz/mocks.h +++ b/test/mocks/ext_authz/mocks.h @@ -17,9 +17,9 @@ class MockClient : public Client { // ExtAuthz::Client MOCK_METHOD0(cancel, void()); - MOCK_METHOD3(check, - void(RequestCallbacks& callbacks, const envoy::api::v2::auth::CheckRequest& request, - Tracing::Span& parent_span)); + MOCK_METHOD3(check, void(RequestCallbacks& callbacks, + const envoy::service::auth::v2::CheckRequest& request, + Tracing::Span& parent_span)); }; class MockCheckRequestGen : public CheckRequestGenerator { @@ -29,11 +29,10 @@ class MockCheckRequestGen : public CheckRequestGenerator { // ExtAuthz::CheckRequestGenerator MOCK_METHOD3(createHttpCheck, void(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, - const Envoy::Http::HeaderMap& headers, - envoy::api::v2::auth::CheckRequest& request)); + envoy::service::auth::v2::CheckRequest& request)); MOCK_METHOD2(createTcpCheck, void(const Network::ReadFilterCallbacks* callbacks, - envoy::api::v2::auth::CheckRequest& request)); + envoy::service::auth::v2::CheckRequest& request)); }; } // namespace ExtAuthz From 93776d88508db6ff2b83fc9323142f3ca7167897 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Mon, 29 Jan 2018 15:39:55 -0800 Subject: [PATCH 10/17] Review feedback for common files. Signed-off-by: Saurabh Mohan --- include/envoy/ext_authz/BUILD | 4 - include/envoy/ext_authz/ext_authz.h | 51 ++------ .../common/access_log/grpc_access_log_impl.cc | 6 - source/common/ext_authz/BUILD | 3 + source/common/ext_authz/ext_authz_impl.cc | 110 +++++++----------- source/common/ext_authz/ext_authz_impl.h | 83 +++++++------ test/common/ext_authz/ext_authz_impl_test.cc | 38 +----- test/mocks/ext_authz/BUILD | 1 + test/mocks/ext_authz/mocks.h | 2 + 9 files changed, 112 insertions(+), 186 deletions(-) diff --git a/include/envoy/ext_authz/BUILD b/include/envoy/ext_authz/BUILD index 7e02622743025..34318465f0e2a 100644 --- a/include/envoy/ext_authz/BUILD +++ b/include/envoy/ext_authz/BUILD @@ -12,10 +12,6 @@ envoy_cc_library( name = "ext_authz_interface", hdrs = ["ext_authz.h"], deps = [ - "//include/envoy/common:optional", - "//include/envoy/http:filter_interface", - "//include/envoy/http:header_map_interface", - "//include/envoy/network:filter_interface", "//include/envoy/tracing:http_tracer_interface", "@envoy_api//envoy/service/auth/v2:external_auth_cc", ], diff --git a/include/envoy/ext_authz/ext_authz.h b/include/envoy/ext_authz/ext_authz.h index b84fbf4704ecc..fd11a284ca85a 100644 --- a/include/envoy/ext_authz/ext_authz.h +++ b/include/envoy/ext_authz/ext_authz.h @@ -5,11 +5,7 @@ #include #include -#include "envoy/common/optional.h" #include "envoy/common/pure.h" -#include "envoy/http/filter.h" -#include "envoy/http/header_map.h" -#include "envoy/network/filter.h" #include "envoy/service/auth/v2/external_auth.pb.h" #include "envoy/tracing/http_tracer.h" @@ -38,7 +34,7 @@ class RequestCallbacks { /** * Called when a check request is complete. The resulting status is supplied. */ - virtual void complete(CheckStatus status) PURE; + virtual void onComplete(CheckStatus status) PURE; }; class Client { @@ -51,7 +47,15 @@ class Client { */ virtual void cancel() PURE; - // A check call. + /** + * Request a check call to an external authorization service which can use the + * passed request parameters to make a permit/deny decision. + * @param callback supplies the completion callbacks. + * NOTE: The callback may happen within the calling stack. + * @param request is the proto message with the attributes of the specific payload. + * @param parent_span source for generating an egress child span as part of the trace. + * + */ virtual void check(RequestCallbacks& callback, const envoy::service::auth::v2::CheckRequest& request, Tracing::Span& parent_span) PURE; @@ -59,40 +63,5 @@ class Client { typedef std::unique_ptr ClientPtr; -/** - * An interface for creating a external authorization client. - */ -class ClientFactory { -public: - virtual ~ClientFactory() {} - - /** - * Return a new authz client. - */ - virtual ClientPtr create(const Optional& timeout) PURE; -}; - -typedef std::unique_ptr ClientFactoryPtr; - -/** - * An interface for creating ext_authz.proto (authorization) request. - * CheckRequestGenerator 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. - */ -class CheckRequestGenerator { -public: - // Destructor - virtual ~CheckRequestGenerator() {} - - virtual void createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, - const Envoy::Http::HeaderMap& headers, - envoy::service::auth::v2::CheckRequest& request) PURE; - virtual void createTcpCheck(const Network::ReadFilterCallbacks* callbacks, - envoy::service::auth::v2::CheckRequest& request) PURE; -}; - -typedef std::unique_ptr CheckRequestGeneratorPtr; - } // namespace ExtAuthz } // namespace Envoy diff --git a/source/common/access_log/grpc_access_log_impl.cc b/source/common/access_log/grpc_access_log_impl.cc index 9d2ed8462cafd..e15d7d17bab69 100644 --- a/source/common/access_log/grpc_access_log_impl.cc +++ b/source/common/access_log/grpc_access_log_impl.cc @@ -130,12 +130,6 @@ void HttpGrpcAccessLog::responseFlagsToAccessLogResponseFlags( if (request_info.getResponseFlag(RequestInfo::ResponseFlag::RateLimited)) { common_access_log.mutable_response_flags()->set_rate_limited(true); } - - // TODO(saumoh): Uncomment once unauthorization has been added to accesslog.proto - // - // if (request_info.getResponseFlag(RequestInfo::ResponseFlag::Unauthorized)) { - // common_access_log.mutable_response_flags()->set_unauthorized(true); - // } } void HttpGrpcAccessLog::log(const Http::HeaderMap* request_headers, diff --git a/source/common/ext_authz/BUILD b/source/common/ext_authz/BUILD index 0fc1c60a16d46..c1c8612cc1a0b 100644 --- a/source/common/ext_authz/BUILD +++ b/source/common/ext_authz/BUILD @@ -16,9 +16,12 @@ envoy_cc_library( "//include/envoy/ext_authz:ext_authz_interface", "//include/envoy/grpc:async_client_interface", "//include/envoy/grpc:async_client_manager_interface", + "//include/envoy/http:filter_interface", + "//include/envoy/http:header_map_interface", "//include/envoy/http:protocol_interface", "//include/envoy/network:address_interface", "//include/envoy/network:connection_interface", + "//include/envoy/network:filter_interface", "//include/envoy/ssl:connection_interface", "//include/envoy/upstream:cluster_manager_interface", "//source/common/common:assert_lib", diff --git a/source/common/ext_authz/ext_authz_impl.cc b/source/common/ext_authz/ext_authz_impl.cc index 98dec8ed1ada4..a133272e9b219 100644 --- a/source/common/ext_authz/ext_authz_impl.cc +++ b/source/common/ext_authz/ext_authz_impl.cc @@ -18,11 +18,6 @@ namespace Envoy { namespace ExtAuthz { -using ::envoy::service::auth::v2::AttributeContext; -using ::envoy::service::auth::v2::AttributeContext_HttpRequest; -using ::envoy::service::auth::v2::AttributeContext_Peer; -using ::envoy::service::auth::v2::AttributeContext_Request; - GrpcClientImpl::GrpcClientImpl(Grpc::AsyncClientPtr&& async_client, const Optional& timeout) : service_method_(*Protobuf::DescriptorPool::generated_pool()->FindMethodByName( @@ -57,7 +52,7 @@ void GrpcClientImpl::onSuccess(std::unique_ptrcomplete(status); + callbacks_->onComplete(status); callbacks_ = nullptr; } @@ -65,53 +60,39 @@ void GrpcClientImpl::onFailure(Grpc::Status::GrpcStatus status, const std::strin Tracing::Span&) { ASSERT(status != Grpc::Status::GrpcStatus::Ok); UNREFERENCED_PARAMETER(status); - callbacks_->complete(CheckStatus::Error); + callbacks_->onComplete(CheckStatus::Error); callbacks_ = nullptr; } -GrpcFactoryImpl::GrpcFactoryImpl(const envoy::api::v2::GrpcService& grpc_service, - Grpc::AsyncClientManager& async_client_manager, - Stats::Scope& scope) { - async_client_factory_ = async_client_manager.factoryForGrpcService(grpc_service, scope); -} - -ClientPtr GrpcFactoryImpl::create(const Optional& timeout) { - return std::make_unique(async_client_factory_->create(), timeout); -} - -std::unique_ptr<::envoy::api::v2::Address> ExtAuthzCheckRequestGenerator::getProtobufAddress( - const Network::Address::InstanceConstSharedPtr& instance) { - - auto addr = std::make_unique<::envoy::api::v2::Address>(); - - if (instance->type() == Network::Address::Type::Ip) { - addr->mutable_socket_address()->set_address(instance->ip()->addressAsString()); - addr->mutable_socket_address()->set_port_value(instance->ip()->port()); +void ExtAuthzCheckRequestGenerator::addressToProtobufAddress( + envoy::api::v2::Address& proto_address, const Network::Address::Instance& address) { + if (address.type() == Network::Address::Type::Pipe) { + proto_address.mutable_pipe()->set_path(address.asString()); } else { - ASSERT(instance->type() == Network::Address::Type::Pipe); - addr->mutable_pipe()->set_path(instance->asString()); + ASSERT(address.type() == Network::Address::Type::Ip); + auto* socket_address = proto_address.mutable_socket_address(); + socket_address->set_address(address.ip()->addressAsString()); + socket_address->set_port_value(address.ip()->port()); } - return addr; } -std::unique_ptr -ExtAuthzCheckRequestGenerator::getConnectionPeer(const Network::Connection* connection, - const std::string& service, const bool local) { - - auto peer = std::make_unique(); +void ExtAuthzCheckRequestGenerator::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(); if (!local) { - peer->set_allocated_address(getProtobufAddress(connection->remoteAddress()).release()); + addressToProtobufAddress(*addr, *connection.remoteAddress()); } else { - peer->set_allocated_address(getProtobufAddress(connection->localAddress()).release()); + addressToProtobufAddress(*addr, *connection.localAddress()); } // Set the principal // Preferably the SAN from the peer's cert or // Subject from the peer's cert. std::string principal; - Ssl::Connection* ssl = const_cast(connection->ssl()); + Ssl::Connection* ssl = const_cast(connection.ssl()); if (ssl != nullptr) { if (!local) { principal = ssl->uriSanPeerCertificate(); @@ -127,19 +108,11 @@ ExtAuthzCheckRequestGenerator::getConnectionPeer(const Network::Connection* conn } } } - peer->set_principal(principal); + peer.set_principal(principal); if (!service.empty()) { - peer->set_service(service); + peer.set_service(service); } - - return peer; -} - -std::unique_ptr -ExtAuthzCheckRequestGenerator::getConnectionPeer(const Network::Connection& connection, - const std::string& service, const bool local) { - return getConnectionPeer(&connection, service, local); } const std::string ExtAuthzCheckRequestGenerator::getProtocolStr(const Envoy::Http::Protocol& p) { @@ -173,43 +146,45 @@ std::string ExtAuthzCheckRequestGenerator::getHeaderStr(const Envoy::Http::Heade return ""; } -std::unique_ptr ExtAuthzCheckRequestGenerator::getHttpRequest( +void ExtAuthzCheckRequestGenerator::setHttpRequest( + ::envoy::service::auth::v2::AttributeContext_HttpRequest& httpreq, const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, const Envoy::Http::HeaderMap& headers) { - auto httpreq = new AttributeContext_HttpRequest(); - // Set id // The streamId is not qualified as a const. Although it is as it does not modify the object. Envoy::Http::StreamDecoderFilterCallbacks* sdfc = const_cast(callbacks); - httpreq->set_id(std::to_string(sdfc->streamId())); + httpreq.set_id(std::to_string(sdfc->streamId())); // Set method - httpreq->set_method(getHeaderStr(headers.Method())); + httpreq.set_method(getHeaderStr(headers.Method())); // Set path - httpreq->set_path(getHeaderStr(headers.Path())); + httpreq.set_path(getHeaderStr(headers.Path())); // Set host - httpreq->set_host(getHeaderStr(headers.Host())); + httpreq.set_host(getHeaderStr(headers.Host())); // Set scheme - httpreq->set_scheme(getHeaderStr(headers.Scheme())); + httpreq.set_scheme(getHeaderStr(headers.Scheme())); // Set size // need to convert to google buffer 64t; - httpreq->set_size(sdfc->requestInfo().bytesReceived()); + httpreq.set_size(sdfc->requestInfo().bytesReceived()); // Set protocol - httpreq->set_protocol(getProtocolStr(sdfc->requestInfo().protocol().value())); + if (sdfc->requestInfo().protocol().valid()) { + httpreq.set_protocol(getProtocolStr(sdfc->requestInfo().protocol().value())); + } // Fill in the headers - auto mhdrs = httpreq->mutable_headers(); + auto mhdrs = httpreq.mutable_headers(); headers.iterate(fillHttpHeaders, mhdrs); +} - auto req = std::make_unique(); - - req->set_allocated_http(httpreq); - - return req; +void ExtAuthzCheckRequestGenerator::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 ExtAuthzCheckRequestGenerator::createHttpCheck( @@ -222,9 +197,10 @@ void ExtAuthzCheckRequestGenerator::createHttpCheck( const_cast(callbacks); std::string service = getHeaderStr(headers.EnvoyDownstreamServiceCluster()); - attrs->set_allocated_source(getConnectionPeer(cb->connection(), service, false).release()); - attrs->set_allocated_destination(getConnectionPeer(cb->connection(), "", true).release()); - attrs->set_allocated_request(getHttpRequest(callbacks, headers).release()); + + setAttrContextPeer(*attrs->mutable_source(), *cb->connection(), service, false); + setAttrContextPeer(*attrs->mutable_destination(), *cb->connection(), "", true); + setAttrContextRequest(*attrs->mutable_request(), callbacks, headers); } void ExtAuthzCheckRequestGenerator::createTcpCheck( @@ -234,8 +210,8 @@ void ExtAuthzCheckRequestGenerator::createTcpCheck( auto attrs = request.mutable_attributes(); Network::ReadFilterCallbacks* cb = const_cast(callbacks); - attrs->set_allocated_source(getConnectionPeer(cb->connection(), "", false).release()); - attrs->set_allocated_destination(getConnectionPeer(cb->connection(), "", true).release()); + setAttrContextPeer(*attrs->mutable_source(), cb->connection(), "", false); + setAttrContextPeer(*attrs->mutable_destination(), cb->connection(), "", true); } } // namespace ExtAuthz diff --git a/source/common/ext_authz/ext_authz_impl.h b/source/common/ext_authz/ext_authz_impl.h index 9912f7b854cbd..640bd06afa53b 100644 --- a/source/common/ext_authz/ext_authz_impl.h +++ b/source/common/ext_authz/ext_authz_impl.h @@ -8,9 +8,12 @@ #include "envoy/ext_authz/ext_authz.h" #include "envoy/grpc/async_client.h" #include "envoy/grpc/async_client_manager.h" +#include "envoy/http/filter.h" +#include "envoy/http/header_map.h" #include "envoy/http/protocol.h" #include "envoy/network/address.h" #include "envoy/network/connection.h" +#include "envoy/network/filter.h" #include "envoy/tracing/http_tracer.h" #include "envoy/upstream/cluster_manager.h" @@ -58,40 +61,45 @@ class GrpcClientImpl : public Client, public ExtAuthzAsyncCallbacks { RequestCallbacks* callbacks_{}; }; -class GrpcFactoryImpl : public ClientFactory { +/** + * An interface for creating ext_authz.proto (authorization) request. + * CheckRequestGenerator 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. + */ +class CheckRequestGenerator { public: - GrpcFactoryImpl(const envoy::api::v2::GrpcService& grpc_service, - Grpc::AsyncClientManager& async_client_manager, Stats::Scope& scope); - - // ExtAuthz::ClientFactory - ClientPtr create(const Optional& timeout) override; - -private: - Grpc::AsyncClientFactoryPtr async_client_factory_; - std::string cluster_name_; + // Destructor + virtual ~CheckRequestGenerator() {} + + /** + * createHttpCheck is used to extract the attributes from the stream and the http headers + * and fill them up in the CheckRequest proto message. + * @param callbacks supplies the Http stream context from which data can be extracted. + * @param headers supplies the heeader map with http headers that will be used to create the + * check request. + * @param request is the reference to the check request that will be filled up. + * + */ + virtual void createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, + const Envoy::Http::HeaderMap& headers, + envoy::service::auth::v2::CheckRequest& request) PURE; + /** + * createTcpCheck is used to extract the attributes from the network layer and fill them up + * in the CheckRequest proto message. + * @param callbacks supplies the networklayer context from which data can be extracted. + * @param request is the reference to the check request that will be filled up. + * + */ + virtual void createTcpCheck(const Network::ReadFilterCallbacks* callbacks, + envoy::service::auth::v2::CheckRequest& request) PURE; }; -class NullClientImpl : public Client { -public: - // ExtAuthz::Client - void cancel() override {} - void check(RequestCallbacks& callbacks, const envoy::service::auth::v2::CheckRequest&, - Tracing::Span&) override { - callbacks.complete(CheckStatus::OK); - } -}; - -class NullFactoryImpl : public ClientFactory { -public: - // ExtAuthz::ClientFactory - ClientPtr create(const Optional&) override { - return ClientPtr{new NullClientImpl()}; - } -}; +typedef std::unique_ptr CheckRequestGeneratorPtr; class ExtAuthzCheckRequestGenerator : public CheckRequestGenerator { public: - // ExtAuthz::CheckRequestGenIntf + // ExtAuthz::CheckRequestGenerator void createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, const Envoy::Http::HeaderMap& headers, envoy::service::auth::v2::CheckRequest& request); @@ -99,14 +107,17 @@ class ExtAuthzCheckRequestGenerator : public CheckRequestGenerator { envoy::service::auth::v2::CheckRequest& request); private: - std::unique_ptr<::envoy::api::v2::Address> - getProtobufAddress(const Network::Address::InstanceConstSharedPtr&); - std::unique_ptr<::envoy::service::auth::v2::AttributeContext_Peer> - getConnectionPeer(const Network::Connection*, const std::string&, const bool); - std::unique_ptr<::envoy::service::auth::v2::AttributeContext_Peer> - getConnectionPeer(const Network::Connection&, const std::string&, const bool); - std::unique_ptr<::envoy::service::auth::v2::AttributeContext_Request> - getHttpRequest(const Envoy::Http::StreamDecoderFilterCallbacks*, const Envoy::Http::HeaderMap&); + void addressToProtobufAddress(envoy::api::v2::Address& proto_address, + const Network::Address::Instance& address); + void setAttrContextPeer(envoy::service::auth::v2::AttributeContext_Peer& peer, + const Network::Connection& connection, const std::string& service, + const bool local); + void setHttpRequest(::envoy::service::auth::v2::AttributeContext_HttpRequest& httpreq, + const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, + const Envoy::Http::HeaderMap& headers); + void setAttrContextRequest(::envoy::service::auth::v2::AttributeContext_Request& req, + const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, + const Envoy::Http::HeaderMap& headers); const std::string getProtocolStr(const Envoy::Http::Protocol&); std::string getHeaderStr(const Envoy::Http::HeaderEntry* entry); static Envoy::Http::HeaderMap::Iterate fillHttpHeaders(const Envoy::Http::HeaderEntry&, void*); diff --git a/test/common/ext_authz/ext_authz_impl_test.cc b/test/common/ext_authz/ext_authz_impl_test.cc index 20d694f547c53..3508ae77b956b 100644 --- a/test/common/ext_authz/ext_authz_impl_test.cc +++ b/test/common/ext_authz/ext_authz_impl_test.cc @@ -32,7 +32,7 @@ namespace ExtAuthz { class MockRequestCallbacks : public RequestCallbacks { public: - MOCK_METHOD1(complete, void(CheckStatus status)); + MOCK_METHOD1(onComplete, void(CheckStatus status)); }; class ExtAuthzGrpcClientTest : public testing::Test { @@ -74,7 +74,7 @@ TEST_F(ExtAuthzGrpcClientTest, Basic) { status->set_code(Grpc::Status::GrpcStatus::PermissionDenied); response->set_allocated_status(status); EXPECT_CALL(span_, setTag("ext_authz_status", "ext_authz_unauthorized")); - EXPECT_CALL(request_callbacks_, complete(CheckStatus::Denied)); + EXPECT_CALL(request_callbacks_, onComplete(CheckStatus::Denied)); client_.onSuccess(std::move(response), span_); } @@ -93,7 +93,7 @@ TEST_F(ExtAuthzGrpcClientTest, Basic) { status->set_code(Grpc::Status::GrpcStatus::Ok); response->set_allocated_status(status); EXPECT_CALL(span_, setTag("ext_authz_status", "ext_authz_ok")); - EXPECT_CALL(request_callbacks_, complete(CheckStatus::OK)); + EXPECT_CALL(request_callbacks_, onComplete(CheckStatus::OK)); client_.onSuccess(std::move(response), span_); } @@ -105,7 +105,7 @@ TEST_F(ExtAuthzGrpcClientTest, Basic) { client_.check(request_callbacks_, request, Tracing::NullSpan::instance()); response.reset(new envoy::service::auth::v2::CheckResponse()); - EXPECT_CALL(request_callbacks_, complete(CheckStatus::Error)); + EXPECT_CALL(request_callbacks_, onComplete(CheckStatus::Error)); client_.onFailure(Grpc::Status::Unknown, "", span_); } } @@ -122,36 +122,11 @@ TEST_F(ExtAuthzGrpcClientTest, Cancel) { client_.cancel(); } -TEST(ExtAuthzGrpcFactoryTest, Create) { - envoy::api::v2::GrpcService config; - config.mutable_envoy_grpc()->set_cluster_name("foo"); - Grpc::MockAsyncClientManager async_client_manager; - Stats::MockStore scope; - EXPECT_CALL(async_client_manager, factoryForGrpcService(ProtoEq(config), Ref(scope))) - .WillOnce(Invoke([](const envoy::api::v2::GrpcService&, Stats::Scope&) { - return std::make_unique>(); - })); - GrpcFactoryImpl factory(config, async_client_manager, scope); - factory.create(Optional()); -} - -TEST(ExtAuthzNullFactoryTest, Basic) { - NullFactoryImpl factory; - ClientPtr client = factory.create(Optional()); - MockRequestCallbacks request_callbacks; - envoy::service::auth::v2::CheckRequest request; - EXPECT_CALL(request_callbacks, complete(CheckStatus::OK)); - client->check(request_callbacks, request, Tracing::NullSpan::instance()); - client->cancel(); -} - class ExtAuthzCheckRequestGeneratorTest : public testing::Test { public: ExtAuthzCheckRequestGeneratorTest() { addr_ = std::make_shared("1.2.3.4", 1111); protocol_ = Envoy::Http::Protocol::Http10; - // ssl_ = new Envoy::Ssl::MockConnection(); - // Network::Address::InstanceConstSharedPtr{new Network::Address::Ipv4Instance("1.2.3.4", 111)}; }; Network::Address::InstanceConstSharedPtr addr_; @@ -185,10 +160,9 @@ TEST_F(ExtAuthzCheckRequestGeneratorTest, BasicHttp) { EXPECT_CALL(connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(connection_, localAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(Const(connection_), ssl()).Times(2).WillRepeatedly(Return(&ssl_)); - EXPECT_CALL(callbacks_, streamId()).WillOnce(Return(0)); - EXPECT_CALL(callbacks_, requestInfo()).Times(2).WillRepeatedly(ReturnRef(req_info_)); - EXPECT_CALL(req_info_, protocol()).WillOnce(ReturnRef(protocol_)); + EXPECT_CALL(callbacks_, requestInfo()).Times(3).WillRepeatedly(ReturnRef(req_info_)); + EXPECT_CALL(req_info_, protocol()).Times(2).WillRepeatedly(ReturnRef(protocol_)); check_request_generator_.createHttpCheck(&callbacks_, headers, request); } diff --git a/test/mocks/ext_authz/BUILD b/test/mocks/ext_authz/BUILD index 725c43b303014..23f64cf66d764 100644 --- a/test/mocks/ext_authz/BUILD +++ b/test/mocks/ext_authz/BUILD @@ -14,5 +14,6 @@ envoy_cc_mock( hdrs = ["mocks.h"], deps = [ "//include/envoy/ext_authz:ext_authz_interface", + "//source/common/ext_authz:ext_authz_lib", ], ) diff --git a/test/mocks/ext_authz/mocks.h b/test/mocks/ext_authz/mocks.h index ca973efd63c19..add0c73f2ec06 100644 --- a/test/mocks/ext_authz/mocks.h +++ b/test/mocks/ext_authz/mocks.h @@ -5,6 +5,8 @@ #include "envoy/ext_authz/ext_authz.h" +#include "common/ext_authz/ext_authz_impl.h" + #include "gmock/gmock.h" namespace Envoy { From e0e6b77c59bf493b3fae8427ef1f7115cd7f3b30 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Mon, 29 Jan 2018 20:34:49 -0800 Subject: [PATCH 11/17] Fix format. Signed-off-by: Saurabh Mohan --- source/common/config/well_known_names.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/config/well_known_names.h b/source/common/config/well_known_names.h index 31f8594d6d07b..22f50ef89407b 100644 --- a/source/common/config/well_known_names.h +++ b/source/common/config/well_known_names.h @@ -136,7 +136,7 @@ class HttpFilterNameValues { const std::string SQUASH = "envoy.squash"; // External Authorization filter const std::string EXT_AUTHORIZATION = "envoy.ext_authz"; - + // Converts names from v1 to v2 const V1Converter v1_converter_; From 1f84caa5090b02317f309e1dccc2465d5c2e6bd4 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Mon, 29 Jan 2018 22:29:59 -0800 Subject: [PATCH 12/17] Add more ut for context peer. Signed-off-by: Saurabh Mohan --- test/common/ext_authz/ext_authz_impl_test.cc | 23 ++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/common/ext_authz/ext_authz_impl_test.cc b/test/common/ext_authz/ext_authz_impl_test.cc index 3508ae77b956b..630987865f9d5 100644 --- a/test/common/ext_authz/ext_authz_impl_test.cc +++ b/test/common/ext_authz/ext_authz_impl_test.cc @@ -166,5 +166,28 @@ TEST_F(ExtAuthzCheckRequestGeneratorTest, BasicHttp) { check_request_generator_.createHttpCheck(&callbacks_, headers, request); } +TEST_F(ExtAuthzCheckRequestGeneratorTest, CheckAttrContextPeer) { + + Http::TestHeaderMapImpl request_headers{{"x-envoy-downstream-service-cluster", "foo"}, + {":path", "/bar"}}; + envoy::service::auth::v2::CheckRequest request; + + EXPECT_CALL(callbacks_, connection()).WillRepeatedly(Return(&connection_)); + EXPECT_CALL(connection_, remoteAddress()).WillRepeatedly(ReturnRef(addr_)); + EXPECT_CALL(connection_, localAddress()).WillRepeatedly(ReturnRef(addr_)); + EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); + EXPECT_CALL(callbacks_, streamId()).WillRepeatedly(Return(0)); + EXPECT_CALL(callbacks_, requestInfo()).WillRepeatedly(ReturnRef(req_info_)); + EXPECT_CALL(req_info_, protocol()).WillRepeatedly(ReturnRef(protocol_)); + + EXPECT_CALL(ssl_, uriSanPeerCertificate()).WillOnce(Return("source")); + EXPECT_CALL(ssl_, uriSanLocalCertificate()).WillOnce(Return("destination")); + check_request_generator_.createHttpCheck(&callbacks_, request_headers, request); + + EXPECT_EQ("source", request.attributes().source().principal()); + EXPECT_EQ("destination", request.attributes().destination().principal()); + EXPECT_EQ("foo", request.attributes().source().service()); +} + } // namespace ExtAuthz } // namespace Envoy From 737171c6b34cdaad1e82cc8f317f0d31bd5c5033 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Tue, 30 Jan 2018 13:30:10 -0800 Subject: [PATCH 13/17] Remove mock for proto generation: common changes. Signed-off-by: Saurabh Mohan --- source/common/ext_authz/ext_authz_impl.h | 26 ++++++------------------ test/mocks/ext_authz/BUILD | 1 - test/mocks/ext_authz/mocks.cc | 3 --- test/mocks/ext_authz/mocks.h | 15 -------------- 4 files changed, 6 insertions(+), 39 deletions(-) diff --git a/source/common/ext_authz/ext_authz_impl.h b/source/common/ext_authz/ext_authz_impl.h index 640bd06afa53b..5228efeba3c55 100644 --- a/source/common/ext_authz/ext_authz_impl.h +++ b/source/common/ext_authz/ext_authz_impl.h @@ -62,16 +62,13 @@ class GrpcClientImpl : public Client, public ExtAuthzAsyncCallbacks { }; /** - * An interface for creating ext_authz.proto (authorization) request. + * For creating ext_authz.proto (authorization) request. * CheckRequestGenerator 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. */ -class CheckRequestGenerator { +class ExtAuthzCheckRequestGenerator { public: - // Destructor - virtual ~CheckRequestGenerator() {} - /** * createHttpCheck is used to extract the attributes from the stream and the http headers * and fill them up in the CheckRequest proto message. @@ -81,9 +78,10 @@ class CheckRequestGenerator { * @param request is the reference to the check request that will be filled up. * */ - virtual void createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, - const Envoy::Http::HeaderMap& headers, - envoy::service::auth::v2::CheckRequest& request) PURE; + void createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, + const Envoy::Http::HeaderMap& headers, + envoy::service::auth::v2::CheckRequest& request); + /** * createTcpCheck is used to extract the attributes from the network layer and fill them up * in the CheckRequest proto message. @@ -91,18 +89,6 @@ class CheckRequestGenerator { * @param request is the reference to the check request that will be filled up. * */ - virtual void createTcpCheck(const Network::ReadFilterCallbacks* callbacks, - envoy::service::auth::v2::CheckRequest& request) PURE; -}; - -typedef std::unique_ptr CheckRequestGeneratorPtr; - -class ExtAuthzCheckRequestGenerator : public CheckRequestGenerator { -public: - // ExtAuthz::CheckRequestGenerator - void createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, - const Envoy::Http::HeaderMap& headers, - envoy::service::auth::v2::CheckRequest& request); void createTcpCheck(const Network::ReadFilterCallbacks* callbacks, envoy::service::auth::v2::CheckRequest& request); diff --git a/test/mocks/ext_authz/BUILD b/test/mocks/ext_authz/BUILD index 23f64cf66d764..725c43b303014 100644 --- a/test/mocks/ext_authz/BUILD +++ b/test/mocks/ext_authz/BUILD @@ -14,6 +14,5 @@ envoy_cc_mock( hdrs = ["mocks.h"], deps = [ "//include/envoy/ext_authz:ext_authz_interface", - "//source/common/ext_authz:ext_authz_lib", ], ) diff --git a/test/mocks/ext_authz/mocks.cc b/test/mocks/ext_authz/mocks.cc index 6365eb1b90dac..97a2cc6cb5493 100644 --- a/test/mocks/ext_authz/mocks.cc +++ b/test/mocks/ext_authz/mocks.cc @@ -6,8 +6,5 @@ namespace ExtAuthz { MockClient::MockClient() {} MockClient::~MockClient() {} -MockCheckRequestGen::MockCheckRequestGen() {} -MockCheckRequestGen::~MockCheckRequestGen() {} - } // namespace ExtAuthz } // namespace Envoy diff --git a/test/mocks/ext_authz/mocks.h b/test/mocks/ext_authz/mocks.h index add0c73f2ec06..057e9fc9263e4 100644 --- a/test/mocks/ext_authz/mocks.h +++ b/test/mocks/ext_authz/mocks.h @@ -5,8 +5,6 @@ #include "envoy/ext_authz/ext_authz.h" -#include "common/ext_authz/ext_authz_impl.h" - #include "gmock/gmock.h" namespace Envoy { @@ -24,18 +22,5 @@ class MockClient : public Client { Tracing::Span& parent_span)); }; -class MockCheckRequestGen : public CheckRequestGenerator { -public: - MockCheckRequestGen(); - ~MockCheckRequestGen(); - - // ExtAuthz::CheckRequestGenerator - MOCK_METHOD3(createHttpCheck, void(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, - const Envoy::Http::HeaderMap& headers, - envoy::service::auth::v2::CheckRequest& request)); - MOCK_METHOD2(createTcpCheck, void(const Network::ReadFilterCallbacks* callbacks, - envoy::service::auth::v2::CheckRequest& request)); -}; - } // namespace ExtAuthz } // namespace Envoy From 3737325fc1233a9f16fe4b4bf374b78f03c1e59a Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Wed, 31 Jan 2018 14:45:55 -0800 Subject: [PATCH 14/17] Review comments common. Signed-off-by: Saurabh Mohan --- include/envoy/http/header_map.h | 10 +++ include/envoy/http/protocol.h | 15 ++++ source/common/ext_authz/BUILD | 1 + source/common/ext_authz/ext_authz_impl.cc | 77 ++++++-------------- source/common/ext_authz/ext_authz_impl.h | 33 ++++----- source/common/network/BUILD | 1 + source/common/network/utility.cc | 12 +++ source/common/network/utility.h | 8 ++ test/common/ext_authz/ext_authz_impl_test.cc | 18 ++--- 9 files changed, 95 insertions(+), 80 deletions(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 1cc91e35f7564..a420c64121e14 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -86,6 +86,16 @@ class HeaderString { */ const char* c_str() const { return buffer_.ref_; } + /** + * @return a std::string. + */ + std::string getString() const { + if (string_length_) { + return std::string(buffer_.ref_, string_length_); + } + return ""; + } + /** * Return the string to a default state. Reference strings are not touched. Both inline/dynamic * strings are reset to zero size. diff --git a/include/envoy/http/protocol.h b/include/envoy/http/protocol.h index df026f781750e..e7012fc4b204d 100644 --- a/include/envoy/http/protocol.h +++ b/include/envoy/http/protocol.h @@ -1,4 +1,5 @@ #pragma once +#include namespace Envoy { namespace Http { @@ -10,5 +11,19 @@ namespace Http { enum class Protocol { Http10, Http11, Http2 }; const size_t NumProtocols = 3; +inline std::string getProtocolString(const Protocol& p) { + switch (p) { + case Protocol::Http10: + return std::string("Http1.0"); + case Protocol::Http11: + return std::string("Http1.1"); + case Protocol::Http2: + return std::string("Http2"); + default: + break; + } + return ""; +} + } // namespace Http } // namespace Envoy diff --git a/source/common/ext_authz/BUILD b/source/common/ext_authz/BUILD index c1c8612cc1a0b..7b658867dfc6c 100644 --- a/source/common/ext_authz/BUILD +++ b/source/common/ext_authz/BUILD @@ -27,6 +27,7 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/grpc:async_client_lib", "//source/common/http:headers_lib", + "//source/common/network:utility_lib", "//source/common/protobuf", "//source/common/tracing:http_tracer_lib", ], diff --git a/source/common/ext_authz/ext_authz_impl.cc b/source/common/ext_authz/ext_authz_impl.cc index a133272e9b219..f115b785c42bd 100644 --- a/source/common/ext_authz/ext_authz_impl.cc +++ b/source/common/ext_authz/ext_authz_impl.cc @@ -11,6 +11,7 @@ #include "common/common/assert.h" #include "common/grpc/async_client_impl.h" #include "common/http/headers.h" +#include "common/network/utility.h" #include "common/protobuf/protobuf.h" #include "fmt/format.h" @@ -64,28 +65,16 @@ void GrpcClientImpl::onFailure(Grpc::Status::GrpcStatus status, const std::strin callbacks_ = nullptr; } -void ExtAuthzCheckRequestGenerator::addressToProtobufAddress( - envoy::api::v2::Address& proto_address, const Network::Address::Instance& address) { - if (address.type() == Network::Address::Type::Pipe) { - proto_address.mutable_pipe()->set_path(address.asString()); - } else { - ASSERT(address.type() == Network::Address::Type::Ip); - auto* socket_address = proto_address.mutable_socket_address(); - socket_address->set_address(address.ip()->addressAsString()); - socket_address->set_port_value(address.ip()->port()); - } -} - -void ExtAuthzCheckRequestGenerator::setAttrContextPeer( - envoy::service::auth::v2::AttributeContext_Peer& peer, const Network::Connection& connection, - const std::string& service, const bool local) { +void CreateCheckRequest::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(); if (!local) { - addressToProtobufAddress(*addr, *connection.remoteAddress()); + Envoy::Network::Utility::addressToProtobufAddress(*connection.remoteAddress(), *addr); } else { - addressToProtobufAddress(*addr, *connection.localAddress()); + Envoy::Network::Utility::addressToProtobufAddress(*connection.localAddress(), *addr); } // Set the principal @@ -115,38 +104,14 @@ void ExtAuthzCheckRequestGenerator::setAttrContextPeer( } } -const std::string ExtAuthzCheckRequestGenerator::getProtocolStr(const Envoy::Http::Protocol& p) { - - switch (p) { - case Envoy::Http::Protocol::Http10: - return std::string("Http1.0"); - case Envoy::Http::Protocol::Http11: - return std::string("Http1.1"); - case Envoy::Http::Protocol::Http2: - return std::string("Http2"); - default: - break; - } - return std::string("unknown"); -} - -Envoy::Http::HeaderMap::Iterate -ExtAuthzCheckRequestGenerator::fillHttpHeaders(const Envoy::Http::HeaderEntry& e, void* ctx) { - Envoy::Protobuf::Map<::std::string, ::std::string>* mhdrs = - static_cast*>(ctx); - (*mhdrs)[std::string(e.key().c_str(), e.key().size())] = - std::string(e.value().c_str(), e.value().size()); - return Envoy::Http::HeaderMap::Iterate::Continue; -} - -std::string ExtAuthzCheckRequestGenerator::getHeaderStr(const Envoy::Http::HeaderEntry* entry) { +std::string CreateCheckRequest::getHeaderStr(const Envoy::Http::HeaderEntry* entry) { if (entry) { - return std::string(entry->value().c_str(), entry->value().size()); + return entry->value().getString(); } return ""; } -void ExtAuthzCheckRequestGenerator::setHttpRequest( +void CreateCheckRequest::setHttpRequest( ::envoy::service::auth::v2::AttributeContext_HttpRequest& httpreq, const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, const Envoy::Http::HeaderMap& headers) { @@ -172,24 +137,31 @@ void ExtAuthzCheckRequestGenerator::setHttpRequest( // Set protocol if (sdfc->requestInfo().protocol().valid()) { - httpreq.set_protocol(getProtocolStr(sdfc->requestInfo().protocol().value())); + httpreq.set_protocol(Envoy::Http::getProtocolString(sdfc->requestInfo().protocol().value())); } // Fill in the headers auto mhdrs = httpreq.mutable_headers(); - headers.iterate(fillHttpHeaders, mhdrs); + headers.iterate( + [](const Envoy::Http::HeaderEntry& e, void* ctx) { + Envoy::Protobuf::Map<::std::string, ::std::string>* mutable_headers = + static_cast*>(ctx); + (*mutable_headers)[e.key().getString()] = e.value().getString(); + return Envoy::Http::HeaderMap::Iterate::Continue; + }, + mhdrs); } -void ExtAuthzCheckRequestGenerator::setAttrContextRequest( +void CreateCheckRequest::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 ExtAuthzCheckRequestGenerator::createHttpCheck( - const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, - const Envoy::Http::HeaderMap& headers, envoy::service::auth::v2::CheckRequest& request) { +void CreateCheckRequest::createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, + const Envoy::Http::HeaderMap& headers, + envoy::service::auth::v2::CheckRequest& request) { auto attrs = request.mutable_attributes(); @@ -203,9 +175,8 @@ void ExtAuthzCheckRequestGenerator::createHttpCheck( setAttrContextRequest(*attrs->mutable_request(), callbacks, headers); } -void ExtAuthzCheckRequestGenerator::createTcpCheck( - const Network::ReadFilterCallbacks* callbacks, - envoy::service::auth::v2::CheckRequest& request) { +void CreateCheckRequest::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 5228efeba3c55..10e534e3a85d9 100644 --- a/source/common/ext_authz/ext_authz_impl.h +++ b/source/common/ext_authz/ext_authz_impl.h @@ -63,11 +63,11 @@ class GrpcClientImpl : public Client, public ExtAuthzAsyncCallbacks { /** * For creating ext_authz.proto (authorization) request. - * CheckRequestGenerator is used to extract attributes from the TCP/HTTP request + * CreateCheckRequest 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. */ -class ExtAuthzCheckRequestGenerator { +class CreateCheckRequest { public: /** * createHttpCheck is used to extract the attributes from the stream and the http headers @@ -78,9 +78,9 @@ class ExtAuthzCheckRequestGenerator { * @param request is the reference to the check request that will be filled up. * */ - void createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, - const Envoy::Http::HeaderMap& headers, - envoy::service::auth::v2::CheckRequest& request); + static void createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, + const Envoy::Http::HeaderMap& headers, + envoy::service::auth::v2::CheckRequest& request); /** * createTcpCheck is used to extract the attributes from the network layer and fill them up @@ -89,23 +89,20 @@ class ExtAuthzCheckRequestGenerator { * @param request is the reference to the check request that will be filled up. * */ - void createTcpCheck(const Network::ReadFilterCallbacks* callbacks, - envoy::service::auth::v2::CheckRequest& request); + static void createTcpCheck(const Network::ReadFilterCallbacks* callbacks, + envoy::service::auth::v2::CheckRequest& request); private: - void addressToProtobufAddress(envoy::api::v2::Address& proto_address, - const Network::Address::Instance& address); - void setAttrContextPeer(envoy::service::auth::v2::AttributeContext_Peer& peer, - const Network::Connection& connection, const std::string& service, - const bool local); - void setHttpRequest(::envoy::service::auth::v2::AttributeContext_HttpRequest& httpreq, - const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, - const Envoy::Http::HeaderMap& headers); - void setAttrContextRequest(::envoy::service::auth::v2::AttributeContext_Request& req, + static void setAttrContextPeer(envoy::service::auth::v2::AttributeContext_Peer& peer, + const Network::Connection& connection, const std::string& service, + const bool local); + static void setHttpRequest(::envoy::service::auth::v2::AttributeContext_HttpRequest& httpreq, const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, const Envoy::Http::HeaderMap& headers); - const std::string getProtocolStr(const Envoy::Http::Protocol&); - std::string getHeaderStr(const Envoy::Http::HeaderEntry* entry); + static void setAttrContextRequest(::envoy::service::auth::v2::AttributeContext_Request& req, + const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, + const Envoy::Http::HeaderMap& headers); + static std::string getHeaderStr(const Envoy::Http::HeaderEntry* entry); static Envoy::Http::HeaderMap::Iterate fillHttpHeaders(const Envoy::Http::HeaderEntry&, void*); }; diff --git a/source/common/network/BUILD b/source/common/network/BUILD index eb9de564cee97..07bcd07b364b7 100644 --- a/source/common/network/BUILD +++ b/source/common/network/BUILD @@ -188,6 +188,7 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/common:utility_lib", "//source/common/protobuf", + "@envoy_api//envoy/api/v2:address_cc", "@envoy_api//envoy/api/v2:base_cc", ], ) diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 2bc91edbf6a62..e3482638796d4 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -371,5 +371,17 @@ absl::uint128 Utility::flipOrder(const absl::uint128& input) { return result; } +void Utility::addressToProtobufAddress(const Address::Instance& address, + envoy::api::v2::Address& proto_address) { + if (address.type() == Address::Type::Pipe) { + proto_address.mutable_pipe()->set_path(address.asString()); + } else { + ASSERT(address.type() == Address::Type::Ip); + auto* socket_address = proto_address.mutable_socket_address(); + socket_address->set_address(address.ip()->addressAsString()); + socket_address->set_port_value(address.ip()->port()); + } +} + } // namespace Network } // namespace Envoy diff --git a/source/common/network/utility.h b/source/common/network/utility.h index 16c58530617e5..de176d27b5bdc 100644 --- a/source/common/network/utility.h +++ b/source/common/network/utility.h @@ -201,6 +201,14 @@ class Utility { */ static absl::uint128 Ip6htonl(const absl::uint128& address); + /** + * Copies the address instance into the protobuf representation of an address. + * @param address is the address to be copied into the protobuf representation of this address. + * @param proto_address is the protobuf address to which the address instance is copied into. + */ + static void addressToProtobufAddress(const Address::Instance& address, + envoy::api::v2::Address& proto_address); + private: static void throwWithMalformedIp(const std::string& ip_address); diff --git a/test/common/ext_authz/ext_authz_impl_test.cc b/test/common/ext_authz/ext_authz_impl_test.cc index 630987865f9d5..0d78ed40040d0 100644 --- a/test/common/ext_authz/ext_authz_impl_test.cc +++ b/test/common/ext_authz/ext_authz_impl_test.cc @@ -122,16 +122,16 @@ TEST_F(ExtAuthzGrpcClientTest, Cancel) { client_.cancel(); } -class ExtAuthzCheckRequestGeneratorTest : public testing::Test { +class CreateCheckRequestTest : public testing::Test { public: - ExtAuthzCheckRequestGeneratorTest() { + CreateCheckRequestTest() { addr_ = std::make_shared("1.2.3.4", 1111); protocol_ = Envoy::Http::Protocol::Http10; }; Network::Address::InstanceConstSharedPtr addr_; Optional protocol_; - ExtAuthzCheckRequestGenerator check_request_generator_; + CreateCheckRequest check_request_generator_; NiceMock callbacks_; NiceMock net_callbacks_; NiceMock connection_; @@ -139,7 +139,7 @@ class ExtAuthzCheckRequestGeneratorTest : public testing::Test { NiceMock req_info_; }; -TEST_F(ExtAuthzCheckRequestGeneratorTest, BasicTcp) { +TEST_F(CreateCheckRequestTest, BasicTcp) { envoy::service::auth::v2::CheckRequest request; @@ -148,10 +148,10 @@ TEST_F(ExtAuthzCheckRequestGeneratorTest, BasicTcp) { EXPECT_CALL(connection_, localAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(Const(connection_), ssl()).Times(2).WillRepeatedly(Return(&ssl_)); - check_request_generator_.createTcpCheck(&net_callbacks_, request); + CreateCheckRequest::createTcpCheck(&net_callbacks_, request); } -TEST_F(ExtAuthzCheckRequestGeneratorTest, BasicHttp) { +TEST_F(CreateCheckRequestTest, BasicHttp) { Http::HeaderMapImpl headers; envoy::service::auth::v2::CheckRequest request; @@ -163,10 +163,10 @@ TEST_F(ExtAuthzCheckRequestGeneratorTest, 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_)); - check_request_generator_.createHttpCheck(&callbacks_, headers, request); + CreateCheckRequest::createHttpCheck(&callbacks_, headers, request); } -TEST_F(ExtAuthzCheckRequestGeneratorTest, CheckAttrContextPeer) { +TEST_F(CreateCheckRequestTest, CheckAttrContextPeer) { Http::TestHeaderMapImpl request_headers{{"x-envoy-downstream-service-cluster", "foo"}, {":path", "/bar"}}; @@ -182,7 +182,7 @@ TEST_F(ExtAuthzCheckRequestGeneratorTest, CheckAttrContextPeer) { EXPECT_CALL(ssl_, uriSanPeerCertificate()).WillOnce(Return("source")); EXPECT_CALL(ssl_, uriSanLocalCertificate()).WillOnce(Return("destination")); - check_request_generator_.createHttpCheck(&callbacks_, request_headers, request); + CreateCheckRequest::createHttpCheck(&callbacks_, request_headers, request); EXPECT_EQ("source", request.attributes().source().principal()); EXPECT_EQ("destination", request.attributes().destination().principal()); From 8ffb69d7e629410ef427b3523aafcd535854a841 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Fri, 2 Feb 2018 15:57:27 -0800 Subject: [PATCH 15/17] Minor review fixes. Signed-off-by: Saurabh Mohan --- source/common/ext_authz/ext_authz_impl.cc | 4 ++-- source/common/ext_authz/ext_authz_impl.h | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/source/common/ext_authz/ext_authz_impl.cc b/source/common/ext_authz/ext_authz_impl.cc index f115b785c42bd..38ece9de5bbcb 100644 --- a/source/common/ext_authz/ext_authz_impl.cc +++ b/source/common/ext_authz/ext_authz_impl.cc @@ -141,7 +141,7 @@ void CreateCheckRequest::setHttpRequest( } // Fill in the headers - auto mhdrs = httpreq.mutable_headers(); + auto mutable_headers = httpreq.mutable_headers(); headers.iterate( [](const Envoy::Http::HeaderEntry& e, void* ctx) { Envoy::Protobuf::Map<::std::string, ::std::string>* mutable_headers = @@ -149,7 +149,7 @@ void CreateCheckRequest::setHttpRequest( (*mutable_headers)[e.key().getString()] = e.value().getString(); return Envoy::Http::HeaderMap::Iterate::Continue; }, - mhdrs); + mutable_headers); } void CreateCheckRequest::setAttrContextRequest( diff --git a/source/common/ext_authz/ext_authz_impl.h b/source/common/ext_authz/ext_authz_impl.h index 10e534e3a85d9..560780df11b13 100644 --- a/source/common/ext_authz/ext_authz_impl.h +++ b/source/common/ext_authz/ext_authz_impl.h @@ -66,6 +66,8 @@ class GrpcClientImpl : public Client, public ExtAuthzAsyncCallbacks { * CreateCheckRequest 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 { public: From 906ec7c7b75028491212423287dddc76b81201e7 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Fri, 2 Feb 2018 17:10:31 -0800 Subject: [PATCH 16/17] Remove code post merge. Signed-off-by: Saurabh Mohan --- include/envoy/http/protocol.h | 15 --------------- source/common/ext_authz/BUILD | 1 + source/common/ext_authz/ext_authz_impl.cc | 4 +++- 3 files changed, 4 insertions(+), 16 deletions(-) diff --git a/include/envoy/http/protocol.h b/include/envoy/http/protocol.h index e7012fc4b204d..df026f781750e 100644 --- a/include/envoy/http/protocol.h +++ b/include/envoy/http/protocol.h @@ -1,5 +1,4 @@ #pragma once -#include namespace Envoy { namespace Http { @@ -11,19 +10,5 @@ namespace Http { enum class Protocol { Http10, Http11, Http2 }; const size_t NumProtocols = 3; -inline std::string getProtocolString(const Protocol& p) { - switch (p) { - case Protocol::Http10: - return std::string("Http1.0"); - case Protocol::Http11: - return std::string("Http1.1"); - case Protocol::Http2: - return std::string("Http2"); - default: - break; - } - return ""; -} - } // namespace Http } // namespace Envoy diff --git a/source/common/ext_authz/BUILD b/source/common/ext_authz/BUILD index 7b658867dfc6c..92c148e17a900 100644 --- a/source/common/ext_authz/BUILD +++ b/source/common/ext_authz/BUILD @@ -27,6 +27,7 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/grpc:async_client_lib", "//source/common/http:headers_lib", + "//source/common/http:utility_lib", "//source/common/network:utility_lib", "//source/common/protobuf", "//source/common/tracing:http_tracer_lib", diff --git a/source/common/ext_authz/ext_authz_impl.cc b/source/common/ext_authz/ext_authz_impl.cc index 38ece9de5bbcb..9ed8145cfbfaf 100644 --- a/source/common/ext_authz/ext_authz_impl.cc +++ b/source/common/ext_authz/ext_authz_impl.cc @@ -11,6 +11,7 @@ #include "common/common/assert.h" #include "common/grpc/async_client_impl.h" #include "common/http/headers.h" +#include "common/http/utility.h" #include "common/network/utility.h" #include "common/protobuf/protobuf.h" @@ -137,7 +138,8 @@ void CreateCheckRequest::setHttpRequest( // Set protocol if (sdfc->requestInfo().protocol().valid()) { - httpreq.set_protocol(Envoy::Http::getProtocolString(sdfc->requestInfo().protocol().value())); + httpreq.set_protocol( + Envoy::Http::Utility::getProtocolString(sdfc->requestInfo().protocol().value())); } // Fill in the headers From 00c957e9eebd0e7bcd73caffceb914d373ae2bb1 Mon Sep 17 00:00:00 2001 From: Saurabh Mohan Date: Tue, 6 Feb 2018 11:21:52 -0800 Subject: [PATCH 17/17] Review comments. Signed-off-by: Saurabh Mohan --- source/common/ext_authz/ext_authz_impl.cc | 24 ++-- source/common/ext_authz/ext_authz_impl.h | 8 +- test/common/ext_authz/ext_authz_impl_test.cc | 110 +++++++++---------- 3 files changed, 66 insertions(+), 76 deletions(-) diff --git a/source/common/ext_authz/ext_authz_impl.cc b/source/common/ext_authz/ext_authz_impl.cc index 9ed8145cfbfaf..d2b2a2eef3cc6 100644 --- a/source/common/ext_authz/ext_authz_impl.cc +++ b/source/common/ext_authz/ext_authz_impl.cc @@ -72,33 +72,31 @@ void CreateCheckRequest::setAttrContextPeer(envoy::service::auth::v2::AttributeC // Set the address auto addr = peer.mutable_address(); - if (!local) { - Envoy::Network::Utility::addressToProtobufAddress(*connection.remoteAddress(), *addr); - } else { + if (local) { Envoy::Network::Utility::addressToProtobufAddress(*connection.localAddress(), *addr); + } else { + Envoy::Network::Utility::addressToProtobufAddress(*connection.remoteAddress(), *addr); } // Set the principal // Preferably the SAN from the peer's cert or // Subject from the peer's cert. - std::string principal; Ssl::Connection* ssl = const_cast(connection.ssl()); if (ssl != nullptr) { - if (!local) { - principal = ssl->uriSanPeerCertificate(); + if (local) { + peer.set_principal(ssl->uriSanLocalCertificate()); - if (principal.empty()) { - principal = ssl->subjectPeerCertificate(); + if (peer.principal().empty()) { + peer.set_principal(ssl->subjectLocalCertificate()); } } else { - principal = ssl->uriSanLocalCertificate(); + peer.set_principal(ssl->uriSanPeerCertificate()); - if (principal.empty()) { - principal = ssl->subjectLocalCertificate(); + if (peer.principal().empty()) { + peer.set_principal(ssl->subjectPeerCertificate()); } } } - peer.set_principal(principal); if (!service.empty()) { peer.set_service(service); @@ -170,7 +168,7 @@ void CreateCheckRequest::createHttpCheck(const Envoy::Http::StreamDecoderFilterC Envoy::Http::StreamDecoderFilterCallbacks* cb = const_cast(callbacks); - std::string service = getHeaderStr(headers.EnvoyDownstreamServiceCluster()); + const std::string service = getHeaderStr(headers.EnvoyDownstreamServiceCluster()); setAttrContextPeer(*attrs->mutable_source(), *cb->connection(), service, false); setAttrContextPeer(*attrs->mutable_destination(), *cb->connection(), "", true); diff --git a/source/common/ext_authz/ext_authz_impl.h b/source/common/ext_authz/ext_authz_impl.h index 560780df11b13..862e0e61933f8 100644 --- a/source/common/ext_authz/ext_authz_impl.h +++ b/source/common/ext_authz/ext_authz_impl.h @@ -33,8 +33,8 @@ struct ConstantValues { typedef ConstSingleton Constants; -// TODO(htuch): We should have only one client per thread, but today we create one per filter stack. -// This will require support for more than one outstanding request per client. +// NOTE: We create gRPC client for each filter stack instead of a client per thread. +// That is ok since this is unary RPC and the cost of doing this is minimal. class GrpcClientImpl : public Client, public ExtAuthzAsyncCallbacks { public: GrpcClientImpl(Grpc::AsyncClientPtr&& async_client, @@ -75,7 +75,7 @@ class CreateCheckRequest { * createHttpCheck is used to extract the attributes from the stream and the http headers * and fill them up in the CheckRequest proto message. * @param callbacks supplies the Http stream context from which data can be extracted. - * @param headers supplies the heeader map with http headers that will be used to create the + * @param headers supplies the header map with http headers that will be used to create the * check request. * @param request is the reference to the check request that will be filled up. * @@ -87,7 +87,7 @@ class CreateCheckRequest { /** * createTcpCheck is used to extract the attributes from the network layer and fill them up * in the CheckRequest proto message. - * @param callbacks supplies the networklayer context from which data can be extracted. + * @param callbacks supplies the network layer context from which data can be extracted. * @param request is the reference to the check request that will be filled up. * */ diff --git a/test/common/ext_authz/ext_authz_impl_test.cc b/test/common/ext_authz/ext_authz_impl_test.cc index 0d78ed40040d0..77a171c68e7f5 100644 --- a/test/common/ext_authz/ext_authz_impl_test.cc +++ b/test/common/ext_authz/ext_authz_impl_test.cc @@ -48,70 +48,62 @@ class ExtAuthzGrpcClientTest : public testing::Test { Tracing::MockSpan span_; }; -TEST_F(ExtAuthzGrpcClientTest, Basic) { +TEST_F(ExtAuthzGrpcClientTest, BasicOK) { + envoy::service::auth::v2::CheckRequest request; std::unique_ptr response; + Http::HeaderMapImpl headers; + EXPECT_CALL(*async_client_, send(_, ProtoEq(request), _, _, _)).WillOnce(Return(&async_request_)); + + client_.check(request_callbacks_, request, Tracing::NullSpan::instance()); + + client_.onCreateInitialMetadata(headers); - { - envoy::service::auth::v2::CheckRequest request; - Http::HeaderMapImpl headers; - EXPECT_CALL(*async_client_, send(_, ProtoEq(request), Ref(client_), _, _)) - .WillOnce(Invoke([this](const Protobuf::MethodDescriptor& service_method, - const Protobuf::Message&, Grpc::AsyncRequestCallbacks&, - Tracing::Span&, - const Optional&) -> Grpc::AsyncRequest* { - EXPECT_EQ("envoy.service.auth.v2.Authorization", service_method.service()->full_name()); - EXPECT_EQ("Check", service_method.name()); - return &async_request_; - })); - - client_.check(request_callbacks_, request, Tracing::NullSpan::instance()); - - client_.onCreateInitialMetadata(headers); - EXPECT_EQ(nullptr, headers.RequestId()); - - response.reset(new envoy::service::auth::v2::CheckResponse()); - ::google::rpc::Status* status = new ::google::rpc::Status(); - status->set_code(Grpc::Status::GrpcStatus::PermissionDenied); - response->set_allocated_status(status); - EXPECT_CALL(span_, setTag("ext_authz_status", "ext_authz_unauthorized")); - EXPECT_CALL(request_callbacks_, onComplete(CheckStatus::Denied)); - client_.onSuccess(std::move(response), span_); - } - - { - envoy::service::auth::v2::CheckRequest request; - Http::HeaderMapImpl headers; - EXPECT_CALL(*async_client_, send(_, ProtoEq(request), _, _, _)) - .WillOnce(Return(&async_request_)); - - client_.check(request_callbacks_, request, Tracing::NullSpan::instance()); - - client_.onCreateInitialMetadata(headers); - - response.reset(new envoy::service::auth::v2::CheckResponse()); - ::google::rpc::Status* status = new ::google::rpc::Status(); - status->set_code(Grpc::Status::GrpcStatus::Ok); - response->set_allocated_status(status); - EXPECT_CALL(span_, setTag("ext_authz_status", "ext_authz_ok")); - EXPECT_CALL(request_callbacks_, onComplete(CheckStatus::OK)); - client_.onSuccess(std::move(response), span_); - } - - { - envoy::service::auth::v2::CheckRequest request; - EXPECT_CALL(*async_client_, send(_, ProtoEq(request), _, _, _)) - .WillOnce(Return(&async_request_)); - - client_.check(request_callbacks_, request, Tracing::NullSpan::instance()); - - response.reset(new envoy::service::auth::v2::CheckResponse()); - EXPECT_CALL(request_callbacks_, onComplete(CheckStatus::Error)); - client_.onFailure(Grpc::Status::Unknown, "", span_); - } + response = std::make_unique(); + auto status = response->mutable_status(); + status->set_code(Grpc::Status::GrpcStatus::Ok); + EXPECT_CALL(span_, setTag("ext_authz_status", "ext_authz_ok")); + EXPECT_CALL(request_callbacks_, onComplete(CheckStatus::OK)); + client_.onSuccess(std::move(response), span_); } -TEST_F(ExtAuthzGrpcClientTest, Cancel) { +TEST_F(ExtAuthzGrpcClientTest, BasicDenied) { + envoy::service::auth::v2::CheckRequest request; std::unique_ptr response; + Http::HeaderMapImpl headers; + EXPECT_CALL(*async_client_, send(_, ProtoEq(request), Ref(client_), _, _)) + .WillOnce( + Invoke([this](const Protobuf::MethodDescriptor& service_method, const Protobuf::Message&, + Grpc::AsyncRequestCallbacks&, Tracing::Span&, + const Optional&) -> Grpc::AsyncRequest* { + EXPECT_EQ("envoy.service.auth.v2.Authorization", service_method.service()->full_name()); + EXPECT_EQ("Check", service_method.name()); + return &async_request_; + })); + + client_.check(request_callbacks_, request, Tracing::NullSpan::instance()); + + client_.onCreateInitialMetadata(headers); + EXPECT_EQ(nullptr, headers.RequestId()); + + response = std::make_unique(); + auto status = response->mutable_status(); + status->set_code(Grpc::Status::GrpcStatus::PermissionDenied); + EXPECT_CALL(span_, setTag("ext_authz_status", "ext_authz_unauthorized")); + EXPECT_CALL(request_callbacks_, onComplete(CheckStatus::Denied)); + client_.onSuccess(std::move(response), span_); +} + +TEST_F(ExtAuthzGrpcClientTest, BasicError) { + envoy::service::auth::v2::CheckRequest request; + EXPECT_CALL(*async_client_, send(_, ProtoEq(request), _, _, _)).WillOnce(Return(&async_request_)); + + client_.check(request_callbacks_, request, Tracing::NullSpan::instance()); + + EXPECT_CALL(request_callbacks_, onComplete(CheckStatus::Error)); + client_.onFailure(Grpc::Status::Unknown, "", span_); +} + +TEST_F(ExtAuthzGrpcClientTest, Cancel) { envoy::service::auth::v2::CheckRequest request; EXPECT_CALL(*async_client_, send(_, _, _, _, _)).WillOnce(Return(&async_request_));