Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions bazel/external/api_protolock.BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")

licenses(["notice"]) # Apache 2

go_binary(
name = "protolock",
srcs = [
"cmd/protolock/main.go",
"cmd/protolock/plugins.go",
],
visibility = ["//visibility:public"],
deps = [
":protolock_extend",
":protolock_root",
],
)

go_library(
name = "protolock_extend",
srcs = [
"extend/plugin.go",
],
importpath = "github.com/nilslice/protolock/extend",
deps = [
":protolock_root",
],
)

go_library(
name = "protolock_root",
srcs = [
"commit.go",
"config.go",
"hints.go",
"init.go",
"parse.go",
"protopath.go",
"report.go",
"rules.go",
"status.go",
"uptodate.go",
],
importpath = "github.com/nilslice/protolock",
deps = [
"@com_github_emicklei_proto//:proto_parsing_lib",
],
)
29 changes: 29 additions & 0 deletions bazel/external/api_protolock_parser.BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
load("@io_bazel_rules_go//go:def.bzl", "go_library")

go_library(
name = "proto_parsing_lib",
srcs = [
"comment.go",
"enum.go",
"extensions.go",
"field.go",
"group.go",
"import.go",
"message.go",
"oneof.go",
"option.go",
"package.go",
"parent_accessor.go",
"parser.go",
"proto.go",
"range.go",
"reserved.go",
"service.go",
"syntax.go",
"token.go",
"visitor.go",
"walk.go",
],
importpath = "github.com/emicklei/proto",
visibility = ["//visibility:public"],
)
18 changes: 18 additions & 0 deletions bazel/repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,8 @@ def envoy_dependencies(skip_targets = []):
_com_github_luajit_luajit()
_com_github_moonjit_moonjit()
_com_github_nghttp2_nghttp2()
_com_github_nilslice_protolock()
_com_github_emicklei_proto()
_com_github_skyapm_cpp2sky()
_com_github_nodejs_http_parser()
_com_github_alibaba_hessian2_codec()
Expand Down Expand Up @@ -505,6 +507,22 @@ def _com_github_ncopa_suexec():
actual = "@com_github_ncopa_suexec//:su-exec",
)

def _com_github_nilslice_protolock():
external_http_archive(
name = "com_github_nilslice_protolock",
build_file = "@envoy//bazel/external:api_protolock.BUILD",
)
native.bind(
name = "protolock",
actual = "@com_github_nilslice_protolock//:protolock",
)

def _com_github_emicklei_proto():
external_http_archive(
name = "com_github_emicklei_proto",
build_file = "@envoy//bazel/external:api_protolock_parser.BUILD",
)

def _com_google_googletest():
external_http_archive("com_google_googletest")
native.bind(
Expand Down
22 changes: 22 additions & 0 deletions bazel/repository_locations.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,28 @@ REPOSITORY_LOCATIONS_SPEC = dict(
release_date = "2020-11-23",
cpe = "cpe:2.3:a:nghttp2:nghttp2:*",
),
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}",
),
Comment on lines +290 to +300
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@htuch htuch Jul 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow up PR that uses buf instead: #17515

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",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}",
),
Comment on lines +301 to +311
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?".

