Split base API package into sub-packages #421

kyessenov · 2018-01-17T07:46:39Z

This PR re-structures the core API proto files around several sub-packages:

envoy/api/v2/auth contains certificate management definitions, previously located in SDS API
envoy/api/v2/endpoint contains endpoint definitions, as well as load report definition
envoy/api/v2/cluster contains cluster definitions
envoy/api/v2/route contains HTTP route definitions
envoy/api/v2/listener contains listener definitions
envoy/api/v2/ratelimit for the rate limit descriptors

Services are moved to independently versioned packages:

envoy/service/discovery/v2 contains definitions of all xDS services
envoy/service/ratelimit/v2 for the rate limit service
envoy/service/accesslog/v2 for the accesslog service (previously located in accesslog filter config)
envoy/service/auth/v2 for the external auth service (previously located in core auth package)
envoy/service/load_stats/v2 for the load reporting service (previously located in EDS)
envoy/service/trace/v2 for distributed trace reporting
envoy/service/metrics/v2 for the metrics service

Service configurations are moved to independently versioned packages:

envoy/config/bootstrap/v2 for the bootstrap configuration
envoy/config/accesslog/v2 for the accesslog service proxy configuration
envoy/config/metrics/v2 for the metrics service configuration, as well as statsd
envoy/config/trace/v2 for the trace service configuration, as well as zipkin, lightstep, etc
envoy/config/ratelimit/v2 for the ratelimit service configuration

No changes are made to existing filter definitions, except what was necessary to make the build pass.

The goal of this PR is to:

start enforcing protobuf namespaces along the logical subdivision
prevent circular dependency formation (as occurred with api/auth.proto); circular dependencies force global packages in languages like go
establish a path for future upgrades of external service definitions

The following detailed changes have been made:

move the entire tree to envoy/api/v2 to align the package name with the directory path
move auth.proto to auth/auth.proto
split cds.proto into discovery/cds.proto and cluster/cluster.proto, cluster/outlier_detection.proto and cluster/circuit_breaker.proto
move eds.proto to discovery/eds.proto
move hds.proto to discovery/hds.proto
split sds.proto into discovery/sds.proto and cert/cert.proto
split rds.proto into discovery/rds.proto and route/route.proto
split lds.proto into discovery/lds.proto and listener/listener.proto
move bootstrap.proto to envoy/bootstrap/v2
split discovery.proto into discovery/common.proto and discovery/ads.proto
move stats.proto, metrics_service.proto, and trace.proto to monitoring/
move rls.proto to ratelimit/ service
move RateLimitServiceConfig from bootstrap.proto to ratelimit/ service
move access log service from access log filter config

Dependency diagram:

TODO:

decide upon LbEndpoint, HDS, and health_check.proto
fix doc generation
avoid a dependency from bootstrap on RLS service config
confirm the versioning approach with @wora
fix docs once again

fixes #420

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

wora · 2018-01-17T13:01:10Z

If we want to be serious about versioning, I recommend we follow https://cloud.google.com/apis/design/versioning. Specifically, one API version in one directory and no circular dependency, such as google.pubsub.v1 and google.type. The directory and the package name are always identical.

Please don't introduce packages like foo.bar.v2.auth. This means when we have to create v3, we have to recreate a ton of protos, which makes version impractically expensive. It is much easier to introduce foo.bar.auth.v3 as you don't need to touch other v2 stuff.

Another important thing about versioning is no breaking change within a version. Moving files around would break build badly. It should not be done when the API is being used. If we follow strict repository structure, we hardly ever need to move files. Google APIs repository has hundreds of API versions, we hardly ever moved any file once it is created.

I recommend Envoy to have strict repository rules, so we don't need to do this kind of refactoring in the future.

htuch · 2018-01-17T13:05:08Z

I don't think we will be independently versioning auth from discovery etc. I think if/when we have v3 of the API, it will be a complete bump of everything.

