ci: Add API protobuf breaking-change-detector CI script (#17793)

This PR is a continuation to #17515 - it adds a script that uses buf to check for breaking changes on proto files in the api folder. It does so by comparing the current api folder against the api folder at the git commit computed by tools/git/last_github_commit.sh - that should hopefully represent the most recent commit on main (if there is a better method to obtain the base branch commit, let me know!). Adding the script also required re-organizing some of the breaking change detector logic from the previous pr: some levels of abstraction were added, and the detector now expects a git repository and ref as the input for initial state (rather than a proto file). The script is invoked in do_ci.sh if bazel.api_compat is specified as the CI_TARGET. This PR also bumps the buf bazel dependency to 0.53.0. If this is preferred to be in a separate PR, let me know and I would be happy to do so Risk Level: low (hopefully) - the CI script will be invoked in a separate CI pipeline job that can be set to be optional on github. The azure pipeline has been added but needs to be set to optional by a CI maintainer Testing: New scripts and logic were tested manually; also ran tests from the previous PR and they still pass. hoping to observe more output of this tool through reading CI logs of other PRs once this is merged (this PR should not affect the existing PR workflow - refer to Risk Level) Docs Changes: n/a Release Notes: n/a Platform Specific Features: CI script uses a linux binary for buf so it cannot be run outside of docker on non-linux systems Fixes #3368 Signed-off-by: Yaseen Alkhafaji <[email protected]>
envoyproxy · Aug 27, 2021 · f30c289 · f30c289
1 parent 189e649
commit f30c289
Show file tree

Hide file tree

Showing 63 changed files with 617 additions and 337 deletions.
diff --git a/.azure-pipelines/pipelines.yml b/.azure-pipelines/pipelines.yml
@@ -244,6 +244,8 @@ stages:
       matrix:
         api:
           CI_TARGET: "bazel.api"
+        api_compat:
+          CI_TARGET: "bazel.api_compat"
         gcc:
           CI_TARGET: "bazel.gcc"
         clang_tidy:

diff --git a/api/BUILD b/api/BUILD
@@ -256,3 +256,11 @@ proto_library(
         ":v3_protos",
     ],
 )
+
+filegroup(
+    name = "proto_breaking_change_detector_buf_config",
+    srcs = [
+        "buf.yaml",
+    ],
+    visibility = ["//visibility:public"],
+)
diff --git a/api/bazel/repository_locations.bzl b/api/bazel/repository_locations.bzl
@@ -122,11 +122,11 @@ REPOSITORY_LOCATIONS_SPEC = dict(
         project_name = "buf",
         project_desc = "A new way of working with Protocol Buffers.",  # Used for breaking change detection in API protobufs
         project_url = "https://buf.build",
-        version = "0.48.2",
-        sha256 = "ee0ea6c4a7bbb016d79b056905c0a1f018e7c5e47b37038c993a77b1bc732c0d",
+        version = "0.53.0",
+        sha256 = "888bb52d358e34a8d6a57ecff426bed896bdf478ad13c78a70a9e1a9a2d75715",
         strip_prefix = "buf",
         urls = ["https://github.com/bufbuild/buf/releases/download/v{version}/buf-Linux-x86_64.tar.gz"],
-        release_date = "2021-07-30",
+        release_date = "2021-08-25",
         use_category = ["api"],
         tags = ["manual"],
     ),

