deps: update protobuf to 3.14 #14253

kyessenov · 2020-12-03T01:23:00Z

Signed-off-by: Kuat Yessenov kuat@google.com

Commit Message: update protobuf to 3.14
Additional Description:
There is an unfortunate change in 3.14 that changed go_package for WKT, which necessitates updating several go dependencies as well:

protoc-gen-validate to 2020-11-30
transitive dependencies from protoc-gen-validate
gazelle to 0.22.2
rules_go to 0.25.0
go to 1.15.5

Risk Level: low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only · 2020-12-03T01:23:05Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #14253 was opened by kyessenov.

see: more, trace.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2020-12-03T02:03:55Z

Please provide guidance how to fix deps check CI. Not sure what I understand what it means.

moderation

Can we use this opportunity to bump the Go version at https://github.com/envoyproxy/envoy/blob/master/bazel/dependency_imports.bzl#L19 to 1.15.5?

Changes to dependencies get flagged for approval by https://github.com/orgs/envoyproxy/teams/dependency-shepherds. Policy at https://github.com/envoyproxy/envoy/blob/master/DEPENDENCY_POLICY.md#dependency-shepherds

moderation · 2020-12-03T06:20:41Z

api/bazel/repository_locations.bzl

+        version = "ec9cd95372b9630aafba2cb4a0e448c43768e0df",
+        sha256 = "60b4b8036497368512795afca0292d1777d1ce651db76c36706af1733179213a",


Can we bump to the latest PGV commit?
1bcea29601b5624234a19b3d7f0ebd9e9984f583
2062bbe50eddf3c98490339721fb02b5b5cd78f610f163b98bbf95ba7105553f

moderation · 2020-12-03T06:21:06Z

api/bazel/repository_locations.bzl

        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-10-28",


release_date for latest PGV commit as per comment above is 2020-11-30

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2020-12-03T23:24:33Z

There is something wrong with scripts:

tools/code_format/check_format.py fix
ERROR: From ./resolved.bzl
ERROR: ./resolved.bzl:3940: unexpected direct external dependency on protobuf, use //source/common/protobuf instead.
ERROR: check format failed. run 'tools/code_format/check_format.py fix'

kyessenov · 2020-12-03T23:31:34Z

What does the following mean:

Dependency validation failed, please check metadata in bazel/repository_locations.bzl
Observed dataplane core deps {'com_github_lyft_protoc_gen_star', 'com_google_absl', 'io_bazel_rules_go', 'com_github_c_ares_c_ares', 'com_github_gabime_spdlog', 'net_zlib', 'com_google_protobuf', 'org_golang_google_genproto', 'com_github_tencent_rapidjson', 'com_github_cncf_udpa', 'com_github_spf13_afero', 'com_envoyproxy_protoc_gen_validate', 'boringssl', 'com_github_fmtlib_fmt', 'com_github_cyan4973_xxhash', 'com_google_googleapis', 'org_golang_x_text', 'com_github_iancoleman_strcase', 'com_github_gperftools_gperftools', 'boringssl_fips', 'com_github_libevent_libevent', 'com_github_grpc_grpc', 'com_github_google_tcmalloc', 'com_github_nghttp2_nghttp2', 'com_github_circonus_labs_libcircllhist', 'com_github_zlib_ng_zlib_ng', 'opencensus_proto', 'com_github_jbeder_yaml_cpp', 'com_googlesource_code_re2', 'com_github_nodejs_http_parser'} is not covered by "use_category" implied core deps {'com_google_absl', 'io_bazel_rules_go', 'prometheus_metrics_model', 'bazel_skylib', 'com_github_c_ares_c_ares', 'com_github_gabime_spdlog', 'net_zlib', 'com_google_protobuf', 'com_github_tencent_rapidjson', 'com_github_cncf_udpa', 'com_envoyproxy_protoc_gen_validate', 'boringssl', 'com_github_fmtlib_fmt', 'com_github_cyan4973_xxhash', 'com_github_openzipkin_zipkinapi', 'com_google_googleapis', 'com_github_gperftools_gperftools', 'boringssl_fips', 'com_github_libevent_libevent', 'com_github_grpc_grpc', 'com_github_google_tcmalloc', 'com_github_circonus_labs_libcircllhist', 'com_github_nghttp2_nghttp2', 'com_github_zlib_ng_zlib_ng', 'rules_proto', 'opencensus_proto', 'com_github_apache_skywalking_data_collect_protocol', 'com_github_jbeder_yaml_cpp', 'com_googlesource_code_re2', 'com_github_nodejs_http_parser'}: {'com_github_lyft_protoc_gen_star', 'org_golang_x_text', 'com_github_iancoleman_strcase', 'org_golang_google_genproto', 'com_github_spf13_afero'} are missing