We are sensitive to the issue of avoiding build churn. Hopefully this is the last major move we do. I think we should also combine this with the envoy/api/v2 prefix directory structure to rip the band-aid.

I like the overall structure of this PR.

htuch · 2018-01-17T13:08:20Z

^^ the above said, I think there is a case to be made for possible changing the envoy.api.v2.discovery service definitions independently. While the base types are expected to be stable and evolve incrementally, I think we might want to version the gRPC endpoints, as we might evolve xDS/ADS as we need to accommodate different consistency models, incremental updates, etc.

htuch · 2018-01-17T14:49:35Z

Thinking about this a bit more, this is probably a good opportunity for discussion on how we want to handle API deprecation and stability. We already have some fields we'd like to deprecate (e.g. https://github.com/envoyproxy/data-plane-api/blob/master/api/bootstrap.proto#L222).

Do we want to use versioning to provide immutable API versions or are we comfortable with treating the API like we do Envoy master, as a moving target that over point releases follows a well specified deprecation story?

mattklein123 · 2018-01-17T16:18:20Z

I agree with @htuch that overall I think this is a big improvement. We should have done this in the first place. Lesson learned.

I think we should also combine this with the envoy/api/v2 prefix directory structure to rip the band-aid.

Agreed. This really needs to be the last major churn. If we are going to churn and require people to fix up a bunch of code, just churn it now.

^^ the above said, I think there is a case to be made for possible changing the envoy.api.v2.discovery service definitions independently. While the base types are expected to be stable and evolve incrementally, I think we might want to version the gRPC endpoints, as we might evolve xDS/ADS as we need to accommodate different consistency models, incremental updates, etc.

Personally, I'm not sure if this warrants split versioning. IMO we can handle enhanced discovery by adding new APIs and/or enhancing the existing APIs. I would prefer to try to keep everything in lockstep for simplicity.

Do we want to use versioning to provide immutable API versions or are we comfortable with treating the API like we do Envoy master, as a moving target that over point releases follows a well specified deprecation story?

This would be my preference, but I'm not sure it will be reasonable long term if there is substantial uptake of the APIs in other domains. IMO we should assume this now, which will allow us to iterate and keep things somewhat clean, but if there is major uptake in the future by other systems I think we will need to revisit. It would be nice to codify this somewhere.

mattklein123 · 2018-01-17T16:19:21Z

@kyessenov @htuch I haven't looked yet, but this will almost definitely have doc tool implications. I had wanted to revisit docs anyway to split out API docs from config docs, so I wouldn't worry about it too much, but we should at least make the docs look reasonably nice and close to what we have now until that is done.

rshriram · 2018-01-17T18:15:43Z

api/auth/BUILD

