Move envoy.api.v2 core protos to envoy.api.v2.core and organize filters by kyessenov · Pull Request #452 · envoyproxy/data-plane-api

kyessenov · 2018-01-31T19:13:31Z

There are several main changes in this PR:

Create envoy.api.v2.core packages to break circular dependencies from xDS on to subpackages on to base protos.
Create individual packages for each filter and add independent versioning to each filter.
Add visibility constraints to prevent formation of dependency cycles.
Add gogoproto annotations to improve go code generation.

After moving xDS service definitions and top-level resource protos back to envoy.core.api.v2, cycles were created, since the second-level definitions depend on base protobuf definitions, and are in turn included from xDS; however xDS and base definitions are in the same package.

The solution is to split the base protos into another package, envoy.api.v2.core. That eliminates dependency cycles (validated using go-control-plane).

Added a few gogoproto annotations to improve golang code generation.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2018-01-31T19:35:24Z

cc @htuch

htuch

This seems reasonable (although a bit more verbose). What guarantee do we have that this will fix all our dependency cycle problems for good? Churn == bad :)

kyessenov · 2018-01-31T20:07:06Z

We need to enforce that only the following dependencies are allowed:

envoy/service -> envoy/api
envoy/config -> envoy/api
envoy/api/v2 -> envoy/api/v2/*
envoy/api/v2/auth -> envoy/api/v2/core
envoy/api/v2/[endpoint,cluster,route,listener] -> envoy/api/v2/[core,auth]

htuch · 2018-01-31T21:12:31Z

Do you know if it there's a cheap way to write a shell script test using Bazel query to validate this (or something like that)? Or even a simple Go equivalent of https://github.com/envoyproxy/data-plane-api/blob/master/test/build/build_test.cc?

kyessenov · 2018-01-31T21:19:31Z

I've tried to add proto_library to sort of enforce that, but the problem is that dependencies do not transfer to them. E.g. once you depend on base.proto, the dependency on base.proto package does not show up in bazel.

kyessenov · 2018-01-31T21:20:49Z

cc @lizan @douglas-reid for bazel expertise. Maybe you have some ideas?

Signed-off-by: Kuat Yessenov <kuat@google.com>

lizan · 2018-01-31T21:28:28Z

Use visibility?

kyessenov · 2018-01-31T21:30:22Z

@htuch for visibility to work, we would need to change deps to use proto packages instead of individual protos. Is there a reason why the current approach of listing individual protos was chosen?

htuch · 2018-01-31T21:49:28Z

@kyessenov Seems fair enough. I would still think a Go litmus test would be nice, but not if it's too much overhead.

kyessenov · 2018-01-31T21:51:25Z

I think we should improve the current go rules (cc @misterwilliam). If only we place protos into packages with these rules, we can automatically enforce package boundaries.

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch

A couple of comments (And thanks for taking this on and keeping it moving; the APIs will be much better positioned as a result).

htuch · 2018-02-01T03:13:43Z

  //   Headers can be specified using *response_headers_to_add* in
  //   :ref:`envoy_api_msg_RouteConfiguration`.
-  DataSource body = 2;
+  core.DataSource body = 2;


Should we fully qualify everywhere to avoid breakage if there is future movement (which hopefully won't happen, but ¯\_(ツ)_/¯).

My heuristic was to qualify names outside of core packages and subpackages. I'm thinking that if we create "v3" then we might want to copy over all subpackages of "v2" to start from, and the relative names there would help. Filters I'm still planning to move away, and I tried to use fully qualified names to enable that.

htuch · 2018-02-01T03:15:45Z

+    deps = [
+        ":base",
+        ":grpc_service",
+    ],


Are you still planning on adding visibility restrictions? I'd like us to do something to reduce the likelihood of future churn happening again; it can be visibility, a Go build test or even some good local build validation that we can add to the PR that can assure us we won't churn further.

I can add visibility, but that requires that dependencies must be proto packages, not individual protos. Go rules is harder to achieve, that amounts to writing all go rules again, and that was hard before and is still (given that @misterwilliam did only partial subset of protos).

PTAL, added "friends" package groups for visibility constraints.

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch · 2018-02-01T16:17:27Z

+    name = "friends",
+    packages = [
+        "-//envoy/api/v2/core",
+        "//envoy/...",


Does it need to be this wide? Would prefer narrower scoping.

Restricted it to //docs as much as possible.

htuch · 2018-02-01T16:18:52Z

 api_proto_library(
    name = "circuit_breaker",
    srcs = ["circuit_breaker.proto"],
+    visibility = ["//envoy/api/v2:friends"],


I wonder if we can make some of these scopes narrowed.

These two got further restricted to just //envoy/api/v2

htuch · 2018-02-01T16:20:11Z

package_group is nice; wondering if we can lock down visibility further than what is currently there, it doesn't seem to fully capture the restrictions discussed at a quick glance (should we write them down somewhere?).

lizan · 2018-02-01T20:43:30Z

-  Metadata metadata = 5;
+  core.Metadata metadata = 5;

  // [#not-implemented-hide:] See base.TransportSocket description.


Can you update the doc to match what Cluster has? Also remove not-implement-hide. Thanks!

? I don't see this is updated.

Oh, I thought you are referring to Metadata.

Can you give more details what needs to go here?

Just like clusters

See :ref:`base.TransportSocket<envoy_api_msg_core.TransportSocket>` description.

Is good, no longer need [#not-implemented-hide:]

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2018-02-01T23:40:05Z

@htuch I tried to restrict visibility further. Let me know if this is OK. The main idea is to prevent cycles within core, as well as prevent dependencies of core on other things. Both are accomplished with visibility.

I also took a stab at organizing filters into their own packages together with versions.

PiotrSikora · 2018-02-02T01:15:10Z

 # TODO(htuch): Automatically generate go_proto_library and go_grpc_library
 # from api_proto_library.
-def api_proto_library(name, srcs = [], deps = [], has_services = 0, require_py = 1):
+def api_proto_library(name, visibility, srcs = [], deps = [], has_services = 0, require_py = 1):


Should this default to "//visibility:public" for backward-compatibility?

No strong opinion. I found it useful for myself to make sure I didn't miss any.

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch

The new approach and layout looks good. Can you update the README.mds across the tree to express the visibility rules in plain English for readers? I'll need to take another pass after reading those and patching this into a local client so I can grep out the visibility rules.

@wora can you take another look?

htuch · 2018-02-02T04:51:08Z

 # TODO(htuch): Automatically generate go_proto_library and go_grpc_library
 # from api_proto_library.
-def api_proto_library(name, srcs = [], deps = [], has_services = 0, require_py = 1):
+def api_proto_library(name, visibility, srcs = [], deps = [], has_services = 0, require_py = 1):


@PiotrSikora as long as this works on the main Envoy repo I think it's OK. We clobber this during import, so it's not an issue inside Google.

htuch · 2018-02-02T05:00:39Z

+api_proto_library(
+    name = "mongo_proxy",
+    srcs = ["mongo_proxy.proto"],
+    visibility = ["//envoy:friends"],


Why does a network filter require such wide visibility?

I changed it just //docs in the last update.

Signed-off-by: Kuat Yessenov <kuat@google.com>

mattklein123

LGTM. Big +1 for adding an "overview" section to STYLE.md on the overall layout, what goes where, the friend stuff, etc. Would love to try to land this today as I would like to play around with some further doc cleanups to make the v2 docs a bit easier to consume.

mattklein123 · 2018-02-02T18:17:44Z

   test:
     docker:
-       - image: envoyproxy/envoy-build:52f6880ffbf761c9b809fc3ac208900956ff16b4
+       - image: envoyproxy/envoy-build:61b38528d7e46ced9d749d278ba185332310ca95


nit: Can you fix the Circle config to use the same ref semantics for the image as is done here: https://github.com/envoyproxy/envoy/blob/master/.circleci/config.yml#L1

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2018-02-02T19:00:34Z

I added a brief explanation to STYLE about layering and visibility.

kyessenov · 2018-02-02T19:27:47Z

@wora do you mind taking a look at this?

mattklein123

LGTM, thanks @kyessenov! @htuch can approve/merge from here.

htuch

LGTM modulo some very minor remaining comments. I've given this a full review, so when they are fixed, it's good to go. +1000 thanks.

htuch · 2018-02-02T21:47:51Z

+`//envoy/api/v2:friends`.
+
+Additionally, packages envoy.api.v2.core and envoy.api.v2.auth are also
+consumed throughout the remaining core API packages, but not by each other.


Nit: "core" here is a bit misleading given "envoy.api.v2.core".

changed to "shared"

htuch · 2018-02-02T21:50:19Z

+api_proto_library(
+    name = "rate_limit",
+    srcs = ["rate_limit.proto"],
+    visibility = ["//docs"],


I think we can make this less verbose by just always adding //doc in api_proto_library (it can't ever hurt), and making visibility optional and by default private.

Cool idea! I made it private by default, and add //docs always (thereby switching from private to ["//docs"] since private is not combinable).

htuch · 2018-02-02T21:51:06Z

@@ -1 +1,9 @@
 Protocol buffer definitions for core API messages.


Yeah, I think we need to rename this use of "core" as well.

It's basically a placeholder for old legacy stuff we can't move and acts just as a parent node otherwise.

made it clear this is just old xDS + top-level resources

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch

Rad.

Risk Level: Low Testing: bazel test. Docs Changes: envoyproxy/data-plane-api#452 Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov added 6 commits January 31, 2018 08:25

moving to core

33881f7

Signed-off-by: Kuat Yessenov <kuat@google.com>

Merge remote-tracking branch 'upstream/master' into control_plane

7f65032

move to core

eda6ce8

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix docs

7f31f2e

Signed-off-by: Kuat Yessenov <kuat@google.com>

merge fix

5ccf598

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix merge

c1e6cf8

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch reviewed Jan 31, 2018

View reviewed changes

kyessenov added 2 commits January 31, 2018 13:23

merge fix

cf1ac6e

Signed-off-by: Kuat Yessenov <kuat@google.com>

merge fix

57e2260

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov mentioned this pull request Jan 31, 2018

Update data-plane-api (core and filter packages) envoyproxy/envoy#2495

Merged

kyessenov mentioned this pull request Feb 1, 2018

Upgrade data-plane-api envoyproxy/go-control-plane#39

Merged

add gogoproto annotations

e1567c8

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch reviewed Feb 1, 2018

View reviewed changes

kyessenov added 3 commits January 31, 2018 21:03

add friends

a9790e5

Signed-off-by: Kuat Yessenov <kuat@google.com>

shorten import

241a006

Signed-off-by: Kuat Yessenov <kuat@google.com>

update bazel

e9c4bd3

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch reviewed Feb 1, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into control_plane

527c6c7

lizan reviewed Feb 1, 2018

View reviewed changes

alyssawilk assigned htuch Feb 1, 2018

kyessenov added 3 commits February 1, 2018 14:23

fix format

d7f4dc2

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix format

372a45c

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix docs

46dad34

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov changed the title ~~Move envoy.api.v2 core protos to envoy.api.v2.core~~ Move envoy.api.v2 core protos to envoy.api.v2.core and organize filters Feb 2, 2018

PiotrSikora reviewed Feb 2, 2018

View reviewed changes

transport socket comment

2572541

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch reviewed Feb 2, 2018

View reviewed changes

kyessenov added 4 commits February 1, 2018 21:26

move confusing docs group

38e8786

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix script

4f15587

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix format

fdb16c9

Signed-off-by: Kuat Yessenov <kuat@google.com>

generate equals methods

e191992

Signed-off-by: Kuat Yessenov <kuat@google.com>

mattklein123 reviewed Feb 2, 2018

View reviewed changes

kyessenov added 3 commits February 2, 2018 10:38

expand README

434bbd1

Signed-off-by: Kuat Yessenov <kuat@google.com>

readme

f240310

Signed-off-by: Kuat Yessenov <kuat@google.com>

use yaml references

8ebaff2

Signed-off-by: Kuat Yessenov <kuat@google.com>

wora approved these changes Feb 2, 2018

View reviewed changes

mattklein123 previously approved these changes Feb 2, 2018

View reviewed changes

htuch reviewed Feb 2, 2018

View reviewed changes

remove docs redundancy

91f7834

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov dismissed mattklein123’s stale review via 91f7834 February 2, 2018 22:10

typo

ed2ced3

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch approved these changes Feb 2, 2018

View reviewed changes

htuch merged commit 4e533f2 into envoyproxy:master Feb 2, 2018

kyessenov deleted the control_plane branch February 2, 2018 22:28

htuch pushed a commit to envoyproxy/envoy that referenced this pull request Feb 4, 2018

Update data-plane-api (core and filter packages) (#2495)

c932e50

Risk Level: Low Testing: bazel test. Docs Changes: envoyproxy/data-plane-api#452 Signed-off-by: Kuat Yessenov <kuat@google.com>

		@@ -1 +1,9 @@
		Protocol buffer definitions for core API messages.

Conversation

kyessenov commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyessenov commented Jan 31, 2018

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Jan 31, 2018

Uh oh!

htuch commented Jan 31, 2018

Uh oh!

kyessenov commented Jan 31, 2018

Uh oh!

kyessenov commented Jan 31, 2018

Uh oh!

lizan commented Jan 31, 2018

Uh oh!

kyessenov commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch commented Jan 31, 2018

Uh oh!

kyessenov commented Jan 31, 2018

Uh oh!

htuch left a comment

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!

htuch commented Feb 1, 2018

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!

kyessenov commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

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!

mattklein123 left a comment

kyessenov commented Jan 31, 2018 •

edited

Loading

kyessenov commented Jan 31, 2018 •

edited

Loading

kyessenov commented Feb 1, 2018 •

edited

Loading