-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Support for external authorization grpc service. #2415
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
79b97f0
06a0eec
bf7d7be
7834705
48edb72
1612396
4a197fb
141a203
0df4b9a
cea2ed6
b4c7fea
93776d8
195df83
e0e6b77
1f84caa
737171c
3737325
61f8f8f
8ffb69d
906ec7c
3d88e76
00c957e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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", | ||
| ], | ||
| ) | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| #pragma once | ||
|
|
||
| #include <chrono> | ||
| #include <memory> | ||
| #include <string> | ||
| #include <vector> | ||
|
|
||
| #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; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I think |
||
| }; | ||
|
|
||
|
|
||
| class Client { | ||
| public: | ||
| // Destructor | ||
| virtual ~Client() {} | ||
|
|
||
| /** | ||
| * Cancel an inflight Check request. | ||
| */ | ||
| virtual void cancel() PURE; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: can there only ever be a single inflight request per client with this interface? Does this imply that each request must have its own
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking about this a bit more, maybe we don't need to do the TLS thing at all here. This is unary RPC, so we don't have a significant cost in rebuilding the client each time, it should be a fairly small construction/destruction cost. I think what you have in this PR is fine, I would be more concerned if we had a bidi streaming auth check.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking that the per-thread gRPC would save us setup time and hence would help even in the unary gRPC case. But if the cluster is going to do that for us then I agree changing it would not buy us much.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, for Envoy gRPC, this will come for free from the cluster pool management. In theory, Google gRPC also does this, based on good authority, but I haven't validated. Let's work on your other PRs first and we can circle back to this later. |
||
|
|
||
| // A check call. | ||
| virtual void check(RequestCallbacks &callback, const envoy::api::v2::auth::CheckRequest& request, | ||
| Tracing::Span& parent_span) PURE; | ||
|
|
||
| }; | ||
|
|
||
| typedef std::unique_ptr<Client> ClientPtr; | ||
|
|
||
| /** | ||
| * An interface for creating a external authorization client. | ||
| */ | ||
| class ClientFactory { | ||
| public: | ||
| virtual ~ClientFactory() {} | ||
|
|
||
| /** | ||
| * Return a new authz client. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @param docs for all parameters on
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also @return for the return type & text. |
||
| */ | ||
| virtual ClientPtr create(const Optional<std::chrono::milliseconds>& timeout) PURE; | ||
| }; | ||
|
|
||
| typedef std::unique_ptr<ClientFactory> 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 { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is really a helper utility class. Generally we don't put these in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Initially I had it in the I think that the mock is needed. @htuch Should I just keep it here or move to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you really need to mock the proto creation? These are basically straight line pure functions. Personally, I would mock the input parameters, since it's just as easy to do and results in easier to understand code.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think i know what you mean here. Let me try to redo the ut's without using the mock.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @htuch last commit removes the mock for proto creation. |
||
| 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<CheckRequestGenerator> CheckRequestGeneratorPtr; | ||
|
|
||
| } // namespace ExtAuthz | ||
| } // namespace Envoy | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,7 +81,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, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like gRPC access logging protos should be enhanced for unauthorized? Can you add a TODO and take care of this in one of your follow ups?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. I'll do it in the follow up pr; thanks. |
||
| "A flag has been added. Fix this code."); | ||
|
|
||
| if (request_info.getResponseFlag(RequestInfo::ResponseFlag::FailedLocalHealthCheck)) { | ||
|
|
@@ -131,6 +131,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)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would remove all of this for now and keep it locally. It will bit rot otherwise. |
||
| // common_access_log.mutable_response_flags()->set_unauthorized(true); | ||
| // } | ||
|
|
||
| } | ||
|
|
||
| void HttpGrpcAccessLog::log(const Http::HeaderMap* request_headers, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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"], | ||
| 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", | ||
| "//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", | ||
| ], | ||
| ) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, the recent merges of envoyproxy/data-plane-api#421 and #2430 will break this (and other parts of your PR). Be prepared to update when you merge master or pull in an updated data-plane-api SHA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the heads up. I'll do another merge with master to pull in the latest data-plane-api and rework the impacted code.