+    name = "auth",
+    proto = ":auth",
+    deps = [
+        "//api:address_go_proto",


why is this at top level?

Go rules? I don't know why we have duplicate go/grpc rules throughout, seems to be an issue with go_rules.

rshriram · 2018-01-17T18:21:08Z

ZOMG!! I am not signing up to update all the bazel references across envoy code base this time!

Apart from that,
what are the benefits of splitting up circuit breaker, outlier and cluster as separate protos? Why not just put them all in one file?

What is the benefit of having route as a separate package or namespace, when we already have filter/http/ ? Since route is http only currently. if we add TCP weighted routing, its going to be fragmented. TCP stuff under filter/network/.. and http stuff sticking out at the top. And when the TCP weighted routing percolates to multiple L4 protos like redis/mongo, etc., we would need to push this up once more.

Thinking out aloud, is there an opportunity to align the structure with the way Envoy high level design is? I doubt it, as this api is aligned with internal code structure. Which is fine, but from an end user looking at docs, its going to be bunch of abstractions to peruse through before they get to the point on how to configure a http listener. But may be thats a docs issue that can be handled separately.

htuch · 2018-01-17T18:33:18Z

Smaller protos are generally best practice to avoid false dependencies, so I think the split is beneficial? Route is a fairly core concept, despite being technically HCM. So, I think it makes sense as its own top level subpackage.

…

On Wed, Jan 17, 2018 at 1:21 PM Shriram Rajagopalan < ***@***.***> wrote: ZOMG!! I am not signing up to update all the bazel references across envoy code base this time! Apart from that, what are the benefits of splitting up circuit breaker, outlier and cluster as separate protos? Why not just put them all in one file? What is the benefit of having route as a separate package or namespace, when we already have filter/http/ ? Since route is http only currently. if we add TCP weighted routing, its going to be fragmented. TCP stuff under filter/network/.. and http stuff sticking out at the top. And when the TCP weighted routing percolates to multiple L4 protos like redis/mongo, etc., we would need to push this up once more. Thinking out aloud, is there an opportunity to align the structure with the way Envoy high level design is? I doubt it, as this api is aligned with internal code structure. Which is fine, but from an end user looking at docs, its going to be bunch of abstractions to peruse through before they get to the point on how to configure a http listener. But may be thats a docs issue that can be handled separately. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#421 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKaLvxWEKxQl2NPz9AdRVmIPTiX1Ho1dks5tLjoVgaJpZM4Rg7bJ> .

kyessenov · 2018-01-17T19:38:20Z

@rshriram the intention behind packaging http route is that it signifies a well-defined logical concept, that can be reused horizontally and vertically across API. For example, both filter/http and filter/network have a dependency on it, since they both utilize notions from http protocol.

I have tried to keep the number of new packages to the minimum. It's possible to move route, cluster, listener, and cert back into the base. The only reason I isolated them is because there are enough definitions in each to warrant a separate namespace.

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

kyessenov · 2018-01-17T21:00:34Z

cc @rodaine @junr03

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

htuch

I like this a lot.

htuch · 2018-01-17T21:51:28Z

envoy/api/v2/BUILD

+    deps = [":base_go_proto"],
+)
+
+api_proto_library(


Suggest a stats subpackage (for metrics+stats).

Yes, trying to keep control over the PR size, but can do it in one shot, too.

htuch · 2018-01-17T21:52:07Z

envoy/api/v2/BUILD

+)
+
+api_go_proto_library(
+    name = "trace",


Suggest a tracing subpackage in light of https://github.com/envoyproxy/data-plane-api/pull/345/files.

Created monitoring subpackage

htuch · 2018-01-17T21:53:08Z

envoy/api/v2/BUILD

+    has_services = 1,
+)
+
+api_go_grpc_library(


Suggest ratelimit subpackage.

Created ratelimit subpackage

htuch · 2018-01-17T21:53:49Z

envoy/api/v2/bootstrap/bootstrap.proto


    // [#not-implemented-hide:]
-    repeated Secret secrets = 3;
+    repeated cert.Secret secrets = 3;


Do we want cert subpackage or a more general security one? @PiotrSikora

I think if we go with security, we should put auth in there, too.

htuch · 2018-01-17T21:54:22Z

envoy/api/v2/cluster/BUILD

+)
+
+api_proto_library(
+    name = "cluster_def",


Prefer jsut cluster, it's cleaner.

Yes, I was trying to keep //envoy/api/v2/cluster as a package, but it's only used by docs.

htuch · 2018-01-17T21:55:23Z

envoy/api/v2/discovery/BUILD

@@ -0,0 +1,155 @@
+load("//bazel:api_build_system.bzl", "api_proto_library", "api_go_proto_library", "api_go_grpc_library")


We could also make this envoy.api.v2.services and put metrics/trace services and RLS here, WDYT? I don't have a strong preference.

I think discovery is more semantically loaded than services, so is a better choice.

rodaine · 2018-01-17T22:40:11Z

This looks great! Should definitely clear up the dependency issues in protoc-gen-go and simplify generation given the matching directory hierarchy. 👍

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

kyessenov · 2018-01-17T23:36:18Z

PTAL, fixed docs:

wora · 2018-01-18T00:04:58Z

While it is feasible to bump version for many packages at once, it is really unnecessary in practice, considering how much time to create Envoy API v2.

If each package has its own version, we can easily increment version at package level, then versioning become much less a drama. That is how we do with https://github.com/googleapis/googleapis. Just FYI.

For individual features, I suggest we use doc annotations, such as @Unimplemented, @Alpha, @Beta, @Deprecated for each individual API element. Normally, alpha has 3 months and beta has 6 months lifespan. proto3 has [deprecated = true] for deprecation, which you can also use.

In practice, it is much easier to deprecate a few API elements than deprecating entire version. While this is against semantic versioning theory, I think it is the better thing to do. Deprecating a version breaks everyone, no matter how we look at it.

When theory conflicts with practice, practice always wins. :)

