Routing proto revision #105

kyessenov · 2017-02-09T01:41:45Z

Goal:

remove all mentions of clusters, tags, "source"
align with existing service model.

kyessenov · 2017-02-09T05:38:10Z

Responded to feedback from @rshriram:

not all destination policies can be applied to service versions; for now, we apply policies to all service instances
added fault injection policy back
renamed to service version, origin to source
clarified multi-port service situation; match attributes are specific to each protocol in the rule
added precedence field to rules

rshriram · 2017-02-09T13:47:29Z

model/proxy/alphav1/config/cfg.proto

-  // ClusterIdentifier, that is used to uniquely identify a version of the
-  // upstream service.
+// Destination declares policies that determine how to handle traffic for a
+// destination service (load balancing policies, failure recovery policies such


Kuat, thinking more about this, the problem with the current t destination is that its equivalent to envoy's virtual host. All routes under that host get the same policy. This might not be okay in several cases, especially in cases of retries. The retry policy is very much dependent on the API being called. Payments/checkcredit can be retried for any error code but payments/makepayment can't be retried if destination returns a 500 or example. That's why Envoy has retry policy per route entry. (Timeout as well). In your previous version, iirc destination was equivalent to the upstream cluster with tag based qualifiers and destination name. Right? Why was this removed here?

Brought it back. I don't like the inconsistency that some policies apply per cluster while others apply per virtual host. Seems like Envoy-specific problem.

rshriram · 2017-02-09T13:48:33Z

model/proxy/alphav1/config/cfg.proto

+  CircuitBreaker circuit_breaker = 5;
+
+  // L7 fault injection policy applies to L7 traffic
+  HTTPFaultInjection http_fault = 6;


One of here? Either a http_fault or l4_fault. Not both

We don't apply L7 fault injection to L4 service ports and vice versa?
Service must declare all its ports and the protocol on each port.

Isn't that the case?
And yes, we don't do L7 fault injection on L4. or vice versa. Envoy or nginx clearly separate stream connections (UDP/TCP) from HTTP and handle HTTP separately.

We don't need oneof constraint since the fault injection policies don't apply to the same traffic. If you have L4 service, you write L4 fault policy; if you have both L4 and L7 ports, you write both fault policies?

rshriram · 2017-02-09T13:49:20Z

model/proxy/alphav1/config/cfg.proto

+
+  // Set of HTTP match conditions based on the request metadata
+  HTTPMatchAttributes http = 6;
+


Also one of here? As per yesterday's discussion.

Same thing here. If my service declares L4 and L7 ports, I shouldn't be writing two rules for them since destinations are likely to be the same.

rshriram · 2017-02-09T14:02:42Z

model/proxy/alphav1/config/cfg.proto

+
+  // Precedence is used to disambiguate the order of application of rules
+  // for the same destination service. A higher number takes priority.
+  int32 precedence = 8;


This is nice. The rules format is evolving into the existing amalgam8 constructs @frankb. We can't escape precedence.

