Cockroach: protobuf: implement proto linting and breaking change detection for internal APIs

[j-low]

Description
Dev Interfaces community service team has been evaluating options for linting protos in CRDB. Buf is an interesting option we have uncovered in the course of evaluation and promote as a candidate to handle linting of internal APIs.

Summary of findings (from Dev Interfaces meeting notes for 2020-11-18):
Observation: Buf contains lint and breaking change checkers for protobuf files, and is also able to invoke protoc to generate the API “stubs” (i.e. server interfaces) in the target languages. For example it can generate the .pb.go files for Go servers.

We can probably use this for internal development everywhere we use protobufs already. For example its PACKAGE linter would have caught the breakage that Netflix encountered and which we did not detect in CI

For HTTP APIs however the tool still needs to be fleshed out: https://docs.buf.build/breaking-checkers/#what-we-left-out-we-think-nothing-except-custom-options

Basically they do not support the option “google.api.http” which turns a protobuf RPC into a HTTP API.

What we would get from it (in terms of HTTP API linting):

• verify that the JSON payloads are stable
• verify that the APIs are “unary”
• lots of naming linters

What we would not get from it (by default as of Buf v0.31.1):

• checks about HTTP API versioning
• checks about API pagination
• checks about authentication and authorization

cc: @knz

[blathers-crl[bot]]

Hi @j-low, I've guessed the C-ategory of your issue and suitably labeled it. Please re-label if inaccurate.

While you're here, please consider adding an A- label to help keep our repository tidy.