diff --git a/api/buf.lock b/api/buf.lock
@@ -0,0 +1,52 @@
+# Generated by buf. DO NOT EDIT.
+version: v1
+deps:
+  - remote: buf.build
+    owner: beta
+    repository: opencensus
+    branch: main
+    commit: 5f5f8259293649d68707d2e5b6285748
+    digest: b1-myYwcdM0Xu05qIwhiy4eWEcARYUuZZ1vTbYvrrHu1mU=
+    create_time: 2021-03-03T20:50:42.079743Z
+  - remote: buf.build
+    owner: beta
+    repository: opentelemetry
+    branch: main
+    commit: 549da630ffe24b53be3983fcee3bb346
+    digest: b1-HVAvWKH61BF6TdZSbHRhrD2SUuC0V7uAlZgCRimGPLI=
+    create_time: 2021-08-09T14:24:57.923964Z
+  - remote: buf.build
+    owner: beta
+    repository: prometheus
+    branch: main
+    commit: a91b42d18a994cd4b07b37f365f87cf9
+    digest: b1-uKmv58fyoNwJI855qg7UEagfdyUl6XNPsFAdDoi57f4=
+    create_time: 2021-06-23T20:16:58.410272Z
+  - remote: buf.build
+    owner: beta
+    repository: protoc-gen-validate
+    branch: main
+    commit: 82388a0a0cb04e98a203f95dfed5e84b
+    digest: b1-lYgUMN58PxyCwvfQoopp40AJ-oHHjWXAzksF7v9U-U4=
+    create_time: 2021-06-21T22:00:30.152545Z
+  - remote: buf.build
+    owner: beta
+    repository: xds
+    branch: main
+    commit: 45f850b92541434cbde4aece01bc7d53
+    digest: b1-QZUL5DC6-nVgMMlajH_hlImwghg5HjRsqlEAOl0dZgI=
+    create_time: 2021-08-09T14:37:06.872899Z
+  - remote: buf.build
+    owner: gogo
+    repository: protobuf
+    branch: main
+    commit: 4df00b267f944190a229ce3695781e99
+    digest: b1-sjLgsg7CzrkOrIjBDh3s-l0aMjE6oqTj85-OsoopKAw=
+    create_time: 2021-08-10T00:14:28.345069Z
+  - remote: buf.build
+    owner: googleapis
+    repository: googleapis
+    branch: main
+    commit: d1a849b8f8304950832335723096e954
+    digest: b1-zJkwX0YeOp1Wa0Jaj_RqMLa2-oEzePH6PJEK8aaMeI4=
+    create_time: 2021-08-26T15:07:19.652533Z
diff --git a/...i_proto_breaking_change_detector/buf.yaml → api/buf.yaml b/...i_proto_breaking_change_detector/buf.yaml → api/buf.yaml
@@ -1,13 +1,19 @@
 version: v1beta1
+deps:
+    - buf.build/googleapis/googleapis
+    - buf.build/beta/opencensus
+    - buf.build/beta/prometheus
+    - buf.build/beta/opentelemetry
+    - buf.build/gogo/protobuf
+    - buf.build/beta/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
-
-    # needed for allowing removal/reserving of fields
     - FIELD_NO_DELETE_UNLESS_NUMBER_RESERVED
     - FIELD_NO_DELETE_UNLESS_NAME_RESERVED
diff --git a/bazel/api_binding.bzl b/bazel/api_binding.bzl
@@ -9,6 +9,7 @@ def _default_envoy_api_impl(ctx):
         "tools",
         "versioning",
         "contrib",
+        "buf.yaml",
     ]
     for d in api_dirs:
         ctx.symlink(ctx.path(ctx.attr.envoy_root).dirname.get_child(ctx.attr.reldir).get_child(d), d)

diff --git a/ci/do_ci.sh b/ci/do_ci.sh
@@ -377,6 +377,16 @@ elif [[ "$CI_TARGET" == "bazel.api" ]]; then
   # We use custom BAZEL_BUILD_OPTIONS here; the API booster isn't capable of working with libc++ yet.
   BAZEL_BUILD_OPTIONS="${BAZEL_BUILD_OPTIONS[*]}" python3.8 "${ENVOY_SRCDIR}"/tools/api_boost/api_boost_test.py
   exit 0