Is it because go dependency somehow became part of data plane? How do I tag "go_repository"?

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2020-12-04T03:18:53Z

@htuch Do you know how to fix protoxform build error:

Traceback (most recent call last):
  File "/build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/sandbox/processwrapper-sandbox/2/execroot/envoy/bazel-out/k8-opt-exec-2B5CBBC6/bin/tools/protoxform/protoxform.runfiles/envoy/tools/protoxform/protoxform.py", line 12, in <module>
    from tools.protoxform import migrate
  File "/build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/sandbox/processwrapper-sandbox/2/execroot/envoy/bazel-out/k8-opt-exec-2B5CBBC6/bin/tools/protoxform/protoxform.runfiles/envoy/tools/protoxform/migrate.py", line 14, in <module>
    from google.api import annotations_pb2
ModuleNotFoundError: No module named 'google.api'

I can't tell what changed to make it fail...

kyessenov · 2020-12-04T15:33:43Z

I tried to debug this and found that

import google
print(google.__path__)

shows bazel-bin/tools/protoxform/protoxform.runfiles/com_google_protobuf/python/google. The path is wrong but I'm not sure where it's coming from. I didn't touch python in this PR at all.

adisuissa · 2020-12-04T16:12:35Z

I've looked at this and found that the difference seems to be in:
bazel-bin/tools/protoxform/protoxform.runfiles/com_google_protobuf/python/google/protobuf/__init__.py

More specifically, the protobuf 3.13.0 version has:

__version__ = '3.13.0'

if __name__ != '__main__':
  try:
    __import__('pkg_resources').declare_namespace(__name__)
  except ImportError:
    __path__ = __import__('pkgutil').extend_path(__path__, __name__)

which is missing in protobuf v3.14.0.

This seems to be coming from: protocolbuffers/protobuf#7902

kyessenov · 2020-12-04T16:18:28Z

https://github.com/protocolbuffers/protobuf/blob/v3.14.0/python/google/__init__.py shows it's there?
But I also noticed it's gone from runfiles.

adisuissa · 2020-12-04T16:21:08Z

I think it's removed from the inner one (in https://github.com/protocolbuffers/protobuf/blob/v3.14.0/python/google/protobuf/__init__.py)

kyessenov · 2020-12-04T16:21:24Z

OK, let me try to add a patch...

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2020-12-04T18:08:24Z

Looks like another CI failure:

##[error]Error: Failed find: ENOENT: no such file or directory, lstat '/srv/azure-pipelines/_work/1/a/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/external/go_sdk/test/fixedbugs/issue27836.dir/�foo.go'

There's a weird character in the file name.

adisuissa · 2020-12-04T22:32:54Z

This looks very similar to this bug: bazel-contrib/rules_go#1880

And it seems that the function that fixed the bug was modified recently:
https://github.com/bazelbuild/rules_go/pull/2729/files#diff-f06d5b9a2d01e5a186f1b0f8df0f7a0e622bf7b5d481a6448c477665c357b722L139-L162

I wonder if a patch in bazel/rules_go.patch that reverts it would work in this case, and I'm not sure how to test because I cannot reproduce on my end:

diff --git a/go/private/sdk.bzl b/go/private/sdk.bzl
index c148c9e2..c03cd495 100644
--- a/go/private/sdk.bzl
+++ b/go/private/sdk.bzl
@@ -177,11 +177,24 @@ def _remote_sdk(ctx, urls, strip_prefix, sha256):
     if len(urls) == 0:
         fail("no urls specified")
     ctx.report_progress("Downloading and extracting Go toolchain")
-    ctx.download_and_extract(
-        url = urls,
-        stripPrefix = strip_prefix,
-        sha256 = sha256,
-    )
+    if urls[0].endswith(".tar.gz"):
+        if strip_prefix != "go":
+            fail("strip_prefix not supported")
+        ctx.download(
+            url = urls,
+            sha256 = sha256,
+            output = "go_sdk.tar.gz",
+        )
+        res = ctx.execute(["tar", "-xf", "go_sdk.tar.gz", "--strip-components=1"])
+        if res.return_code:
+            fail("error extracting Go SDK:\n" + res.stdout + res.stderr)
+        ctx.execute(["rm", "go_sdk.tar.gz"])
+    else:
+      ctx.download_and_extract(
+          url = urls,
+          stripPrefix = strip_prefix,
+          sha256 = sha256,
+      )

 def _local_sdk(ctx, path):
     for entry in ["src", "pkg", "bin"]:

