Skip to content

build: take two on upstream visibility rules#12692

Merged
alyssawilk merged 6 commits intoenvoyproxy:masterfrom
alyssawilk:vis_fix
Aug 27, 2020
Merged

build: take two on upstream visibility rules#12692
alyssawilk merged 6 commits intoenvoyproxy:masterfrom
alyssawilk:vis_fix

Conversation

@alyssawilk
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk commented Aug 17, 2020

Setting things up so downstream folks can just override with visibility:public but upstream avoids core code depending on extensions.

Risk Level: medium (of breaking builds, instructions of how to unbreak included)
Testing: manually swapped to public, all works.
Docs Changes: included
Release Notes: overkill
Fixes #12444

@alyssawilk
build: take two on visibility rules
b87c6ad 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@lizan
Copy link
Copy Markdown
Member

lizan commented Aug 17, 2020

I just merged envoyproxy/envoy-filter-example#128, can we bump filter-example SHA in CI to see if it works?

@alyssawilk
Copy link
Copy Markdown
Contributor Author

alyssawilk commented Aug 17, 2020

pass-through filter is manually tagged public (not using the default rules) so I think it's orthogonal from this change.

More interesting would be if it were private, and envoy filter example had an override to explicitly depend on it, but I agree that one should be public (and possibly moved to core but organization tbd)

@lizan
Copy link
Copy Markdown
Member

lizan commented Aug 17, 2020

@alyssawilk it is manually tagged public after the previous take. There were a couple PRs.

@alyssawilk
Copy link
Copy Markdown
Contributor Author

alyssawilk commented Aug 17, 2020

Right, but this PR doesn't revert that being public, so I guess I'm not sure what bumping proves?
I can do it either way, I would just expect this change and that change to not interact.

@alyssawilk
bumping filter example
4e9ddbd 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
htuch
htuch reviewed Aug 18, 2020
View reviewed changes
source/extensions/extensions_build_config.bzl Outdated
# These can be changed to ["//visibility:public"], for downstream builds which
# need to directly reference Envoy extensions.
EXTENSION_CONFIG_VISIBILITY = ["//:extension_config"]
EXTENSION_EXTENSION_PACKAGE_VISIBILITY = ["//:extension_library"]
Copy link
Copy Markdown
Member

@htuch htuch Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable. I can imagine a finer grained system, but this is fine as a starting point to limit the visibility in Envoy.

lizan
lizan reviewed Aug 18, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks this looks good. just one nit

source/extensions/extensions_build_config.bzl Outdated
# These can be changed to ["//visibility:public"], for downstream builds which
# need to directly reference Envoy extensions.
EXTENSION_CONFIG_VISIBILITY = ["//:extension_config"]
EXTENSION_EXTENSION_PACKAGE_VISIBILITY = ["//:extension_library"]
Copy link
Copy Markdown
Member

@lizan lizan Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just name this EXTENSION_PACKAGE_VISIBILITY?

@alyssawilk
fixing_fixing
b3fa48d 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Merge branch 'master' into vis_fix
7954012 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Merge branch 'master' into vis_fix
a53cdec 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@lizan
Copy link
Copy Markdown
Member

lizan commented Aug 21, 2020

@rgs1 can you test this PR with your BUILD?

@lizan
Copy link
Copy Markdown
Member

lizan commented Aug 21, 2020

/wait-any

@rgs1
Copy link
Copy Markdown
Member

rgs1 commented Aug 24, 2020

@rgs1 can you test this PR with your BUILD?

Hey sorry for the late response I was OOO; yes I can give this a try. Mind merging master first tho?

@alyssawilk
Merge branch 'master' into vis_fix
092339c 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@rgs1
Copy link
Copy Markdown
Member

rgs1 commented Aug 25, 2020

So with:

diff --git a/bazel/build_config/extensions_build_config.bzl b/bazel/build_config/extensions_build_config.bzl
index ab61ec7..5e1f725 100644
--- a/bazel/build_config/extensions_build_config.bzl
+++ b/bazel/build_config/extensions_build_config.bzl
@@ -101,3 +101,6 @@ WINDOWS_EXTENSIONS = {
 }

 ADDITIONAL_VISIBILITY = []
+
+EXTENSION_CONFIG_VISIBILITY = ["//visibility:public"]
+EXTENSION_PACKAGE_VISIBILITY = ["//visibility:public"]

this works for us. Thanks!

[ we do reference a lot of Envoy extensions... the gzip stuff, the crypto stuff, etc. ]

rgs1
rgs1 approved these changes Aug 25, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@rgs1 rgs1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for us -- thanks!

@alyssawilk
Copy link
Copy Markdown
Contributor Author

alyssawilk commented Aug 25, 2020

@lizan any other thoughts?

@alyssawilk
Copy link
Copy Markdown
Contributor Author

alyssawilk commented Aug 27, 2020

@lizan ping?

@alyssawilk alyssawilk merged commit d245c02 into envoyproxy:master Aug 27, 2020
@alyssawilk
Copy link
Copy Markdown
Contributor Author

alyssawilk commented Aug 27, 2020

cc @yanavlasov @auni53

@alyssawilk alyssawilk deleted the vis_fix branch December 10, 2020 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lambdai lambdai Awaiting requested review from lambdai

@PiotrSikora PiotrSikora Awaiting requested review from PiotrSikora

4 more reviewers

@lizan lizan lizan left review comments

@htuch htuch htuch left review comments

@rgs1 rgs1 rgs1 approved these changes

@snowp snowp snowp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lizan lizan

@rgs1 rgs1

@htuch htuch

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

visibility issues after #12337

5 participants

@alyssawilk @lizan @rgs1 @snowp @htuch