VirtualService simple merging of http routes at Gateway #8113

rshriram · 2018-08-21T20:30:50Z

Fixes #7793 for Gateways only.
This is the rudimentary merge of http routes, pushing
the wildcard/catch all routes to the end.

We already support merging of TCP and TLS routes for gateways.

Merging virtual services at the sidecar requires a lot of intrusive changes to the RDS code.

Signed-off-by: Shriram Rajagopalan shriramr@vmware.com

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

costinm · 2018-08-21T21:03:18Z

pilot/pkg/networking/core/v1alpha3/gateway.go

@@ -225,7 +225,7 @@ func (configgen *ConfigGeneratorImpl) buildGatewayHTTPRouteConfig(env *model.Env
 	// NOTE: WE DO NOT SUPPORT two gateways on same workload binding to same virtual service
 	virtualServices := push.VirtualServices(merged.Names)
 	virtualHosts := make([]route.VirtualHost, 0, len(virtualServices))


I think it should be *VirtualHost - since it gets changed.

See code below. I am copying from the map to this array

Envoy wants values and not pointers for some reason. or its probably gogo

frankbu · 2018-08-21T21:05:34Z

pilot/pkg/networking/core/v1alpha3/route/route.go

+)
+
+// CombineVHostRoutes will concatenate route blocks from two virtual hosts
+func CombineVHostRoutes(first []route.Route, second []route.Route) []route.Route {


This seems to have a nasty side effect of even reordering match clauses that come from the same resource, where the user has intentionally got it right. Would it be better to try to leave order that the user has explicitly set intact?

Yes. We would have to change our docs that routes will be reordered by paths, prefixes and then regexes

Shouldn't we prioritize user intent over heuristics? If the user has a specific order in mind (with regex first, to blackhole something, for example), and writes it down in a single resource, I think it should be respected.

rshriram · 2018-08-21T23:21:45Z

/test istio-unit-tests

…ple_vs_concat

nrjpoddar · 2018-08-21T23:42:42Z

pilot/pkg/networking/core/v1alpha3/route/route.go

+		// longest regex before shorter regex. it doesn't make any difference, but
+		// we do it anyway to get a stable ordering
+		if iType == jType {
+			return len(iVal) < len(jVal)


This should be

return len(iVal) > len(jVal)

based on the comments above which describes the merge logic.

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

codecov · 2018-08-22T00:49:51Z

Codecov Report

Merging #8113 into release-1.0 will decrease coverage by 1%.
The diff coverage is 62%.

@@             Coverage Diff              @@
##           release-1.0   #8113    +/-   ##
============================================
- Coverage           72%     72%   -<1%     
============================================
  Files              358     358            
  Lines            31137   31228    +91     
============================================
+ Hits             22235   22252    +17     
- Misses            7951    8023    +72     
- Partials           951     953     +2

Impacted Files	Coverage Δ
pilot/pkg/networking/util/util.go	`30% <ø> (ø)`	⬆️
pilot/pkg/networking/core/v1alpha3/gateway.go	`0% <0%> (ø)`	⬆️
pilot/pkg/networking/core/v1alpha3/httproute.go	`25% <0%> (+1%)`	⬆️
pilot/pkg/networking/core/v1alpha3/route/route.go	`37% <86%> (+6%)`	⬆️
...olarwinds/internal/papertrail/papertrail_logger.go	`62% <0%> (-18%)`	⬇️
mixer/adapter/rbac/rbacStore.go	`76% <0%> (-3%)`	⬇️
mixer/adapter/kubernetesenv/kubernetesenv.go	`84% <0%> (-1%)`	⬇️
mixer/adapter/statsd/statsd.go	`96% <0%> (-1%)`	⬇️
mixer/pkg/protobuf/yaml/resolver.go	`59% <0%> (-1%)`	⬇️
mixer/adapter/circonus/circonus.go	`71% <0%> (ø)`	⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f3f178...936ced6. Read the comment docs.

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

ijsnellf · 2018-08-22T16:25:49Z

pilot/pkg/networking/core/v1alpha3/gateway.go

 		}
 	}

