Skip to content

build: cleaning up visibility rules #20592

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.

Sign up for GitHub

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

Merged
alyssawilk merged 4 commits into from
Mar 31, 2022
Merged

build: cleaning up visibility rules #20592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0cc728b
build: fixing visibility rules
alyssawilk Mar 30, 2022
5aed49a
contrib
alyssawilk Mar 30, 2022
41ea1ed
allowing contrib to see extensions
alyssawilk Mar 30, 2022
b20da26
fixing examples build :-(
alyssawilk Mar 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,10 @@ package_group(
"//contrib/...",
],
)

package_group(
name = "examples_library",
packages = [
"//examples/...",
],
)
6 changes: 2 additions & 4 deletions source/extensions/access_loggers/filters/cel/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@ envoy_cc_extension(
name = "cel_lib",
srcs = ["cel.cc"],
hdrs = ["cel.h"],
extra_visibility = [
"//test:__subpackages__",
],
deps = [
"//envoy/access_log:access_log_interface",
"//envoy/http:header_map_interface",
Expand All @@ -37,8 +34,9 @@ envoy_cc_extension(
"-DUSE_CEL_PARSER",
],
}),
# legacy test use
extra_visibility = [
"//test:__subpackages__",
"//test/common/access_log:__subpackages__",
],
deps = [
":cel_lib",
Expand Down
7 changes: 5 additions & 2 deletions source/extensions/common/wasm/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ envoy_cc_library(
"wasm.h",
"wasm_vm.h",
],
visibility = ["//visibility:public"],
deps = [
"//envoy/http:codes_interface",
"//envoy/http:filter_interface",
Expand Down Expand Up @@ -67,7 +66,11 @@ envoy_cc_library(
"-DWASM_USE_CEL_PARSER",
],
}),
visibility = ["//visibility:public"],

@kyessenov kyessenov Apr 6, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be reverted to public, since Wasm C++ extensions depend on it.

@alyssawilk alyssawilk Apr 6, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think this should be public, as we don't want Envoy core code depending on it. The visibility rules are to prevent dependency leakage upstream (see the associaed issue)

source/extensions/extensions_build_config.bzl allows you to define overrides for downstream visibility changes, so if we're having trouble with import we can override there. Alternately if we want a rule for WASM_VISIBILTY which in envoy defaults to EXTENSION_VISIBILITY but for istio/import can be more broad I'd be happy to add that.

@kyessenov kyessenov Apr 6, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a different situation than extensions. Null VM binaries are linked against the headers @proxy_wasm_cpp_host//:null_lib which are implemented by this library. So to run tests that are not associated with any extension, we need some runtime. Are you suggesting to create an extension for this target that can be exposed to unit tests in downstream repos?

@alyssawilk alyssawilk Apr 6, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Where are those binaries linked? I'm still not understanding why the default visibility rules cant' be changed for the use case which is encountering the problem.
cc @phlax @lizan @mattklein123

@kyessenov kyessenov Apr 6, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The intent here is to actually encourage using Wasm C++ ABI via @proxy_wasm_cpp_host, so hiding the implementation seems like going against it. If you insist, we can add a variable to control visibility, but it seems it's the easiest to just make Wasm library public.

@kyessenov kyessenov Apr 6, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sample binary https://github.com/istio/proxy/blob/master/extensions/common/BUILD#L95. It's a test for a library in a Wasm extension.

@mattklein123 mattklein123 Apr 6, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I will admit that I don't fully understand the moving pieces here, but are you saying that this is not really an extension but basically an export library that must be linked into null-VM extensions? How would one then link the null VM extension into Envoy itself? Via the normal mechanisms?

Is there some way we could make it easy to just expose this export library (and I would rename to wasm_export_lib or something to make it clear) to consuming extensions? I'm not sure what is possible with bazel.

@kyessenov kyessenov Apr 6, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Linking the Wasm extension using envoy_extension_cc_test into tests solves the issue. Sorry for the noise. I was confused myself since these tests don't really import anything Envoy except proxy-wasm headers, so it's a bit awkward but not a big deal.

