api: shadowing for API protos. #9429

htuch · 2019-12-20T04:28:35Z

This is intended to simplify the internal handling of deprecations during API boosting.

See https://docs.google.com/document/d/1mGO9LtVo7t4Lph7WlmyGCxXye3h6j29z3JZvIBbs_D0/edit

Ultimately the plan is to hide this all as a build artifact in Bazel cache, but due to the technical complexity of the pure Bazel solution (involving changes spanning PGV, protoxform, API build rules), we will use checked-in artifacts for 1.13.0.

Risk level: Low
Testing: Additional API test and protoxform golden test.

Part of #8082

This should be unwound in the future with #9479

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

htuch · 2019-12-20T04:31:36Z

This is a sketch for what the shadow protos would look like. I think I can get them working end-to-end with only a small amount of additional work on import paths and BUILD files. At this point, given that it sort-of-works, I'm interested in hearing from @lizan how hard it might be to turn this into some sort of aspect rule based approach that hides the shadows in the Bazel cache.

This is intended to simplify the internal handling of deprecations during API boosting. See https://docs.google.com/document/d/1mGO9LtVo7t4Lph7WlmyGCxXye3h6j29z3JZvIBbs_D0/edit Ultimately the plan is to hide this all as a build artifact in Bazel cache, but due to the technical complexity of the pure Bazel solution (involving changes spanning PGV, protoxform, API build rules), we will use checked-in artifacts for 1.13.0. Risk level: Low Testing: Additional API test and protoxform golden test. Part of envoyproxy#8082 Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2019-12-25T19:59:47Z

@lizan @mattklein123 this is now ready for review and is the plan-of-record for 1.13.0 + v3.

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

api/envoy/admin/v3alpha/BUILD

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

mattklein123

Thanks this makes sense to me. I have 2 small comments and also one general question: I'm a little fearful that this is going to cause a bunch of pain for consuming projects that consume Envoy as a submodule. Do you have any intuition of what this will look like? Do you think we should run the tooling in the envoy-filter-example repo to expose issues like this? This should probably done as a P1 ASAP thing? WDYT?

mattklein123 · 2019-12-26T21:21:13Z

api_shadow/BUILD

@@ -0,0 +1,157 @@
+# DO NOT EDIT. This file is generated by tools/proto_sync.py.


Should we maybe put this somewhere else that is not top level so that people don't see it, ignore the README, and get pretty confused? My first thought was a top level generated/ directory, but I think we have scripts that put stuff in a directory by that name so that might get confusing. Also, it's still top level. Maybe somewhere under bazel/ as it is a baked in part of our build that should eventually just go away? WDYT?

How about prebuillt/api_shadow, internal/api_shadow, vendored/api_shadow? Prefer not under bazel/, as this doesn't technically relate to Bazel infra (but it is build related).

Also, maybe generated_api?

I kind of like prebuilt/api_shadow but I don't feel that strongly about it. (2nd place would be vendored/)

I put it in generated_api_shadow; it's only 3 lines to change it to whatever folks want, so if @lizan has any strong preferences, we can do that.

api_shadow/envoy/api/v3alpha/cds.proto

htuch · 2019-12-26T22:08:57Z

@mattklein123 re: submodules, I think this should be largely transparent. As long as folks are using api_binding.bzl in their WORKSPACE, the use of shadowing should not matter. What will be really confusing and break-the-world is the move to v3, but that affects everyone, not just consuming projects (e.g. filter developers).

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

lizan · 2019-12-27T06:33:16Z

.gitattributes

@@ -1,6 +1,8 @@
 /docs/root/intro/version_history.rst merge=union
 *.generated.pb_text linguist-generated=true
 *.generated.pb_text -diff -merge