+elif [[ "$CI_TARGET" == "bazel.api_compat" ]]; then
+  echo "Building buf..."
+  bazel build @com_github_bufbuild_buf//:buf
+  BUF_PATH=$(realpath "bazel-source/external/com_github_bufbuild_buf/bin/buf")
+  echo "Checking API for breaking changes to protobuf backwards compatibility..."
+  BASE_BRANCH_REF=$("${ENVOY_SRCDIR}"/tools/git/last_github_commit.sh)
+  COMMIT_TITLE=$(git log -n 1 --pretty='format:%C(auto)%h (%s, %ad)' "${BASE_BRANCH_REF}")
+  echo -e "\tUsing base commit ${COMMIT_TITLE}"
+  "${ENVOY_SRCDIR}"/tools/api_proto_breaking_change_detector/detector_ci.sh "${BUF_PATH}" "${BASE_BRANCH_REF}"
+  exit 0
 elif [[ "$CI_TARGET" == "bazel.coverage" || "$CI_TARGET" == "bazel.fuzz_coverage" ]]; then
   setup_clang_toolchain
   echo "${CI_TARGET} build with tests ${COVERAGE_TEST_TARGETS[*]}"

diff --git a/generated_api_shadow/BUILD b/generated_api_shadow/BUILD
@@ -256,3 +256,11 @@ proto_library(
         ":v3_protos",
     ],
 )
+
+filegroup(
+    name = "proto_breaking_change_detector_buf_config",
+    srcs = [
+        "buf.yaml",
+    ],
+    visibility = ["//visibility:public"],
+)
diff --git a/generated_api_shadow/bazel/repository_locations.bzl b/generated_api_shadow/bazel/repository_locations.bzl
diff --git a/tools/api_proto_breaking_change_detector/BUILD b/tools/api_proto_breaking_change_detector/BUILD
@@ -1,35 +1,56 @@
-load("@rules_python//python:defs.bzl", "py_binary", "py_test")
+load("@rules_python//python:defs.bzl", "py_binary", "py_library", "py_test")
 
 licenses(["notice"])  # Apache 2
 
 py_binary(
-    name = "proto_breaking_change_detector",
-    srcs = ["detector.py"],
+    name = "detector",
+    srcs = [
+        "detector.py",
+    ],
     data = [
-        ":buf.lock",
-        ":buf.yaml",
         "@com_github_bufbuild_buf//:buf",
+        "@envoy_api_canonical//:proto_breaking_change_detector_buf_config",
     ],
     main = "detector.py",
     tags = ["manual"],
     deps = [
+        ":buf_utils",
+        ":detector_errors",
         "//tools:run_command",
     ],
 )
 
+py_library(
+    name = "buf_utils",
+    srcs = [
+        "buf_utils.py",
+    ],
+    deps = [
+        ":detector_errors",
+        "//tools/base:utils",
+    ],
+)
+
+py_library(
+    name = "detector_errors",
+    srcs = [
+        "detector_errors.py",
+    ],
+)
+
 py_test(
-    name = "proto_breaking_change_detector_test",
+    name = "detector_test",
     srcs = ["detector_test.py"],
     data = [
         "//tools/testdata/api_proto_breaking_change_detector:proto_breaking_change_detector_testdata",
-        "@com_github_bufbuild_buf//:buf",
     ],
     main = "detector_test.py",
     python_version = "PY3",
     srcs_version = "PY3",
     tags = ["manual"],
     deps = [
-        ":proto_breaking_change_detector",
+        ":detector",
+        "//tools:run_command",
         "@rules_python//python/runfiles",
     ],
 )