io_opentracing_cpp = dict(
project_name = "OpenTracing",
project_desc = "Vendor-neutral APIs and instrumentation for distributed tracing",
Expand Down
47 changes: 47 additions & 0 deletions tools/api_protolock/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
load("@rules_python//python:defs.bzl", "py_test")

licenses(["notice"]) # Apache 2

py_test(
name = "protolock_test",
srcs = ["protolock_test.py"],
data = [
"//tools/testdata/api_protolock:allowed/test_add_comment_current",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"//tools/testdata/api_protolock:allowed/test_add_comment_next",
"//tools/testdata/api_protolock:allowed/test_add_enum_value_current",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"//tools/testdata/api_protolock:allowed/test_add_enum_value_next",
"//tools/testdata/api_protolock:allowed/test_add_field_current",
"//tools/testdata/api_protolock:allowed/test_add_field_next",
"//tools/testdata/api_protolock:allowed/test_add_option_current",
"//tools/testdata/api_protolock:allowed/test_add_option_next",
"//tools/testdata/api_protolock:allowed/test_force_breaking_change_current",
"//tools/testdata/api_protolock:allowed/test_force_breaking_change_next",
"//tools/testdata/api_protolock:allowed/test_remove_and_reserve_field_current",
"//tools/testdata/api_protolock:allowed/test_remove_and_reserve_field_next",
"//tools/testdata/api_protolock:breaking/test_change_field_from_oneof_current",
"//tools/testdata/api_protolock:breaking/test_change_field_from_oneof_next",
"//tools/testdata/api_protolock:breaking/test_change_field_id_current",
"//tools/testdata/api_protolock:breaking/test_change_field_id_next",
"//tools/testdata/api_protolock:breaking/test_change_field_name_current",
"//tools/testdata/api_protolock:breaking/test_change_field_name_next",
"//tools/testdata/api_protolock:breaking/test_change_field_plurality_current",
"//tools/testdata/api_protolock:breaking/test_change_field_plurality_next",
"//tools/testdata/api_protolock:breaking/test_change_field_to_oneof_current",
"//tools/testdata/api_protolock:breaking/test_change_field_to_oneof_next",
"//tools/testdata/api_protolock:breaking/test_change_field_type_current",
"//tools/testdata/api_protolock:breaking/test_change_field_type_next",
"//tools/testdata/api_protolock:breaking/test_change_package_name_current",
"//tools/testdata/api_protolock:breaking/test_change_package_name_next",
"//tools/testdata/api_protolock:breaking/test_change_pgv_field_current",
"//tools/testdata/api_protolock:breaking/test_change_pgv_field_next",
"//tools/testdata/api_protolock:breaking/test_change_pgv_message_current",
"//tools/testdata/api_protolock:breaking/test_change_pgv_message_next",
"//tools/testdata/api_protolock:breaking/test_change_pgv_oneof_current",
"//tools/testdata/api_protolock:breaking/test_change_pgv_oneof_next",
"@com_github_nilslice_protolock//:protolock",
],
python_version = "PY3",
srcs_version = "PY3",
visibility = ["//visibility:public"],
deps = ["@rules_python//python/runfiles"],
)
149 changes: 149 additions & 0 deletions tools/api_protolock/protolock_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
import os
import unittest
import tempfile
from shutil import copyfile
import io
import subprocess
from rules_python.python.runfiles import runfiles

DEFAULT_PROTOLOCK_ARGS = ["--plugins="]


class ProtolockTests(unittest.TestCase):

def setUp(self):
# create a temporary directory to house the proto.lock file and current proto file
self.temp_dir = tempfile.TemporaryDirectory()
self.protolock_path = runfiles.Create().Rlocation(
"com_github_nilslice_protolock/protolock_/protolock")

def tearDown(self):
self.temp_dir.cleanup()

def run_protolock_test(self, testtype, testname, additional_args=None, expect_no_changes=False):
# there's probably a better/safer way to navigate
# but it seems like check_spelling_pedantic_test.py takes this approach as well
tests_path = os.path.join(
os.path.dirname(os.path.realpath(__file__)), "..", "testdata", "api_protolock",
testtype)
Comment thread
YaseenAlk marked this conversation as resolved.
protolock_args = DEFAULT_PROTOLOCK_ARGS + [
f"--protoroot={self.temp_dir.name}", f"--lockdir={self.temp_dir.name}"
]
if additional_args is not None:
protolock_args.extend(additional_args)

# check that test files exist
current = os.path.join(tests_path, f"{testname}_current")
changed = os.path.join(tests_path, f"{testname}_next")
self.assertTrue(os.path.isfile(current))
self.assertTrue(os.path.isfile(changed))

# 1) copy start file into temp dir
# 2) protolock init
# 3) copy changed file into temp dir
# 4) protolock commit
# 5) check for differences (if changes are breaking, there should be none)

target = os.path.join(self.temp_dir.name, f"{testname}.proto")
copyfile(current, target)

# TODO: change this to use tools/run_command.py?
Comment thread
YaseenAlk marked this conversation as resolved.
initial_result = subprocess.run([self.protolock_path, "init", *protolock_args],
capture_output=True)

self.assertEqual(len(initial_result.stdout), 0)
self.assertEqual(len(initial_result.stderr), 0)

lock_location = os.path.join(self.temp_dir.name, "proto.lock")
with open(lock_location) as f:
initial_lock = f.readlines()

copyfile(changed, target)

final_result = subprocess.run([self.protolock_path, "commit", *protolock_args],
capture_output=True)
with open(lock_location) as f:
post_lock = f.readlines()

if testtype == "breaking":
Comment thread
YaseenAlk marked this conversation as resolved.
self.assertGreater(len(final_result.stdout), 0)
self.assertRegex(str(final_result.stdout), r"CONFLICT")

self.assertListEqual(initial_lock, post_lock)
elif testtype == "allowed":
self.assertEqual(len(final_result.stdout), 0)
self.assertEqual(len(final_result.stderr), 0)

if not expect_no_changes:
self.assertGreater(len(post_lock), 0)
self.assertNotEqual(initial_lock, post_lock)
Comment thread
YaseenAlk marked this conversation as resolved.
else:
raise ValueError(f"Unknown test type: {testtype}")


class TestBreakingChanges(ProtolockTests):

def test_change_field_id(self):
self.run_protolock_test("breaking", self.test_change_field_id.__name__)

def test_change_field_type(self):
self.run_protolock_test("breaking", self.test_change_field_type.__name__)

def test_change_field_plurality(self):
self.run_protolock_test("breaking", self.test_change_field_plurality.__name__)

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)")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment with tracking issue link