_{:owl: Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[bufdev]

Let us know if we can be of any assistance here, or if you'd like to chat. We have a public slack linked from our homepage https://buf.build, feel free to reach out. Note that buf generate is also now available to replace your protoc calls (I think you may have found this though!).

[knz]

@bufdev thanks for chiming in.
Yes we found it out.

Our two main challenges are:

• we make use of multiple extensions (at least gogo/protobuf + google api http)
• we need text post-processing on generated code to adjust import paths
• we'll want linting on the URL query path for RPC-over-HTTP

It's still unclear how to tweak buf in that direction so if you're aware of other folk who try to do those things, we'd like to be in touch.

[bufdev]

Happy to talk about these.

Getting Buf to work

To start out, I did a clone of this respoitory, and here's what I did to make buf work.

Happy to talk through points made below.

Start with this buf.yaml file in the root of this repository:

version: v1beta1
build:
  roots:
    - pkg
    - vendor/github.com/cockroachdb/errors
    - vendor/github.com/gogo/protobuf
    - vendor/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis
    - vendor/github.com/prometheus
    - vendor/go.etcd.io
  excludes:
    # gogo/protobuf has extra test files that make it so that
    # if you include github.com/gogo/protobuf as a root, buf will
    # detect these as well, so they need to be manually ignored
    # the "correct" solution is gogo/protobuf moving gogoproto/gogo.proto
    # to a top-level proto directory, but we're way past that in 2020
    - vendor/github.com/gogo/protobuf/jsonpb
    - vendor/github.com/gogo/protobuf/proto
    - vendor/github.com/gogo/protobuf/protobuf
    - vendor/github.com/gogo/protobuf/protoc-gen-gogo
    - vendor/github.com/gogo/protobuf/vanity
lint:
  # I did a pass on what lint rules you would abide by roughly, and
  # this is it - if you want to see what rules are failing where
  # from default, remove the "except" and "ignore_only" configurations,
  # and run "buf check lint --error-format=config-ignore-yaml".
  use:
    - DEFAULT
  except:
    - DIRECTORY_SAME_PACKAGE
    - ENUM_VALUE_PREFIX
    - ENUM_VALUE_UPPER_SNAKE_CASE
    - ENUM_ZERO_VALUE_SUFFIX
    - FIELD_LOWER_SNAKE_CASE
    - PACKAGE_DIRECTORY_MATCH
    - PACKAGE_SAME_GO_PACKAGE
    - PACKAGE_VERSION_SUFFIX
    - RPC_REQUEST_STANDARD_NAME
    - RPC_RESPONSE_STANDARD_NAME
    - SERVICE_SUFFIX
  ignore_only:
    FILE_LOWER_SNAKE_CASE:
      - roachpb/io-formats.proto
    PACKAGE_SAME_DIRECTORY:
      - kv/kvserver/protectedts/ptpb/protectedts.proto
      - kv/kvserver/protectedts/ptstorage/storage.proto
    RPC_REQUEST_RESPONSE_UNIQUE:
      - blobs/blobspb/blobs.proto
      - rpc/heartbeat.proto
      - server/serverpb/admin.proto
      - server/serverpb/status.proto
      - sql/execinfrapb/api.proto

Make some small edits

• You are including files from github.com/prometheus/client_model as if their include path (root)
  was the directory vendor/github.com, i.e. effectively doing -I vendor/github.com. I changed
  it to at least vendor/github.com/prometheus above, and had to do the following edits:

diff --git a/pkg/ts/catalog/chart_catalog.proto b/pkg/ts/catalog/chart_catalog.proto
index d111414391..92dfd0a3ae 100644
--- a/pkg/ts/catalog/chart_catalog.proto
+++ b/pkg/ts/catalog/chart_catalog.proto
@@ -15,7 +15,7 @@ option go_package = "catalog";
 import "ts/tspb/timeseries.proto";

 import "gogoproto/gogo.proto";
-import "prometheus/client_model/metrics.proto";
+import "client_model/metrics.proto";

 // AxisUnits describes the Unit options available in the Admin UI. It is defined here
 // as opposed to importing the value from the Admin UI because the existing pb doesn't
diff --git a/pkg/util/metric/metric.proto b/pkg/util/metric/metric.proto
index 29066705e8..9db06133b3 100644
--- a/pkg/util/metric/metric.proto
+++ b/pkg/util/metric/metric.proto
@@ -14,7 +14,7 @@ package cockroach.util.metric;
 option go_package = "metric";

 import "gogoproto/gogo.proto";
-import "prometheus/client_model/metrics.proto";
+import "client_model/metrics.proto";

To make these imports consistent with the rest of the codebase, these imports should really be
metrics.proto, with the root being vendor/github.com/prometheus/client_model, but this
might be diffult - the real issue here is that metrics.proto should be structured in a directory
within github.com/prometheus/client_model that matches the package path (more on this below)

Change something in github.com/cockroachdb/errors

Inside vendor, the following edit has to be made:

diff --git a/github.com/cockroachdb/errors/errorspb/markers.proto b/github.com/cockroachdb/errors/errorspb/markers.proto
index 02ef1c82b..3ea30ee80 100644
--- a/github.com/cockroachdb/errors/errorspb/markers.proto
+++ b/github.com/cockroachdb/errors/errorspb/markers.proto
@@ -2,7 +2,7 @@ syntax = "proto3";
 package cockroach.errorspb;
 option go_package = "errorspb";

-import "github.com/cockroachdb/errors/errorspb/errors.proto";
+import "errorspb/errors.proto";
 import "gogoproto/gogo.proto";

 // MarkPayload is the error payload for a forced marker.

The issue here is that buf builds entire directories - we feel that all directories with Protobuf
files should be buildable (and includable) themselves, not just choosing indivudal files from the
directories to include. Inside cockroachdb/cockroach, you import errors.proto as
errorspb/errors.proto, but cockroachdb/errors imports it as
github.com/cockroachdb/errors/errorspb/errors.proto inside markers.proto, following a pattern
of doing protoc -I ${GOPATH}/src that emerged (and subsequently unemerged) circa 2015.
This is something that should likely be made to be consistent anyways - see root requirements:

Which file is being imported? Is it foo/baz/baz.proto? bar/baz/baz.proto? The answer depends on the order of the -I flags given to protoc, or (if Buf didn't error in this scenario pre-compilation, which Buf does) the order of the imports given to the internal compiler. If the authors are being honest, we can't remember if it's the first -I or second -I that wins - we have outlawed this in our own builds for a long time.

If you were to do, for example, protoc -I vendor/github.com/cockroachdb/errors -I vendor (which
you must be doing some form of now, including for prometheus), this is going to cause issues -
files included from two different paths are treated as unique by protoc, but since they have
the same types, this can cause an error such as foo.bar.Baz: duplicate type during Protobuf
compilation. protoc happily carries on in some cases depending on what and where you specifically
include this file, but buf does not - we think this should appropriately be an error.

Breaking change detection

Because your build relies on vendored files that are not checked in, you can't use buf check breaking --against .git#branch=master or other simple inputs.
You can, however, build an image for your files, and
check against this if it is checked in:

# generate on the release branch
$ buf build -o image.bin

# check against this on your current branch
$ buf check breaking --against image.bin

Concerns raised

we make use of multiple extensions (at least gogo/protobuf + google api http)

No problem I believe - note that migrating to buf generate
will take more work, given your custom setup (although you can switch to buf protoc
instead of protoc if you want one less tool to install).

we need text post-processing on generated code to adjust import paths

I saw this in the Makefile :-) I think some of this is touched on above, perhaps this is how the
errorspb/errors.proto problem is solved?

we'll want linting on the URL query path for RPC-over-HTTP

This is something we likely won't tackle soon. Our view is that the google/api annotations
are a temporary fix for the ecosystem to deal with the issue that Protobuf isn't easily
usable from frontends - as Protobuf consumption/distribution gets easier (hopefully with our help!),
and efforts to standardize HTTP/1.1 POST RPCs, along with grpc-web, get traction, we won't need
to rely on additional mappings via the annotations - having them effectively means every RPC
has two shapes (one for the RPC, one for the annotation), which isn't ideal. Of course, we're
not there yet, but Buf is forward-looking on this aspect, so we're concentrating our efforts
on making the Buf Schema Registry widely avaiable, which via various mechanisms, should
make this much easier.

Happy to discuss this topic more, I know that's likely not the most satisfying answer, and there's
plenty of room for discussion on it!

[knz]

thank you!!!