I would also add a version field to the rule. So that it gives the end user the ability to rollback to a stable rule if they mess up something. this would certainly mean that manager needs to provide a rollback API. (Even if you don't want to do this now, can we just add this comment so that we can track this issue. Matter of fact, a CDN company I spoke to actually wanted this capability).

I would add twl more lines here.
E.g. for http, routes under a virtual host need to be ordered. Here precedence is priority.

Secondly, depending on the platform, the storage for the rules format may not support atomic replace (and concurrently will be a bigger mess). The easiest way to tackle this is to ask the user to create a new rule with higher priority and delete the old rule with lower priority. Priority essentially here becomes a rule version of some sort. E.g. send all traffic to helloworld/hello to v1. Then next rule is split traffic to helloworld/hello between v1 and v2. Both rules apply to same service and same route and virtual host.

To provide atomic update semantics to the user, the platform must support a transactional storage and patch/edit semantics as well. K8s may have etcd but unsure if mesos has.

There are no transactions in k8s, there's only optimistic concurrency. Hence, we should be dealing with revision at a higher-level than this proto. Every proto has its etcd revision number implicitly. That way we can provide optimistic apply/edit like kubctl. I don't think Manager can provide strong guarantees about rollouts though, given how k8s is structured.
The priority can be used for this too, and there's nothing wrong with that. I'll add a comment.

rshriram · 2017-02-09T14:04:43Z

model/proxy/alphav1/config/cfg.proto

-    // (for an unhealthy upstream cluster) number of consecutive requests that
-    // should succeed before the upstream cluster is marked healthy.
+    // (for an unhealthy upstream) number of consecutive requests that
+    // should succeed before the upstream is marked healthy.


Please add a TODO to add all Envoy CB features.

frankbu · 2017-02-09T17:13:16Z

model/proxy/alphav1/config/cfg.proto

+// Service instances - set of pods/VMs/containers where the service is running.
+// The members share one or more common attributes. For e.g., all pods in a
+// group could share a common set of labels, or be running the same version of
+// the application binary.


This seems confusing. I would say service instances share only one thing - they are all implementing some variation of the same service. I also think that before defining the concept of Service instances, we need to clearly define the term "Service" itself. Specifically, explain that a service is a name (string) of some functionality (e.g., ServiceA) representing part of an application, and point out that clients use this name to refer to the functionality being called.

Yes, deleted the old phrasing.

frankbu · 2017-02-09T17:14:27Z

model/proxy/alphav1/config/cfg.proto

+message Destination {
+  // Service for which the service version is defined.
  string destination = 1;
-  oneof route_rule {


Wouldn't "service" be a better that "destination" for this field?

"service" is a keyword in protos. In the routing context, service name is the routing destination that the rules modify.

I see. Maybe it's not a big deal, but it just seems strange to have a field name "destination" inside the "Destination" structure. Given that service is a reserved word, I understand the problem, although I would prefer something like "service_name", myself, but that's just a personal nit, so feel free to ignore.

frankbu · 2017-02-09T18:43:16Z

model/proxy/alphav1/config/cfg.proto

+// Destination declares policies that determine how to handle traffic for a
+// destination service (load balancing policies, failure recovery policies such
+// as timeouts, retries, circuit breakers, etc).  Policies are applicable per
+// individual service versions. It is an error to define multiple policies for


From the Destination structure, it looks more like Policies are applicable to all versions of a service. How are Policies applicable per individual service version, given that the Destination structure only identifies the service and does not include any specific version info?

Never mind. I see this has been fixed already.

frankbu · 2017-02-09T19:36:09Z

model/proxy/alphav1/config/cfg.proto

-// itself from the evolution of dependent services.
+//
+// Service is a unit of an application with a unique name that other services
+// can refer to the functionality being called. Service instances are


s/can/use to/

frankbu · 2017-02-09T19:36:43Z

model/proxy/alphav1/config/cfg.proto

+//
+// Service is a unit of an application with a unique name that other services
+// can refer to the functionality being called. Service instances are
+// pods/VMs/containers that comprise the service.


s/comprise/implement/ ?

kyessenov · 2017-02-09T20:14:13Z

I have a question about ingress: are we planning to represent ingress resource as a routing rule or should we keep them separate?

frankbu · 2017-02-09T20:33:41Z

model/proxy/alphav1/config/cfg.proto

+  //
+  // N.B. The map is used instead of pstruct due to lack of serialization support
+  // in golang protobuf library (see https://github.com/golang/protobuf/pull/208)
+  map<string, string> version = 1;


Version should be = 2, and the following fields bumped by 1

frankbu · 2017-02-09T20:34:20Z

model/proxy/alphav1/config/cfg.proto

+// service versions for a given service (see the discussion on versions above).
+// The proxy would choose the version based on various routing rules.
+//
+// Applications address only the destination service using without knowledge of


kyessenov · 2017-02-09T20:56:29Z

An attempt to capture ingress routing as well:

add optional destination to destination-weight for ingress
add "cluster.local" destination option for the route rule
clarify that service names are fully qualified

kyessenov · 2017-02-09T21:00:05Z

Made up examples:

destination: svc.cluster.local
match:
  http:
    uri:
      prefix: /service/v1/
route:
  - destination: service.default.svc.cluster.local
    version:
     env: staging

destination: a.default.cluster.local
route:
  - version:
       env: staging
    weight: 5
  - version:
       env: prod
    weight: 95

kyessenov · 2017-02-09T21:49:13Z

Changes:

add CB policies
replace all uints with ints
qualify policies by HTTP prefix when they only apply to HTTP traffic
replace HTTP match attributes with just a map

kyessenov requested review from rshriram and louiscryan February 9, 2017 01:41

rshriram suggested changes Feb 9, 2017

View reviewed changes

frankbu reviewed Feb 9, 2017

View reviewed changes

kyessenov added 5 commits February 9, 2017 12:12

Revise proto

6f24bd8

revision

f8b3820

remove trailing whitespace

10b9c52

More comments

b167938

Typos

af30b78

kyessenov force-pushed the proto_revision branch from bb38f2c to af30b78 Compare February 9, 2017 20:12

Clarify precedence

9f93123

frankbu reviewed Feb 9, 2017

View reviewed changes

comments

580df12

Another attempt

ffbe552

kyessenov added 3 commits February 9, 2017 13:52

minor comment edit

a643773

more comments about ingress

d08538e

Pull out match conditions; declare HTTP as default

eb8ac9e

kyessenov requested review from mandarjog and smawson February 9, 2017 22:11

rshriram approved these changes Feb 9, 2017

View reviewed changes

kyessenov merged commit c1b4675 into istio:master Feb 9, 2017

kyessenov deleted the proto_revision branch February 9, 2017 22:24

This was referenced Feb 9, 2017

Change ClusterId to a string, use as opaque config reference. istio/api#32

Closed

Rename UpstreamCluster to something that doesn't include "Cluster" istio/api#33

Closed


		// Set of HTTP match conditions based on the request metadata
		HTTPMatchAttributes http = 6;

Routing proto revision #105

Routing proto revision #105

Uh oh!

Conversation

kyessenov commented Feb 9, 2017

Uh oh!

kyessenov commented Feb 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Feb 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Feb 9, 2017

Uh oh!

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

Uh oh!

Uh oh!

kyessenov commented Feb 9, 2017

Uh oh!

Uh oh!

kyessenov commented Feb 9, 2017 •

edited

Loading