diff --git a/tools/api_proto_breaking_change_detector/buf.lock b/tools/api_proto_breaking_change_detector/buf.lock
diff --git a/tools/api_proto_breaking_change_detector/buf_utils.py b/tools/api_proto_breaking_change_detector/buf_utils.py
@@ -0,0 +1,105 @@
+from pathlib import Path
+from typing import List, Union, Tuple
+
+from detector_errors import ChangeDetectorError, ChangeDetectorInitializeError
+from tools.base.utils import cd_and_return
+from tools.run_command import run_command
+
+
+def _generate_buf_args(target_path, config_file_loc, additional_args):
+    buf_args = []
+
+    # buf requires relative pathing for roots
+    target_relative = Path(target_path).absolute().relative_to(Path.cwd().absolute())
+
+    # buf does not accept . as a root; if we are already in the target dir, no need for a --path arg
+    if str(target_relative) != ".":
+        buf_args.extend(["--path", str(target_relative)])
+
+    if config_file_loc:
+        buf_args.extend(["--config", str(config_file_loc)])
+
+    buf_args.extend(additional_args or [])
+
+    return buf_args
+
+
+def _cd_into_config_parent(config_file_loc):
+    config_parent = Path(config_file_loc).parent if config_file_loc else Path.cwd()
+    return cd_and_return(config_parent)
+
+
+def pull_buf_deps(
+        buf_path: Union[str, Path],
+        target_path: Union[str, Path],
+        config_file_loc: Union[str, Path] = None,
+        additional_args: List[str] = None) -> None:
+    """Updates buf.lock file and downloads any BSR dependencies specified in buf.yaml
+
+    Note that in order for dependency downloading to trigger, `buf build`
+    must be invoked, so `target_path` must contain valid proto syntax.
+
+    Args:
+        buf_path {Union[str, Path]} -- path to buf binary to use
+        target_path {Union[str, Path]} -- path to directory containing protos to run buf on
+
+        config_file_loc {Union[str, Path]} -- absolute path to buf.yaml configuration file (if not provided, uses default buf configuration)
+        additional_args {List[str]} -- additional arguments passed into the buf binary invocations
+
+    Raises:
+        ChangeDetectorInitializeError: if buf encounters an error while attempting to update the buf.lock file or build afterward
+    """
+    with _cd_into_config_parent(config_file_loc):
+        buf_args = _generate_buf_args(target_path, config_file_loc, additional_args)
+
+        update_code, _, update_err = run_command(f'{buf_path} mod update')
+        # for some reason buf prints out the "downloading..." lines on stderr
+        if update_code != 0:
+            raise ChangeDetectorInitializeError(
+                f"Error running `buf mod update`: exit status code {update_code} | stderr: {''.join(update_err)}"
+            )
+        if not Path.cwd().joinpath("buf.lock").exists():
+            raise ChangeDetectorInitializeError(
+                "buf mod update did not generate a buf.lock file (silent error... incorrect config?)"
+            )
+
+        run_command(' '.join([f'{buf_path} build', *buf_args]))
+
+
+def check_breaking(
+        buf_path: Union[str, Path],
+        target_path: Union[str, Path],
+        git_ref: str,
+        git_path: Union[str, Path],
+        config_file_loc: Union[str, Path] = None,
+        additional_args: List[str] = None,
+        subdir: str = None) -> Tuple[int, List[str], List[str]]:
+    """Runs `buf breaking` to check for breaking changes between the `target_path` protos and the provided initial state
+
+    Args:
+        buf_path {Union[str, Path]} -- path to buf binary to use
+        target_path {Union[str, Path]} -- path to directory containing protos to check for breaking changes
+        git_ref {str} -- git reference to use for the initial state of the protos (typically a commit hash)
+        git_path {Union[str, Path]} -- absolute path to .git folder for the repository of interest
+
+        subdir {str} -- subdirectory within git repository from which to search for .proto files (default: None, e.g. stay in root)
+        config_file_loc {Union[str, Path]} -- absolute path to buf.yaml configuration file (if not provided, uses default buf configuration)
+        additional_args {List[str]} -- additional arguments passed into the buf binary invocations
+
+    Returns:
+        Tuple[int, List[str], List[str]] -- tuple of (exit status code, stdout, stderr) as provided by run_command. Note stdout/stderr are provided as string lists
+    """
+    with _cd_into_config_parent(config_file_loc):
+        if not Path(git_path).exists():
+            raise ChangeDetectorError(f'path to .git folder {git_path} does not exist')
+
+        buf_args = _generate_buf_args(target_path, config_file_loc, additional_args)
+
+        initial_state_input = f'{git_path}#ref={git_ref}'
+
+        if subdir:
+            initial_state_input += f',subdir={subdir}'
+
+        final_code, final_out, final_err = run_command(
+            ' '.join([buf_path, f"breaking --against {initial_state_input}", *buf_args]))
+        return final_code, final_out, final_err