proto: unify envoy_proto_library/api_proto_library. by htuch · Pull Request #4233 · envoyproxy/envoy

htuch · 2018-08-22T19:47:22Z

In the latest iteration of
#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 htuch@google.com

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 <htuch@google.com>

lizan · 2018-08-22T21:30:13Z

bazel/envoy_build_system.bzl

@@ -1,4 +1,6 @@
 load("@com_google_protobuf//:protobuf.bzl", "cc_proto_library", "py_proto_library")
+load("@com_lyft_protoc_gen_validate//bazel:pgv_proto_library.bzl", "pgv_cc_proto_library")


Is this used?

No, I will fix. But, now you have volunteered yourself as reviewer :)

Signed-off-by: Harvey Tuch <htuch@google.com>

zuercher · 2018-08-22T22:41:11Z

Oof. That mac failure is real -- I think the protoc command line is too long for the /usr/bin/env that comes with OS X.

htuch · 2018-08-22T22:48:19Z

@zuercher I know. See my comment in #3786, I think some recent nice improvements made by @sesmith177 to PGV will avoid having duplicate -I in the protoc command line, and shrink this massively. But, we're blocked on some ongoing dependency and build engineering work.

dio · 2018-08-22T23:00:58Z

re: osx, I experienced this once: #3795

Signed-off-by: Harvey Tuch <htuch@google.com>

lizan · 2018-08-24T05:16:15Z

test/proto/BUILD

-        "http_api_protos",
-        "well_known_protos",
-    ],
+    external_cc_proto_deps = ["@googleapis//:api_httpbody_protos"],


If here is the only instance probably fine for now, but can we have this proto_deps/cc_proto_deps map somewhere in .bzl files and automatically inject them in envoy_proto_library?

lizan · 2018-08-24T05:18:33Z

bazel/envoy_build_system.bzl

-            visibility = ["//visibility:public"],
-        )
+def envoy_proto_library(name, **kwargs):
+    return api_proto_library(name, visibility = ["//visibility:public"], **kwargs)


I don't think you need visibility here, since envoy_package() already set default visibility to public.

just make this envoy_proto_library = api_proto_library?

That doesn't quite work, because api_proto_library overrides defaults itself; untangling that ball of string is best left as an exercise for another PR :)

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2018-08-24T19:09:48Z

@lizan tests finally pass 🏏

Signed-off-by: Harvey Tuch <htuch@google.com>

Signed-off-by: Jose Nino jnino@lyft.com Description: the legacy ratelimit message is selectively used, so it is not linked by default. Previous to #4233 we had forced linking. In #4233 linking was forced while running tests, but --understandably so -- not in production code. This means that when this path is executed on a compiled binary, we would get a segfault due to a failure to resolved the service_method_ here. Risk Level: medium, fixes a crash Testing: Fixed the concrete repro showing the invalid reference on gdb, turn into a valid reference.

