Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

visibility issues after #12337 #12444

Closed
rgs1 opened this issue Aug 3, 2020 · 5 comments · Fixed by #12692
Closed

visibility issues after #12337 #12444

rgs1 opened this issue Aug 3, 2020 · 5 comments · Fixed by #12692

Comments

@rgs1
Copy link
Member

rgs1 commented Aug 3, 2020

After picking up #12337, I can't get our visibility overrides going.

For context, we don't have Envoy as a git submodule and instead it we fetch it via bazel as a tarball using http_archive.

Doing something like:

ADDITIONAL_VISIBILITY = [
  '@foo//pinterest:__subpackages__'
]

in our extensions_build_config.bzl fails with:

ERROR: /private/var/tmp/_bazel_rgs/6fea2029bf5452e6659bcb3f721f95cc/external/envoy/BUILD:18:14: invalid package name '@foo//pinterest:__subpackages__': must start with '//'

cc: @lizan @alyssawilk

@rgs1
Copy link
Member Author

rgs1 commented Aug 3, 2020

In case it wasn't clear from the initial description, I am trying to get our filters to see core extensions, e.g.:

/Users/rgs/code/envoy-pinterest/pinterest/filters/http/copy_header/BUILD:10:17: 
in cc_library rule //pinterest/filters/http/copy_header:copy_header_lib: Target
'//pinterest/filters/http/copy_header:copy_header_lib' violates visibility of target
'@envoy//source/extensions/filters/http/common:pass_through_filter_lib'. 

@lizan
Copy link
Member

lizan commented Aug 3, 2020

I'll look into this later today. With some workable solution in envoy-filter-example.

In general //source/extensions/filters/http/common:pass_through_filter_lib should be public though.

rgs1 pushed a commit to rgs1/envoy that referenced this issue Aug 3, 2020
Workaround for envoyproxy#12444, while we figure out a permanent solution.

Signed-off-by: Raul Gutierrez Segales <[email protected]>
@rgs1
Copy link
Member Author

rgs1 commented Aug 3, 2020

If we can't figure out a solution by EOD, we could merge #12451 to workaround this in the meanwhile.

lizan pushed a commit that referenced this issue Aug 4, 2020
Workaround for #12444, while we figure out a permanent solution.

Signed-off-by: Raul Gutierrez Segales <[email protected]>
@dio dio added the area/build label Aug 4, 2020
@rgs1
Copy link
Member Author

rgs1 commented Aug 4, 2020

We still need to make the pass through filter public visible, will send a PR shortly.

@rgs1
Copy link
Member Author

rgs1 commented Aug 4, 2020

Regardless of the fix here we definitely want the pass through filter to be public I'd think (see #12463).

chaoqin-li1123 pushed a commit to chaoqin-li1123/envoy that referenced this issue Aug 7, 2020
Workaround for envoyproxy#12444, while we figure out a permanent solution.

Signed-off-by: Raul Gutierrez Segales <[email protected]>
chaoqin-li1123 pushed a commit to chaoqin-li1123/envoy that referenced this issue Aug 7, 2020
Workaround for envoyproxy#12444, while we figure out a permanent solution.

Signed-off-by: Raul Gutierrez Segales <[email protected]>
Signed-off-by: chaoqinli <[email protected]>
lizan pushed a commit that referenced this issue Aug 11, 2020
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 <[email protected]>
alyssawilk added a commit that referenced this issue Aug 27, 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
Signed-off-by: Alyssa Wilk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants