tools: updating buf.lock googleapis dependency

[adisuissa]

Commit Message: tools: updating buf.lock googleapis dependency
Additional Description:
Buf googleapis version was updated in the BSR. This PR updates that version, and should fix the error we are seeing in CI.

Risk Level: Low - CI only
Testing: N/A. tools dependency update.
Docs Changes: N/A.
Release Notes: N/A.

Signed-off-by: Adi Suissa-Peleg [email protected]

[phlax]

@adisuissa afaict this is still failing as its trying to use the buf install from the base branch

i guess we either need a senior maintainer to override ci or to add something to ensure it reads the correct buf.lock and uses the correct version of buf (not sure exactly of the effect of that)

[moderation]

Also wondering how the buf.lock config interacts with the direct API dependency for googleapis at https://github.com/envoyproxy/envoy/blob/main/api/bazel/repository_locations.bzl#L65-L76

[adisuissa]

Also wondering how the buf.lock config interacts with the direct API dependency for googleapis at https://github.com/envoyproxy/envoy/blob/main/api/bazel/repository_locations.bzl#L65-L76

That's a good point. @johanbrandhorst can you help us here?
If we use a specific version of googleapis (version-82944da21578a53b74e547774cf62ed31a05b841), do we need to keep updating our buf.lock with the newer version from the BSR?

[johanbrandhorst]

Also wondering how the buf.lock config interacts with the direct API dependency for googleapis at https://github.com/envoyproxy/envoy/blob/main/api/bazel/repository_locations.bzl#L65-L76

That's a good point. @johanbrandhorst can you help us here?
If we use a specific version of googleapis (version-82944da21578a53b74e547774cf62ed31a05b841), do we need to keep updating our buf.lock with the newer version from the BSR?

You can lock to a specific version in your buf.yaml (set

envoy/api/buf.yaml

Line 3 in 01ea37b

- buf.build/googleapis/googleapis

to - buf.build/googleapis/googleapis:82944da21578a53b74e547774cf62ed31a05b841, see https://docs.buf.build/configuration/v1/buf-yaml#deps for details on deps, see https://buf.build/googleapis/googleapis/history for available commits and tags. We tag all updates on googleapis/googleapis with the git SHA, so you can use that to easily depend on the same version.), so ideally they'd be updated in lock step, otherwise they would eventually get out of sync. Running buf mod update will get the latest version from the BSR. The risk of any conflict is reasonably small though, seeing as you're only using it for breaking change detection.

[johanbrandhorst]

I went and checked and that particular SHA is not available since it's from 2019, but as I said I wouldn't be too worried about drift right now.

[adisuissa]

@alyssawilk I think this PR solves the api_compat issue.
I guess that we can wait for the rest of the CI checks to become green, but I don't think they will be impacted.
Once this is merged we can close #18029.

[moderation]

More questions after reviewing this PR. It appears we now have 3 places to define, manage and maintain API dependencies?:

• api/bazel/repository_locations.bzl
• api/buf.lock
• api/buf.yaml

After this PR we now have the buf.yaml aligned with buf.lock with SHA d1a849b8f8304950832335723096e954. In buf.yaml only googleapis is the only dependency with a SHA.

We used to have gogo/protobuf as a dependency in this repo but it was removed. I can't find the PR but this is related - bufbuild/protoc-gen-validate#338

Having multiple locations for the same dependencies that have already drifted into different states is not ideal.

Can api/bazel/repository_locations.bzl be removed all together so there is a single source of API dependencies?

Are we really sure we want to bring gogo/protobuf back in after removing it?

buf.{yaml,lock} currently covers:

googleapis
opencensus
opentelemetry
prometheus
protobuf
protoc-gen-validate
xds

api/bazel/repository_locations.bzl covers:

buf.build
bazelbuild/bazel-skylib
bazelbuild/buildtools
bazelbuild/rules_proto
census-instrumentation/opencensus-proto
cncf/xds
envoyproxy/protoc-gen-validate
googleapis/googleapis
open-telemetry/opentelemetry-proto
openzipkin/zipkin-api
prometheus/client_model

so the delta of the locations is:

buf
bazelbuild/bazel-skylib
bazelbuild/buildtools
bazelbuild/rules_proto
openzipkin/zipkin-api

/cc @envoyproxy/dependency-shepherds

Edit: And... buf is Linux x86_64 only - #17515

[johanbrandhorst]

Edit: And... buf is Linux x86_64 only - #17515

I may be misunderstanding you, but I don't believe this is accurate, we build release assets for most common platforms and architectures, including Linux, Darwin and Windows (x86_64 and arm64).

buf.{yaml,lock} currently covers:

I spoke with @YaseenAlk separately about some of these dependencies when #17515 was in review, but I will add my notes here too:

The repositories under the beta organization are deprecated, and we (Buf) do not maintain them anymore. The hope is that community contributors will push their protobuf files to the registry if they want users to be able to consume them via the registry. It's fine to depend on these for your current purposes, which is just to provide an easy way for you to check that changes to your own protobufs aren't breaking, but before this project considers pushing their own protobuf files to the BSR, it would be best to ensure that the dependencies have been added too. If some very important package is without maintainers (like gogo/protobuf), we may opt to maintain it for the benefit of the community, but this will happen on a case-by-case basis. The repositories that we have committed to maintaining are:

• buf.build/googleapis/googleapis
• buf.build/gogo/protobuf
• buf.build/envoyproxy/protoc-gen-validate

Eventually, of course, we hope to be able to give these organizations and projects back to their respective owners, too.

Are we really sure we want to bring gogo/protobuf back in after removing it?

I expect the only reason this was added as a dependency in buf.yaml was because some protobuf file in your current tree (or a dependency) imports gogo.proto. It doesn't necessarily mean that you suffer from the runtime issues that were associated with the gogo/protobuf project.

Can api/bazel/repository_locations.bzl be removed all together so there is a single source of API dependencies?

I think this would require moving more of the protobuf tooling into buf, e.g. file generation. We don't have an amazing Bazel integration story yet, so I don't know how feasable this is with your current setup. Would be happy to work with you to figure this out if you are keen. I think the easier solution might be to move breaking change detection into bazel via our protoc plugin protoc-gen-buf-breaking, until we have better Bazel support.

Having said all of that though, I don't think the drift is a significant issue for your current use case. You're using it for local breaking change detection. You'll need to update the dependencies if one of your dependencies make a change that you want to consume (probably most likely to come from xds), since otherwise you will get an error trying to build the image for the breaking change detection comparison, but other than that case, drift between the sources you use with Bazel and those you use with Buf shouldn't really matter too much.

[adisuissa]

Following your comments, submitted #18042 to remove the gogo dependency. AFAICT it was added to test breaking changes against old PRs where the API used it.

I agree with @moderation that having a single place where the packages and their versions are defined is the desired goal.
I assume that the benefits of using the bazel repo-locations file as the source of truth, is that one can build against these api targets, but maybe there's additional functionality here which is good to have.

I'll take a look at the protoc-gen-buf-breaking plugin that @johanbrandhorst suggested, and see if we can use it. If it doesn't require the BSR versions, then it might be a good option, and we'll be able to remove the buf.yaml/lock files and the source of truth will still be the repo-locations bazel file.

As a next step, I think it will be wise to pin the versions used by the buf script to the most compatible one. I'll do it.

[moderation]

@johanbrandhorst

I may be misunderstanding you, but I don't believe this is accurate, we build release assets for most common platforms and architectures, including Linux, Darwin and Windows (x86_64 and arm64).

I should have clarified that Envoy's usage of buf is Linux only for the time being - see https://github.com/envoyproxy/envoy/blob/main/api/bazel/repository_locations.bzl#L128

I'm confused about the commit SHAs. https://github.com/envoyproxy/envoy/blob/main/api/buf.yaml#L3

buf.build/googleapis/googleapis:d1a849b8f8304950832335723096e954

However you can't use that value to find the commit on googleapis - googleapis/googleapis@d1a849b

My suggestion is that until we work out where this goes, we should manually synchronize the commit SHA's in the buf.{lock,yaml} files and the corresponding entries in api/bazel/repository_locations.bzl. This would be

https://github.com/googleapis/googleapis/commit/b64d30b2b245cb805d1aa328e50258fdff6e628e
https://buf.build/googleapis/googleapis/tree/b64d30b2b245cb805d1aa328e50258fdff6e628e

using the latest Sept. 3 commit.

Thanks for the follow up @adisuissa and 👍 to removing gogo

[johanbrandhorst]

buf.build/googleapis/googleapis:d1a849b8f8304950832335723096e954

The source of the confusion is that this (noticeably shorter string) references a BSR commit, which is independent of the VCS SHAs that may be associated with the files at the time of pushing to the BSR. If you just always use a reasonably recent googleapis, you can just copy the SHA directly into your BSR reference, we have automatic syncing of the googleapis SHAs to tags set up so you can reference them directly.

[johanbrandhorst]

Looping back on this discussion, we've since made https://buf.build/envoyproxy/envoy a managed third party plugin, so we're monitoring and automatically updating it. This means all dependencies are also available, and up to date (including https://buf.build/cncf/xds). As a result, you should be able to update your buf.yaml to use the new dependencies:

version: v1
deps:
    - buf.build/googleapis/googleapis
    - buf.build/opencensus/opencensus
    - buf.build/prometheus/client-model
    - buf.build/opentelemetry/opentelemetry
    - buf.build/cncf/xds
breaking:
  ignore_unstable_packages: true
  use:
    - FIELD_SAME_ONEOF
    - FIELD_SAME_JSON_NAME
    - FIELD_SAME_NAME
    - FIELD_SAME_TYPE
    - FIELD_SAME_LABEL
    - FILE_SAME_PACKAGE
    - FIELD_NO_DELETE_UNLESS_NUMBER_RESERVED
    - FIELD_NO_DELETE_UNLESS_NAME_RESERVED

This should fix any issues you have around files missing in xds for your breaking change detection. Would you like me to submit a PR to make these changes, @adisuissa?