+/api_shadow/envoy/** linguist-generated=true


update this?

lizan · 2019-12-27T06:36:03Z

include/envoy/config/BUILD

@@ -44,7 +44,7 @@ envoy_cc_library(
    hdrs = ["subscription_factory.h"],
    deps = [
        ":subscription_interface",
-        "@envoy_api//envoy/api/v2/core:pkg_cc_proto",
+        "@envoy_api_shadow//envoy/api/v2/core:pkg_cc_proto",


can we switch the bind, (i.e. use @envoy_api for shadow)? I think that makes less breaking for BUILD files under include/ and source/, and less breaking for consuming project, as @mattklein123 pointed out.

Otherwise this will easily cause ODR violation when some consuming project trying to link @envoy_api//envoy/api/v2/core:pkg_cc_proto.

I have done this, but it comes with tradeoffs. While most of the source now deals with @envoy_api, we have to switch all developer tools to an alias for the real artifacts, @envoy_api_canonical (lest we have some weird dependence of generating tools on generated artifacts). Then we need to symlink across Bazel rules. It works, PTAL.

Yeah it comes with tradeoffs but I prefer this way to be less breaking for consuming projects, as well as less breaking when we merge envoy_api_canonical with envoy_api.

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

htuch · 2019-12-27T15:37:10Z

@lizan Windows breaks due to the symlink, do you know if we can enable "developer mode" on CI to solve this? https://www.quora.com/How-does-Git-handle-symbolic-links-on-Windows

lizan · 2019-12-28T03:54:23Z

@lizan Windows breaks due to the symlink, do you know if we can enable "developer mode" on CI to solve this? https://www.quora.com/How-does-Git-handle-symbolic-links-on-Windows

Hmm, not sure how to do this, were we previously not using symlink?

htuch · 2019-12-29T04:06:48Z

OK, will merge for now and we can sort out Windows next week. Thanks.

api: shadowing for API protos. (envoyproxy#9429)

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

This is intended to simplify the internal handling of deprecations during API boosting. See https://docs.google.com/document/d/1mGO9LtVo7t4Lph7WlmyGCxXye3h6j29z3JZvIBbs_D0/edit Ultimately the plan is to hide this all as a build artifact in Bazel cache, but due to the technical complexity of the pure Bazel solution (involving changes spanning PGV, protoxform, API build rules), we will use checked-in artifacts for 1.13.0. Risk level: Low Testing: Additional API test and protoxform golden test. Part of envoyproxy#8082 This should be unwound in the future with envoyproxy#9479 Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: Prakhar <prakhar_au@yahoo.com>

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

htuch requested review from mattklein123 and lizan December 20, 2019 04:28

htuch assigned lizan and mattklein123 Dec 20, 2019

htuch force-pushed the upgrade-shadow branch from cc84c19 to 4ff07db Compare December 25, 2019 19:57

htuch changed the title ~~RFC: Envoy internal shadow for API protos.~~ api: shadowing for API protos. Dec 25, 2019

htuch marked this pull request as ready for review December 25, 2019 19:59

htuch added the api/v3 Major version release @ end of Q3 2019 label Dec 26, 2019

htuch added 2 commits December 26, 2019 08:47

Merge remote-tracking branch 'upstream/master' into upgrade-shadow

69cc2ba

fix format

f63c079

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

lizan reviewed Dec 26, 2019

View reviewed changes

api/envoy/admin/v3alpha/BUILD Outdated Show resolved Hide resolved

Parallel @envoy_api_shadow tree.

41edac4

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

htuch requested review from alyssawilk, dio, jmarantz, snowp and zuercher as code owners December 26, 2019 16:14

htuch added 5 commits December 26, 2019 11:16

wip

27725e8

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

Fix .gitattributes.

7289d91

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

check_format test

72ce62b

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

Merge remote-tracking branch 'upstream/master' into upgrade-shadow

be0fb04

Misc. BUILD fixes.

3656ace

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

mattklein123 reviewed Dec 26, 2019

View reviewed changes

mattklein123 added the waiting label Dec 26, 2019

Switch to generated_api_shadow.

486a6fe

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

repokitteh-read-only bot removed the waiting label Dec 27, 2019

lizan reviewed Dec 27, 2019

View reviewed changes

htuch added 7 commits December 27, 2019 08:43

Fix .gitattributes.

02e0ea3

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

Swizel @envoy_api to the shadow version. Need to symlink bazel/.

391c43d

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

Merge remote-tracking branch 'upstream/master' into upgrade-shadow

b09685c

Bump to master.

d24a0be

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

Swtich to @envoy_api_canonical.

2281f5a

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

Fix bazel.api.

552ea49

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

Fix docs.

6bdb90a

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

lizan approved these changes Dec 28, 2019

View reviewed changes

htuch merged commit da5fb5c into envoyproxy:master Dec 29, 2019

htuch deleted the upgrade-shadow branch December 29, 2019 04:07

sthagen added a commit to sthagen/envoyproxy-envoy that referenced this pull request Dec 29, 2019

Merge pull request #127 from envoyproxy/master

a678c11

api: shadowing for API protos. (envoyproxy#9429)

htuch added a commit to htuch/envoy that referenced this pull request Dec 30, 2019

ci: fix Windows target broken by envoyproxy#9429.

39c67b3

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

htuch added a commit that referenced this pull request Dec 30, 2019

ci: fix Windows target broken by #9429. (#9519)

11934bd

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

junr03 mentioned this pull request Jan 2, 2020

fmt: fix master #9538

Merged

prakhag1 pushed a commit to prakhag1/envoy that referenced this pull request Jan 3, 2020

ci: fix Windows target broken by envoyproxy#9429. (envoyproxy#9519)

1472997

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: shadowing for API protos. #9429

api: shadowing for API protos. #9429

htuch commented Dec 20, 2019 •

edited

Loading

htuch commented Dec 20, 2019

htuch commented Dec 25, 2019

mattklein123 left a comment

mattklein123 Dec 26, 2019

htuch Dec 26, 2019

htuch Dec 26, 2019

mattklein123 Dec 26, 2019

htuch Dec 27, 2019

htuch commented Dec 26, 2019

lizan Dec 27, 2019

lizan Dec 27, 2019

htuch Dec 27, 2019 •

edited

Loading

lizan Dec 28, 2019 •

edited

Loading

htuch commented Dec 27, 2019

lizan commented Dec 28, 2019

htuch commented Dec 29, 2019

		@@ -0,0 +1,157 @@
		# DO NOT EDIT. This file is generated by tools/proto_sync.py.

api: shadowing for API protos. #9429

api: shadowing for API protos. #9429

Conversation

htuch commented Dec 20, 2019 • edited Loading

htuch commented Dec 20, 2019

htuch commented Dec 25, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Dec 26, 2019

Choose a reason for hiding this comment

htuch Dec 26, 2019

Choose a reason for hiding this comment

htuch Dec 26, 2019

Choose a reason for hiding this comment

mattklein123 Dec 26, 2019

Choose a reason for hiding this comment

htuch Dec 27, 2019

Choose a reason for hiding this comment

htuch commented Dec 26, 2019

lizan Dec 27, 2019

Choose a reason for hiding this comment

lizan Dec 27, 2019

Choose a reason for hiding this comment

htuch Dec 27, 2019 • edited Loading

Choose a reason for hiding this comment

lizan Dec 28, 2019 • edited Loading

Choose a reason for hiding this comment

htuch commented Dec 27, 2019

lizan commented Dec 28, 2019

htuch commented Dec 29, 2019

htuch commented Dec 20, 2019 •

edited

Loading

htuch Dec 27, 2019 •

edited

Loading

lizan Dec 28, 2019 •

edited

Loading