External AuthZ C++ Data plane enablement #7459

nickrmc83 · 2019-07-03T16:18:51Z

Description:
This change defines C++ grpc bindings for the external AuthZ interface. In the istio Security WG we're planning on using this interface to provide transparent authentication of requests. As this interface lies on the data plane we wish to implement it in C++ to maintain consistent and predictable performance

The changes allow a C++ implementation built using bazel to import Envoy as a workspace dependency and generate the C++ gRPC bindings directly without the need for complex import and generation scripts.

Risk Level:
Medium
Testing:
N/A
Docs Changes:
N/A
Release Notes:
N/A

- Initial build-system changes to include definition of C++ stub services. - Added new target for ext_authz service. Signed-off-by: Nick A. Smith <[email protected]>

Signed-off-by: Nick A. Smith <[email protected]>

junr03 · 2019-07-03T17:30:01Z

@lizan could you take a look at this?

lizan

/retest

I believe the tsan failure is irrelevant.

lizan · 2019-07-08T22:36:47Z

api/envoy/api/v2/core/BUILD

@@ -35,7 +35,7 @@ api_proto_library_internal(
    name = "base",
    srcs = ["base.proto"],
    visibility = [
-        ":friends",
+        "//visibility:public",


why did you need to change this?

Without the public visibility qualifier and given the bazel WORKSPACE extract shown below, the following error is encountered:

# Envoy API definitions ## We import envoy_api as a sub-repo of the mono-repo git_repository( name = "envoy", branch = "data-plane", remote = "https://github.com/thales-e-security/envoy.git", verbose = True, ) # Create @envoy_api virtual repository. load("@envoy//bazel:api_repositories.bzl", "envoy_api_dependencies") envoy_api_dependencies() load("@envoy_api//bazel:repositories.bzl", "api_dependencies") # api_dependencies() must be called first to import the correct versions of dependencies. api_dependencies()

ERROR: xxx/.cache/bazel/_bazel_xxx/xxx/external/envoy_api/envoy/service/auth/v2/BUILD:5:1: in proto_library rule @envoy_api//envoy/service/auth/v2:attribute_context: target '@envoy_api//envoy/api/v2/core:base' is not visible from target '@envoy_api//envoy/service/auth/v2:attribute_context'. Check the visibility declaration of the former target if you think the dependency is legitimate

It appears https://github.com/envoyproxy/envoy/blob/master/api/envoy/api/v2/BUILD#L15 allows the external authz service definition to use types defined in base.proto. However, external implementers of https://github.com/envoyproxy/envoy/blob/master/api/envoy/service/auth/v2/external_auth.proto are not afforded the same rights.

The options I considered were:

Make the contents of base.proto public

Move the required types into another proto file and make that public.

The chose the former for simplicity as well as not wanting to cause significant rework for existing filters.

I cannot reproduce your error with bazel 0.27.0 with a project importing envoy like

local_repository( name = "envoy", path = "/Users/lizan/github/lizan/envoy/7459", ) load("@envoy//bazel:api_repositories.bzl", "envoy_api_dependencies") envoy_api_dependencies() load("@envoy//bazel:repositories.bzl", "GO_VERSION", "envoy_dependencies") load("@envoy//bazel:cc_configure.bzl", "cc_configure") envoy_dependencies() load("@rules_foreign_cc//:workspace_definitions.bzl", "rules_foreign_cc_dependencies") rules_foreign_cc_dependencies() cc_configure() load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies") go_rules_dependencies() go_register_toolchains(go_version = GO_VERSION)

The code in /Users/lizan/github/lizan/envoy/7459 contains this PR and reverted this line.

bazel build @envoy_api//envoy/service/auth/v2:external_auth_service_cc_grpc

works well too.

The visibility restriction is to avoid accidental unindented dependencies to core, so this shouldn't be changed.

Hmmm. With bazel 0.25.2 and 0.27.1 on Linux I get the original error even with the WORKSPACE snippet provided. I'll dig more tomorrow.

@lizan you were right. I will revert to a visibility of :friends.

repokitteh-read-only · 2019-07-08T22:37:06Z

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #7459 (review) was submitted by @lizan.

see: more, trace.

lizan · 2019-07-11T23:23:15Z

@nickrmc83 can you fix DCO by rebase or squash? otherwise LGTM.

/wait

stale · 2019-07-19T00:19:36Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2019-07-26T00:50:31Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

sungsk · 2019-07-29T21:39:27Z

Gentle bump -
This PR seems ready to be merged, but was closed due to staleness. Can we expect this to be merged soon?

dio · 2019-07-29T22:12:13Z

Let's reopen this. And hope @nickrmc83 could help on fixing the DCO.

stale · 2019-08-05T22:50:32Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Subbed //visibility:public to :friends as per code review Signed-off-by: Nick Smith <[email protected]>

repokitteh-read-only · 2019-08-06T11:46:10Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7459 was synchronize by nickrmc83.

see: more, trace.

htuch · 2019-08-07T03:49:08Z

api/envoy/service/auth/v2/BUILD

@@ -26,3 +26,11 @@ api_proto_library_internal(
        "//envoy/type:http_status",
    ],
 )
+
+api_cc_grpc_library(


Why can't we build this under the hood of api_proto_library_internal, so we don't need to do explicit declaration for each of these? I'd prefer that things "just work" rather than having to boiler plate each individual thing we want to make a service consumable outside of Envoy.

This is something I pondered. I opted for this route because I thought it was unlikely that most services would require CC protos and services. Is the preference to add to api_proto_library a require_cc option analogous to require_py?

Yep, I think that would be a good idea (the additional option). Maybe call it grpc_services and make it a bool.

@htuch and @lizan see latest changes whereby api_cc_grpc_library and api_go_grpc_library are used by api_proto_library when has_services is set. I've refactored some of the services so they make use of this change. Some services that are Go-specific have remained the same so they can specialize calls to api_go_grpc_library as necessary.

`api_proto_library` will now enable the generatation of gRPC service stubs for Go and C++ if the `has_services` field is set. Updted services with Envoy-only dependencies to use new functionality removing now redundant `api_go_proto_library` declarations. Services with external dependencies have not been updated so that they can specialize `api_go_proto_library` declarations as necessary. Signed-off-by: Nick Smith <[email protected]>

repokitteh-read-only · 2019-08-08T11:43:08Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.
CC @envoyproxy/udpa-wg: FYI only for changes made to api/udpa/.

🐱

Caused by: #7459 was synchronize by nickrmc83.

see: more, trace.

repokitteh-read-only · 2019-08-08T12:07:21Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.
CC @envoyproxy/udpa-wg: FYI only for changes made to api/udpa/.

🐱

Caused by: #7459 was synchronize by nickrmc83.

see: more, trace.

repokitteh-read-only · 2019-08-08T12:17:31Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.
CC @envoyproxy/udpa-wg: FYI only for changes made to api/udpa/.

🐱

Caused by: #7459 was synchronize by nickrmc83.

see: more, trace.

repokitteh-read-only · 2019-08-08T13:00:32Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.
CC @envoyproxy/udpa-wg: FYI only for changes made to api/udpa/.

🐱

Caused by: #7459 was synchronize by nickrmc83.

see: more, trace.

repokitteh-read-only · 2019-08-08T13:09:06Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.
CC @envoyproxy/udpa-wg: FYI only for changes made to api/udpa/.

🐱

Caused by: #7459 was synchronize by nickrmc83.

see: more, trace.

stale · 2019-08-26T14:38:36Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2019-09-02T15:00:01Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

nickrmc83 · 2019-09-02T16:04:18Z

@htuch and @lizan This branch has just been updated with the final merge from master and so is ready to go.

repokitteh-read-only · 2019-09-03T02:15:48Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7459 was synchronize by htuch.

see: more, trace.

Signed-off-by: Nick Smith <[email protected]>

nickrmc83 · 2019-09-03T14:14:06Z

Looks like envoy-linux and envoy-linux (bazel release) both failed due to a time with the same test: //test/extensions/transport_sockets/tls:ssl_socket_test. I don't think they are a result of these changes.

htuch · 2019-09-03T22:45:53Z

@nickrmc83 sorry to do this, but I'm going to merge #8003 first, since it also mucks with api_build_system.bzl but is a much larger change, so we want to get it in ASAP now that it is LGTMed.

htuch · 2019-09-04T03:09:23Z

@nickrmc83 OK, #8003 is now merged; if you can merge master we can get this PR in as well, thanks again.

stale · 2019-09-11T03:23:11Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

The metrics and trace APIs have external dependencies which are not yet handled when generating CC stubs. Signed-off-by: Nick A. Smith <[email protected]>

nickrmc83 · 2019-09-15T17:49:53Z

@htuch this should be good to go.

htuch

Thanks!

This change defines C++ grpc bindings for the external AuthZ interface. In the istio Security WG we're planning on using this interface to provide transparent authentication of requests. As this interface lies on the data plane we wish to implement it in C++ to maintain consistent and predictable performance The changes allow a C++ implementation built using bazel to import Envoy as a workspace dependency and generate the C++ gRPC bindings directly without the need for complex import and generation scripts. Signed-off-by: Nick A. Smith <[email protected]>

Nick A. Smith added 2 commits June 10, 2019 10:56

Adding C++ envoy stub creation for ext_authz

2d09c5a

- Initial build-system changes to include definition of C++ stub services. - Added new target for ext_authz service. Signed-off-by: Nick A. Smith <[email protected]>

Adding public visibility to types, new naming for service.

d380900

Signed-off-by: Nick A. Smith <[email protected]>

junr03 assigned lizan Jul 3, 2019

lizan reviewed Jul 8, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 11, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 19, 2019

stale bot closed this Jul 26, 2019

dio reopened this Jul 29, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 29, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 5, 2019

Reverting base.proto visibility specification

f1f436c

Subbed //visibility:public to :friends as per code review Signed-off-by: Nick Smith <[email protected]>

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 6, 2019

repokitteh-read-only bot removed the waiting label Aug 6, 2019

htuch self-assigned this Aug 7, 2019

htuch reviewed Aug 7, 2019

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 26, 2019

Merge remote-tracking branch 'upstream/master' into data-plane

3f54bbb

stale bot closed this Sep 2, 2019

htuch reopened this Sep 3, 2019

htuch dismissed their stale review via 3f54bbb September 3, 2019 02:15

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Sep 3, 2019

repokitteh-read-only bot removed the waiting label Sep 3, 2019

Reinstating missing python protobuf dependencies.

37b4f45

Signed-off-by: Nick Smith <[email protected]>

nickrmc83 closed this Sep 3, 2019

nickrmc83 reopened this Sep 3, 2019

htuch added the waiting label Sep 4, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Sep 11, 2019

Merge remote-tracking branch 'upstream/master' into data-plane

cb203f8

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Sep 11, 2019

repokitteh-read-only bot removed the waiting label Sep 11, 2019

Disabled building external cc service bindingsi for metrics and trace

906f62e

The metrics and trace APIs have external dependencies which are not yet handled when generating CC stubs. Signed-off-by: Nick A. Smith <[email protected]>

htuch approved these changes Sep 16, 2019

View reviewed changes

htuch merged commit 41932e9 into envoyproxy:master Sep 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

External AuthZ C++ Data plane enablement #7459

External AuthZ C++ Data plane enablement #7459

nickrmc83 commented Jul 3, 2019

junr03 commented Jul 3, 2019

lizan left a comment

lizan Jul 8, 2019

nickrmc83 Jul 9, 2019

lizan Jul 10, 2019

nickrmc83 Jul 10, 2019

nickrmc83 Jul 11, 2019

repokitteh-read-only bot commented Jul 8, 2019

lizan commented Jul 11, 2019

stale bot commented Jul 19, 2019

stale bot commented Jul 26, 2019

sungsk commented Jul 29, 2019

dio commented Jul 29, 2019

stale bot commented Aug 5, 2019

repokitteh-read-only bot commented Aug 6, 2019

htuch Aug 7, 2019

nickrmc83 Aug 7, 2019

htuch Aug 8, 2019

nickrmc83 Aug 8, 2019

repokitteh-read-only bot commented Aug 8, 2019

repokitteh-read-only bot commented Aug 8, 2019

repokitteh-read-only bot commented Aug 8, 2019

repokitteh-read-only bot commented Aug 8, 2019

repokitteh-read-only bot commented Aug 8, 2019

stale bot commented Aug 26, 2019

stale bot commented Sep 2, 2019

nickrmc83 commented Sep 2, 2019 •

edited

Loading

repokitteh-read-only bot commented Sep 3, 2019

nickrmc83 commented Sep 3, 2019

htuch commented Sep 3, 2019

htuch commented Sep 4, 2019

stale bot commented Sep 11, 2019

nickrmc83 commented Sep 15, 2019

htuch left a comment

External AuthZ C++ Data plane enablement #7459

External AuthZ C++ Data plane enablement #7459

Conversation

nickrmc83 commented Jul 3, 2019

junr03 commented Jul 3, 2019

lizan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented Jul 8, 2019

lizan commented Jul 11, 2019

stale bot commented Jul 19, 2019

stale bot commented Jul 26, 2019

sungsk commented Jul 29, 2019

dio commented Jul 29, 2019

stale bot commented Aug 5, 2019

repokitteh-read-only bot commented Aug 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented Aug 8, 2019

repokitteh-read-only bot commented Aug 8, 2019

repokitteh-read-only bot commented Aug 8, 2019

repokitteh-read-only bot commented Aug 8, 2019

repokitteh-read-only bot commented Aug 8, 2019

stale bot commented Aug 26, 2019

stale bot commented Sep 2, 2019

nickrmc83 commented Sep 2, 2019 • edited Loading

repokitteh-read-only bot commented Sep 3, 2019

nickrmc83 commented Sep 3, 2019

htuch commented Sep 3, 2019

htuch commented Sep 4, 2019

stale bot commented Sep 11, 2019

nickrmc83 commented Sep 15, 2019

htuch left a comment

Choose a reason for hiding this comment

nickrmc83 commented Sep 2, 2019 •

edited

Loading