kyessenov · 2020-12-04T22:50:04Z

@adisuissa I'm more inclined to think this is a bug in Azure task. The tests seem to pass for me locally as well.

lizan · 2020-12-05T22:26:29Z

Yeah seems to be a bug of Azure task. Though Windows failure seems real.

htuch · 2020-12-05T23:20:18Z

bazel/protobuf.patch

+@@ -31,3 +31,9 @@
+ # Copyright 2007 Google Inc. All Rights Reserved.
+
+ __version__ = '3.14.0'


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

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.

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?

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.

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 :(

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:

Remove google/__init__.py from googleapis/google-auth-library-python. (@busunkim96 to comment on that.)

Pre-install the package with an up-to-date version of pip, which should install using the native namespace.

Update Bazel's pip rules to make sure it does (2).

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

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

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?

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.

test/common/upstream/upstream_impl_test.cc

htuch

Thanks for taking on the upgrade work, we're seriously out-of-date 😱

kyessenov · 2020-12-11T18:11:07Z

Thanks @jayconrod, I found out it's opencensus repo that has a conflict.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2020-12-11T22:20:00Z

/retest

repokitteh-read-only · 2020-12-11T22:20:04Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14253 (comment) was created by @kyessenov.

see: more, trace.

kyessenov · 2020-12-12T00:22:11Z

/retest

repokitteh-read-only · 2020-12-12T00:22:15Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14253 (comment) was created by @kyessenov.

see: more, trace.

kyessenov · 2020-12-14T19:14:53Z

@htuch @moderation This is ready for review again. Let me know if you want to resolve python patch here, or in a followup.

moderation · 2020-12-14T19:50:09Z

/lgtm deps

moderation · 2020-12-14T19:47:41Z

bazel/dependency_imports.bzl

@@ -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"


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

htuch · 2020-12-15T01:24:54Z

@kyessenov I'm good with merging this if you can followup on the Python protobuf patch as a followup.

kyessenov · 2020-12-15T01:26:55Z

@htuch sure, let's open an issue for tracking then. We'll need to include relevant people, since I don't write python with bazel myself and unaware of best practices.

kyessenov · 2020-12-15T22:28:41Z

Can we merge this? I have cel-cpp update staged which needs this protobuf version.

htuch

LGTM, thanks so much for persisting through the obstacle course run!

* master: (49 commits) sds: allow multiple init managers share sds target (envoyproxy#14357) [http] Remove legacy codecs (envoyproxy#14381) http2: Add integration tests for METADATA and RST_STREAM frame flood mitigation for upstream servers (envoyproxy#14365) test: start dissolving :printers_include rule. (envoyproxy#14429) integration tests: re-enable set_node_on_first_message_only (envoyproxy#14270) formatter: add a formatter that returns a google::protobuf::Struct rather than a string (envoyproxy#14258) ratelimit: support returning custom response bodies for non-OK responses from the external ratelimit service (envoyproxy#14189) deps: update protobuf to 3.14 (envoyproxy#14253) stream_info: add setResponseCode and update local_reply to take a normal StreamInfo (envoyproxy#14402) http: alpn upstream (envoyproxy#13922) Moved starttls integration test to test/extensions/transport_sockets/starttls. (envoyproxy#14425) generic conn pool: directly use thread local cluster (envoyproxy#14423) wasm: add mathetake to CODEOWNERS (envoyproxy#14427) wasm: clear route cache when modifying HTTP request headers. (envoyproxy#14318) tls: disable TLS inspector injection (envoyproxy#14404) aggregate cluster: cleanups (envoyproxy#14411) Mark starttls_integration_test flaky on Windows (envoyproxy#14419) tcp: improved unit testing (envoyproxy#14415) config: making protocol config explicit (envoyproxy#14362) wasm: dead code (envoyproxy#14407) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

rgs1 · 2020-12-17T20:36:43Z

@kyessenov fyi the rules_go update breaks rules_docker (for us internally, maybe for others too):

https://github.com/bazelbuild/rules_docker/blob/master/repositories/go_repositories.bzl#L34

which calls go_register_toolchains() without passing a version... (this was fine before, it'd would fallback to a default).

kyessenov · 2020-12-17T20:39:28Z

@rgs1 Sorry about that. It's hard to get around breaking changes since only the latest version of rules_go supports 3.14 protoc but the latest version also turns duplicate linker warnings into errors.

rgs1 · 2020-12-17T20:41:43Z

All good, I'll send a PR for rules_docker -- just commenting in case anyone else was hitting this.

rgs1 · 2020-12-17T20:46:36Z

bazelbuild/rules_docker#1700 is maybe enough.

update protobuf

5313dc7

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Dec 3, 2020

kyessenov added 2 commits December 2, 2020 17:54

revert googleapis change; it is also breaking :(

919e1cd

Signed-off-by: Kuat Yessenov <kuat@google.com>

typo

18691fb

Signed-off-by: Kuat Yessenov <kuat@google.com>

moderation suggested changes Dec 3, 2020

View reviewed changes

jmarantz assigned moderation and adisuissa Dec 3, 2020

code review

6e8acda

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov added 4 commits December 3, 2020 17:17

fix tests

5a51bdc

Signed-off-by: Kuat Yessenov <kuat@google.com>

testing

2fe0b36

Signed-off-by: Kuat Yessenov <kuat@google.com>

testing

c113ef7

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix deps

494314b

Signed-off-by: Kuat Yessenov <kuat@google.com>

patch protobuf rule

b34523b

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch reviewed Dec 5, 2020

View reviewed changes

test/common/upstream/upstream_impl_test.cc Outdated Show resolved Hide resolved

htuch reviewed Dec 5, 2020

View reviewed changes

This was referenced Dec 11, 2020

resolve rules_go conflict cncf/udpa#43

Merged

Please update rules_go to 0.25.0 census-instrumentation/opencensus-proto#221

Open

kyessenov added 2 commits December 11, 2020 12:09

update go build

f01d9f1

Signed-off-by: Kuat Yessenov <kuat@google.com>

update udpa

c8ef105

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Dec 14, 2020

moderation reviewed Dec 14, 2020

View reviewed changes

htuch approved these changes Dec 15, 2020

View reviewed changes

kyessenov mentioned this pull request Dec 15, 2020

Resolve python dependency patch in protobuf #14412

Closed

htuch approved these changes Dec 16, 2020

View reviewed changes

htuch merged commit b3bb0f9 into envoyproxy:master Dec 16, 2020

This was referenced Jan 14, 2021

Upgrade third_party/protobuf to v3.14.0 (for v1.35.x) grpc/grpc#25131

Merged

DO NOT MERGE grpc/grpc#25171

Closed

jtattermusch mentioned this pull request Jan 18, 2021

Try upgrading third_party/protobuf version to 3.14.0-rc2 grpc/grpc#24723

Closed

vam-google mentioned this pull request Jan 26, 2021

Remove uses of pkg_resources in non-namespace packages. protocolbuffers/protobuf#7902

Merged

		version = "ec9cd95372b9630aafba2cb4a0e448c43768e0df",
		sha256 = "60b4b8036497368512795afca0292d1777d1ce651db76c36706af1733179213a",

deps: update protobuf to 3.14 #14253

deps: update protobuf to 3.14 #14253

Conversation

kyessenov commented Dec 3, 2020 • edited Loading

repokitteh-read-only bot commented Dec 3, 2020

kyessenov commented Dec 3, 2020

moderation left a comment

Choose a reason for hiding this comment

moderation Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moderation Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov commented Dec 3, 2020

kyessenov commented Dec 3, 2020 • edited Loading

kyessenov commented Dec 4, 2020

kyessenov commented Dec 4, 2020 • edited Loading

adisuissa commented Dec 4, 2020

kyessenov commented Dec 4, 2020

adisuissa commented Dec 4, 2020

kyessenov commented Dec 4, 2020

kyessenov commented Dec 4, 2020

adisuissa commented Dec 4, 2020

kyessenov commented Dec 4, 2020

lizan commented Dec 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

kyessenov commented Dec 11, 2020

kyessenov commented Dec 11, 2020

repokitteh-read-only bot commented Dec 11, 2020

kyessenov commented Dec 12, 2020

repokitteh-read-only bot commented Dec 12, 2020

kyessenov commented Dec 14, 2020

moderation commented Dec 14, 2020

Choose a reason for hiding this comment

htuch commented Dec 15, 2020

kyessenov commented Dec 15, 2020

kyessenov commented Dec 15, 2020

htuch left a comment

Choose a reason for hiding this comment

rgs1 commented Dec 17, 2020

kyessenov commented Dec 17, 2020

rgs1 commented Dec 17, 2020

rgs1 commented Dec 17, 2020

kyessenov commented Dec 3, 2020 •

edited

Loading

moderation Dec 3, 2020 •

edited

Loading

moderation Dec 3, 2020 •

edited

Loading

kyessenov commented Dec 3, 2020 •

edited

Loading

kyessenov commented Dec 4, 2020 •

edited

Loading