Define the Galley API #122

mandarjog · 2017-06-12T14:38:23Z

This PR proposes an abstract config API service loosely based on the Kubernetes API server.
It adds a streaming watch to satisfy istio use cases efficiently, Stream watcher API follows etcdv3 watch API.

mandarjog · 2017-06-12T16:42:59Z

@lavalamp Can you please take a look at this?

ZackButcher

I do worry about discoverability here: as a human I'm not confident I could ever call the API myself and get it correct. I know the primary mode of use will be via istioctl, but for the most part we're just shuttling around bytes, which is hard to use.

However, I don't know of another way we could do it and not require istioctl to know every proto type we're using (which is impossible in the case of Mixer config).

ZackButcher · 2017-06-12T16:50:10Z

config/service.proto

+
+syntax = "proto3";
+
+package istio.config;


If all the api methods are v1/... then this package should be istio.config.v1 too so that the gRPC service is hosted as istio.config.v1.IstioConfig, rather than istio.config.IstioConfig. On that note, the service name should be changed, making comment below.

Including the version in the proto package should save us pain on the gRPC side later.

fixed naming issues.

ZackButcher · 2017-06-12T16:51:13Z

config/service.proto

+//
+// /apiGroup/version/objecttype/{objectGroup}/objectname
+// + etcdv3 stream watch API
+service IstioConfig {


IstioConfig -> Config, or Service. We're already in the package istio.config, the repeated info is redundant. If anything, you might call this Service so it's addressed as istio.config.v1.Service.

ZackButcher · 2017-06-12T16:52:49Z

config/service.proto

+  // that is within an object group.
+  rpc ListObjects(ListObjectsRequest) returns (ObjectList) {
+        option (google.api.http) = {
+            get: "/v1/{meta.api_group}/{meta.object_type}" // ?meta.object_group=group1}"


Leftover comment here? // ?meta.object_group=group1}"

ZackButcher · 2017-06-12T16:53:12Z

config/service.proto

+        option (google.api.http) = {
+            get: "/v1/{meta.api_group}/{meta.object_type}" // ?meta.object_group=group1}"
+            additional_bindings: {
+              get: "/v1/{meta.api_group}/{meta.object_type}/meta.object_group}"


Missing a { for the object_group:

"/v1/{meta.api_group}/{meta.object_type}/{meta.object_group}"

ZackButcher · 2017-06-12T17:34:28Z

config/service.proto

+
+syntax = "proto3";
+
+package istio.v1.config;


Are we sure we want istio.v1.config and not istio.config.v1? The first implies that this API surface can only change when Istio changes version, while the second implies that the config API can be versioned independently of Istio overall. I think we'll want to freedom to iterate on this API independently of Istio's overall version.

FWIW Kubernetes buckets its apis into "groups" which are versioned separately.

jmuk · 2017-06-12T17:54:54Z

config/service.proto

+  // kv holds the KeyValue for the event.
+  // A PUT event contains current kv pair.
+  // A PUT event with kv.Version=1 indicates the creation of a key.
+  // A DELETE/EXPIRE event contains the deleted key with


DELETE/EXPIRE -> DELETE only for now? Are we going to support expiration of configs?

jmuk · 2017-06-12T18:28:35Z

config/service.proto

+  ObjectMeta key = 1;
+
+  // start watching from this revision.
+  int64 start_revision = 2; 


It seems no revisions are set in any of the responses. What does the revision means here?

versions are available in ObjectMeta

Is it the revision of the storage? I was thinking it's per-object revision. Could you modify the comment in ObjectMeta?

It's probably better to use a single word for a concept, either version or revision, but not both.

ZackButcher

LGTM

lavalamp

I don't have time to say as much as I would like, but here's a few thoughts.

lavalamp · 2017-06-12T20:25:24Z

config/service.proto

+
+syntax = "proto3";
+
+package istio.v1.config;


FWIW Kubernetes buckets its apis into "groups" which are versioned separately.

lavalamp · 2017-06-12T20:29:16Z

config/service.proto

+  };
+
+  // Delete a single object. May return validation errors.
+  rpc DeleteObject(DeleteObjectRequest) returns (google.protobuf.Empty) {


I'd have to double check but I think Kubernetes might send back the last state of the object.

API style guide mandates Delete methods return Empty .

lavalamp · 2017-06-12T20:30:28Z

config/service.proto

+  // if true object(s) should include data.
+  bool data = 1;
+
+  // if true object(s) should include source_data.


what is the difference between data and source_data?

lavalamp · 2017-06-12T20:31:55Z

config/service.proto

+  }
+}
+
+message ObjectFieldSelector {


Kubenetes is generalizing this concept into a composable value clients can set in the Accept: header. E.g., metadata only, json, proto, and csv (for dumb clients).

lavalamp · 2017-06-12T20:32:59Z

config/service.proto

+// 
+message ObjectList {
+  // meta is the metadata associated with this list.
+  ObjectMeta meta = 1;


Kubernetes has ObjectMeta and ListMeta-- the latter has many fewer things in it.

lavalamp · 2017-06-12T21:13:01Z

config/v1/service.proto

+  Meta meta = 1;
+
+  // select selects fields to include in the result.
+  ObjectFieldSelector select = 2; 


A Kubernetes field selector is used to find an object(s), not to select which fields to return.

Changed name.

I will leave it out arbitrary query selection for now. This is an optimization.

lavalamp · 2017-06-12T21:13:28Z

config/v1/service.proto

+
+message ObjectList {
+  // meta is the metadata associated with this list.
+  Meta meta = 1;


K8s uses separate ObjectMeta and ListMeta types, the latter is much shorter.

Meta could describe any part of the hierarchy. I would like to keep it this way and then specialized it later.

lavalamp · 2017-06-12T21:15:13Z

config/v1/service.proto

+message Meta {
+  // api_group is the top level delegation.
+  // All messages from an api group are handled by the same delegation handler.
+  string api_group = 1;


It is not clear that it's useful to have proto versions of the api_group & api_version concepts, since definitionally clients must have the proto version compiled in to even talk to the endpoint.

The client does not need all proto definitions. It only needs struct, bytes and other basic protos. However if a client does have the proto definitions it can parse bytes into a typed proto.

lavalamp · 2017-06-12T21:16:33Z

config/v1/service.proto

+
+  // version
+  // version of the object.
+  int64 version = 5;


What kind of version? resource version, like in k8s? something else? Comment should probably explain.

lavalamp · 2017-06-12T21:16:46Z

config/v1/service.proto

+  string object_group = 3;
+
+  // name of the object.
+  string name = 4;


jmuk · 2017-06-13T00:24:15Z

config/v1/service.proto

+  // For example:
+  // If `souce_data` contains an expressions in text form 
+  // `data` may contain a parsed AST
+  bytes data = 3; 


If it's assumed to be protobuf-encoding, should this be google.protobuf.Any?

google.protobuf.Any has repercussions for the grpc transcoding proxy. It needs to have access to the underlying protos defined by the internal type field of any.
That defeats the purpose of having an opaque proto message that only the code that converts it needs to understand.

What happens if grpc transcending proxy doesn't have access to underlying protos? Does it fail in a reasonable way (i.e. do nothing) or does it corrupt data, etc?

it fails hard, if the underlying protos are not available. This establishes an unwanted coupling.

ayj · 2017-06-13T20:05:29Z

config/v1/service.proto

+  };
+
+  // Create an object. This may result in configuration validation errors.
+  // Referrential integrity is not generally guranteed, however individual validators


s/guranteed/guaranteed/

ayj · 2017-06-13T20:06:18Z

config/v1/service.proto

@@ -0,0 +1,305 @@
+// Copyright 2017 Istio Authors
+// vim: set expandtab ts=2 sw=2:


Accidentally left in vim modeline?

Left it in while it is being edited.

ayj · 2017-06-13T20:26:40Z

config/v1/service.proto

+    UPDATE = 0;  // ADD and UPDATE
+    DELETE = 1;
+  }
+  // type is the kind of event. If type is a PUT, it indicates


s/PUT/UPDATE here and lines 297 and 298

ayj · 2017-06-13T20:49:02Z

config/v1/service.proto

+  Meta meta = 1;
+
+  // source_data is the data as it was specified in json / yaml format.
+  google.protobuf.Struct source_data = 2;


Would we ever want to allow users to create/update objects using the explicit proto message type vs. Struct to avoid unnecessary conversions?

good point, I will add it here.

ayj · 2017-06-13T20:50:57Z

config/v1/service.proto

+  // For example:
+  // If `souce_data` contains an expressions in text form 
+  // `data` may contain a parsed AST
+  bytes data = 3; 


What happens if grpc transcending proxy doesn't have access to underlying protos? Does it fail in a reasonable way (i.e. do nothing) or does it corrupt data, etc?

ayj · 2017-06-13T20:56:47Z

config/v1/service.proto

+
+  enum FilterType {
+    // filter out put event.
+    NOPUT = 0;


Use UPDATE instead of PUT to be consistent with EventType below. Also, can you use CAPITALIZED_NAMES_WITH_UNDERSCORES to be consistent with https://cloud.google.com/apis/design/naming_convention?

removed filter types for now.

jmuk · 2017-06-15T00:26:26Z

config/v1/service.proto

+    option (google.api.http) = {
+      get: "/{meta.api_group}/{meta.api_group_version}"
+      additional_bindings: {
+        get: "/"


I've tried the grpc-gateway and met an error on this line. It seems grpc-gateway assumes the path not ending with /.

--grpc-gateway_out: segment neither wildcards, literal or variable: expected "{" but got "\x00": /

maybe add a special word for this section, like "/__all__"?

jmuk · 2017-06-15T00:30:22Z

config/v1/service.proto

+  // Get a single object.
+  rpc GetObject(GetObjectRequest) returns (Object) {
+    option (google.api.http) = {
+      get: "/{meta.api_group}/{meta.api_group_version}/{meta.object_type}/{meta.object_group}/{meta.name}"


This doesn't allow to specify incl, and since booleans are false by defualt, the server can't respond any resources. Possibly you might want to reconsider it from include to exclude.

geeknoid · 2017-06-15T13:55:42Z

config/v1/service.proto

@@ -0,0 +1,322 @@
+// Copyright 2017 Istio Authors


I think this file should be in galley/v1 following the pattern for mixer.

geeknoid · 2017-06-15T13:56:41Z

config/v1/service.proto

+// api_group = /mixer/v1  or
+// api_group = /proxy/v1
+// + etcdv3 stream watch API
+service Service {


Service -> Galley

geeknoid · 2017-06-15T13:57:12Z

config/v1/service.proto

+import "google/protobuf/empty.proto";
+import "google/api/annotations.proto";
+
+// The Istio Config API service follows


Istio Config API Service -> Galley

geeknoid · 2017-06-15T13:57:31Z

config/v1/service.proto

+import "google/api/annotations.proto";
+
+// The Istio Config API service follows
+// the kubenertes API server delegation and resource model.


kubernetes -> Kubernetes

geeknoid · 2017-06-15T13:58:25Z

config/v1/service.proto

+// 
+// api_group = /mixer/v1  or
+// api_group = /proxy/v1
+// + etcdv3 stream watch API


What does this etcdv3 line mean?

geeknoid · 2017-06-15T14:12:55Z

config/v1/service.proto

+
+  // revision
+  // revision of the repository, the last time Object was updated.
+  int64 revision = 7;


What's 'repository'? The whole config store as a whole? Can we expand the word 'revision' to be more descriptive?

geeknoid · 2017-06-15T14:13:38Z

config/v1/service.proto

+  map<string, string> labels = 8;
+}
+
+// watchers follows etcdv3 watch pattern.


Remove reference to external stuff and document the pattern here instead. It's OK to go steal comments from k8s if you need to.

geeknoid · 2017-06-15T14:14:20Z

config/v1/service.proto

+
+// watchers follows etcdv3 watch pattern.
+message WatchRequest {
+  // request_union is a request to either create a new watcher or cancel an existing watcher.


"is a request to either" -> "indicates whether to"

geeknoid · 2017-06-15T14:15:30Z

config/v1/service.proto

+// watchers follows etcdv3 watch pattern.
+message WatchRequest {
+  // request_union is a request to either create a new watcher or cancel an existing watcher.
+  oneof request_union {


Why this weird pattern? Shouldn't we just have different methods, one to create a watch and one to delete it, rather than having this union?

// Create and cancel are part of the same message because the WatchRequest message
// is used in a streaming API. The client may add new watchers and remove old watchers
// on an existing stream.

Added comments.

geeknoid · 2017-06-15T14:15:55Z

config/v1/service.proto

+  // used to identify the watched subtree
+  Meta subtree = 1;
+
+  // start watching from this revision.


Revision of what? The repository?

yes repository. updated comment.

geeknoid · 2017-06-15T14:34:57Z

I'm not thrilled with this API. I think we're going too far in being generic, the API provides little semantic clarity over what parameters are expected and how they interact.

I realize we have some open-ended config stuff to deal with. But at the same time, there are various chunks of well-known config that we will handle (routing rules for example, or all of Mixer's well-known templates). I believe we should have dedicated strongly-typed APIs for those things we know, and only fallback to generic bland API for the open-ended stuff.

ayj · 2017-06-15T21:37:15Z

re: @geeknoid's concerns, it would be good to understand and document the motivation for a more generic API. We can probably live with some churn while the API settles down considering we'll be auto-generating the more tedious boilerplate code if that is the only motivation.

geeknoid · 2017-06-16T16:31:46Z

galley/v1/service.proto

+  // meta is the metadata associated with the root where listing begins.
+  Meta meta = 1;
+
+  // incl(ude) selects fields to include in the result. 


Update comment.

geeknoid · 2017-06-16T16:32:34Z

galley/v1/service.proto

+  // meta is the metadata associated with the object.
+  Meta meta = 1;
+
+  // oneof source_data or data


Remove this three line comment.

geeknoid · 2017-06-16T16:32:56Z

galley/v1/service.proto

+  // source_data is the data as it was specified in json / yaml format.
+  google.protobuf.Struct source_data = 2;
+
+  // data is protos encoded as bytes only specify  


Fix comment.

geeknoid · 2017-06-16T16:33:16Z

galley/v1/service.proto

+  // meta is the metadata associated with the object.
+  Meta meta = 1;
+
+  // incl(ude) selects fields to include in the result.


Fix comment.

geeknoid · 2017-06-16T16:33:33Z

galley/v1/service.proto

+  Meta meta = 1;
+}
+
+// Object


Say something useful here.

geeknoid · 2017-06-16T16:34:32Z

galley/v1/service.proto

+  // source_data is the data as it was specified in json / yaml format.
+  google.protobuf.Struct source_data = 2;
+
+  // data is the binary encoded protobuf data


The comment here and on ObjectRequest.data probably should match.

geeknoid · 2017-06-16T16:35:12Z

galley/v1/service.proto

+  Meta meta = 1;
+
+  // list of objects returned.
+  repeated Object objects = 2;


We probably need pagination support for any list API.

geeknoid · 2017-06-16T16:37:19Z

galley/v1/service.proto

+  map<string, string> labels = 8;
+}
+
+// WatchRequest creates on cancels a watch.


Probably worth putting the watcher definitions into their own proto file.

geeknoid · 2017-06-16T16:38:38Z

galley/v1/service.proto

+// /apiGroup/version/objecttype/{objectGroup}/objectname
+// 
+// api_group = /mixer/v1  or
+// api_group = /proxy/v1


I remain unconvinced that api_group is a good thing. It encourages component-based config state. Having "core" as an api group and putting everything in there doesn't seem useful. Why have "core" then?

geeknoid · 2017-06-16T16:41:51Z

As I mentioned in my previous review, I'm not a fan of this API. This is really just a totally generic "store protos in a hierarchy" API. Is this all we can or should do? Do we expect to create a usability layer on top of this which has stronger typing?

ZackButcher · 2017-06-16T17:06:21Z

Do we expect to create a usability layer on top of this which has stronger typing?

I think this is the only way this API can work - as I said earlier, I'm not confident I could correctly curl it.

With respect to adding strong types for things like well-known templates in Mixer, I'd really prefer we don't do that. In my opinion we should not special case things just because we happen to ship them instead of some other third party. On the other hand, I want a usable API. But I don't want to have to rebuild everything, even my CLI tools, every time I change my Mixer deployment. It's a hard balance to strike.

Is it an option for us to have the fully generic API for the reasons that've been laid out (mostly to avoid proto dependencies everywhere), but also ship a more strongly typed API? As a strawman, consider generating a strongly typed API server from a set of inputs (the templates from Mixer, whatever is required by Envoy, etc) that acts as a wrapper dispatching to this generic API. Our CLI tools can go directly against the generic API and still provide a good experience, but we can also offer a reasonable experience for operators not using our tooling. Such a wrapper API would likely make building a UI easier as well.

geeknoid · 2017-06-16T17:18:53Z

I'm dubious whether the generic API can lead to a usable CLI tools. It's likely that the CLI tools would need to be as generic as the API and so let users add/delete/edit documents in a hierarchy, with no clear semantics on what these documents are. You'd just have to pass a blob and see if errors come back. So what if the model is that we have this generic API and then all the stuff above this API depends on a plug-in model for usability? So we'd have istioctl which dynamically loads auto-generated plug-ins based on things like Mixer templates?

…

On Fri, Jun 16, 2017 at 10:06 AM, Zack ***@***.***> wrote: Do we expect to create a usability layer on top of this which has stronger typing? I think this is the only way this API can work - as I said earlier, I'm not confident I could correctly curl it. With respect to adding strong types for things like well-known templates in Mixer, I'd really prefer we don't do that. In my opinion we should not special case things just because we happen to ship them instead of some other third party. On the other hand, I want a usable API. But I don't want to have to rebuild everything, even my CLI tools, every time I change my Mixer deployment. It's a hard balance to strike. Is it an option for us to have the fully generic API for the reasons that've been laid out (mostly to avoid proto dependencies everywhere), but *also* ship a more strongly typed API? As a strawman, consider generating a strongly typed API server from a set of inputs (the templates from Mixer, whatever is required by Envoy, etc) that acts as a wrapper dispatching to this generic API. Our CLI tools can go directly against the generic API and still provide a good experience, but we can also offer a reasonable experience for operators not using our tooling. Such a wrapper API would likely make building a UI easier as well. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#122 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AVucHddzShHTxJr1-xbdlB5B-fa4I8Kuks5sErYPgaJpZM4N3NYD> .

lavalamp · 2017-06-16T17:29:03Z

Take a look at the kubernetes command line tool. It provides CRUD against resources it doesn't have compiled in. (I will leave it to you to judge whether it is "usable" or not :) )

To make this work we are opinionated about the resource data format (must have consistent metadata). A proto-first system like the one in this PR is going to make this hard, because proto is not self-describing. Maybe you can have rules about the tag number of the metadata field. You absolutely must have some way of identifying the type of a blob of data; in kubernetes, our protos have an envelope layer that declares the type and format (i.e., compressed, encrypted, json, proto).

lavalamp · 2017-06-16T17:30:06Z

I am happy to get in a room with all of you and give a brief explanation of how Kubernetes solves these problems if that would be helpful, btw.

mandarjog · 2017-06-16T17:33:54Z

@geeknoid
Discoverability problem can be solved orthogonally.

Every validator (which already tightly coupled with the components) serves config description as Open API Spec at a well known endpoint. This should include adapters that were not present at build time.
Galley aggregates these specs to form a spec per installation.
Istioctl downloads the spec to provide up to date help messages.
As a post build step, every component produces config descriptions
The Istio release step aggregates the descriptions and packages them with istioctl, so config descriptions known at compile time are all pre packaged.

guptasu · 2017-06-17T08:14:21Z

galley/v1/service.proto

+// Object validator service validates objects before they are committed to storage.
+// Galley maintains a map of object_types to validators. A single validator may validate many types.
+// For example MixerValidator Service should validate all Mixer resources.
+service ObjectValidator {


Can we rename this to ObjectValidatorAndConverter ?

guptasu · 2017-06-17T08:17:08Z

galley/v1/service.proto

+  int64 revision = 7;
+
+  // labels associated with the object.
+  map<string, string> labels = 8;


For the case of mixer, Is this where the templateName will be passed as a key to the map ?

I believe that labels are entirely owned by the user. The user assigns and manages labels on pieces of state. So we shouldn't be using those for system-level state.

That is correct. System related information is encoded in resource URI.
As we talked about before, templateName is am implementation detail of how Mixer implements mesh Functions

/core/metrics/v1/mesh/request_count ==> templateName = metrics subject = mesh constructorName = request_count

labels are owned by operator. I also think labels may be used by UI to record metadata.

ZackButcher · 2017-06-19T18:06:31Z

galley/v1/service.proto

+
+message Event {
+  enum EventType {
+    PUT = 0;  // ADD and UPDATE


Why do we use HTTP verbs here? If PUT represents an ADD or an UPDATE why don't we just have variants ADD, UPDATE, DELETE - is there a reason to constrain this set to HTTP-like things?

…ch subscription response

change api_group_version -> object_type_version,

geeknoid · 2017-06-20T22:30:12Z

I'm worried that just returning a google.rpc.status for errors in insufficient, and so the general API structure may need to change as a result. If there are several validation errors, just returning a single google.rpc.Status will not create a good experience for the user. I think we want a more structured thing that returns individual errors in an array, with some well-known metadata for things like filenames and/or line numbers.

…

On Tue, Jun 20, 2017 at 2:34 PM, mandarjog ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In galley/v1/service.proto <#122 (comment)>: > +// +// api_group = /core/ +// api_group = /vendor1 +service Galley { + // Get a single object. + rpc GetObject(GetObjectRequest) returns (ConfigObject) { + option (google.api.http) = { + get: "/{key.api_group}/{key.object_type}/{key.object_type_version}/{key.object_group}/{key.name}" + }; + }; + + // Get a list of objects. + // Other arguments like query.object_group, query.name are optional + // This lets the caller list all objects of a type, or all objects of a type + // that is within an object group. + rpc ListObjects(ListObjectsRequest) returns (ObjectList) { per API guide, rpc GetBook(GetBookRequest) returns (Book) So unless there is a need to have other information Book is the return value and on failure we return google.rcp.Status — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#122 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AVucHSd4s8yDJIhTyZ5CvAFWzHtjZVoSks5sGDrTgaJpZM4N3NYD> .

ZackButcher · 2017-06-20T22:34:42Z

Status allows us to return details per failure via it's details field. We can return a single status representing the logic result of the user's operation, and additionally return one detail per failure. We can use the existing error_details proto as the payloads, or define our own message for failures.

geeknoid · 2017-06-20T22:38:07Z

galley/v1/service.proto

+  // key associated with the requested object.
+  Meta key = 1;
+
+  // oneof source_data or data


Delete this comment.

geeknoid · 2017-06-20T22:42:07Z

galley/v1/service.proto

+  int32 max_page_size = 3;
+}
+
+message ObjectRequest {


Probably should be CreateObjectRequest

geeknoid · 2017-06-20T22:43:09Z

galley/v1/service.proto

+  // api_group is the top level delegation and extension point.
+  string api_group = 1;
+
+  // objectType is the type of an object.


object_type

geeknoid · 2017-06-20T22:43:19Z

galley/v1/service.proto

+  // every object type is independently versioned.  
+  string object_type_version = 3;
+
+  // objectGroup is an arbitrary hierarchical grouping of resources.


object_group

geeknoid · 2017-06-20T22:43:47Z

galley/v1/service.proto

+  string object_type_version = 3;
+
+  // objectGroup is an arbitrary hierarchical grouping of resources.
+  // For Mixer configuration object_group == mesh, svc:svc1, svc:svc2 ...


Delete the mixer/proxy-specific comment.

geeknoid · 2017-06-20T22:44:33Z

galley/v1/service.proto

+
+
+// Meta is the metadata associated with an object.
+message Meta {


Do we want a payload_type enum as suggested yesterday in our conversation with Daniel?

geeknoid · 2017-06-20T22:45:41Z

galley/v1/watcher.proto

+}
+
+
+// The client should WatchResponse


Not sure what this comment is trying to say.

geeknoid · 2017-06-20T22:46:30Z

galley/v1/watcher.proto

+  // its modification revision set to the revision of deletion.
+  ConfigObject kv = 2;
+
+  // prev_kv holds the key-value pair before the event happens.


happens -> happened

lizan · 2017-06-20T22:51:54Z

galley/v1/watcher.proto

+// Create and cancel are part of the same message because the WatchRequest message
+// is used in a streaming API. The client may add new watchers and remove old watchers
+// on an existing stream.
+message WatchRequest {


There are known issues to implement bidi streaming for HTTP1 (See RFC6202), and combines multiple watches in one streaming API adds too much complexity to both server and client implementation.

It sounds like implementing multiplexing by hand.

Since gRPC comes with multiplexing and flow-control, it is better to just use 1 stream per WatchCreateRequest, and use the stream state to handling cancel etc.

Is there any strong reason not to do so?

Having multiple watch subscriptions arrive serialized thru the same pipe is the semantic that we are want here. The main consumer of this API is a grpc client.

I don't think it adds complexity at the point of use. Stream provides a good place to maintain subscription state .

Will bidi streaming simply not work with http1 ?

Where is multiplexing occurring here? The client initiates listening to a stream of updates by informing the server which updates its interested in. The client can subsequently inform the server that it's interested in more (or less) updates without breaking its existing connection to the server. From the perspective of both the client and server the stream of changes is in order.

If anything, I'd argue that streaming is simpler for the client. By forcing the client to handle a stream for each subset of events it cares about, the client now has to open N connections to the server rather than one, and has to handle listening to all of them. Chances are the client will wire up the same listener to each of the N connections - at that point, what's the advantage of multiple streams vs one stream? Further, gRPC supports flow control on streaming APIs.

And is HTTP1 support that important? We expect the primary consumers of this API to use gRPC. We can expose a streaming REST interface over HTTP2. Is it really a huge loss to not be able to stream listening to changes over HTTP1?

Even the main consumer is gRPC client, can't multiple one directional streaming API calls in same gRPC channel provide same semantics? (i.e. let gRPC library manage streams state instead of clients)

Bidi streaming rarely works well with HTTP1, it requires client stack to be stream aware.

The watch_id here is logically stream_id and you are re-implementing multiplexing here, no? I'm not saying this should be unary, but 1 stream per WatchCreateRequest sounds more reasonable.

That's a good point, interpreted that way the stream is multiplexed. However I think the hard parts of multiplexing are lining up the (request, response) pairs and handling per-message cancellations and deadlines (none of which gRPC supports). This API doesn't expose any of those semantics. Instead, I think of it as allowing the client to expand or restrict the stream of changes the server is sending to it.

Suppose instead of using an ID, the cancel request looked like:

message WatchCancelRequest { Meta subtree = 1; }

That's effectively what the watch_id is a shorthand for. At that point, I'd argue that the API doesn't look nearly as much like its attempting to multiplex.

client never has to open N connections (for gRPC or HTTP2) client. And for HTTP1 it has to but still better than bidi.

Sorry, I was unclear: I mean that complexity for the client was increased because it has to manage N logical (application level) connections to the server, regardless of how gRPC chooses to transmit the data over the network.

However I think the hard parts of multiplexing are lining up the (request, response) pairs and handling per-message cancellations and deadlines (none of which gRPC supports). This API doesn't expose any of those semantics. Instead, I think of it as allowing the client to expand or restrict the stream of changes the server is sending to it.

Sorry I don't understand that part. Suppose we have one directional streaming API like

service Watcher { rpc Watch(WatchCreateRequest) returns (stream WatchResponse) { option (google.api.http) = { post: "/events/v1:watch" body: "*" }; }; } message WatchResponse { oneof response_union { WatchCreated created = 3; WatchEvents events = 4; WatchProgress progress = 6; }; }

So each streaming call maps to a watch_id here. Since gRPC has cancellation support, client can cancel any stream.

Open a new stream instead of sending WatchCreateRequest to bidi stream.

Cancel existing stream instead of sending WatchCancelRequest.

Isn't gRPC taking care of the hard parts you mentioned?

Sorry, I was unclear: I mean that complexity for the client was increased because it has to manage N logical (application level) connections to the server, regardless of how gRPC chooses to transmit the data over the network.

It really depends on how client are going to use the data watcher API. If you are watching multiple same kind (object_type) of ConfigObjects that maybe true, but if client are watching different kind of ConfigObjects, the client has to handle them differently.

This just reminds me when we started mixer API with bidi streaming and eventually moved to unary. I'm skeptical that the bidi would work efficiently here, given the complexity that you have to maintain watch_id in both server and client.

I meant that the hard parts of a bidi multiplexed stream are those things, which gRPC does not handle for you in its implementation of streaming. But that that is fine because we don't use those things in this API.

This just reminds me when we started mixer API with bidi streaming and eventually moved to unary.

I think this use case is very different than the intent of the original streaming Mixer API. The goal of that API was to use streaming to save state so we could be more efficient on the wire (the lower overhead per-request of streaming vs an unary message was also a nice benefit). Here, we're not streaming for efficiency; we're streaming because we really do have a logical stream of changes.

If the intent of this API is for the client to dispatching to handlers based on the watch_id then I agree that your proposed API, where the client opens a stream per thing it cares about, is clearly better. However, I do not think that that is the intent of this API. I think the client will dispatch based on the value of the response_union, and doesn't particularly care about the watch_id.

we're streaming because we really do have a logical stream of changes.

OK, then the intent should be clearly commented, whether the client should send a WatchCreateRequest to existing gRPC streaming call or start a new gRPC streaming call.

Though I'm not completely convinced that we really need the ability to change the logical stream dynamically. (How often and how many WatchCreateRequest or WatchCancelRequest we send per gRPC stream?) If you look at K8s watch API, it doesn't provide such ability but works for most cases.

It would be helpful to describe a concrete use case, where a client needs (or does not need) to change subscription of a watch. While useful in theory, in many cases I would expect a component to establish a watch "near the (logical) top of its hierarchy", possibly with filtering by type, so it gets all events of interest without granular control.

kyessenov · 2017-06-21T00:29:02Z

Also please elaborate on the flow control on the watcher. How do we batch updates and rate limit them?

…

On Tue, Jun 20, 2017, 5:24 PM Lizan Zhou ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In galley/v1/watcher.proto <#122 (comment)>: > + // for several watches at once. + // The watch creation call will result in returning the current state of the subtree. + rpc Watch(stream WatchRequest) returns (stream WatchResponse) { + option (google.api.http) = { + post: "/events/v1:watch" + body: "*" + }; + }; +}; + + +// WatchRequest creates on cancels a watch. +// Create and cancel are part of the same message because the WatchRequest message +// is used in a streaming API. The client may add new watchers and remove old watchers +// on an existing stream. +message WatchRequest { However I think the hard parts of multiplexing are lining up the (request, response) pairs and handling per-message cancellations and deadlines (none of which gRPC supports). This API doesn't expose any of those semantics. Instead, I think of it as allowing the client to expand or restrict the stream of changes the server is sending to it. Sorry I don't understand that part. Suppose we have one directional streaming API like service Watcher { rpc Watch(WatchCreateRequest) returns (stream WatchResponse) { option (google.api.http) = { post: "/events/v1:watch" body: "*" }; }; } message WatchResponse { oneof response_union { WatchCreated created = 3; WatchEvents events = 4; WatchProgress progress = 6; }; } So each streaming call maps to a watch_id here. Since gRPC has cancellation support, client can cancel any stream. - Open a new stream instead of sending WatchCreateRequest to bidi stream. - Cancel existing stream instead of sending WatchCancelRequest. Isn't gRPC taking care of the hard parts you mentioned? Sorry, I was unclear: I mean that complexity for the client was increased because it has to manage N logical (application level) connections to the server, regardless of how gRPC chooses to transmit the data over the network. It really depends on how client are going to use the data watcher API. If you are watching multiple same kind (object_type) of ConfigObjects that maybe true, but if client are watching different kind of ConfigObjects, the client has to handle them differently. This just reminds me when we started mixer API with bidi streaming and eventually moved to unary. I'm skeptical that the bidi would work efficiently here, given the complexity that you have to maintain watch_id in both server and client. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#122 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJGIxmJWxVZcjnBtAc8fqO0BhucpZ63tks5sGGK6gaJpZM4N3NYD> .

lizan · 2017-06-21T00:39:47Z

I just meant having multiple one directional streaming let gRPC provide more fine grained flow-control (flow control are per gRPC stream).

jmuk

some errors I've met when compiling the PR actually.

jmuk · 2017-06-22T20:45:12Z

galley/v1/service.proto

+  // within an api group.
+  rpc ListObjectTypes(ListObjectTypesRequest) returns (ListObjectTypesResponse) {
+    option (google.api.http) = {
+      get: "/{meta.api_group}"


nit: meta.api_group -> query.api_group

jmuk · 2017-06-22T20:46:03Z

galley/v1/service.proto

+  // Delete a single object. May return validation errors.
+  rpc DeleteObject(DeleteObjectRequest) returns (DeleteObjectResponse) {
+    option (google.api.http) = {
+      delete: "/{meta.api_group}/{meta.object_type}/{meta.object_type_version}/{meta.object_group}/{meta.name}"


nit: meta -> key

jmuk · 2017-06-22T20:47:00Z

galley/v1/validator.proto

+import "google/rpc/status.proto";
+import "google/api/annotations.proto";
+
+import "service.proto";


nit: import line should specify the path from the root. i.e. galley/v1/service.proto

jmuk · 2017-06-22T20:47:10Z

galley/v1/watcher.proto

+import "google/rpc/status.proto";
+import "google/api/annotations.proto";
+
+import "service.proto";


ditto: galley/v1/service.proto

jmuk · 2017-06-22T20:49:10Z

galley/v1/service.proto

+    option (google.api.http) = {
+      get: "/{meta.api_group}"
+      additional_bindings: {
+        get: "/"


As I commented previously, grpc-gateway can't compile with this spec (see grpc-ecosystem/grpc-gateway#414). Consider removing this additional binding for the time being.

jmuk · 2017-06-22T22:33:50Z

galley/v1/service.proto

+  };
+
+  // Create an object. This may result in configuration validation errors.
+  // Referential integrity should be maximally enforced, except where not possible or desirable.


I'm not sure what this line means. Could you elaborate more?

jmuk · 2017-06-22T22:35:15Z

galley/v1/watcher.proto

+
+  // kv holds the KeyValue for the event.
+  // A PUT event contains current kv pair.
+  // A PUT event with kv.Version=1 indicates the creation of a key.


ConfigObject does not have version property.

Maybe comment should refer to ConfigObject.Meta.revision?

jmuk · 2017-06-22T22:37:52Z

galley/v1/watcher.proto

+  // A PUT event contains current kv pair.
+  // A PUT event with kv.Version=1 indicates the creation of a key.
+  // A DELETE event contains the deleted key with
+  // its modification revision set to the revision of deletion.


It's hard to ensure to deliver the exact revision when the deletion happened -- some compaction or bundled event delivery would lose such information. Maybe, we can just say that the revision means that the kv doesn't exist at that point.

jmuk · 2017-06-22T22:43:24Z

galley/v1/watcher.proto

+// WatchEvents indicates that this message contains events from the watch stream.
+message WatchEvents {
+  // returns events for the specified watch id. 
+  repeated Event events = 4;


Can it also has the current_revision too?
Otherwise the watcher client can't tell what's the exact revision right now, it needs to wait for the next WatchProgress event. That sounds like redundant.

xiaolanz · 2017-06-20T19:39:19Z

galley/v1/service.proto

@@ -33,22 +33,22 @@ import "google/api/annotations.proto";
 // api_group = /vendor1
 service Galley {
  // Get a single object.
-  rpc GetObject(GetObjectRequest) returns (Object) {
+  rpc GetObject(GetObjectRequest) returns (ConfigObject) {


consider rename rpc to use Config instead of Object? like GetConfig, ListConfig.

xiaolanz · 2017-06-22T20:33:43Z

galley/v1/service.proto

+//
+// It generalizes the concept of namespace to an arbitrary grouping.
+//
+// /apiGroup/objecttype/version/{objectGroup}/objectname


consistent with camel case

jmuk · 2017-06-29T19:52:05Z

galley/v1/service.proto

+  // The primary field that was in error, if available.
+  string field = 2;
+  // The line number on which error occurred, if available.
+  int32 line_number = 3;


Since the config object has no information about the original source (source_data is already converted into Struct proto), it's probably impossible to respond with line number in the original source. Maybe we should keep the original source string in the config object too?

geeknoid · 2017-06-29T20:06:56Z

We have to maintain provenance information. Every layer that performs a transformation on some textual input needs to keep track of the line the input came from.

…

On Thu, Jun 29, 2017 at 12:52 PM, Jun Mukai ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In galley/v1/service.proto <#122 (comment)>: > + + // state of the object after the change. + ConfigObject object = 2; + + // errors if st + repeated ValidationError validation_errors = 3; +} + +message ValidationError { + // The key of the object where error was detected. + // This may not be the object that introduced the error. + string key = 1; + // The primary field that was in error, if available. + string field = 2; + // The line number on which error occurred, if available. + int32 line_number = 3; Since the config object has no information about the original source ( source_data is already converted into Struct proto), it's probably impossible to respond with line number in the original source. Maybe we should keep the original source string in the config object too? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#122 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AVucHX-EH_ZyLvutDcZ-UhGx_GGKDm3Wks5sJACJgaJpZM4N3NYD> .

elevran · 2017-07-11T14:53:59Z

galley/v1/service.proto

+import "google/rpc/status.proto";
+
+// Galley follows
+// the Kubenertes API server delegation and resource model.


the Galley design seems to abandon the k8s approach:
A granular resource oriented API based on the Kubernetes resource model was considered here. However it does not provide the required CI/CD experience therefore this document proposes a hierarchical resource API.
Can this be clarified?

elevran · 2017-07-11T15:12:08Z

galley/v1/service.proto

+
+message DeleteObjectRequest {
+  // key associated with the requested object.
+  Meta key = 1;


any validation that the meta.version, meta.revision, etc. matches the current state before deletion?

elevran · 2017-07-11T15:14:32Z

galley/v1/service.proto

+  // object_group is an arbitrary hierarchical grouping of resources.
+  // For Mixer configuration object_group == mesh, svc:svc1, svc:svc2 ...
+  // For Proxy it can be mesh, destination. 
+  string object_group = 4;


the comment above seems to suggest that string could have a format (e.g., svc:svc1, etc.) that indicates the grouping.
Is this the intent? Is Galley aware of the grouping and/or syntax used?

elevran · 2017-07-11T15:17:52Z

galley/v1/validator.proto

+    // If this object was deleted.
+    // In this case Validator should check referential integrity.
+    // Only metadata about the object being deleted is sent.
+    Meta deleted = 2;


And deny deletion if it could result in loss of referential integrity?
What about cross-component referential integrity - needed, possible?

elevran · 2017-07-11T15:24:48Z

galley/v1/watcher.proto

+// Create and cancel are part of the same message because the WatchRequest message
+// is used in a streaming API. The client may add new watchers and remove old watchers
+// on an existing stream.
+message WatchRequest {


It would be helpful to describe a concrete use case, where a client needs (or does not need) to change subscription of a watch. While useful in theory, in many cases I would expect a component to establish a watch "near the (logical) top of its hierarchy", possibly with filtering by type, so it gets all events of interest without granular control.

elevran · 2017-07-11T15:28:09Z

galley/v1/watcher.proto

+
+  // kv holds the KeyValue for the event.
+  // A PUT event contains current kv pair.
+  // A PUT event with kv.Version=1 indicates the creation of a key.


Maybe comment should refer to ConfigObject.Meta.revision?

mandarjog · 2017-07-11T16:14:18Z

@elevran I am abandoning this PR in favor of https://github.com/istio/galley/pull/51

Signed-off-by: Shriram Rajagopalan <[email protected]>

mandarjog requested a review from geeknoid June 12, 2017 15:52

mandarjog requested a review from ldemailly June 12, 2017 16:51

ZackButcher reviewed Jun 12, 2017

View reviewed changes

jmuk reviewed Jun 12, 2017

View reviewed changes

ZackButcher approved these changes Jun 12, 2017

View reviewed changes

lavalamp reviewed Jun 12, 2017

View reviewed changes

jmuk reviewed Jun 13, 2017

View reviewed changes

ayj reviewed Jun 13, 2017

View reviewed changes

mandarjog force-pushed the config_api_service branch from 87990c7 to 9627bbe Compare June 13, 2017 21:33

jmuk reviewed Jun 15, 2017

View reviewed changes

geeknoid suggested changes Jun 15, 2017

View reviewed changes

geeknoid changed the title ~~Config API Service~~ Defining the Galley API. Jun 15, 2017

mandarjog force-pushed the config_api_service branch from 10b5977 to b28b8a9 Compare June 15, 2017 22:55

geeknoid suggested changes Jun 16, 2017

View reviewed changes

mandarjog force-pushed the config_api_service branch from 1b16070 to 8ab4ab7 Compare June 16, 2017 20:42

guptasu reviewed Jun 17, 2017

View reviewed changes

geeknoid requested review from geeknoid and xiaolanz June 19, 2017 17:17

ZackButcher reviewed Jun 19, 2017

View reviewed changes

mandarjog added 13 commits June 20, 2017 14:24

Rename ObjectMeta to Meta and Change package

9767050

move service into v1

e78028a

version --> revision

73527c0

change version/group --> group/version

aaca5ee

convert 2 bools to enum for responseType

7297a29

Separate Watcher in a different service. Add initial subtree with wat…

34927ca

…ch subscription response

Review comments

16f13e8

Move service definition to a new location.

fdd5db3

Add ObjectValidator grpc service

6853e76

change api_group_version -> object_type_version,

Add batching of validation requests, useful during initial import

112c7cd

add max_page_size to list requests

e69e266

Object --> ConfigObject, change names of Meta to key or query

69c7618

review comments

4d671ad

mandarjog force-pushed the config_api_service branch from 56ace26 to 4d671ad Compare June 20, 2017 21:24

geeknoid approved these changes Jun 20, 2017

View reviewed changes

lizan reviewed Jun 20, 2017

View reviewed changes

split into 3 files, add ValidationError message

308f87b

jmuk reviewed Jun 22, 2017

View reviewed changes

xiaolanz reviewed Jun 22, 2017

View reviewed changes

jmuk reviewed Jun 29, 2017

View reviewed changes

elevran reviewed Jul 11, 2017

View reviewed changes

mandarjog closed this Jul 11, 2017

nacx pushed a commit to nacx/api that referenced this pull request Feb 23, 2022

Update gateway examples to use server names (istio#122)

c1469fa

Signed-off-by: Shriram Rajagopalan <[email protected]>

		@@ -0,0 +1,305 @@
		// Copyright 2017 Istio Authors
		// vim: set expandtab ts=2 sw=2:



		// Meta is the metadata associated with an object.
		message Meta {

Define the Galley API #122

Define the Galley API #122

Conversation

mandarjog commented Jun 12, 2017 • edited Loading

mandarjog commented Jun 12, 2017

ZackButcher left a comment

Choose a reason for hiding this comment

ZackButcher Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZackButcher Jun 12, 2017 • edited Loading

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

ZackButcher left a comment

Choose a reason for hiding this comment

lavalamp 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

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

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

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

geeknoid commented Jun 15, 2017

ayj commented Jun 15, 2017 • edited Loading

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

mandarjog commented Jun 12, 2017 •

edited

Loading

ZackButcher Jun 12, 2017 •

edited

Loading

ZackButcher Jun 12, 2017 •

edited

Loading

ayj commented Jun 15, 2017 •

edited

Loading