build: marking extensions as extension-only visible by default #12337

alyssawilk · 2020-07-28T18:04:08Z

Risk Level: medium (of build breakage)
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Part of #9953

alyssawilk · 2020-07-28T18:04:50Z

cc @yanavlasov for import (where I think we can just define envoy_cc_extension_library to envoy_cc_library and ignore the dependency issue)

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2020-07-28T18:09:42Z

This only does half of it -
envoy_cc_extension needs it too

alyssawilk · 2020-07-28T18:56:02Z

Ok I take it back - I ended up defining extra_visibility so there will need to be some bzl tweaks.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2020-07-28T18:57:05Z

It feels like this PR basically consists of tagging every filter in the integration tests. Still, I think it'll help future proof, like we should have moved the redirect integration test into extensions when we started adding pluggable retry predicates. So everything is included everywhere but hopefully it won't be going forward?

lizan · 2020-07-29T10:15:54Z

This is likely breaking many downstream builds. We have bunch of extension point that defined in extensions, for example: envoy.thrift_proxy.protocols, envoy.thrift_proxy.protocols and so on. Limiting visibility of those extension point make downstream builds unable to define extension from there.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2020-07-29T12:24:25Z

Unfortunately, one can't do //visibility:public -//source:__subpackages

How about this: I'll split the rules into their own build file, and add instructions on how folks can override on their imports?

lizan · 2020-07-29T19:56:16Z

Yeah, wondering whether visibility can be selected? If so we can do that as part of compile time option.

lizan · 2020-07-29T19:57:24Z

source/extensions/access_loggers/BUILD

@@ -1,16 +1,18 @@
 load(
    "//bazel:envoy_build_system.bzl",
-    "envoy_cc_library",
+    "envoy_cc_extension_library",
    "envoy_package",
 )

 licenses(["notice"])  # Apache 2

 envoy_package()


We can define a envoy_extension_package with stricter default_visibility, use that instead of envoy_package in extensions. That limit visibility beyond cc_library.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

lizan

LGTM, one TODO to add.

lizan · 2020-07-30T20:58:03Z

BUILD

+# These two definitions exist to help reduce Envoy upstream core code depending on extensions.
+# To avoid visibility problems, one can replace the package lists below with "//visibility:public"
+#
+# TODO(#9953) //test/config_test:__pkg__ should probably be split up and removed.
+# TODO(#9953) the config fuzz tests should be moved somewhere local and //test/config_test and //test/server removed.
+package_group(
+    name = "extension_config",
+    packages = [
+        "//source/exe",
+        "//source/extensions/...",
+        "//test/config_test",
+        "//test/extensions/...",
+        "//test/server",
+        "//test/server/config_validation",
+    ],
+)
+
+package_group(
+    name = "extension_library",
+    packages = [
+        "//source/extensions/...",
+        "//test/extensions/...",
+    ],
+)


Can we add a TODO here to make this override more easier? I think potentially we can embed part of this in source/extensions/extensions_build_config.bzl (something like additional_visibility), that's where the overrides happen for downstream builds normally.

Gah, that's a fair point.
I hate to make folks overwrite the build file, then move their workarounds when I do the TODO. Let me see if I can get something working tonight.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

lizan · 2020-07-30T21:50:33Z

BUILD

@@ -1,3 +1,8 @@
+load(
+    "//source/extensions:extensions_build_config.bzl",


Suggested change

"//source/extensions:extensions_build_config.bzl",

"@envoy_build_config//:extensions_build_config.bzl",

Is the real overridden path :)

lizan · 2020-07-30T21:52:06Z

BUILD

@@ -6,7 +11,7 @@ exports_files([
 ])

 # These two definitions exist to help reduce Envoy upstream core code depending on extensions.
-# To avoid visibility problems, one can replace the package lists below with "//visibility:public"
+# To avoid visibility problems, one can extend ADDITIONAL_VISIBILITY in source/extensions/extensions_build_config.bzl


re: how to override is documented in https://github.com/envoyproxy/envoy/tree/master/bazel#disabling-extensions, can you update there too?

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

lizan · 2020-07-31T01:31:56Z

bazel/README.md