mattklein123 · 2018-01-18T00:37:49Z

I will defer to @wora on the right way of laying this out. I really don't care that much, beyond wanting this to really, truly be the last time we do a move of this size. So if there are learnings let's just use them. @kyessenov can you sync with @wora and update as needed?

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

htuch · 2018-01-24T22:48:43Z

Can you update the top commit message to fix an issue in "Service configurations are moved to independently versioned packages:"; the non-bootstrap configs are in the envoy. rather than envoy.config. namespace?

kyessenov · 2018-01-24T22:51:06Z

Done, thanks

kyessenov · 2018-01-24T22:52:37Z

Actually, I did place them into envoy.config namespace as in envoy.config.trace.v2, to make it symmetrical with envoy.service.trace.v2.

htuch

This is awesome, LGTM modulo a few small comments.

htuch · 2018-01-24T22:52:42Z

envoy/service/discovery/v2/ads.proto

+
+import "envoy/service/discovery/v2/common.proto";
+
+// [#not-implemented-hide:] Not configuration. Workaround c++ protobuf issue with importing


Can you file an issue on protobuf GH and include it in all these comments, together with a TODO to cleanup at some point?

protocolbuffers/protobuf#4221

htuch · 2018-01-24T22:54:06Z

envoy/service/discovery/v2/eds.proto

