source and gateways in match condition #276

rshriram · 2017-12-15T19:41:32Z

Allows users to select which gateways a rule should apply to.
@vadimeisenbergibm / @louiscryan FYI.

vadimeisenbergibm · 2017-12-15T20:45:33Z

routing/v1alpha2/route_rule.proto

@@ -62,7 +62,9 @@ package istio.routing.v1alpha2;
 // For example, the following rule routes all traffic by default to pods of
 // reviews service with label "version: v1". In addition, HTTP requests
 // containing for /wpcatalog/, /consumercatalog/ url prefixes will be
-// rewritten to /api/v2 and send to pods with label version: v2.
+// rewritten to /api/v2 and send to pods with label version: v2. The rule
+// will be applied at the gateway named bookinfo as well as all the


Wording: The rule will be applied at the gateway named bookinfo as well as at all the sidecars

vadimeisenbergibm · 2017-12-15T20:46:08Z

routing/v1alpha2/route_rule.proto

@@ -71,6 +73,9 @@ package istio.routing.v1alpha2;
 //     spec:
 //       hosts:
 //       - reviews
+//       gateways: #if omitted, defaults to "sidecars"


if omitted defaults to "mesh"

vadimeisenbergibm · 2017-12-15T20:47:12Z

routing/v1alpha2/route_rule.proto

-  // source match condition in HTTP/TCP routes.
+  // The names of gateways and sidecars that should apply these routes. A
+  // single route rule could be used for sidecars inside the mesh as well
+  // as one or more gateways. The selection condition imposed by this field


... as well as for one or more gateways

vadimeisenbergibm · 2017-12-15T20:49:12Z

routing/v1alpha2/route_rule.proto

+  // single route rule could be used for sidecars inside the mesh as well
+  // as one or more gateways. The selection condition imposed by this field
+  // can be overridden using source field in match condition in HTTP/TCP
+  // routes. The reserved word "mesh" is used to imply all sidecars in


all the sidecars in the mesh

vadimeisenbergibm · 2017-12-15T20:49:41Z

routing/v1alpha2/route_rule.proto

+  // can be overridden using source field in match condition in HTTP/TCP
+  // routes. The reserved word "mesh" is used to imply all sidecars in
+  // the mesh. When this field is omitted, the default gateway ("mesh")
+  // will be used, which would apply the rule to all sidecars in the


to all the sidecars in the mesh

vadimeisenbergibm · 2017-12-15T20:50:18Z

routing/v1alpha2/route_rule.proto

+  // routes. The reserved word "mesh" is used to imply all sidecars in
+  // the mesh. When this field is omitted, the default gateway ("mesh")
+  // will be used, which would apply the rule to all sidecars in the
+  // mesh. If a list of gateway names are provided, the rule will apply


If a list of gateway names is provided...

vadimeisenbergibm · 2017-12-15T20:53:35Z

routing/v1alpha2/route_rule.proto

