tooling: Add protolock bazel dependency and tests by YaseenAlk · Pull Request #17375 · envoyproxy/envoy

YaseenAlk · 2021-07-16T00:13:22Z

Signed-off-by: Yaseen Alkhafaji yaseena@google.com

Commit Message: tooling: Add protolock bazel dependency and protolock tests

Additional Description:

protolock is a tool that enforces backwards compatibility of APIs by detecting breaking changes of .proto files. It does so by parsing .proto files and saving the state of the protobufs in a proto.lock file. Between invocations of protolock, the lock file is compared to the current state, and the diff is evaluated against a set of rules. If any rules are violated, then protolock raises an error.

This PR introduces protolock as an external bazel dependency, and also adds some tests that evaluate protolock against some simple protobuf changes, categorized either as "breaking" (e.g. changes that require incrementing the envoy API version) and "allowed" (e.g. changes that can be introduced in the same API version).

protolock is intended to be run against the protobufs in envoy/api/* to detect breaking API changes. In the coming weeks, I will be:

adding tooling to allow protolock to be run arbitrarily
extending protolock by adding more envoy-specific rules, for example breaking changes to PGV annotations.

I have been using the backwards compatibility rules in the API_VERSIONING doc as a model for rules to introduce. The rules list is still a WIP, so if there are additional rules that are desired, please do reach out!

Risk Level: Low
Testing: Tests that evaluate protolock against "allowed" and "breaking" protobuf API changes. Currently a few tests are skipped - these tests will be enabled in future PRs as I modify protolock to evaluate PGV and some other rules it does not currently address

References #3368 and maybe #6271

Docs Changes: -
Release Notes: -
Platform Specific Features: -

Signed-off-by: Yaseen Alkhafaji <yaseena@google.com>

repokitteh-read-only · 2021-07-16T00:13:25Z

Hi @YaseenAlk, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #17375 was opened by YaseenAlk.

see: more, trace.

repokitteh-read-only · 2021-07-16T00:13:28Z

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: #17375 was opened by YaseenAlk.

see: more, trace.

YaseenAlk · 2021-07-16T00:15:45Z

Not sure if I'm able to add reviewers but I know @adisuissa @akonradi and @htuch are interested

moderation · 2021-07-16T15:21:51Z

bazel/repository_locations.bzl

+    com_github_nilslice_protolock = dict(
+        project_name = "protolock",
+        project_desc = "Track your .proto files and prevent changes to messages and services which impact API compatibility.",
+        project_url = "https://github.com/nilslice/protolock/",
+        version = "0.15.2",
+        sha256 = "26d0b491471866c4959a75a1418a577eab432782b9923fdf022535bb7c322374",
+        urls = ["https://github.com/nilslice/protolock/archive/refs/tags/v{version}.tar.gz"],
+        release_date = "2021-02-18",
+        use_category = ["api"],
+        strip_prefix = "protolock-{version}",
+    ),


scorecard --repo=https://github.com/nilslice/protolock

RESULTS ------- Repo: github.com/nilslice/protolock |--------|------------|-----------------------------| | STATUS | CONFIDENCE | NAME | |--------|------------|-----------------------------| | Pass | 10 | Binary-Artifacts | |--------|------------|-----------------------------| | Pass | 10 | Code-Review | |--------|------------|-----------------------------| | Pass | 10 | Contributors | |--------|------------|-----------------------------| | Pass | 8 | Pull-Requests | |--------|------------|-----------------------------| | Pass | 10 | Token-Permissions | |--------|------------|-----------------------------| | Pass | 10 | Vulnerabilities | |--------|------------|-----------------------------| | Fail | 10 | Active | |--------|------------|-----------------------------| | Fail | 3 | Automatic-Dependency-Update | |--------|------------|-----------------------------| | Fail | 0 | Branch-Protection | |--------|------------|-----------------------------| | Fail | 5 | CI-Tests | |--------|------------|-----------------------------| | Fail | 10 | CII-Best-Practices | |--------|------------|-----------------------------| | Fail | 10 | Frozen-Deps | |--------|------------|-----------------------------| | Fail | 10 | Fuzzing | |--------|------------|-----------------------------| | Fail | 0 | Packaging | |--------|------------|-----------------------------| | Fail | 10 | SAST | |--------|------------|-----------------------------| | Fail | 10 | Security-Policy | |--------|------------|-----------------------------| | Fail | 10 | Signed-Releases | |--------|------------|-----------------------------| | Fail | 10 | Signed-Tags | |--------|------------|-----------------------------|

Since this is really primarily a test/dev tool I'm willing to give a pass on most of the FAILs I think. @YaseenAlk what do you think of Protolock maintenance story? I'm not seeing a lot of activity in past year+ and CI is broken. Would we consider forking if this isn't actively maintained?

@YaseenAlk also looked at buf which is more actively maintained.
The 2 downsides, IIRC, are that buf also attempts to address more protobuf related operations, and it might require more work to add/modify rules (compared to protolock's plugins).
One thing to note is that buf's breaking changes rules seem to be much more comprehensive than protolock's.

Yeah, buf looks a lot more active / maintained. How much harder is it in reality to do stuff in buf?

I can get direct help from the buf team if needed. Do you want to talk to them about our needs? I think they would be excited to work with us.

Out-of-the-box, buf seems to be targeting many of the issues we would like to guard against.
I guess the open question is how hard will it be to add PGV rules to buf. @YaseenAlk, we can discuss this tomorrow.

We actually have a migration doc concerning protolock https://docs.buf.build/migration-protolock including a pros/cons list. You likely want to use buf. Happy to chat!

Follow up PR that uses buf instead: #17515

moderation · 2021-07-16T15:22:50Z

bazel/repository_locations.bzl

+        project_name = "proto",
+        project_desc = "parser for Google ProtocolBuffers definition",
+        project_url = "https://github.com/emicklei/proto/",
+        version = "1.7.0",


Latest is 1.9.0 - https://github.com/emicklei/proto/releases/tag/v1.9.0

moderation · 2021-07-16T15:24:52Z

bazel/repository_locations.bzl

+    com_github_emicklei_proto = dict(
+        project_name = "proto",
+        project_desc = "parser for Google ProtocolBuffers definition",
+        project_url = "https://github.com/emicklei/proto/",
+        version = "1.7.0",
+        sha256 = "e93272fea9e4f993b9d160440bf980d015970147907090834492771bb1c4510c",
+        urls = ["https://github.com/emicklei/proto/archive/refs/tags/v{version}.tar.gz"],
+        release_date = "2019-10-05",
+        use_category = ["api"],
+        strip_prefix = "proto-{version}",
+    ),


scorecard --repo=https://github.com/emicklei/proto

RESULTS ------- Repo: github.com/emicklei/proto |--------|------------|-----------------------------| | STATUS | CONFIDENCE | NAME | |--------|------------|-----------------------------| | Pass | 10 | Binary-Artifacts | |--------|------------|-----------------------------| | Pass | 10 | CI-Tests | |--------|------------|-----------------------------| | Pass | 10 | Code-Review | |--------|------------|-----------------------------| | Pass | 10 | Contributors | |--------|------------|-----------------------------| | Pass | 10 | Frozen-Deps | |--------|------------|-----------------------------| | Pass | 10 | Token-Permissions | |--------|------------|-----------------------------| | Pass | 10 | Vulnerabilities | |--------|------------|-----------------------------| | Fail | 10 | Active | |--------|------------|-----------------------------| | Fail | 3 | Automatic-Dependency-Update | |--------|------------|-----------------------------| | Fail | 10 | Branch-Protection | |--------|------------|-----------------------------| | Fail | 10 | CII-Best-Practices | |--------|------------|-----------------------------| | Fail | 10 | Fuzzing | |--------|------------|-----------------------------| | Fail | 0 | Packaging | |--------|------------|-----------------------------| | Fail | 3 | Pull-Requests | |--------|------------|-----------------------------| | Fail | 10 | SAST | |--------|------------|-----------------------------| | Fail | 10 | Security-Policy | |--------|------------|-----------------------------| | Fail | 0 | Signed-Releases | |--------|------------|-----------------------------| | Fail | 10 | Signed-Tags | |--------|------------|-----------------------------|

If we were to fork Protolock, I'd ask "why can't we rely on the standard protoc plugin FileDescriptorProto for the AST parse tree here?".

adisuissa

Thanks for working on this! Looks like a very good start, and I've got a few comments.

I think we should introduce a new python class/script that wraps the call to protolock, which will store the entire logic of calling protolock. The test function will use it to run protolock, and this can be also called by CI later on. By doing so we are making sure that both the test and the non-test uses of protolock have the same behavior.

adisuissa · 2021-07-16T15:03:58Z

tools/api_protolock/BUILD

+    name = "protolock_test",
+    srcs = ["protolock_test.py"],
+    data = [
+        "//tools/testdata/api_protolock:allowed/test_add_comment_current",


It may be possible to have this list of testdata files in a build file in the testdata directory, and just reference that.

tools/api_protolock/protolock_test.py

adisuissa · 2021-07-16T15:20:29Z

tools/api_protolock/protolock_test.py

+    def test_change_field_name(self):
+        self.run_protolock_test("breaking", self.test_change_field_name.__name__)
+
+    @unittest.skip("package name detection not yet added to Protolock (simply requires plugin)")


Please add a comment with tracking issue link

tools/api_protolock/protolock_test.py

htuch · 2021-07-19T02:05:20Z

tools/api_protolock/BUILD

+    data = [
+        "//tools/testdata/api_protolock:allowed/test_add_comment_current",
+        "//tools/testdata/api_protolock:allowed/test_add_comment_next",
+        "//tools/testdata/api_protolock:allowed/test_add_enum_value_current",


Out of curiosity, how come some equivalent of these tests doesn't exist in Protolock upstream? Would these belong there at some point?

htuch · 2021-08-05T03:34:11Z

@YaseenAlk should we close this out given the move to buf?
/wait

YaseenAlk · 2021-08-05T17:08:44Z

@YaseenAlk should we close this out given the move to buf?
/wait

Yes, closing in favor of #17515 which uses buf

Follow-up to #17375 where it was agreed that protolock is not actively maintained enough to depend on. This PR "migrates" the tests from that PR to use buf instead, and also cleans some of the code per a few of the review comments. Still a few outstanding points: - buf build on the envoy/api folder requires several protobuf dependencies such as udpa to be available to buf to consume. Suggested solution by buf is to point buf's config to necessary BSR modules that the buf team is hosting. - These lines are commented out in this PR as I had some trouble automating it for the tests, and it is not necessary for the tests to pass - May introduce issues if buf is not pointing to the same version of modules that bazel builds for envoy. May need to introduce some way to couple them, or (ideally) find a way to run the breaking change detector without building the dependencies - Currently bazel is using a binary release of buf (for linux). Goal is to move to building it from source in the near future - It may be in our interest to expand the list of API-breaking-change rules (buf provides an extensive list of rules we could adopt) Risk Level: Low Testing: Tests that evaluate buf against "allowed" and "breaking" protobuf API changes. Currently 4 tests are skipped - 3 of them are PGV-related (we need to communicate our desired PGV rules to the buf team so they may add them in the near future). The 4th is a test I had originally written to evaluate protolock but may not apply to buf ("forcing" a breaking change) - refer to comments Docs Changes: Release Notes: Platform Specific Features: buf binary imported by bazel is linux-only. Hopefully the ["manual"] tags attribute prevents any issues for non-linux users Signed-off-by: Yaseen Alkhafaji <yaseena@google.com>

Add protolock dependency and tests to envoy

197bae5

Signed-off-by: Yaseen Alkhafaji <yaseena@google.com>

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Jul 16, 2021

snowp assigned htuch Jul 16, 2021

moderation reviewed Jul 16, 2021

View reviewed changes

adisuissa requested changes Jul 16, 2021

View reviewed changes

htuch assigned adisuissa Jul 19, 2021

htuch reviewed Jul 19, 2021

View reviewed changes

YaseenAlk mentioned this pull request Jul 28, 2021

tooling: Add buf bazel dependency and tests to evaluate it #17515

Merged

repokitteh-read-only bot added the waiting label Aug 5, 2021

YaseenAlk closed this Aug 5, 2021

Conversation

YaseenAlk commented Jul 16, 2021

Uh oh!

repokitteh-read-only bot commented Jul 16, 2021

Uh oh!

repokitteh-read-only bot commented Jul 16, 2021

Uh oh!

YaseenAlk commented Jul 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Aug 5, 2021

Uh oh!

YaseenAlk commented Aug 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

htuch Jul 19, 2021 •

edited

Loading