proto: unify envoy_proto_library/api_proto_library. (envoyproxy#4233)

In the latest iteration of
envoyproxy#4220, it was necessary to use
PGV constraints on fuzzer inputs. To do this would require PGV
generation in envoy_build_system.bzl.

There is also quite a bit of mess in
how we were doing envoy_proto_library() today. So, this PR allows us to
throw away the custom envoy_proto_library() and benefit from leveraging
a single source of Envoy proto build truth.

Risk level: Low
Testing: bazel test //test/...

Signed-off-by: Harvey Tuch <[email protected]>

api/bazel/api_build_system.bzl

@@ -88,7 +88,16 @@ def api_proto_library_internal(visibility = ["//visibility:private"], **kwargs):
 # gRPC stub generation.
 # TODO(htuch): Automatically generate go_proto_library and go_grpc_library
 # from api_proto_library.
-def api_proto_library(name, visibility = ["//visibility:private"], srcs = [], deps = [], has_services = 0, require_py = 1):
+def api_proto_library(
+        name,
+        visibility = ["//visibility:private"],
+        srcs = [],
+        deps = [],
+        external_proto_deps = [],
+        external_cc_proto_deps = [],
+        has_services = 0,
+        linkstatic = None,
+        require_py = 1):
     # This is now vestigial, since there are no direct consumers in
     # the data plane API. However, we want to maintain native proto_library support
     # in the proto graph to (1) support future C++ use of native rules with
@@ -99,15 +108,14 @@ def api_proto_library(name, visibility = ["//visibility:private"], srcs = [], de
     native.proto_library(
         name = name,
         srcs = srcs,
-        deps = deps + [
+        deps = deps + external_proto_deps + [
             "@com_google_protobuf//:any_proto",
             "@com_google_protobuf//:descriptor_proto",
             "@com_google_protobuf//:duration_proto",
             "@com_google_protobuf//:empty_proto",
             "@com_google_protobuf//:struct_proto",
             "@com_google_protobuf//:timestamp_proto",
             "@com_google_protobuf//:wrappers_proto",
-            "@googleapis//:api_httpbody_protos_proto",
             "@googleapis//:http_api_protos_proto",
             "@googleapis//:rpc_status_protos_lib",
             "@com_github_gogo_protobuf//:gogo_proto",
@@ -123,17 +131,29 @@ def api_proto_library(name, visibility = ["//visibility:private"], srcs = [], de
     pgv_cc_proto_library(
         name = _Suffix(name, _CC_SUFFIX),
         srcs = srcs,
+        linkstatic = linkstatic,
         deps = [_LibrarySuffix(d, _CC_SUFFIX) for d in deps],
-        external_deps = [
+        external_deps = external_cc_proto_deps + [
             "@com_google_protobuf//:cc_wkt_protos",
             "@googleapis//:http_api_protos",
             "@googleapis//:rpc_status_protos",
             "@com_github_gogo_protobuf//:gogo_proto_cc",
         ],
         visibility = ["//visibility:public"],
     )
+    py_export_suffixes = []
     if (require_py == 1):
         api_py_proto_library(name, srcs, deps, has_services)
+        py_export_suffixes = ["_py", "_py_genproto"]
+
+    # Allow unlimited visibility for consumers
+    export_suffixes = ["", "_cc", "_cc_validate", "_cc_proto", "_cc_proto_genproto"] + py_export_suffixes
+    for s in export_suffixes:
+        native.alias(
+            name = name + "_export" + s,
+            actual = name + s,
+            visibility = ["//visibility:public"],
+        )
 
 def api_cc_test(name, srcs, proto_deps):
     native.cc_test(

bazel/envoy_build_system.bzl

@@ -1,4 +1,5 @@
 load("@com_google_protobuf//:protobuf.bzl", "cc_proto_library", "py_proto_library")
+load("@envoy_api//bazel:api_build_system.bzl", "api_proto_library")
 
 def envoy_package():
     native.package(default_visibility = ["//visibility:public"])
@@ -398,55 +399,23 @@ def _proto_header(proto_path):
     return None
 
 # Envoy proto targets should be specified with this function.
-def envoy_proto_library(
-        name,
-        srcs = [],
-        deps = [],
-        external_deps = [],
-        generate_python = True):
-    # Ideally this would be native.{proto_library, cc_proto_library}.
-    # Unfortunately, this doesn't work with http_api_protos due to the PGV
-    # requirement to also use them in the non-native protobuf.bzl
-    # cc_proto_library; you end up with the same file built twice. So, also
-    # using protobuf.bzl cc_proto_library here.
-    cc_proto_deps = []
-    py_proto_deps = ["@com_google_protobuf//:protobuf_python"]
-
+def envoy_proto_library(name, external_deps = [], **kwargs):
+    external_proto_deps = []
+    external_cc_proto_deps = []
     if "api_httpbody_protos" in external_deps:
-        cc_proto_deps.append("@googleapis//:api_httpbody_protos")
-        py_proto_deps.append("@googleapis//:api_httpbody_protos_py")
-
-    if "http_api_protos" in external_deps:
-        cc_proto_deps.append("@googleapis//:http_api_protos")
-        py_proto_deps.append("@googleapis//:http_api_protos_py")
-
-    if "well_known_protos" in external_deps:
-        # WKT is already included for Python as part of standard deps above.
-        cc_proto_deps.append("@com_google_protobuf//:cc_wkt_protos")
-
-    cc_proto_library(
-        name = name,
-        srcs = srcs,
-        default_runtime = "@com_google_protobuf//:protobuf",
-        protoc = "@com_google_protobuf//:protoc",
-        deps = deps + cc_proto_deps,
+        external_cc_proto_deps.append("@googleapis//:api_httpbody_protos")
+        external_proto_deps.append("@googleapis//:api_httpbody_protos_proto")
+    return api_proto_library(
+        name,
+        external_cc_proto_deps = external_cc_proto_deps,
+        external_proto_deps = external_proto_deps,
         # Avoid generating .so, we don't need it, can interfere with builds
         # such as OSS-Fuzz.
         linkstatic = 1,
-        alwayslink = 1,
         visibility = ["//visibility:public"],
+        **kwargs
     )
 
-    if generate_python:
-        py_proto_library(
-            name = name + "_py",
-            srcs = srcs,
-            default_runtime = "@com_google_protobuf//:protobuf_python",
-            protoc = "@com_google_protobuf//:protoc",
-            deps = deps + py_proto_deps,
-            visibility = ["//visibility:public"],
-        )
-
 # Envoy proto descriptor targets should be specified with this function.
 # This is used for testing only.
 def envoy_proto_descriptor(name, out, srcs = [], external_deps = []):

source/common/ratelimit/BUILD

@@ -14,7 +14,7 @@ envoy_cc_library(
     srcs = ["ratelimit_impl.cc"],
     hdrs = ["ratelimit_impl.h"],
     deps = [
-        ":ratelimit_proto",
+        ":ratelimit_proto_cc",
         "//include/envoy/grpc:async_client_interface",
         "//include/envoy/grpc:async_client_manager_interface",
         "//include/envoy/ratelimit:ratelimit_interface",

test/common/access_log/BUILD

@@ -24,7 +24,6 @@ envoy_cc_library(
 envoy_proto_library(
     name = "access_log_formatter_fuzz_proto",
     srcs = ["access_log_formatter_fuzz.proto"],
-    generate_python = 0,
     deps = ["//test/fuzz:common_proto"],
 )
 
@@ -33,7 +32,7 @@ envoy_cc_fuzz_test(
     srcs = ["access_log_formatter_fuzz_test.cc"],
     corpus = "access_log_formatter_corpus",
     deps = [
-        ":access_log_formatter_fuzz_proto",
+        ":access_log_formatter_fuzz_proto_cc",
         "//source/common/access_log:access_log_formatter_lib",
         "//test/fuzz:utility_lib",
     ],

test/common/grpc/BUILD

@@ -18,7 +18,7 @@ envoy_cc_test(
         "//test/mocks/http:http_mocks",
         "//test/mocks/tracing:tracing_mocks",
         "//test/mocks/upstream:upstream_mocks",
-        "//test/proto:helloworld_proto",
+        "//test/proto:helloworld_proto_cc",
     ],
 )
 
@@ -40,7 +40,7 @@ envoy_cc_test(
     deps = [
         "//source/common/buffer:buffer_lib",
         "//source/common/grpc:codec_lib",
-        "//test/proto:helloworld_proto",
+        "//test/proto:helloworld_proto_cc",
     ],
 )
 
@@ -51,7 +51,7 @@ envoy_cc_test(
         "//source/common/grpc:common_lib",
         "//source/common/http:headers_lib",
         "//test/mocks/upstream:upstream_mocks",
-        "//test/proto:helloworld_proto",
+        "//test/proto:helloworld_proto_cc",
         "//test/test_common:utility_lib",
     ],
 )
@@ -66,7 +66,7 @@ envoy_cc_test(
         "//source/common/tracing:http_tracer_lib",
         "//test/mocks/grpc:grpc_mocks",
         "//test/mocks/tracing:tracing_mocks",
-        "//test/proto:helloworld_proto",
+        "//test/proto:helloworld_proto_cc",
         "//test/test_common:test_time_lib",
     ] + envoy_select_google_grpc(["//source/common/grpc:google_async_client_lib"]),
 )
@@ -99,7 +99,7 @@ envoy_cc_test_library(
         "//source/common/http/http2:conn_pool_lib",
         "//test/integration:integration_lib",
         "//test/mocks/local_info:local_info_mocks",
-        "//test/proto:helloworld_proto",
+        "//test/proto:helloworld_proto_cc",
         "//test/test_common:test_time_lib",
     ],
 )

test/common/http/BUILD

@@ -96,8 +96,6 @@ envoy_cc_test_library(
 envoy_proto_library(
     name = "conn_manager_impl_fuzz_proto",
     srcs = ["conn_manager_impl_fuzz.proto"],
-    external_deps = ["well_known_protos"],
-    generate_python = 0,
     deps = [
         "//test/fuzz:common_proto",
     ],
@@ -108,7 +106,7 @@ envoy_cc_fuzz_test(
     srcs = ["conn_manager_impl_fuzz_test.cc"],
     corpus = "conn_manager_impl_corpus",
     deps = [
-        ":conn_manager_impl_fuzz_proto",
+        ":conn_manager_impl_fuzz_proto_cc",
         "//source/common/common:empty_string",
         "//source/common/http:conn_manager_lib",
         "//source/common/http:date_provider_lib",
@@ -211,15 +209,14 @@ envoy_cc_test(
 envoy_proto_library(
     name = "header_map_impl_fuzz_proto",
     srcs = ["header_map_impl_fuzz.proto"],
-    external_deps = ["well_known_protos"],
 )
 
 envoy_cc_fuzz_test(
     name = "header_map_impl_fuzz_test",
     srcs = ["header_map_impl_fuzz_test.cc"],
     corpus = "header_map_impl_corpus",
     deps = [
-        ":header_map_impl_fuzz_proto",
+        ":header_map_impl_fuzz_proto_cc",
         "//source/common/http:header_map_lib",
     ],
 )
@@ -254,7 +251,7 @@ envoy_cc_fuzz_test(
     srcs = ["utility_fuzz_test.cc"],
     corpus = "utility_corpus",
     deps = [
-        ":utility_fuzz_proto",
+        ":utility_fuzz_proto_cc",
         "//source/common/http:utility_lib",
         "//test/test_common:utility_lib",
     ],

test/common/http/conn_manager_impl_corpus/example

@@ -60,7 +60,7 @@ actions {
   stream_action {
     stream_id: 0
     response {
-      continue_100_headers {}
+      continue_headers {}
     }
   }
 }

test/common/http/conn_manager_impl_fuzz.proto

@@ -66,7 +66,7 @@ message RequestAction {
 // TODO(htuch): Model and fuzz encoder filter buffering/resumption and different status returns.
 message ResponseAction {
   oneof response_action_selector {
-    test.fuzz.Headers continue_100_headers = 1;
+    test.fuzz.Headers continue_headers = 1;
     test.fuzz.Headers headers = 2;
     uint32 data = 3;
     test.fuzz.Headers trailers = 4;

test/common/http/conn_manager_impl_fuzz_test.cc

@@ -290,10 +290,10 @@ class FuzzStream {
                       const test::common::http::ResponseAction& response_action) {
     const bool end_stream = response_action.end_stream();
     switch (response_action.response_action_selector_case()) {
-    case test::common::http::ResponseAction::kContinue100Headers: {
+    case test::common::http::ResponseAction::kContinueHeaders: {
       if (state == StreamState::PendingHeaders) {
         auto headers = std::make_unique<TestHeaderMapImpl>(
-            Fuzz::fromHeaders(response_action.continue_100_headers()));
+            Fuzz::fromHeaders(response_action.continue_headers()));
         headers->setReferenceKey(Headers::get().Status, "100");
         decoder_filter_->callbacks_->encode100ContinueHeaders(std::move(headers));
       }

test/common/http/http2/BUILD

@@ -13,8 +13,6 @@ envoy_package()
 envoy_proto_library(
     name = "codec_impl_fuzz_proto",
     srcs = ["codec_impl_fuzz.proto"],
-    external_deps = ["well_known_protos"],
-    generate_python = 0,
     deps = ["//test/fuzz:common_proto"],
 )
 
@@ -23,7 +21,7 @@ envoy_cc_fuzz_test(
     srcs = ["codec_impl_fuzz_test.cc"],
     corpus = "codec_impl_corpus",
     deps = [
-        ":codec_impl_fuzz_proto",
+        ":codec_impl_fuzz_proto_cc",
         "//source/common/http:header_map_lib",
         "//source/common/http/http2:codec_lib",
         "//test/fuzz:utility_lib",

test/common/http/http2/codec_impl_corpus/100-continue

@@ -29,7 +29,7 @@ actions {
   stream_action {
     stream_id: 0
     response {
-      continue_100_headers {
+      continue_headers {
         headers {
           key: ":status"
           value: "100"

test/common/http/http2/codec_impl_corpus/example

@@ -221,7 +221,7 @@ actions {
   stream_action {
     stream_id: 3
     request {
-      reset: 0
+      reset_stream: 0
     }
   }
 }
@@ -252,7 +252,7 @@ actions {
   stream_action {
     stream_id: 4
     response {
-      reset: 0
+      reset_stream: 0
     }
   }
 }

test/common/http/http2/codec_impl_fuzz.proto

@@ -15,11 +15,11 @@ message NewStream {
 
 message DirectionalAction {
   oneof directional_action_selector {
-    test.fuzz.Headers continue_100_headers = 1;
+    test.fuzz.Headers continue_headers = 1;
     test.fuzz.Headers headers = 2;
     uint32 data = 3;
     test.fuzz.Headers trailers = 4;
-    uint32 reset = 5;
+    uint32 reset_stream = 5;
     bool read_disable = 6;
   }
   bool end_stream = 7;

test/common/http/http2/codec_impl_fuzz_test.cc

@@ -99,10 +99,9 @@ class Stream : public LinkedObject<Stream> {
                          const test::common::http::http2::DirectionalAction& directional_action) {
     const bool end_stream = directional_action.end_stream();
     switch (directional_action.directional_action_selector_case()) {
-    case test::common::http::http2::DirectionalAction::kContinue100Headers: {
+    case test::common::http::http2::DirectionalAction::kContinueHeaders: {
       if (state == StreamState::PendingHeaders) {
-        Http::TestHeaderMapImpl headers =
-            Fuzz::fromHeaders(directional_action.continue_100_headers());
+        Http::TestHeaderMapImpl headers = Fuzz::fromHeaders(directional_action.continue_headers());
         headers.setReferenceKey(Headers::get().Status, "100");
         encoder.encode100ContinueHeaders(headers);
       }
@@ -130,10 +129,10 @@ class Stream : public LinkedObject<Stream> {
       }
       break;
     }
-    case test::common::http::http2::DirectionalAction::kReset: {
+    case test::common::http::http2::DirectionalAction::kResetStream: {
       if (state != StreamState::Closed) {
         encoder.getStream().resetStream(
-            static_cast<Http::StreamResetReason>(directional_action.reset()));
+            static_cast<Http::StreamResetReason>(directional_action.reset_stream()));
         request_state_ = response_state_ = StreamState::Closed;
       }
       break;

test/common/ratelimit/ratelimit_impl_test.cc

@@ -47,6 +47,8 @@ class RateLimitGrpcClientTest : public testing::TestWithParam<bool> {
           Grpc::AsyncClientPtr{async_client_}, absl::optional<std::chrono::milliseconds>(),
           "envoy.service.ratelimit.v2.RateLimitService.ShouldRateLimit"));
     } else {
+      // Force link time dependency on deprecated message type.
+      pb::lyft::ratelimit::RateLimit _ignore;
       client_.reset(new GrpcClientImpl(Grpc::AsyncClientPtr{async_client_},
                                        absl::optional<std::chrono::milliseconds>(),
                                        "pb.lyft.ratelimit.RateLimitService.ShouldRateLimit"));