@@ -270,7 +278,7 @@ message TCPRoute {
 // route rule to be applied to the HTTP request. For example, the following
 // route rule restricts the rule to match only requests where the URL path
 // starts with /ratings/v2/ and the request contains a "cookie" with value
-// "user=jason",
+// "user=jason", coming either from pods


it seems the sentence is not finished: coming either from pods... or ...

vadimeisenbergibm · 2017-12-15T20:54:19Z

routing/v1alpha2/route_rule.proto

  map<string, string> source_labels = 7;
+
+  // Names of gateways where the rule should be applied to. Gateway names
+  // at top of the rule (if any) are overridden. The gateway match is


at the top of the rule

vadimeisenbergibm · 2017-12-15T20:55:17Z

routing/v1alpha2/route_rule.proto

  map<string, string> source_labels = 4;
+
+  // Names of gateways where the rule should be applied to. Gateway names
+  // at top of the rule (if any) are overridden. The gateway match is


at the top of the rule

ZackButcher · 2017-12-15T20:49:30Z

routing/v1alpha2/gateway.proto

-// The following routing rule forwards traffic arriving at (external)
-// port 2379 from 172.17.16.* subnet to internal Mongo server on port 5555.
+// The following routing rule forwards traffic arriving at (external) port
+// 2379 from 172.17.16.* subnet to internal Mongo server on port 5555. This


nit: use a CIDR block to describe the subnet (172.17.16.0/24)?

ZackButcher · 2017-12-15T20:50:49Z

routing/v1alpha2/route_rule.proto

@@ -62,7 +62,9 @@ package istio.routing.v1alpha2;
 // For example, the following rule routes all traffic by default to pods of
 // reviews service with label "version: v1". In addition, HTTP requests
 // containing for /wpcatalog/, /consumercatalog/ url prefixes will be
-// rewritten to /api/v2 and send to pods with label version: v2.
+// rewritten to /api/v2 and send to pods with label version: v2. The rule
+// will be applied at the gateway named bookinfo as well as all the


s/bookinfo/"bookinfo"

ZackButcher · 2017-12-15T20:51:17Z

routing/v1alpha2/route_rule.proto

-  // and a specific internal workload by specifying gateways as well as the
-  // source match condition in HTTP/TCP routes.
+  // The names of gateways and sidecars that should apply these routes. A
+  // single route rule could be used for sidecars inside the mesh as well


s/could/can

ZackButcher · 2017-12-15T20:54:52Z

routing/v1alpha2/route_rule.proto

+  // The names of gateways and sidecars that should apply these routes. A
+  // single route rule could be used for sidecars inside the mesh as well
+  // as one or more gateways. The selection condition imposed by this field
+  // can be overridden using source field in match condition in HTTP/TCP


=> using the source field in the match conditions of HTTP/TCP routes.

ZackButcher · 2017-12-15T20:56:04Z

routing/v1alpha2/route_rule.proto

@@ -270,7 +278,7 @@ message TCPRoute {
 // route rule to be applied to the HTTP request. For example, the following
 // route rule restricts the rule to match only requests where the URL path
 // starts with /ratings/v2/ and the request contains a "cookie" with value
-// "user=jason",
+// "user=jason", coming either from pods


I assume you were going to add a gateway to this example and say

"coming either from pods or outside of the network" or similar?

ZackButcher · 2017-12-15T20:56:28Z

routing/v1alpha2/gateway.proto

@@ -86,6 +86,7 @@ package istio.routing.v1alpha2;
 //       - eu.bookinfo.com
 //       gateways:
 //       - my-gateway
+//       - mesh #applies to sidecars in the mesh


s/#applies/# applies

to the sidecars in the mesh

vadimeisenbergibm · 2017-12-15T20:58:01Z

/lgtm

louiscryan · 2017-12-15T21:03:10Z

routing/v1alpha2/route_rule.proto

@@ -361,8 +369,15 @@ message HTTPMatchRequest {
  PortSelector port = 6;

  // One or more labels that constrain the applicability of a rule to
-  // sources with the given labels.
+  // sidecars with the given labels. If the route rule has a list of


its not about "sidecars with the given labels" - they dont have labels, the workloads they are attached to do.

louiscryan · 2017-12-15T21:04:15Z

Largely LG. Schema is good, just to doc cleanup

frankbu · 2017-12-17T20:54:12Z

routing/v1alpha2/route_rule.proto

+  // will be used, which would apply the rule to all sidecars in the
+  // mesh. If a list of gateway names is provided, the rule will apply
+  // only to the gateways. To apply a rule to both gateways and sidecars,
+  // specify "mesh" as one of the gateway names.


This "mesh" reserved name approach seems kind of inelegant. Why not just make rules apply to all internal sidecars by default and use source_labels to filter them out? That's the way it works when you restrict access to specific internal sources, so it would seem like a clean extension to just do the same to restrict the rule to only the gateway(s) source. Something like this would apply the rule only to the gateways:

spec: hosts: - reviews gateways: - bookinfo http: - match: - sourceLabels: server: istio-gateway route: - destination: name: reviews ...

It also leaves the issue of finer granularity: how about per-namespace ?
Could it be mesh://foonamespace ?
In general - we need some form of namespace or URL for gateway as well,
and everything else. Even if k8s is not used, some internal organization is needed.

@costinm I think sourceLabels (and maybe other potential source identifiers) is the way to handle finer granularity. The problem with using this gateway special value approach is that it is not consistent with the fine-grain filtering approach we use for internal calls.

@frankbu this option was something of a compromise but it was better than the other options that were toyed with, e.g.

gateways & boolean field to disableMesh' -- Want the default of a boolean field to be false so would need to use a negative 'disableMesh' - yuck

gateways & source labels -- Want to promote the one-route-rule-per-host model which would be hard if we allowed source labels at the top-level. Given that we expect source-labels to be rarer we pushed them down to matches. However we do expect the segregration of hosts between mesh & gateways to frequently be per-host and to make more sense at the top-level

We didn't want to use labels to match gateways as we don't actually dictate that they have labels at all (e.g. they may be run outside of Kubernetes). Also gateways have identifiers that represent their intent and we didn't expect there to be too many of them, hence the desire to reference them by name.

@louiscryan I like the boolean disableMesh option much better. The negative term doesn't bother me, but you could always name it something like gatewayOnly or gatewayExclusive instead.

What I find really undesirable about the reserved gateway name approach is that mesh is a default gateway until other gateways are added. This switching the default behavior, not to mention the fact that mesh isn't really a gateway anyway, seems pretty ugly.

I'm also wondering if you've considered making sourceLabels in match clauses override the visibility defined at the top level gateways? It would seem more natural to say that sourceLabels override applicability of the individual rule rather than the current approach which says they are ignored.

costinm · 2017-12-18T18:46:15Z

routing/v1alpha2/gateway.proto

@@ -86,6 +86,7 @@ package istio.routing.v1alpha2;
 //       - eu.bookinfo.com
 //       gateways:
 //       - my-gateway
+//       - mesh # applies to all the sidecars in the mesh


You mean for the internal service-to-service traffic ?

costinm · 2017-12-18T18:51:15Z

routing/v1alpha2/route_rule.proto

+  // will be used, which would apply the rule to all sidecars in the
+  // mesh. If a list of gateway names is provided, the rule will apply
+  // only to the gateways. To apply a rule to both gateways and sidecars,
+  // specify "mesh" as one of the gateway names.


It also leaves the issue of finer granularity: how about per-namespace ?
Could it be mesh://foonamespace ?
In general - we need some form of namespace or URL for gateway as well,
and everything else. Even if k8s is not used, some internal organization is needed.

costinm · 2017-12-18T18:54:47Z

routing/v1alpha2/route_rule.proto

+
+  // Names of gateways where the rule should be applied to. Gateway names
+  // at the top of the rule (if any) are overridden. The gateway match is
+  // independent of sourceLabels.


We have the source service and labels, destination service name and labels.

Would it make sense to use the same 'labels' model for gateways as well ? And do we expect
this to be used for the zero-vpn ( service -> egress -> ingress -> service ), or only regular ingress ?

We could move to such a model later but I'd expect binding to mostly be done by name because a named gateway defines a virtual host. We can always support labels later if needed.

…cxx (istio#276)

Shriram Rajagopalan added 2 commits December 15, 2017 14:27

source and gateway matches

aa04cba

mesh

d20960a

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Dec 15, 2017

rshriram requested a review from ZackButcher December 15, 2017 20:19

vadimeisenbergibm reviewed Dec 15, 2017

View reviewed changes

ZackButcher reviewed Dec 15, 2017

View reviewed changes

louiscryan reviewed Dec 15, 2017

View reviewed changes

nits

5e81bb7

ZackButcher approved these changes Dec 15, 2017

View reviewed changes

rshriram merged commit 04f0973 into master Dec 15, 2017

frankbu reviewed Dec 17, 2017

View reviewed changes

costinm reviewed Dec 18, 2017

View reviewed changes

rshriram deleted the gateway_sidecar branch January 5, 2018 04:24

frankbu mentioned this pull request Jan 10, 2018

Foreign services in Istio #273

Merged

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

Update the latest commit id of cloudendpoints/service-control-client-…

cac6771

…cxx (istio#276)

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

remove unnecessary dependency (istio#276)

1457f64

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

source and gateways in match condition #276

source and gateways in match condition #276

rshriram commented Dec 15, 2017

vadimeisenbergibm Dec 15, 2017

vadimeisenbergibm Dec 15, 2017

vadimeisenbergibm Dec 15, 2017

vadimeisenbergibm Dec 15, 2017

vadimeisenbergibm Dec 15, 2017

vadimeisenbergibm Dec 15, 2017

vadimeisenbergibm Dec 15, 2017

vadimeisenbergibm Dec 15, 2017

vadimeisenbergibm Dec 15, 2017

ZackButcher Dec 15, 2017 •

edited

Loading

ZackButcher Dec 15, 2017

ZackButcher Dec 15, 2017

louiscryan Dec 15, 2017

ZackButcher Dec 15, 2017

ZackButcher Dec 15, 2017

ZackButcher Dec 15, 2017

vadimeisenbergibm Dec 15, 2017

vadimeisenbergibm commented Dec 15, 2017

louiscryan Dec 15, 2017 •

edited

Loading

louiscryan commented Dec 15, 2017

frankbu Dec 17, 2017 •

edited

Loading

costinm Dec 18, 2017

frankbu Dec 18, 2017

louiscryan Dec 19, 2017

frankbu Dec 20, 2017

costinm Dec 18, 2017

costinm Dec 18, 2017

costinm Dec 18, 2017

louiscryan Dec 19, 2017

source and gateways in match condition #276

source and gateways in match condition #276

Conversation

rshriram commented Dec 15, 2017

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 Dec 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimeisenbergibm commented Dec 15, 2017

louiscryan Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

louiscryan commented Dec 15, 2017

frankbu Dec 17, 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 Dec 15, 2017 •

edited

Loading

louiscryan Dec 15, 2017 •

edited

Loading

frankbu Dec 17, 2017 •

edited

Loading