Pulling the following changes from github.com/envoyproxy/envoy: f936fc6 ssl: serialize accesses to SSL socket factory contexts (envoyproxy#4345) e34dcd6 Fix crash in tcp_proxy (envoyproxy#4323) ae6a252 router: fix matching when all domains have wildcards (envoyproxy#4326) aa06142 test: Stop fake_upstream methods from accidentally succeeding (envoyproxy#4232) 5d73187 rbac: update the authenticated.user to a StringMatcher. (envoyproxy#4250) c6bfc7d time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers (envoyproxy#4257) 752483e Fixing the fix (envoyproxy#4333) 83487f6 tls: update BoringSSL to ab36a84b (3497). (envoyproxy#4338) 7bc210e test: fixing interactions between waitFor and ignore_spurious_events (envoyproxy#4309) 69474b3 admin: order stats in clusters json admin (envoyproxy#4306) 2d155f9 ppc64le build (envoyproxy#4183) 07efc6d fix static initialization fiasco problem (envoyproxy#4314) 0b7e3b5 test: Remove declared but undefined class methods (envoyproxy#4297) 1485a13 lua: make sure resetting dynamic metadata wrapper when request info is marked dead d243cd6 test: set to zero when start_time exceeds limit (envoyproxy#4328) 0a1e92a test: fix heap use-after-free in ~IntegrationTestServer. (envoyproxy#4319) cddc732 CONTRIBUTING: Document 'kick-ci' trick. (envoyproxy#4335) f13ef24 docs: remove reference to deprecated value field (envoyproxy#4322) e947a27 router: minor doc fixes in stream idle timeout (envoyproxy#4329) 0c2e998 tcp-proxy: fixing a TCP proxy bug where we attempted to readDisable a closed connection (envoyproxy#4296) 00ffe44 utility: fix strftime overflow handling. (envoyproxy#4321) af1183c Re-enable TcpProxySslIntegrationTest and make the tests pass again. (envoyproxy#4318) 3553461 fuzz: fix H2 codec fuzzer post envoyproxy#4262. (envoyproxy#4311) 42f6048 Proto string issue fix (envoyproxy#4320) 9c492a0 Support Envoy to fetch secrets using SDS service. (envoyproxy#4256) a857219 ratelimit: revert `revert rate limit failure mode config` and add tests (envoyproxy#4303) 1d34172 dns: fix exception unsafe behavior in c-ares callbacks. (envoyproxy#4307) 1212423 alts: add gRPC TSI socket (envoyproxy#4153) f0363ae fuzz: detect client-side resets in H2 codec fuzzer. (envoyproxy#4300) 01aa3f8 test: hopefully deflaking echo integration test (envoyproxy#4304) 1fc0f4b ratelimit: link legacy proto when message is being used (envoyproxy#4308) aa4481e fix rare List::remove(&target) segfault (envoyproxy#4244) 89e0f23 headers: fixing fast fail of size-validation (envoyproxy#4269) 97eba59 build: bump googletest version. (envoyproxy#4293) 0057e22 fuzz: avoid false positives in HCM fuzzer. (envoyproxy#4262) 9d094e5 Revert ac0bd74 (envoyproxy#4295) ddb28a4 Add validation context provider (envoyproxy#4264) 3b47cba added histogram latency information to Hystrix dashboard stream (envoyproxy#3986) cf87d50 docs: update SNI FAQ. (envoyproxy#4285) f952033 config: fix update empty stat for eds (envoyproxy#4276) 329e591 router: Add ability of custom headers to rely on per-request data (envoyproxy#4219) 68d20b4 thrift: refactor build files and imports (envoyproxy#4271) 5fa8192 access_log: log requested_server_name in tcp proxy (envoyproxy#4144) fa45bb4 fuzz: libc++ clocks don't like nanos. (envoyproxy#4282) 53f8944 stats: add symbol table for future stat name encoding (envoyproxy#3927) c987b42 test infra: Remove timeSource() from the ClusterManager api (envoyproxy#4247) cd171d9 websocket: tunneling websockets (and upgrades in general) over H2 (envoyproxy#4188) b9dc5d9 router: disallow :path/host rewriting in request_headers_to_add. (envoyproxy#4220) 0c91011 network: skip socket options and source address for UDS client connections (envoyproxy#4252) da1857d build: fixing a downstream compile error by noting explicit fallthrough (envoyproxy#4265) 9857cfe fuzz: cleanup per-test environment after each fuzz case. (envoyproxy#4253) 52beb06 test: Wrap proto string in std::string before comparison (envoyproxy#4238) f5e219e extensions/thrift_proxy: Add header matching to thrift router (envoyproxy#4239) c9ce5d2 fuzz: track read_disable_count bidirectionally in codec_impl_fuzz_test. (envoyproxy#4260) 35103b3 fuzz: use nanoseconds for SystemTime in RequestInfo. (envoyproxy#4255) ba6ba98 fuzz: make runtime root hermetic in server_fuzz_test. (envoyproxy#4258) b0a9014 time: Add 'format' test to ensure no one directly instantiates Prod*Time from source. (envoyproxy#4248) 8567460 access_log: support beginning of epoch in START_TIME. (envoyproxy#4254) 28d5f41 proto: unify envoy_proto_library/api_proto_library. (envoyproxy#4233) f7d3cb6 http: fix allocation bug introduced in envoyproxy#4211. (envoyproxy#4245) Fixes istio/istio#8310 (once pulled into istio/istio). Signed-off-by: Piotr Sikora <piotrsikora@google.com>

htuch mentioned this pull request Aug 22, 2018

router: disallow :path/host rewriting in request_headers_to_add. #4220

Merged

lizan reviewed Aug 22, 2018

View reviewed changes

htuch assigned lizan Aug 22, 2018

Remove unused line from dev.

3c48acf

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch mentioned this pull request Aug 22, 2018

build: bazel changes to build on Windows #3786

Closed

htuch added 2 commits August 24, 2018 00:04

Make api_httpbody_protos optional, should workaround OS X issue.

0d4384b

Signed-off-by: Harvey Tuch <htuch@google.com>

Merge remote-tracking branch 'upstream/master' into unify-envoy-proto

2b7be84

lizan reviewed Aug 24, 2018

View reviewed changes

htuch added 3 commits August 24, 2018 08:12

.bzl feedback.

5515ef1

Signed-off-by: Harvey Tuch <htuch@google.com>

Fix ratelimit test legacy proto deps.

f440098

Signed-off-by: Harvey Tuch <htuch@google.com>

Fix @envoy_api//... tests.

f848653

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch added 2 commits August 24, 2018 15:10

Merge remote-tracking branch 'upstream/master' into unify-envoy-proto

d921afe

Fix merge.

f2394a1

Signed-off-by: Harvey Tuch <htuch@google.com>

lizan approved these changes Aug 24, 2018

View reviewed changes

htuch merged commit 28d5f41 into envoyproxy:master Aug 24, 2018

htuch deleted the unify-envoy-proto branch August 24, 2018 20:38

junr03 mentioned this pull request Aug 30, 2018

ratelimit: link legacy proto when message is being used #4308

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proto: unify envoy_proto_library/api_proto_library.#4233

proto: unify envoy_proto_library/api_proto_library.#4233
htuch merged 9 commits intoenvoyproxy:masterfrom
htuch:unify-envoy-proto

htuch commented Aug 22, 2018

Uh oh!

lizan Aug 22, 2018

Uh oh!

htuch Aug 22, 2018

Uh oh!

zuercher commented Aug 22, 2018

Uh oh!

htuch commented Aug 22, 2018 •

edited by zuercher

Loading

Uh oh!

dio commented Aug 22, 2018 •

edited

Loading

Uh oh!

lizan Aug 24, 2018

Uh oh!

lizan Aug 24, 2018

Uh oh!

htuch Aug 24, 2018

Uh oh!

htuch commented Aug 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		@@ -1,4 +1,6 @@
		load("@com_google_protobuf//:protobuf.bzl", "cc_proto_library", "py_proto_library")
		load("@com_lyft_protoc_gen_validate//bazel:pgv_proto_library.bzl", "pgv_cc_proto_library")

Conversation

htuch commented Aug 22, 2018

Uh oh!

lizan Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

htuch Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

zuercher commented Aug 22, 2018

Uh oh!

htuch commented Aug 22, 2018 • edited by zuercher Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dio commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lizan Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

lizan Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

htuch Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

htuch commented Aug 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

htuch commented Aug 22, 2018 •

edited by zuercher

Loading

dio commented Aug 22, 2018 •

edited

Loading