visibility = [
"//source/extensions:__subpackages__",
"//test/extensions:__subpackages__",
"//test/test_common:__subpackages__",
],
deps = [
":wasm_hdr",
":wasm_runtime_factory_interface",
Expand Down
8 changes: 0 additions & 8 deletions source/extensions/common/wasm/ext/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ envoy_cc_library(
"envoy_null_vm_wasm_api.h",
"envoy_proxy_wasm_api.h",
],
visibility = ["//visibility:public"],
deps = [
"@proxy_wasm_cpp_sdk//:api_lib",
"@proxy_wasm_cpp_sdk//:common_lib",
Expand All @@ -29,7 +28,6 @@ envoy_cc_library(
"envoy_null_plugin.h",
"envoy_proxy_wasm_api.h",
],
visibility = ["//visibility:public"],
deps = [
":declare_property_cc_proto",
"//source/common/grpc:async_client_lib",
Expand All @@ -43,7 +41,6 @@ cc_library(
srcs = ["envoy_proxy_wasm_api.cc"],
hdrs = ["envoy_proxy_wasm_api.h"],
tags = ["manual"],
visibility = ["//visibility:public"],
deps = [
":declare_property_cc_proto",
":node_subset_cc_proto",
Expand All @@ -55,28 +52,24 @@ cc_library(
filegroup(
name = "envoy_proxy_wasm_api_js",
srcs = ["envoy_proxy_wasm_api.js"],
visibility = ["//visibility:public"],
)

# NB: this target is compiled both to native code and to Wasm. Hence the generic rule.
proto_library(
name = "declare_property_proto",
srcs = ["declare_property.proto"],
visibility = ["//visibility:public"],
)

# NB: this target is compiled both to native code and to Wasm. Hence the generic rule.
cc_proto_library(
name = "declare_property_cc_proto",
visibility = ["//visibility:public"],
deps = [":declare_property_proto"],
)

# NB: this target is compiled both to native code and to Wasm. Hence the generic rule.
proto_library(
name = "node_subset_proto",
srcs = ["node_subset.proto"],
visibility = ["//visibility:public"],
deps = [
"@com_google_protobuf//:struct_proto",
],
Expand All @@ -85,7 +78,6 @@ proto_library(
# NB: this target is compiled both to native code and to Wasm. Hence the generic rule.
cc_proto_library(
name = "node_subset_cc_proto",
visibility = ["//visibility:public"],
deps = [
":node_subset_proto",
# "//external:protobuf_clib",
Expand Down
4 changes: 2 additions & 2 deletions source/extensions/extensions_build_config.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,6 @@ EXTENSIONS = {

# These can be changed to ["//visibility:public"], for downstream builds which
# need to directly reference Envoy extensions.
EXTENSION_CONFIG_VISIBILITY = ["//:extension_config"]
EXTENSION_PACKAGE_VISIBILITY = ["//:extension_library"]
EXTENSION_CONFIG_VISIBILITY = ["//:extension_config", "//:contrib_library", "//:examples_library"]
EXTENSION_PACKAGE_VISIBILITY = ["//:extension_library", "//:contrib_library", "//:examples_library"]
CONTRIB_EXTENSION_PACKAGE_VISIBILITY = ["//:contrib_library"]
1 change: 0 additions & 1 deletion source/extensions/filters/common/expr/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ envoy_cc_library(
hdrs = [
"cel_state.h",
],
visibility = ["//visibility:public"],
deps = [
"//envoy/stream_info:filter_state_interface",
"//source/common/protobuf",
Expand Down
1 change: 0 additions & 1 deletion source/extensions/filters/common/mutation_rules/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ envoy_cc_library(
name = "mutation_rules_lib",
srcs = ["mutation_rules.cc"],
hdrs = ["mutation_rules.h"],
visibility = ["//visibility:public"],
deps = [
"//envoy/http:header_map_interface",
"//source/common/common:macros",
Expand Down
1 change: 0 additions & 1 deletion source/extensions/filters/http/admission_control/evaluators/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ envoy_cc_library(
"response_evaluator.h",
"success_criteria_evaluator.h",
],
visibility = ["//visibility:public"],
deps = [
"//envoy/grpc:status",
"//source/common/common:enum_to_int",
Expand Down
1 change: 0 additions & 1 deletion source/extensions/filters/http/wasm/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ envoy_cc_library(
name = "wasm_filter_lib",
srcs = ["wasm_filter.cc"],
hdrs = ["wasm_filter.h"],
visibility = ["//visibility:public"],
deps = [
"//envoy/http:codes_interface",
"//envoy/server:filter_config_interface",
Expand Down
1 change: 0 additions & 1 deletion source/extensions/filters/listener/proxy_protocol/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ envoy_cc_library(
"proxy_protocol.h",
"proxy_protocol_header.h",
],
visibility = ["//visibility:public"],
deps = [
"//envoy/event:dispatcher_interface",
"//envoy/network:filter_interface",
Expand Down
4 changes: 0 additions & 4 deletions source/extensions/http/stateful_session/cookie/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ envoy_cc_library(
name = "cookie_lib",
srcs = ["cookie.cc"],
hdrs = ["cookie.h"],
# This extension is core code.
visibility = ["//visibility:public"],
deps = [
"//envoy/http:stateful_session_interface",
"//source/common/common:base64_lib",
Expand All @@ -28,8 +26,6 @@ envoy_cc_extension(
name = "config",
srcs = ["config.cc"],
hdrs = ["config.h"],
# This extension is core code.
visibility = ["//visibility:public"],
deps = [
":cookie_lib",
"//envoy/http:stateful_session_interface",
Expand Down
1 change: 0 additions & 1 deletion source/extensions/transport_sockets/starttls/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ envoy_cc_extension(
name = "config",
srcs = ["config.cc"],
hdrs = ["config.h"],
visibility = ["//visibility:public"],
deps = [
":starttls_socket_lib",
"//envoy/network:transport_socket_interface",
Expand Down
1 change: 0 additions & 1 deletion source/extensions/transport_sockets/tls/cert_validator/spiffe/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ envoy_cc_extension(
"abseil_base",
"abseil_hash",
],
visibility = ["//visibility:public"],
deps = [
"//envoy/ssl:context_config_interface",
"//envoy/ssl:ssl_socket_extended_info_interface",
Expand Down
1 change: 0 additions & 1 deletion source/extensions/transport_sockets/tls/private_key/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ envoy_cc_library(
hdrs = [
"private_key_manager_impl.h",
],
visibility = ["//visibility:public"],
deps = [
"//envoy/event:dispatcher_interface",
"//envoy/registry",
Expand Down