-	if len(virtualHosts) == 0 {
+	if len(vHostDedupMap) == 0 {


nit: move virtualHosts := make([]route.VirtualHost, 0, len(virtualServices)) down here to line 259 so it is clear we don't use it until we convert vHostDedupMap?

ZackButcher

/lgtm

ZackButcher · 2018-08-22T18:20:27Z

pilot/pkg/networking/core/v1alpha3/route/route.go

+		var iType, jType envoyRouteType
+		var iVal, jVal string
+
+		switch iR := allroutes[i].Match.PathSpecifier.(type) {


would be nice to pull this out into a helper to re-use, would improve readability here quite a bit I think:

func routeTypeAndVal(r route.Route) (envoyRouteType, string) { var ttype envoyRouteType var val string switch rr := route.Match.PathSpecifier.(type) { case *route.RouteMatch_Path: ttype = envoyPath val = rr.Path case *route.RouteMatch_Prefix: if rr.Prefix == "/" { ttype = envoyCatchAll } else { ttype = envoyPrefix } val = rr.Prefix case *route.RouteMatch_Regex: if rr.Regex == "*" { ttype = envoyCatchAll } else { ttype = envoyRegex } val = rr.Regex } return ttype, val } func SortRoutes(allroutes []route.Route) { sort.SliceStable(allroutes, func(i, j int) bool { iType, iVal := routeTypeAndValue(allroutes[i]) jType, jVal := routeTypeAndValue(allroutes[j]) if iType == jType { if len(iVal) == len(jVal) { if len(allroutes[i].Match.Headers) == len(allroutes[j].Match.Headers) { return iVal > jVal } return len(allroutes[i].Match.Headers) > len(allroutes[j].Match.Headers) } return len(iVal) > len(jVal) } return iType < jType }) }

(oh and maybe routeTypeAndVal(r *route.Route) instead to avoid copying)

istio-testing · 2018-08-22T18:20:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ZackButcher
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: rshriram

If they are not already assigned, you can assign the PR to them by writing /assign @rshriram in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pilot/pkg/networking/OWNERS~~ [ZackButcher]
tests/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

frankbu · 2018-08-22T18:39:20Z

pilot/pkg/networking/core/v1alpha3/gateway.go

+		// we have to maintain the same ordering semantics for non-merged vhost routes as well.
+		// So, sort each virtual host's routes based on our custom sort criteria and build the virtual host array
+		for _, v := range vHostDedupMap {
+			istio_route.SortRoutes(v.Routes)


This is not backward compatible.

Yes, that may be a problem - if you have a single VHost the order should be preserved.
But on the other side - it may be good to update the API to specify a better/more scalable order.

costinm · 2018-08-22T19:25:56Z

pilot/pkg/networking/core/v1alpha3/route/route.go

+// regex before catchall prefix / or catch all regex *
+// longest path before shorter path
+// longest prefix before shorter prefix
+// longest regex before shorter regex. it doesn't make any difference, but


For regex - I think it makes a difference, we should preserve the original order ( or not allow merging - i.e. just one virtual service should be allowed to have regex ).

I think the safest option remains to only allow one 'main' VirtualService to be merged with path+prefix only rules.

+1 to that. As I said in issue, we should put regexes in creation time order (which necessarily keeps regexes from the same VS together as a single block).

costinm

Looks like a good start - but for backward compat I would only sort if there are at least 2 VirtualServices. So users with a single VS keep the order, and if they have 2 VS - we sort.

And I'm not very comfortable with the regex story.

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

istio-testing · 2018-08-22T19:59:52Z

New changes are detected. LGTM label has been removed.

rshriram · 2018-08-22T20:04:00Z

@ZackButcher / @costinm dumped the reordering completely. After discussion with @frankbu , here is what we have now:

 new routes = 
   append all routes from first virtual service except the catch all routes
   append all routes from second virtual service except catch all routes
   append all catchall routes from first
   append all catchall routes from second

where catchAll is a route with no header match and a prefix match of / or regex match of *.

This technique preserves the relative ordering of routes within a VirtualService, eliminates the need to compare regular expressions. The burden is now on the user to ensure that the blocks of routes in two virtual services don't conflict with each other

ZackButcher · 2018-08-22T20:20:16Z

This gets back to my point in the issue, there is no one winning way to do this. Like we discussed in the Networking WG, simply concatenating the clauses together is not good enough, it doesn't solve the user problem at hand meaningfully. If we're going to do this merging stuff, we should disregard the user order and reorder ourselves. We likely want to do this anyway in the long run, since it allows for optimizations in Envoy like a hashmap exact URI match followed by trie based prefix matching, etc.

cc @louiscryan

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

costinm · 2018-08-22T20:53:42Z

pilot/pkg/networking/core/v1alpha3/route/route.go

+					if rType == envoyCatchAll {
+						// We have a catch all route. No point building other routes, with match conditions
+						break allroutes
+					}


Should we add something to pushcontext - if the user has other routes after, it's likely a bad config.

Or we could update the doc and change behavior - so default route is applied last.

I think adding to push context would be a lot ? If anything, we should catch this in validation and warn.

costinm

It's a start :-)

I think we'll need to iterate - but this should move us forward.

In particular - we should still extract all the exact and prefix, and move them at the top, reverse sorted by length.

frankbu · 2018-08-22T21:03:00Z

I think this is a good and safe thing to try in 1.0.1, to get some feedback. Given that this only works in cases where there really are no conflicts, we aren't setting ourselves up for backward compatibility problems if we decide on doing some reordering in the future release.

frankbu · 2018-08-22T21:08:44Z

pilot/pkg/networking/core/v1alpha3/route/route.go

+
+	// A route is catch all if and only if it has no header/query param match
+	// and has a prefix / or regex *.
+	if (iVal == "/" && iType == envoyPrefix) || (iVal == "*" && iType == envoyRegex) {


If we want do the absolute minimum, to keep all options open for how things are reordered in the future, I would even reduce this check to only the prefix / case. It's the legitimate VirtualService-without-a-match-clause catch-all case.

Not necessarily. User could also create a match clause with just prefix /

good point. bottom line is moving these rules could cause breakage, but I think it's a pretty unlikely corner case.

rshriram · 2018-08-23T15:27:48Z

Lets merge and iterate. Would like to have some variant in the release tomorrow. and need some time for this to soak, and test.

rshriram · 2018-08-23T15:28:21Z

If we decide to fully reorder, please let me know by EOD today (after consensus)

frankbu · 2018-08-23T17:03:39Z

The more I think about it, the more I think that ANY merge algorithm, other than the simple concatenate and preserve user's order, will not be backward compatible (or break things when a second resource is merged) if we do it by default (without explicit user request). I think something like @rshriram's mergeKey suggestion, which lets the user control merge ordering, might be the right future enhancement, but for 1.0.1 the simple concatenate is the best and safest way to go. I think the latest comments from @ZackButcher and @bmarshall13 in #7793 support this thinking.

@ZackButcher, I do think the the simple concatenate does solve some users problem in a meaningful way, the problem of clearly separating the host into contextually independent parts, presumably managed by different teams. I think the feedback from 1.0.1 will then help us understand where we need to go from there.

ZackButcher · 2018-08-24T22:47:47Z

SGTM

christian-posta · 2018-08-27T13:52:45Z

Are there any notes about how one would test this?

frankbu · 2018-08-27T14:27:05Z

@christian-posta A little bit of documentation is here: https://istio.io/help/ops/traffic-management/deploy-guidelines/#multiple-virtual-services-and-destination-rules-for-the-same-host

erSitzt · 2020-04-16T18:38:14Z

@rshriram
You said

Merging virtual services at the sidecar requires a lot of intrusive changes to the RDS code.

Why is that and whats different about the gateways ?
Any feedback is welcome :)

I know this is an old PR, but i didnt find any other references to VirtualService merging..

VirtualService simple merging of http routes

58ec462

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

googlebot added the cla: yes label Aug 21, 2018

istio-testing requested review from ijsnellf and ostromart August 21, 2018 20:30

rshriram requested review from ZackButcher and removed request for ostromart August 21, 2018 20:31

rshriram mentioned this pull request Aug 21, 2018

VirtualService simple merging of http routes at Gateway #8114

Merged

rshriram requested a review from frankbu August 21, 2018 20:34

undo random changes

06a85de

costinm reviewed Aug 21, 2018

View reviewed changes

frankbu reviewed Aug 21, 2018

View reviewed changes

Merge branch 'release-1.0' of https://github.com/istio/istio into sim…

99a358d

…ple_vs_concat

nrjpoddar reviewed Aug 21, 2018

View reviewed changes

tests

28ca68b

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

Shriram Rajagopalan added 4 commits August 21, 2018 20:53

undo unnecessary changes

861043e

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

fix yamls

9089bb2

more debugging

09d0d15

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

restrict merging to gateways only

16ac9cc

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

rshriram changed the title ~~VirtualService simple merging of http routes~~ VirtualService simple merging of http routes at Gateway Aug 22, 2018

ijsnellf approved these changes Aug 22, 2018

View reviewed changes

nits

2563a6d

ZackButcher reviewed Aug 22, 2018

View reviewed changes

istio-testing assigned ZackButcher Aug 22, 2018

istio-testing added the lgtm label Aug 22, 2018

frankbu reviewed Aug 22, 2018

View reviewed changes

costinm reviewed Aug 22, 2018

View reviewed changes

new concat algorithm

676cf36

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

istio-testing removed the lgtm label Aug 22, 2018

lint

936ced6

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

costinm reviewed Aug 22, 2018

View reviewed changes

costinm approved these changes Aug 22, 2018

View reviewed changes

frankbu reviewed Aug 22, 2018

View reviewed changes

rshriram merged commit cd0e828 into istio:release-1.0 Aug 23, 2018

frankbu mentioned this pull request Aug 24, 2018

Is there a way to publish two different services by ONE domain name, while each service has its own VirtualService? #6070

Closed

rshriram deleted the simple_vs_concat branch August 24, 2018 14:01

dreadbird mentioned this pull request Dec 19, 2018

Routing to a wrong endpoint with multiple virtualservices for the same host, when the virtualservices have header rules. #10485

Closed

VirtualService simple merging of http routes at Gateway #8113

VirtualService simple merging of http routes at Gateway #8113

Conversation

rshriram commented Aug 21, 2018 • edited

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

rshriram commented Aug 21, 2018

nrjpoddar Aug 21, 2018 • edited

Choose a reason for hiding this comment

codecov bot commented Aug 22, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

ZackButcher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

istio-testing commented Aug 22, 2018

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

costinm left a comment

Choose a reason for hiding this comment

istio-testing commented Aug 22, 2018

rshriram commented Aug 22, 2018

ZackButcher commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costinm left a comment

Choose a reason for hiding this comment

frankbu commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshriram commented Aug 23, 2018

rshriram commented Aug 23, 2018

frankbu commented Aug 23, 2018

ZackButcher commented Aug 24, 2018

christian-posta commented Aug 27, 2018

frankbu commented Aug 27, 2018

erSitzt commented Apr 16, 2020

rshriram commented Aug 21, 2018 •

edited

nrjpoddar Aug 21, 2018 •

edited

codecov bot commented Aug 22, 2018 •

edited