+By default, Envoy extensions are set up to only be visible to code within the
+[//source/extensions](../source/extensions/), or the Envoy server target. To adjust this,
+add any additional targets you need to `ADDITIONAL_VISIBILITY` in
+[extensions_build_config.bzl](../source/extensions/extensions_build_config.bzl).


To inject this file should be done like what https://github.com/envoyproxy/envoy/blob/master/bazel/README.md#disabling-extensions describes, add a link?

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

…proxy#12337) Risk Level: medium (of build breakage) Testing: n/a Docs Changes: n/a Release Notes: n/a Part of envoyproxy#9953 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

…proxy#12337) Risk Level: medium (of build breakage) Testing: n/a Docs Changes: n/a Release Notes: n/a Part of envoyproxy#9953 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: chaoqinli <chaoqinli@google.com>

Commit Message: Revert buffer filter visibility back to public Additional Description: After bringing in #12337, we are unable to build the router check tool as we build it with the buffer filter extension, which is no longer visible to the target we use. This change reverts the visibility change for the buffer filter back to public. Risk Level: Low Testing: N/A Docs Changes: N/A Release Notes: N/A Part of #12444 Signed-off-by: Lisa Lu <lisalu@lyft.com>

alyssawilk requested review from dio, htuch, jmarantz, junr03, lambdai, lizan, mattklein123, PiotrSikora, snowp and zuercher as code owners July 28, 2020 18:04

alyssawilk assigned lizan Jul 28, 2020

build: marking extensions as extension-only visible by default

d1ca7f2

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the visibility branch from 11a21ee to d1ca7f2 Compare July 28, 2020 18:08

and the rest

83d0054

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

improvements for downstream

0fd2556

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

lizan reviewed Jul 29, 2020

View reviewed changes

lizan added the waiting label Jul 29, 2020

alyssawilk added 2 commits July 30, 2020 14:31

Merge branch 'master' into visibility

cf2bcee

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

package

1b46b11

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Jul 30, 2020

alyssawilk added 2 commits July 30, 2020 16:52

fix build?

4cc486f

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'master' into visibility

c7929d9

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

lizan reviewed Jul 30, 2020

View reviewed changes

alyssawilk added 2 commits July 30, 2020 17:00

really build fix?

6ad8733

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

review comments

6a5fe07

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

lizan reviewed Jul 30, 2020

View reviewed changes

comments

8a374ac

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

lizan reviewed Jul 31, 2020

View reviewed changes

comments

ba8d270

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

lizan approved these changes Aug 1, 2020

View reviewed changes

lizan merged commit da403fa into envoyproxy:master Aug 1, 2020

rgs1 mentioned this pull request Aug 3, 2020

visibility issues after #12337 #12444

Closed

LisaLudique mentioned this pull request Aug 10, 2020

Revert buffer filter visibility back to public #12579

Merged

alyssawilk deleted the visibility branch December 10, 2020 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: marking extensions as extension-only visible by default #12337

build: marking extensions as extension-only visible by default #12337

alyssawilk commented Jul 28, 2020

alyssawilk commented Jul 28, 2020

alyssawilk commented Jul 28, 2020

alyssawilk commented Jul 28, 2020

alyssawilk commented Jul 28, 2020

lizan commented Jul 29, 2020

alyssawilk commented Jul 29, 2020

lizan commented Jul 29, 2020

lizan Jul 29, 2020

lizan left a comment

lizan Jul 30, 2020

alyssawilk Jul 30, 2020

lizan Jul 30, 2020

lizan Jul 30, 2020

lizan Jul 31, 2020

		@@ -1,3 +1,8 @@
		load(
		"//source/extensions:extensions_build_config.bzl",

	"//source/extensions:extensions_build_config.bzl",
	"@envoy_build_config//:extensions_build_config.bzl",

build: marking extensions as extension-only visible by default #12337

build: marking extensions as extension-only visible by default #12337

Conversation

alyssawilk commented Jul 28, 2020

alyssawilk commented Jul 28, 2020

alyssawilk commented Jul 28, 2020

alyssawilk commented Jul 28, 2020

alyssawilk commented Jul 28, 2020

lizan commented Jul 29, 2020

alyssawilk commented Jul 29, 2020

lizan commented Jul 29, 2020

lizan Jul 29, 2020

Choose a reason for hiding this comment

lizan left a comment

Choose a reason for hiding this comment

lizan Jul 30, 2020

Choose a reason for hiding this comment

alyssawilk Jul 30, 2020

Choose a reason for hiding this comment

lizan Jul 30, 2020

Choose a reason for hiding this comment

lizan Jul 30, 2020

Choose a reason for hiding this comment

lizan Jul 31, 2020

Choose a reason for hiding this comment