+}
+
+// An Endpoint that Envoy can route traffic to.
+message LbEndpoint {


I think LbEndpoint (and below LocalityLbEndpoints) are fairly core concepts in the API, so belong over in envoy.api.v2.endpoint. CluserLoadAssignment can probably stay as it's only used as a response message.

Yes, I like that. Will apply that.

htuch · 2018-01-24T22:56:08Z

envoy/service/load_stats/v2/lrs.proto

 // These are stats Envoy reports to GLB every so often. Report frequency is
 // defined by
-// :ref:`LoadStatsResponse.load_reporting_interval<envoy_api_field_LoadStatsResponse.load_reporting_interval>`.
+// :ref:`LoadStatsResponse.load_reporting_interval<envoy_api_field_load_stats.LoadStatsResponse.load_reporting_interval>`.
 // Stats per upstream region/zone and optionally per subzone.
 // [#not-implemented-hide:] Not configuration. TBD how to doc proto APIs.
 message UpstreamLocalityStats {


These could probably live in envoy.api.v2.endpoint within a file called load_report.proto.

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

kyessenov · 2018-01-25T01:03:00Z

Updated envoyproxy/envoy#2430

htuch

Ready to ship once @mattklein123 and @wora sign-off. Really appreciate the heavy lifting here, the APIs are in much better shape as a result.

mattklein123 · 2018-01-25T17:13:08Z

+1 from me. @kyessenov thanks for slogging through this. Agreed this is a huge improvement. Can you do one final master merge (we will hold all other merges at this point). Also please quadruple check that we aren't dropping any recent changes with this PR - it's very difficult to review from that aspect (and I would imagine that it would be easy to accidentally miss something during merge). Thank you!

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

kyessenov · 2018-01-25T18:52:22Z

Thanks for the feedback! I compared last several commit diffs with the tip of the PR, and everything seems to be in place.

wora · 2018-01-25T22:00:44Z

/lgtm

kyessenov added 9 commits January 16, 2018 21:32

shuffle files and create discovery/cluster packages

2c4ba98

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

shuffle files and create discovery/cluster packages

5f7b322

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

split cluster.proto

7861bba

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

listener and route

f4a9e8d

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

move bootstrap

75703d1

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

split ads

0e9e75a

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

proto packages

51892ee

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

format

a1b9ea9

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

add go_package annotations

6026989

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

rshriram reviewed Jan 17, 2018

View reviewed changes

kyessenov added 2 commits January 17, 2018 11:49

move package to envoy/api/v2

7ab2a90

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

fix ci

8dcf677

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

move hds.proto to discovery

a83ba75

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

htuch reviewed Jan 17, 2018

View reviewed changes

kyessenov added 2 commits January 17, 2018 15:25

fix docs

958c99f

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

fix format

8981995

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

kyessenov added 8 commits January 24, 2018 13:23

stats config

41d75c5

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

accesslog config

b477d99

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

add ratelimit config

1ee8679

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

missing files

dc5e55f

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

trace config

2d30be2

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

fix imports

8163206

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

fix format

cd734a5

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

fix docs

a1bf2ad

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

htuch suggested changes Jan 24, 2018

View reviewed changes

kyessenov added 3 commits January 24, 2018 15:28

create endpoint package

f0106d3

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

fix format

fff0ab5

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

add github bug reference

b649d10

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

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

43943e6

htuch previously approved these changes Jan 25, 2018

View reviewed changes

kyessenov added 2 commits January 25, 2018 10:35

merge fix

d826004

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

merge fix

bd258d0

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

kyessenov dismissed htuch’s stale review via bd258d0 January 25, 2018 18:40

wora approved these changes Jan 25, 2018

View reviewed changes

mattklein123 approved these changes Jan 25, 2018

View reviewed changes

mattklein123 merged commit a66448b into envoyproxy:master Jan 25, 2018

kyessenov deleted the packages branch January 25, 2018 22:04

htuch mentioned this pull request Jan 25, 2018

Support for external authorization grpc service. envoyproxy/envoy#2415

Merged

taiki45 mentioned this pull request Jan 28, 2018

docs: fix missing links in API_OVERVIEW.md #440

Closed

		@@ -0,0 +1,155 @@
		load("//bazel:api_build_system.bzl", "api_proto_library", "api_go_proto_library", "api_go_grpc_library")


		import "envoy/service/discovery/v2/common.proto";

		// [#not-implemented-hide:] Not configuration. Workaround c++ protobuf issue with importing

Split base API package into sub-packages #421

Split base API package into sub-packages #421

Conversation

kyessenov commented Jan 17, 2018 • edited Loading

wora commented Jan 17, 2018

htuch commented Jan 17, 2018

htuch commented Jan 17, 2018 • edited Loading

htuch commented Jan 17, 2018

mattklein123 commented Jan 17, 2018

mattklein123 commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshriram commented Jan 17, 2018

htuch commented Jan 17, 2018 via email

kyessenov commented Jan 17, 2018 • edited Loading

kyessenov commented Jan 17, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodaine commented Jan 17, 2018

kyessenov commented Jan 17, 2018

wora commented Jan 18, 2018

mattklein123 commented Jan 18, 2018

htuch commented Jan 24, 2018 • edited Loading

kyessenov commented Jan 24, 2018

kyessenov commented Jan 24, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov commented Jan 25, 2018

htuch left a comment

Choose a reason for hiding this comment

mattklein123 commented Jan 25, 2018

kyessenov commented Jan 25, 2018

wora commented Jan 25, 2018

kyessenov commented Jan 17, 2018 •

edited

Loading

htuch commented Jan 17, 2018 •

edited

Loading

kyessenov commented Jan 17, 2018 •

edited

Loading

htuch commented Jan 24, 2018 •

edited

Loading