Migrate mixer API and config to this repo. #2

geeknoid · 2016-12-16T19:24:30Z

This clearly separates mixer's API and configuration surface from
the implementation. This is designed to make it possible to
replace the mixer in a fully compatible way.

Note that I put the config into mixer/config instead of istio/config since it is in fact all specific to the mixer. These configuration definitions aren't used by the proxy or manager themselves.

Note to Doug: this clearly doesn't build as it needs the bazel treatment (mainly a WORKSPACE file I think). I hope you can supply this after this initial checkin.

geeknoid · 2016-12-16T19:57:39Z

I added a WORKSPACE file (which I just copied from the mixer repo) and I'm able to build the config directory, but the API directory fails due to some missing dependencies. Better, but still needs attention from someone that knows what they're doing with bazel...

douglas-reid · 2016-12-16T21:02:03Z

What if we don't bazel-ify this repo?

We could potentially have the other repos define the build rules for various languages, etc.?

@qiwzhang would that work for the proxy stuff?

geeknoid · 2016-12-16T21:12:01Z

Where would I go to make sure my proto syntax is correct? I'd have to go to the mixer repo for example? Seems a bit odd. But you're the bazel guy, if that's what you want I'll nuke the BUILD files.

…

On Fri, Dec 16, 2016 at 1:02 PM, Douglas Reid ***@***.***> wrote: What if we don't bazel-ify this repo? We could potentially have the other repos define the build rules for various languages, etc.? @qiwzhang <https://github.com/qiwzhang> would that work for the proxy stuff? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AVucHcYo3_n6U2nYUwfB5CaLqzreuBRpks5rIvxLgaJpZM4LPg6Q> .

douglas-reid · 2016-12-16T21:26:13Z

Well, hopefully, PR reviews would catch the syntax issues... but that obviously has flaws.

We could also add generated code here too (with a script for doing the generation) and require updates on any commit to both the source and gen'd libs.

Why don't we either (a) commit without bazel now or (b) commit with the broken bits and I'll work with @qiwzhang to get a build setup working for the repo afterwards?

geeknoid · 2016-12-16T21:27:27Z

OK, I'll commit as-is and then you can decide to update/delete the build files.

…

On Fri, Dec 16, 2016 at 1:26 PM, Douglas Reid ***@***.***> wrote: Well, hopefully, PR reviews would catch the syntax issues... but that obviously has flaws. We could also add generated code here too (with a script for doing the generation) and require updates on any commit to both the source and gen'd libs. Why don't we either (a) commit without bazel now or (b) commit with the broken bits and I'll work with @qiwzhang <https://github.com/qiwzhang> to get a build setup working for the repo afterwards? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AVucHRd8WYphVy6LSFgvXiIpzdDusMIUks5rIwH1gaJpZM4LPg6Q> .

douglas-reid · 2016-12-16T21:27:10Z

WORKSPACE

@@ -0,0 +1,112 @@
+workspace(name = "com_github_istio_mixer")


name this com_github_istio_api

douglas-reid · 2016-12-16T21:28:11Z

WORKSPACE