def test_change_package_name(self):
self.run_protolock_test("breaking", self.test_change_package_name.__name__)

@unittest.skip(
"oneof support not yet added to Protolock (requires Protolock changes AND plugin)")
def test_change_field_from_oneof(self):
self.run_protolock_test("breaking", self.test_change_field_from_oneof.__name__)

@unittest.skip(
"oneof support not yet added to Protolock (requires Protolock changes AND plugin)")
def test_change_field_to_oneof(self):
self.run_protolock_test("breaking", self.test_change_field_to_oneof.__name__)

@unittest.skip("PGV field support not yet added to Protolock (simply requires plugin)")
def test_change_pgv_field(self):
self.run_protolock_test("breaking", self.test_change_pgv_field.__name__)

@unittest.skip("PGV message option support not yet added to Protolock (simply requires plugin)")
def test_change_pgv_message(self):
self.run_protolock_test("breaking", self.test_change_pgv_message.__name__)

@unittest.skip(
"PGV oneof option support not yet added to Protolock (requires Protolock changes AND plugin)"
)
def test_change_pgv_oneof(self):
self.run_protolock_test("breaking", self.test_change_pgv_oneof.__name__)


class TestAllowedChanges(ProtolockTests):

def test_add_comment(self):
self.run_protolock_test("allowed", self.test_add_comment.__name__, expect_no_changes=True)

def test_add_field(self):
self.run_protolock_test("allowed", self.test_add_field.__name__)

def test_add_option(self):
self.run_protolock_test("allowed", self.test_add_option.__name__)

def test_add_enum_value(self):
self.run_protolock_test("allowed", self.test_add_enum_value.__name__)

def test_remove_and_reserve_field(self):
self.run_protolock_test("allowed", self.test_remove_and_reserve_field.__name__)

def test_force_breaking_change(self):
self.run_protolock_test("allowed", self.test_force_breaking_change.__name__, ["--force"])


if __name__ == '__main__':
unittest.main()
36 changes: 36 additions & 0 deletions tools/testdata/api_protolock/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
licenses(["notice"]) # Apache 2

exports_files([
"allowed/test_add_comment_current",
"allowed/test_add_enum_value_next",
"allowed/test_add_option_current",
"allowed/test_force_breaking_change_next",
"allowed/test_add_comment_next",
"allowed/test_add_field_current",
"allowed/test_add_option_next",
"allowed/test_remove_and_reserve_field_current",
"allowed/test_add_enum_value_current",
"allowed/test_add_field_next",
"allowed/test_force_breaking_change_current",
"allowed/test_remove_and_reserve_field_next",
"breaking/test_change_field_from_oneof_current",
"breaking/test_change_field_name_next",
"breaking/test_change_field_type_current",
"breaking/test_change_pgv_field_next",
"breaking/test_change_field_from_oneof_next",
"breaking/test_change_field_plurality_current",
"breaking/test_change_field_type_next",
"breaking/test_change_pgv_message_current",
"breaking/test_change_field_id_current",
"breaking/test_change_field_plurality_next",
"breaking/test_change_package_name_current",
"breaking/test_change_pgv_message_next",
"breaking/test_change_field_id_next",
"breaking/test_change_field_to_oneof_current",
"breaking/test_change_package_name_next",
"breaking/test_change_pgv_oneof_current",
"breaking/test_change_field_name_current",
"breaking/test_change_field_to_oneof_next",
"breaking/test_change_pgv_field_current",
"breaking/test_change_pgv_oneof_next",
])
10 changes: 10 additions & 0 deletions tools/testdata/api_protolock/allowed/test_add_comment_current
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
syntax = "proto3";

package envoy.protolock.testdata;

option java_package = "io.envoyproxy.envoy.protolock";
option java_outer_classname = "EnvoyProtolock";
option java_multiple_files = true;

message CommonExtensionConfig {
}
11 changes: 11 additions & 0 deletions tools/testdata/api_protolock/allowed/test_add_comment_next
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
syntax = "proto3";

package envoy.protolock.testdata;

option java_package = "io.envoyproxy.envoy.protolock";
option java_outer_classname = "EnvoyProtolock";
option java_multiple_files = true;

// Common configuration for all tap extensions.
message CommonExtensionConfig {
}
Loading