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.

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

Jump to bottom

deps: update protobuf to 3.14 #14253

Merged
merged 20 commits into from Dec 16, 2020
Merged

deps: update protobuf to 3.14 #14253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
5313dc7
update protobuf
kyessenov Dec 3, 2020
919e1cd
revert googleapis change; it is also breaking :(
kyessenov Dec 3, 2020
18691fb
typo
kyessenov Dec 3, 2020
6e8acda
code review
kyessenov Dec 3, 2020
5a51bdc
fix tests
kyessenov Dec 4, 2020
2fe0b36
testing
kyessenov Dec 4, 2020
c113ef7
testing
kyessenov Dec 4, 2020
494314b
fix deps
kyessenov Dec 4, 2020
b34523b
patch protobuf rule
kyessenov Dec 4, 2020
0a1acbe
Merge remote-tracking branch 'upstream/master' into protobuf_update
kyessenov Dec 7, 2020
233b6d9
relax tests
kyessenov Dec 7, 2020
de32d28
try windows debug
kyessenov Dec 8, 2020
7e73763
Revert "try windows debug"
kyessenov Dec 8, 2020
1b37116
missing include
kyessenov Dec 9, 2020
8eab565
missing include
kyessenov Dec 9, 2020
722db6d
missing include
kyessenov Dec 9, 2020
dc33299
hack to get rid of bad files
kyessenov Dec 9, 2020
d961c83
debug CI
lizan Dec 10, 2020
f01d9f1
update go build
kyessenov Dec 11, 2020
c8ef105
update udpa
kyessenov Dec 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions .azure-pipelines/bazel.yml
View file
Open in desktop
Expand Up @@ -66,6 +66,8 @@ steps:
- bash: |
echo "disk space at end of build:"
df -h
# Cleanup offending files with unicode names
rm -rf $(Build.StagingDirectory)/tmp/*/*/external/go_sdk/test/fixedbugs
lizan marked this conversation as resolved.
Show resolved Hide resolved
displayName: "Check disk space at end"
condition: always()

Expand Down
16 changes: 8 additions & 8 deletions api/bazel/api_build_system.bzl
View file
Open in desktop
Expand Up @@ -186,14 +186,14 @@ def api_proto_package(
proto = name,
visibility = ["//visibility:public"],
deps = depset([_go_proto_mapping(dep) for dep in deps] + [
"@com_github_golang_protobuf//ptypes:go_default_library",
"@com_github_golang_protobuf//ptypes/any:go_default_library",
"@com_github_golang_protobuf//ptypes/duration:go_default_library",
"@com_github_golang_protobuf//ptypes/struct:go_default_library",
"@com_github_golang_protobuf//ptypes/timestamp:go_default_library",
"@com_github_golang_protobuf//ptypes/wrappers:go_default_library",
"@com_envoyproxy_protoc_gen_validate//validate:go_default_library",
"@com_google_googleapis//google/api:annotations_go_proto",
"@com_google_googleapis//google/rpc:status_go_proto",
"@com_github_golang_protobuf//ptypes:go_default_library_gen",
"@go_googleapis//google/api:annotations_go_proto",
"@go_googleapis//google/rpc:status_go_proto",
"@io_bazel_rules_go//proto/wkt:any_go_proto",
"@io_bazel_rules_go//proto/wkt:duration_go_proto",
"@io_bazel_rules_go//proto/wkt:struct_go_proto",
"@io_bazel_rules_go//proto/wkt:timestamp_go_proto",
"@io_bazel_rules_go//proto/wkt:wrappers_go_proto",
]).to_list(),
)
19 changes: 13 additions & 6 deletions api/bazel/repository_locations.bzl
View file
Open in desktop
Expand Up @@ -14,23 +14,30 @@ REPOSITORY_LOCATIONS_SPEC = dict(
project_name = "protoc-gen-validate (PGV)",
project_desc = "protoc plugin to generate polyglot message validators",
project_url = "https://github.com/envoyproxy/protoc-gen-validate",
version = "278964a8052f96a2f514add0298098f63fb7f47f",
sha256 = "e368733c9fb7f8489591ffaf269170d7658cc0cd1ee322b601512b769446d3c8",
version = "1bcea29601b5624234a19b3d7f0ebd9e9984f583",
sha256 = "2062bbe50eddf3c98490339721fb02b5b5cd78f610f163b98bbf95ba7105553f",
strip_prefix = "protoc-gen-validate-{version}",
urls = ["https://github.com/envoyproxy/protoc-gen-validate/archive/{version}.tar.gz"],
release_date = "2020-06-08",
release_date = "2020-11-30",
use_category = ["api"],
implied_untracked_deps = [
"com_github_iancoleman_strcase",
"com_github_lyft_protoc_gen_star",
"com_github_spf13_afero",
"org_golang_google_genproto",
"org_golang_x_text",
],
),
com_github_cncf_udpa = dict(
project_name = "xDS API",
project_desc = "xDS API Working Group (xDS-WG)",
project_url = "https://github.com/cncf/udpa",
# During the UDPA -> xDS migration, we aren't working with releases.
version = "5459f2c994033b0afed7e4a70ac7e90c90c1ffee",
sha256 = "c1f5c2438cf725b5f66aa4210dbc4bb691020c5ed4f64d2bc6638b06a11482f1",
version = "cc1b757b3eddccaaaf0743cbb107742bb7e3ee4f",
sha256 = "822a007cf155855d0c08a2e753a39e222e5816b904436196244066a818a8a230",
strip_prefix = "udpa-{version}",
urls = ["https://github.com/cncf/udpa/archive/{version}.tar.gz"],
release_date = "2020-11-20",
release_date = "2020-12-11",
use_category = ["api"],
),
com_github_openzipkin_zipkinapi = dict(
Expand Down
2 changes: 0 additions & 2 deletions api/test/build/BUILD
View file
Open in desktop
Expand Up @@ -23,11 +23,9 @@ api_go_test(
deps = [
"//envoy/api/v2:pkg_go_proto",
"//envoy/api/v2/auth:pkg_go_proto",
"//envoy/config/bootstrap/v2:pkg_go_proto",
"//envoy/service/accesslog/v2:pkg_go_proto",
"//envoy/service/discovery/v2:pkg_go_proto",
"//envoy/service/metrics/v2:pkg_go_proto",
"//envoy/service/ratelimit/v2:pkg_go_proto",
"//envoy/service/trace/v2:pkg_go_proto",
],
)
2 changes: 0 additions & 2 deletions api/test/build/go_build_test.go
View file
Open in desktop
Expand Up @@ -5,12 +5,10 @@ import (

_ "github.com/envoyproxy/go-control-plane/envoy/api/v2"
_ "github.com/envoyproxy/go-control-plane/envoy/api/v2/auth"
_ "github.com/envoyproxy/go-control-plane/envoy/config/bootstrap/v2"
_ "github.com/envoyproxy/go-control-plane/envoy/service/accesslog/v2"
_ "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v2"
_ "github.com/envoyproxy/go-control-plane/envoy/service/metrics/v2"
_ "github.com/envoyproxy/go-control-plane/envoy/service/ratelimit/v2"
_ "github.com/envoyproxy/go-control-plane/envoy/service/trace/v2"
)

func TestNoop(t *testing.T) {
Expand Down
20 changes: 19 additions & 1 deletion bazel/dependency_imports.bzl
View file
Open in desktop
Expand Up @@ -16,7 +16,7 @@ load("@rules_antlr//antlr:deps.bzl", "antlr_dependencies")
load("@proxy_wasm_rust_sdk//bazel:dependencies.bzl", "proxy_wasm_rust_sdk_dependencies")

# go version for rules_go
GO_VERSION = "1.14.7"
GO_VERSION = "1.15.5"
Copy link
Contributor

Choose a reason for hiding this comment

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

1.15.6 is out but I can bump this in a follow up


def envoy_dependency_imports(go_version = GO_VERSION):
rules_foreign_cc_dependencies()
Expand Down Expand Up @@ -57,6 +57,24 @@ def envoy_dependency_imports(go_version = GO_VERSION):
sum = "h1:g61tztE5qeGQ89tm6NTjjM9VPIm088od1l6aSorWRWg=",
version = "v0.3.0",
)
go_repository(
name = "com_github_spf13_afero",
importpath = "github.com/spf13/afero",
sum = "h1:8q6vk3hthlpb2SouZcnBVKboxWQWMDNF38bwholZrJc=",
version = "v1.3.4",
)
go_repository(
name = "com_github_lyft_protoc_gen_star",
importpath = "github.com/lyft/protoc-gen-star",
sum = "h1:sImehRT+p7lW9n6R7MQc5hVgzWGEkDVZU4AsBQ4Isu8=",
version = "v0.5.1",
)
go_repository(
name = "com_github_iancoleman_strcase",
importpath = "github.com/iancoleman/strcase",
sum = "h1:ux/56T2xqZO/3cP1I2F86qpeoYPCOzk+KF/UH/Ar+lk=",
version = "v0.0.0-20180726023541-3605ed457bf7",
)

config_validation_pip_install()
configs_pip_install()
Expand Down
14 changes: 14 additions & 0 deletions bazel/protobuf.patch
View file
Open in desktop
Expand Up @@ -21,3 +21,17 @@ index efc3d8e7f..746ad4851 100644

################################################################################
# Protobuf Runtime Library
diff --git a/python/google/protobuf/__init__.py b/python/google/protobuf/__init__.py
index 97ac28028..8b7585d9d 100644
--- a/python/google/protobuf/__init__.py
+++ b/python/google/protobuf/__init__.py
@@ -31,3 +31,9 @@
# Copyright 2007 Google Inc. All Rights Reserved.

__version__ = '3.14.0'
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to maintain a patch or can we get a change landed upstream to avoid this? CC @envoyproxy/dependency-shepherds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I don't quite understand what's the deal with google dependencies, python, and bazel. If someone who understands it better could pick it up, I'd appreciate it. I only see it at the level that this was there before, and it's gone, and it works with it.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is from protocolbuffers/protobuf@66e3562. @dlj-NaN do you know why that PR would have caused google.api imports to start breaking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing to keep in mind is that I can't update googleapis because some python rules are being removed (I need to find out why). So there's that.

Copy link
Member

Choose a reason for hiding this comment

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

OK. I don't think any of our build experts has a much better grasp of the Python/Bazel/GoogleAPIs internals, I'd probably start from first principles debugging :(

Copy link

Choose a reason for hiding this comment

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

protocolbuffers/protobuf@66e3562 gets pretty deep into the semantics of Python imports and different packaging. The interaction addressed by that commit is very tricky, and may be hidden by varying the order of imports.

(I'm guessing the reason this appeared with a protobuf commit was exactly due to order of imports... google.auth comes after google.api alphabetically, but due to transitive imports, google.protobuf is effectively a very early import in many scenarios.)

The upshot is that there shouldn't be any files named google/__init__.py in other installed packages. Just as a first guess, this might be caused by installing googleapis/google-auth-library-python as an egg (maybe via setup.py install?), but other google packages using native namespaces. (This also gets into what Bazel does in its pip rules....)

Anyhow, some fixes might be:

  1. Remove google/__init__.py from googleapis/google-auth-library-python. (@busunkim96 to comment on that.)
  2. Pre-install the package with an up-to-date version of pip, which should install using the native namespace.
  3. Update Bazel's pip rules to make sure it does (2).

Copy link

Choose a reason for hiding this comment

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

Remove google/init.py from googleapis/google-auth-library-python.

Removing google/__init__.py from the google-auth will probably be done next quarter as we officially drop 2.7. google-auth is last on the list since it is transitively required by nearly all the cloud client libraries.

Over the past year we've gradually moved google-cloud-API packages to using native namespace packages (example: google-cloud-texttospeech) while other libraries like google-api-core, google-cloud-core, and google-auth have kept google/__init__.py with pkg_resources style namespace packages. I see now that setuptools does not recommend this, but we have not seen customers open issues about it. Our docs always point folks pip install packages.

One thing to keep in mind is that I can't update googleapis because some python rules are being removed (I need to find out why). So there's that.

Are you referring to Bazel rules in https://github.com/googleapis/googleapis or something else?

I don't have much context here so I'm not sure it matters, but the google-cloud-* libraries get their copy of many commonly used protos (google.api, google.type, ...) from https://github.com/googleapis/python-api-common-protos, published on PyPI as google-apis-common-protos

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is related but I'm trying to land an update to how we process pip dependencies using the latest rules_python at #14329

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened an issue for further discussion #14412.
We're using googleapis/googleapis here because some protos are missing from https://github.com/googleapis/python-api-common-protos (e.g. google.api.expr for CEL). Does this mean we can't use PIP approach since there's no pre-generated package I am aware for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

pip handling changes with the latest rules_python - https://github.com/bazelbuild/rules_python/releases/tag/0.1.0. PR #14329 that moves us to that handling is blocked due to Windows (/cc @envoyproxy/windows-dev). In theory pip supports installing from Git - https://pip.pypa.io/en/stable/reference/pip_install/#git but no idea if this works for the current deprecated pip method or the new method we are trying to get to.

+
+if __name__ != '__main__':
+ try:
+ __import__('pkg_resources').declare_namespace(__name__)
+ except ImportError:
+ __path__ = __import__('pkgutil').extend_path(__path__, __name__)
18 changes: 9 additions & 9 deletions bazel/repository_locations.bzl
View file
Open in desktop
Expand Up @@ -15,10 +15,10 @@ REPOSITORY_LOCATIONS_SPEC = dict(
project_name = "Gazelle",
project_desc = "Bazel BUILD file generator for Go projects",
project_url = "https://github.com/bazelbuild/bazel-gazelle",
version = "0.21.1",
sha256 = "cdb02a887a7187ea4d5a27452311a75ed8637379a1287d8eeb952138ea485f7d",
version = "0.22.2",
sha256 = "b85f48fa105c4403326e9525ad2b2cc437babaa6e15a3fc0b1dbab0ab064bc7c",
urls = ["https://github.com/bazelbuild/bazel-gazelle/releases/download/v{version}/bazel-gazelle-v{version}.tar.gz"],
release_date = "2020-05-28",
release_date = "2020-10-02",
use_category = ["build"],
),
bazel_toolchains = dict(
Expand Down Expand Up @@ -495,12 +495,12 @@ REPOSITORY_LOCATIONS_SPEC = dict(
project_name = "Protocol Buffers",
project_desc = "Language-neutral, platform-neutral extensible mechanism for serializing structured data",
project_url = "https://developers.google.com/protocol-buffers",
version = "3.13.0",
sha256 = "465fd9367992a9b9c4fba34a549773735da200903678b81b25f367982e8df376",
version = "3.14.0",
sha256 = "6dd0f6b20094910fbb7f1f7908688df01af2d4f6c5c21331b9f636048674aebf",
strip_prefix = "protobuf-{version}",
urls = ["https://github.com/protocolbuffers/protobuf/releases/download/v{version}/protobuf-all-{version}.tar.gz"],
use_category = ["dataplane_core", "controlplane"],
release_date = "2020-08-14",
release_date = "2020-11-13",
cpe = "cpe:2.3:a:google:protobuf:*",
),
grpc_httpjson_transcoding = dict(
Expand All @@ -520,11 +520,11 @@ REPOSITORY_LOCATIONS_SPEC = dict(
project_name = "Go rules for Bazel",
project_desc = "Bazel rules for the Go language",
project_url = "https://github.com/bazelbuild/rules_go",
version = "0.23.7",
sha256 = "0310e837aed522875791750de44408ec91046c630374990edd51827cb169f616",
version = "0.25.0",
sha256 = "6f111c57fd50baf5b8ee9d63024874dd2a014b069426156c55adbf6d3d22cb7b",
urls = ["https://github.com/bazelbuild/rules_go/releases/download/v{version}/rules_go-v{version}.tar.gz"],
use_category = ["build", "api"],
release_date = "2020-08-06",
release_date = "2020-12-02",
implied_untracked_deps = [
"com_github_golang_protobuf",
"io_bazel_rules_nogo",
Expand Down
16 changes: 8 additions & 8 deletions generated_api_shadow/bazel/api_build_system.bzl
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 13 additions & 6 deletions generated_api_shadow/bazel/repository_locations.bzl
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions source/common/config/protobuf_link_hacks.h
View file
Open in desktop
Expand Up @@ -47,6 +47,7 @@ const envoy::service::route::v3::RdsDummy _rds_dummy_v3;
const envoy::service::cluster::v3::CdsDummy _cds_dummy_v3;
const envoy::service::endpoint::v3::EdsDummy _eds_dummy_v3;
const envoy::service::route::v3::SrdsDummy _srds_dummy_v3;
const envoy::service::extension::v3::EcdsDummy _ecds_dummy_v3;

// With the v2 -> v3 migration there is another, related linking issue.
// Symbols for v2 protos which headers are not included in any file in the codebase are being
Expand Down
2 changes: 1 addition & 1 deletion test/common/access_log/access_log_impl_test.cc
View file
Open in desktop
Expand Up @@ -1135,7 +1135,7 @@ name: accesslog
)EOF";

EXPECT_THROW_WITH_REGEX(AccessLogFactory::fromProto(parseAccessLogFromV3Yaml(yaml), context_),
EnvoyException, ".*\"NOT_A_VALID_CODE\" for type TYPE_ENUM.*");
EnvoyException, "NOT_A_VALID_CODE");
}

TEST_F(AccessLogImplTest, GrpcStatusFilterBlock) {
Expand Down
2 changes: 1 addition & 1 deletion test/common/upstream/cluster_manager_impl_test.cc
View file
Open in desktop
Expand Up @@ -272,7 +272,7 @@ TEST_F(ClusterManagerImplTest, UnknownClusterType) {
)EOF";

EXPECT_THROW_WITH_REGEX(create(parseBootstrapFromV3Json(json)), EnvoyException,
"invalid value \"foo\" for type TYPE_ENUM");
"invalid value \"foo\"");
}

TEST_F(ClusterManagerImplTest, LocalClusterNotDefined) {
Expand Down
1 change: 1 addition & 0 deletions test/common/upstream/eds_speed_test.cc
View file
Open in desktop
Expand Up @@ -10,6 +10,7 @@

#include "common/config/grpc_mux_impl.h"
#include "common/config/grpc_subscription_impl.h"
#include "common/config/protobuf_link_hacks.h"
#include "common/config/utility.h"
#include "common/singleton/manager_impl.h"
#include "common/upstream/eds.h"
Expand Down
7 changes: 2 additions & 5 deletions test/common/upstream/upstream_impl_test.cc
View file
Open in desktop
Expand Up @@ -1917,7 +1917,7 @@ TEST_F(StaticClusterImplTest, UnsupportedLBType) {
socket_address: { address: 192.168.1.2, port_value: 44 }
)EOF";

EXPECT_THROW_WITH_MESSAGE(
EXPECT_THROW_WITH_REGEX(
{
envoy::config::cluster::v3::Cluster cluster_config = parseClusterFromV3Yaml(yaml);
Envoy::Stats::ScopePtr scope =
Expand All @@ -1930,10 +1930,7 @@ TEST_F(StaticClusterImplTest, UnsupportedLBType) {
StaticClusterImpl cluster(cluster_config, runtime_, factory_context, std::move(scope),
false);
},
EnvoyException,
"Protobuf message (type envoy.config.cluster.v3.Cluster reason "
"INVALID_ARGUMENT:(lb_policy): invalid "
"value \"fakelbtype\" for type TYPE_ENUM) has unknown fields");
EnvoyException, "invalid value \"fakelbtype\"");
}

TEST_F(StaticClusterImplTest, MalformedHostIP) {
Expand Down
3 changes: 3 additions & 0 deletions test/extensions/filters/http/compressor/config_test.cc
View file
Open in desktop
@@ -1,5 +1,6 @@
#include "extensions/filters/http/compressor/config.h"

#include "test/extensions/filters/http/compressor/mock_compressor_library.pb.h"
#include "test/mocks/server/factory_context.h"

#include "gtest/gtest.h"
Expand All @@ -12,6 +13,8 @@ namespace {

using testing::NiceMock;

const ::test::mock_compressor_library::Unregistered _mock_compressor_library_dummy;

TEST(CompressorFilterFactoryTests, MissingCompressorLibraryConfig) {
const envoy::extensions::filters::http::compressor::v3::Compressor proto_config;
CompressorFilterFactory factory;
Expand Down
1 change: 1 addition & 0 deletions test/integration/BUILD
View file
Open in desktop
Expand Up @@ -886,6 +886,7 @@ envoy_cc_test(
"//test/mocks/http:http_mocks",
"//test/test_common:utility_lib",
"@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto",
"@envoy_api//envoy/config/filter/http/grpc_http1_bridge/v2:pkg_cc_proto",
"@envoy_api//envoy/config/route/v3:pkg_cc_proto",
"@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto",
],
Expand Down
3 changes: 3 additions & 0 deletions test/integration/integration_test.cc
View file
Open in desktop
Expand Up @@ -3,6 +3,7 @@
#include <string>

#include "envoy/config/bootstrap/v3/bootstrap.pb.h"
#include "envoy/config/filter/http/grpc_http1_bridge/v2/config.pb.h"
#include "envoy/config/route/v3/route_components.pb.h"
#include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h"

Expand Down Expand Up @@ -418,6 +419,8 @@ TEST_P(IntegrationTest, UpstreamDisconnectWithTwoRequests) {
test_server_->waitForCounterGe("cluster.cluster_0.upstream_rq_200", 2);
}

const ::envoy::config::filter::http::grpc_http1_bridge::v2::Config _grpc_http1_bridge_dummy;

// Test hitting the bridge filter with too many response bytes to buffer. Given
// the headers are not proxied, the connection manager will send a local error reply.
TEST_P(IntegrationTest, HittingGrpcFilterLimitBufferingHeaders) {
Expand Down