+        "google/rpc/status.proto",
+    ],
+    imports = [
+        "../../external/com_github_google_protobuf/src",


i think to use this setup, we need the .bazelrc file that adds the "--standalone" like bits (as this escapes the sandbox).

douglas-reid · 2016-12-16T21:28:44Z

bazel-api

@@ -0,0 +1 @@
+/usr/local/google/home/mtail/.cache/bazel/_bazel_mtail/1875d8bd5ec708fecb970aa8fdf4f58a/execroot/com_github_istio_mixer


we need a to add a git ignore entry for bazel-*

douglas-reid · 2016-12-16T21:38:01Z

mixer/api/v1/attribute_update.proto

+//
+//   * Any deleted attributes are removed from the attribute context.
+//
+message AttributeUpdate {


This doesn't feel quite right to me. Why not just call this Attributes? And have the dictionary be understood to be context-dependent (either complete or overriding depending on whether or not a context is provided)?

Renamed to Attributes.

Added more text about how the dictionary is used. Added the reset_context boolean to force everything to start from scratch. And added a few more comments here and there for clarity.

douglas-reid · 2016-12-16T21:38:36Z

mixer/api/v1/attribute_update.proto

+  map<int32, string> email_attributes = 10;
+
+  // Attributes that should be removed
+  repeated int32 deleted_attributes = 11;


Can we expand on what it means to be removed? Removed from the set of AttributeValues and the Dictionary?

douglas-reid · 2016-12-16T21:39:21Z

mixer/api/v1/attribute_update.proto

+  //
+  // Once a new dictionary has been sent over, it stays in effect until
+  // a new dictionary is sent.
+  map<string, int32> dictionary_update = 1;


does this imply that the same attribute can be referred to by different IDs by different proxies, etc., across the system?

Yes. It prevents us having to maintain any kind of registry of attribute ids. It's dynamic in nature, per connection to the mixer. As proxies are updated in a staged roll out and new attributes are added, then the dictionary is sent accordingly.

Ultimately I think it makes sense to reserve a block of ids for well-known things. This is what HTTP2 did, I would just add a TODO for the moment.

douglas-reid · 2016-12-16T21:40:16Z

mixer/api/v1/attribute_update.proto

+// The general idea is to leverage the stateful connection from the
+// proxy to the mixer to keep to a minimum the 'attribute chatter'.
+// Only delta attributes are sent over, multiple concurrent attribute
+// contexts can be used to avoid trashing, and attribute indices are used to


s/avoid trashing/avoid thrashing/ ?

douglas-reid · 2016-12-16T21:41:50Z

mixer/config/v1/log_entry_descriptor.proto

+  string name = 1;
+
+  // The set of attributes that are necessary to describe a log entry of this type.
+  repeated string attributes = 2;


(here and elsewhere) to be clear, these are attribute descriptor names?

douglas-reid · 2016-12-16T21:44:18Z

mixer/config/v1/attribute_descriptor.proto

+  //     Name = IDENT { "." IDENT } ;
+  //
+  // Where `IDENT` must match the regular expression `[a-z][a-z0-9]+`.
+  string name = 1;


Do we want to clarify anything about uniqueness of this name?

geeknoid · 2016-12-16T22:31:49Z

PTAL.

qiwzhang

I will like to Not have any bazel build files.
Other repos that use it can have bazel build files.
They will fail, they they update their "commit"

qiwzhang · 2016-12-16T23:18:56Z

mixer/api/v1/BUILD

@@ -0,0 +1,57 @@
+load("@org_pubref_rules_protobuf//go:rules.bzl", "go_proto_library")
+


Please don't put a BUILD file here.

OK, they're gone.

louiscryan · 2016-12-16T23:22:38Z

mixer/api/v1/attributes.proto

+
+  // Attributes being updated within the specified attribute context. When reset_context is false,
+  // then these maps are treated as deltas, so only what's changed needs to be included.
+  map<int32, int64> int64_attributes = 4;


Seems like overkill I would just start with int64, string, boolean and maybe double if we have a case for it.

It seems odd to be mixing semantic types and data types here when we also have all the semantics in descriptors.

I would use string for ipaddress & email & int64 for timestamp, otherwise following this pattern we will end up with separate fields for DNS names, durations, GUIDs, ... and I don't think that's useful.

FWIW 'bytes' might be a better type than string for ip addresses and would cover both types adequately based on the length of the bytestring

louiscryan · 2016-12-16T23:23:12Z

mixer/api/v1/attributes.proto

+// An instance of this is delivered to the mixer with every
+// API call.
+//
+// The general idea is to leverage the stateful connection from the


s/connection/stream

louiscryan · 2016-12-16T23:25:46Z

mixer/api/v1/attributes.proto

+// proxy to the mixer to keep to a minimum the 'attribute chatter'.
+// Only delta attributes are sent over, multiple concurrent attribute
+// contexts can be used to avoid thrashing, and attribute indices are used to
+// keep the wire protocol maximally efficient.


This sounds like a pretty complex protocol. Can we start with one implicit context and add disjoint contexts when we have evidence of need. After all multiple contexts can be handled by having multiple GRPC streams.

qiwzhang

Can we just put the proto files here for now? I am blocked by this.

douglas-reid · 2016-12-16T23:38:30Z

I vote to put the protos in, without bazel stuff, to unblock everyone.

louiscryan · 2016-12-16T23:44:58Z

mixer/api/v1/service.proto

+  //
+  // This method is intended for use by the Istio proxy and not by
+  // individual services.
+  rpc Check(stream CheckRequest) returns (stream CheckResponse) {}


Attribute context will span check & report all the time so having them be independent streams seems quite wasteful given all the effort put into dictionary management above. I would either:

1 - punt on dictionary management possibly in favor of a partially static dictionary (see HTTP2 https://http2.github.io/http2-spec/compression.html#static.table.definition)
2. - Allow the client to define the context in which the dictionary grain is useful and to choose which operation to perform in that context

The former seems a lot simpler than the latter frankly

I like the lack of special cases in this model, and I like the uniformity. All of the API methods work the same way, both on the server and client, so they'll just be some common code on both ends. The actual runtime overhead of multiple dictionaries is tiny both in CPU time and memory.

I'd like to keep this as-is and see how it pans out.

I don't strictly mind repeating the same dictionary or attribute context three times so that its available for each operation type for the context at hand. Need to document how to handle the failure mode when an attribute context cannot be allocated by the server, I suspect we should terminate the stream and reset everything and require the client to recover.

I'm fine with this, will add some clarifying doc comments.

louiscryan · 2016-12-16T23:59:06Z

mixer/config/v1/attribute_descriptor.proto

+  string description = 3;
+
+  // Types of supported attribute values.
+  enum ValueType {


Continuing the earlier note about semantic types vs. data types, this enum is mixing both. I think we should have two enums

enum DataType {
INT, //as int64
FLOAT, //as double
BOOL,
STRING,
BYTES
}

enum ValueType {
LABEL,
URI.
DNS_NAME,
EMAIL,
TIMESTAMP,
IP_ADDR,
COUNTER,
GAUGE,
SUMMATION,
DURATION, // can standardize units here
}

This clarifies which fields in Attributes should be used for each descriptor field. It allows us to codify how coercion between DataTypes should occur for a specific ValueType, converting STRING to INT for a COUNTER

Example mappings to (data type, value type)
ipaddress -> (BYTES, IP_ADDR)
username -> (STRING, EMAIL)
k8s pob label -> (STRING, LABEL)
k8s service DNS -> (STRING, DNS_NAME)
system uptime -> (INT, DURATION)

This is appealing. Let me think about it for a bit.

I've been playing with this split and I'm not sure it's useful or desirable.

With my design, as new well-known and/or efficient storage formats are adopted, we just add a new value type for it. For example, Timestamp, Duration, IPv6Address, and likely more over time. With your approach, all those end up being encoded as raw bytes.

I think perhaps what threw you off is the choice of names. What if instead of what I had, we used instead:

enum ValueType {
// A generic 64-bit signed integer.
UNDISCRIMINATED_INT = 0;

// A 64-bit floating-point value.
UNDISCRIMINATED_FLOAT = 1;

// A variable-length string.
UNDISCRIMINATED_STRING = 2;

// Boolean; true or false.
UNDISCRIMINATED_BOOL = 3;

// A point in time, encoded as bytes or string
TIMESTAMP = 4;

// A quantity of time, encoded as bytes or string
DURATION = 5;

// An IP address, encoded as bytes or string
IP_ADDR = 6;

// An email address, encoded as a string
EMAIL_ADDR = 7;

// A URL, encoded as a string
URI = 8;
}

Attributes will be used by selectors and policies. We need to know the types to apply expressions on them. That is the most important issue we need to cover. Splitting the value format from value type is attractive, but also adds more complexity. I suggest we don't pay for it initially.

I think the most useful feature needed in our use case is string, int, double, timestamp, ipaddress. Let's start with them first.

While proto does have duration type, most other systems don't have it. It would be much easier to store duration as a double seconds, then it is easy to use in an expression. It is much harder to manipulate 96-bit duration and we almost never need that much precision.

I think in either form we'd have to handle migration to concrete schema types over time by coercion so I don't think that's a major difference really.

I do think the names threw me off so I'm OK with the single enum. Comments would have been enough to disambiguate I think so no need to make the names longer I think. What would be most useful would probably be examples.

INT -- Codes and enum like values such as HTTP status. You could argue these are POINT metrics too given that they can have thresholds applied to them, they just can't be aggregated
FLOAT -- I have a hard time coming up with an example that is not really a metric, lat/long maybe but those seem like they should be metrics with a type of POINT ? If we don't have a real use-case lets drop it and add it back?
STRING -- NAME or LABEL would be better choices for the the enum value?
BOOL -- I think its fairly self explanatory, no need to elaborate

The only way to interact with proxies and REST APIs is via HTTP header. So we want to make sure all values are expressible as strings. The expression engine will become insanely complicated as we add more types. We should really limit the choice as much as we can. We can always add more types later, but impossible to remove anything.

Done. I improved this somewhat. We'll tweak more as we learn where we'd like more discriminated typing and/or more efficient encodings.

louiscryan · 2016-12-17T00:03:46Z

mixer/config/v1/metric_descriptor.proto

+  string display_name = 8;
+
+  // The kind of measurement. It describes how the data is reported.
+  enum MetricKind {


Ok, I see how you're intending to partition the semantic types. Probably want to put DURATION, SUMMATION and maybe TIMESTAMP in here

louiscryan · 2016-12-17T00:04:54Z

mixer/config/v1/quota_descriptor.proto

+  // for a quota of this type.
+  repeated string dimension_attributes = 3;
+
+  // The unit in which the quota's values are reported. The


Some examples here would be useful

I think that all these are istio configs, regardless of where they are used at present.
If it is something that a User sees and interacts with while configuring istio, It is istio config. For example, if this is how someone configures the quota aspect, It surely is istio config.

Unless something is purely between the proxy and mixer, it should be istio config.
Looks like all these descriptors will form a vocabulary of {attributes, quotas, ... }
At configuration time, you either define a quota or a rate limit inline, or use a reference from the vocabulary.

For majority of the config the user should be configuring istio. They should only need to configure individual software components sparingly.

Where these types live in the repo is immaterial to what the user experiences, right?

For the programmer, these types exist to configure the mixer, so I prefer the definitions be closely coupled with the mixer.

Consider for example if these types change, the only code that needs to change is in the mixer. If we have multiple implementations of the mixer, they'll each need to deal with these configs. If we have multiple implementations of the proxy and manager, none of them will deal with these types.

louiscryan · 2016-12-17T00:06:38Z

mixer/config/v1/quota_descriptor.proto

+  // The default imposed maximum amount for values of this quota.
+  int64 max_amount = 7;
+
+  // The type of quota behavior expected, defaults to NORMAL if not specified


NORMAL not in enum

louiscryan · 2016-12-17T00:07:55Z

mixer/config/v1/quota_descriptor.proto

+  // Whether the quota's current value is tracked precisely or not.
+  // Precisely tracked quotas only allow a relatively modest level of
+  // scaling, whereas imprecise quotas can support a nearly unbounded
+  // level of scaling at the cost of potentially having small


I would eliminate 'small'

louiscryan · 2016-12-17T00:26:23Z

mixer/config/v1/metric_descriptor.proto

+  //
+  // **Basic units (UNIT)**
+  //
+  // * `bit`   bit


Do we really need schema for things like bytes? Is this just to facilitate display of aggregates in a standard fashion?

louiscryan · 2016-12-17T00:27:26Z

mixer/config/v1/metric_descriptor.proto

+  //
+  // **Grammar**
+  //
+  // The grammar includes the dimensionless unit `1`, such as `1/s`.


This seems like an odd example in the context of a metric assuming 1/s means '1 per second'.

I assume we just want to present either things where the unit is implied in the name, or things with units that are specified above which is currently limited to bytes & time. Is this worth the effort?

This is all directly lifted from One Platform. I've no particular
attachment to this stuff if we want to simplify. We could just delete the
field completely for the time being.

Removed the unit field completely.

louiscryan · 2016-12-17T00:47:55Z

mixer/api/v1/attributes.proto

+//
+// The processing order for this state in the mixer is:
+//
+//   * Any dictionary updates are applied


Is it an 'update' or a replace, comment on line 56 indicates a replace. Phrasing...

'The new dictionary is installed if provided'

Also it's probably an API error for the first request not to install a dictionary.

louiscryan · 2016-12-17T00:49:46Z

mixer/api/v1/attributes.proto

+//   * Any dictionary updates are applied
+//
+//   * The requested attribute context is looked up. If no such context has been defined, a
+//     new context is automatically created and initialized to the empty state.


Add examples of the events that might cause a context to be created, e.g. a new socket is opened to a server, a new user logs in etc. Could go on the field instead.

louiscryan · 2016-12-17T00:50:53Z

mixer/api/v1/attributes.proto

+//   * If reset_context is true, then the attribute context is reset to the
+//     empty state.
+//
+//   * Any attribute deltas are applied to the attribute context.


'deltas applied' gives me the impression that aggregation occurs, 'overwrite' seems like a better term

louiscryan · 2016-12-17T00:52:43Z

mixer/api/v1/attributes.proto

+  // A dictionary that provides a mapping of attribute name to
+  // a shorthand index value.
+  //
+  // This is intended to leverage the stateful connection from the


Be careful with the term 'connection' here. We're really using GRPCs notion of streams which might cause folks to get confused with TCP connections

louiscryan · 2016-12-17T00:55:07Z

mixer/api/v1/attributes.proto

+  //
+  // Dictionaries are independent of the attribute context and are thus global
+  // for the current gRPC stream.
+  map<string, int32> dictionary_update = 1;


Nit. See note above about 'update'. Could just make the field name 'active_dictionary', 'new_dictionary' etc.

louiscryan · 2016-12-17T00:57:20Z

mixer/api/v1/attributes.proto

+  // for the current gRPC stream.
+  map<string, int32> dictionary_update = 1;
+
+  // The attribute context against which the new attributes are applied.


The id of the attribute context...

louiscryan · 2016-12-17T00:59:06Z

mixer/api/v1/attributes.proto

+
+  // When true, resets the current attribute context to the empty state before
+  // applying any new incoming attributes.
+  bool reset_context = 3;


Consider providing guidance about when an attribute context should be reset so that the server can reclaim memory. E.g. If an HTTP2 connection terminated we should be able to reclaim the memory occupied by the context by re-using its id for a new connection later on.

louiscryan · 2016-12-17T05:21:26Z

Makes sense. LGTM

…

-louis (from phone) On Dec 16, 2016 8:46 PM, "Martin Taillefer" <[email protected]> wrote: PTAL. I'd like to go ahead with this change as-is so we can unblock other work. Subsequent adjustments can be done in follow-on PRs. Thanks. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIoKPGPOba8zlYw9X0nkO8fHaCZcCXnyks5rI2k3gaJpZM4LPg6Q> .

douglas-reid

Please remove BUILD before merge. Otherwise, LGTM.

wora · 2016-12-17T01:12:42Z

mixer/config/v1/attribute_descriptor.proto

+  string description = 3;
+
+  // Types of supported attribute values.
+  enum ValueType {


Attributes will be used by selectors and policies. We need to know the types to apply expressions on them. That is the most important issue we need to cover. Splitting the value format from value type is attractive, but also adds more complexity. I suggest we don't pay for it initially.

I think the most useful feature needed in our use case is string, int, double, timestamp, ipaddress. Let's start with them first.

While proto does have duration type, most other systems don't have it. It would be much easier to store duration as a double seconds, then it is easy to use in an expression. It is much harder to manipulate 96-bit duration and we almost never need that much precision.

wora · 2016-12-17T01:13:33Z

mixer/config/v1/attribute_descriptor.proto

+    DOUBLE = 1;
+
+    // A variable-length string.
+    STRING = 2;


Please change STRING to use 0, we use the most popular one for 0. I think string should be more popular than INT64.

wora · 2016-12-17T01:14:42Z

mixer/config/v1/attribute_descriptor.proto

+    IPV6 = 6;
+
+    // An email address.
+    EMAIL = 7;


Let's drop this for now and use string instead. Email is mutable user id. Use it for any real purpose is risky, we should discourage it.

wora · 2016-12-17T01:15:53Z

mixer/config/v1/attribute_descriptor.proto

+    IPV4 = 5;
+
+    // An IPv6 address.
+    IPV6 = 6;


Let's use IP_ADDRESS instead. We can't control which system uses which IP address, mixed IP will be common in many cases. We should force every component can always handle both.

wora · 2016-12-17T01:26:33Z

mixer/config/v1/quota_descriptor.proto

+  // Precisely tracked quotas only allow a relatively modest level of
+  // scaling, whereas imprecise quotas can support a nearly unbounded
+  // level of scaling at the cost of potentially having inaccuracies.
+  bool precise = 9;


Please remove this. We plan to drop this for Google services. When the traffic is too hard, we automatically degrade the precision. The reason is simple. This is a user option, and there is always human mistake. If a user noticed the traffic is too high, and wants to switch to in-precise, there is no way to support in a live system. Then the service will be broken.

wora · 2016-12-17T01:27:00Z

mixer/config/v1/quota_descriptor.proto

+
+    // Quota limit expresses a maximum amount over a rolling time interval
+    RATE_LIMIT = 1;
+  }


This is already implied by the quota unit, right?

wora · 2016-12-17T01:28:09Z

mixer/config/v1/quota_descriptor.proto

+// values are reset to 0 automatically at a fixed interval.
+message QuotaDescriptor {
+  // The name of this descriptor.
+  string name = 1;


We should drop this.

Descriptors must have names so they can be referenced.

wora · 2016-12-17T01:33:01Z

mixer/config/v1/quota_descriptor.proto

+  // * `NAME` is a sequence of non-blank printable ASCII characters not
+  //    containing '{' or '}'.
+  //
+  string unit = 4;


I think we can use a much simpler design. For example, a quota limit of 100 request per second per ip address, it can be expressed as.

int64 limit = 100;
repeated string dimensions = ["origin.ip"];
int32 interval = 1;

So all other fields in this message is not needed. We don't plan to use display_name and description anyway since they are not localized. The UI should automatically generate localized text based on limit, dimensions, interval.

I nuked the unit field.

wora · 2016-12-17T01:37:00Z

mixer/config/v1/attribute_descriptor.proto

+  string description = 3;
+
+  // Types of supported attribute values.
+  enum ValueType {


The only way to interact with proxies and REST APIs is via HTTP header. So we want to make sure all values are expressible as strings. The expression engine will become insanely complicated as we add more types. We should really limit the choice as much as we can. We can always add more types later, but impossible to remove anything.

douglas-reid · 2016-12-17T06:34:45Z

mixer/api/v1/check.proto

+package istio.mixer.v1;
+
+import "google/rpc/status.proto";
+import "mixer/api/v1/attribute_update.proto";


should this be "mixer/api/v1/attributes.proto"; ?

douglas-reid · 2016-12-17T06:35:02Z

mixer/api/v1/quota.proto

+package istio.mixer.v1;
+
+import "google/rpc/status.proto";
+import "mixer/api/v1/attribute_update.proto";


attributes.proto ?

douglas-reid · 2016-12-17T06:35:17Z

mixer/api/v1/report.proto

+package istio.mixer.v1;
+
+import "google/rpc/status.proto";
+import "mixer/api/v1/attribute_update.proto";


attributes.proto?

douglas-reid · 2016-12-17T06:35:44Z

mixer/api/v1/service.proto

+
+import "api/mixer/v1/check.proto";
+import "api/mixer/v1/report.proto";
+import "api/mixer/v1quota.proto";


missing a '/' between 'v1' and 'quota.proto' ?

wora · 2016-12-17T06:43:32Z

On general comment, attribute will become primitive types for the upcoming expression and policy language. The less types we support, the easier for implementation and more usable.

For example, if a component expects EMAIL attribute, you can't use STRING if everything is strongly typed (inconvenient), or we have to define various implicit conversion rules (complicated), or we have to cast everywhere (ugly). We should minimize features until we truly need them.

douglas-reid · 2016-12-17T06:52:59Z

mixer/api/v1/attributes.proto

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


i feel like the packages should be mixer.api.v1 instead of istio.mixer.v1.

For Google APIs, we never use the term "api". When you have v1 in the package, it is pretty much an api. Adding "api" everywhere doesn't add much value, almost all protos are api anyway.

Should we be using mixer.v1 or is istio.mixer.v1 better?

Removing api is fine. However, these protos live in mixer/api/v1. It seems a bit appropriate to match that structure in the package.

In any case, I feel like "istio.mixer.v1" is not ideal. The generated go code will lead to some imports like "istio.io/istio/mixer/v1". I'd prefer something like "istio.io/mixer/v1".

We can address this later, once we switch the code over.

This clearly separates mixer's API and configuration surface from the implementation. This is designed to make it possible to replace the mixer in a fully compatible way.

wora · 2016-12-19T15:47:43Z

I think the istio.mixer.v1 is better choice. We should not care what it looks like in Go. If we design the API as proto, we can't make it happy with every programming language. We can not own the top level proto package mixer, it implies we own entire proto namespace. Everything shall be under istio package name, so we can accept other people's contribution.

louiscryan · 2016-12-19T18:09:55Z

mixer/config/v1/quota_descriptor.proto

+    PER_SECOND_LIMIT = 1;
+
+    // Quota limit expresses a maximum amount over a rolling time interval
+    PER_MINUTE_LIMIT = 1;


Why bother with per_second and per_minute ?

* Define interface for client. * Remove per_request transport. * Add quota interface. * Update copyright headers. * Update namespace from google to istio.

Signed-off-by: Dhi Aurrahman <[email protected]>

Add ambient mode to meshconfig

geeknoid requested review from mandarjog and douglas-reid December 16, 2016 19:24

douglas-reid reviewed Dec 16, 2016

View reviewed changes

qiwzhang suggested changes Dec 16, 2016

View reviewed changes

louiscryan reviewed Dec 16, 2016

View reviewed changes

qiwzhang reviewed Dec 16, 2016

View reviewed changes

louiscryan reviewed Dec 16, 2016

View reviewed changes

louiscryan reviewed Dec 17, 2016

View reviewed changes

douglas-reid approved these changes Dec 17, 2016

View reviewed changes

wora reviewed Dec 17, 2016

View reviewed changes

douglas-reid reviewed Dec 17, 2016

View reviewed changes

Migrate mixer API and config to this repo.

dd5b6ce

This clearly separates mixer's API and configuration surface from the implementation. This is designed to make it possible to replace the mixer in a fully compatible way.

geeknoid merged commit cf7f8a4 into istio:master Dec 17, 2016

louiscryan reviewed Dec 19, 2016

View reviewed changes

geeknoid mentioned this pull request Jan 10, 2017

Add unary API surface for use by mixer and proxy. #14

Closed

frankbu mentioned this pull request Jan 12, 2017

proto structs for routing rules and proxy config #21

Merged

ZackButcher mentioned this pull request Mar 14, 2017

Another gogo improvement: inline stringmap structs to avoid another u… #64

Merged

louiscryan mentioned this pull request Dec 14, 2017

DestinationRules #274

Merged

frankbu mentioned this pull request Jan 16, 2018

Foreign Services - followup #307

Merged

louiscryan mentioned this pull request Apr 13, 2018

Envoy filter configurations #447

Merged

incfly pushed a commit to incfly/api that referenced this pull request Jun 13, 2018

Define interface for client. (istio#2)

3a0ec2c

* Define interface for client. * Remove per_request transport. * Add quota interface. * Update copyright headers. * Update namespace from google to istio.

geeknoid mentioned this pull request Dec 2, 2019

Add proto for analysis messages #1179

Closed

istio-testing changed the title ~~Migrate mixer API and config to this repo.~~ Automator: update common-files@master in istio/api@master Jan 24, 2020

istio-testing added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 24, 2020

clarketm changed the title ~~Automator: update common-files@master in istio/api@master~~ Migrate mixer API and config to this repo. Jan 24, 2020

clarketm mentioned this pull request Feb 3, 2020

explicitly search for "repo" in "UpdatePR" kubernetes/test-infra#16109

Merged

nacx pushed a commit to nacx/api that referenced this pull request Apr 15, 2020

Fix circle config (istio#2)

8515dad

Signed-off-by: Dhi Aurrahman <[email protected]>

jacob-delgado mentioned this pull request May 14, 2021

remove authentication/v1alpha1 authorization policy #1991

Closed

howardjohn pushed a commit that referenced this pull request Sep 7, 2022

Merge pull request #2 from dhawton/add-sidecarless-to-meshconfig

b76e722

Add ambient mode to meshconfig

ramaraochavali mentioned this pull request Feb 22, 2023

Add ecdh_curves support #2543

Merged

keithmattix mentioned this pull request Aug 18, 2023

Define targetRef proto #2888

Merged

		@@ -0,0 +1 @@
		/usr/local/google/home/mtail/.cache/bazel/_bazel_mtail/1875d8bd5ec708fecb970aa8fdf4f58a/execroot/com_github_istio_mixer

		@@ -0,0 +1,57 @@
		load("@org_pubref_rules_protobuf//go:rules.bzl", "go_proto_library")

Migrate mixer API and config to this repo. #2

Migrate mixer API and config to this repo. #2

Conversation

geeknoid commented Dec 16, 2016 • edited by clarketm Loading

geeknoid commented Dec 16, 2016

douglas-reid commented Dec 16, 2016

geeknoid commented Dec 16, 2016 via email

douglas-reid commented Dec 16, 2016

geeknoid commented Dec 16, 2016 via email

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 Dec 16, 2016

qiwzhang 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

qiwzhang left a comment

Choose a reason for hiding this comment

douglas-reid commented Dec 16, 2016

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

louiscryan Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

mandarjog Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

geeknoid Dec 17, 2016 • 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

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

louiscryan commented Dec 17, 2016 via email

geeknoid commented Dec 16, 2016 •

edited by clarketm

Loading

louiscryan Dec 17, 2016 •

edited

Loading

mandarjog Dec 17, 2016 •

edited

Loading

geeknoid Dec 17, 2016 •

edited

Loading