Also provision empty route rules for revisions that are not yet Active

pkg/controller/route/route.go

@@ -764,6 +764,48 @@ func (c *Controller) computeRevisionRoutes(
 	return ret, nil
 }
 
+// computeEmptyRevisionRoutes is a hack to work around https://github.com/istio/istio/issues/5204.
+// Here we add empty/dummy route rules for non-target revisions to prepare to switch traffic to
+// them in the future.  We are tracking this issue in https://github.com/elafros/elafros/issues/348.
+//
+// TODO:  Even though this fixes the 503s when switching revisions, revisions will have empty route
+// rules to them for perpetuity, therefore not ideal.  We should remove this hack once
+// https://github.com/istio/istio/issues/5204 is fixed, probably in 0.8.1.
+func (c *Controller) computeEmptyRevisionRoutes(
+	route *v1alpha1.Route, configMap map[string]*v1alpha1.Configuration, revMap map[string]*v1alpha1.Revision) ([]RevisionRoute, error) {
+	ns := route.Namespace
+	elaNS := controller.GetElaNamespaceName(ns)
+	revClient := c.elaclientset.ElafrosV1alpha1().Revisions(ns)
+	revRoutes := []RevisionRoute{}
+	for _, tt := range route.Spec.Traffic {
+		configName := tt.ConfigurationName
+		if configName != "" {
+			// Get the configuration's LatestReadyRevisionName
+			latestReadyRevName := configMap[configName].Status.LatestReadyRevisionName
+			revs, err := revClient.List(metav1.ListOptions{
+				LabelSelector: fmt.Sprintf("%s=%s", ela.ConfigurationLabelKey, configName),
+			})
+			if err != nil {
+				glog.Errorf("Failed to fetch revisions of Configuration %q: %s", configName, err)
+				return nil, err
+			}
+			for _, rev := range revs.Items {
+				if _, ok := revMap[rev.Name]; !ok && rev.Name != latestReadyRevName {
+					// This is a non-target revision.  Adding a dummy rule to make Istio happy,
+					// so that if we switch traffic to them later we won't cause Istio to
+					// throw spurious 503s. See https://github.com/istio/istio/issues/5204.
+					revRoutes = append(revRoutes, RevisionRoute{
+						RevisionName: rev.Name,
+						Service:      fmt.Sprintf("%s.%s", rev.Status.ServiceName, elaNS),
+						Weight:       0,
+					})
+				}
+			}
+		}
+	}
+	return revRoutes, nil
+}
+
 func (c *Controller) createOrUpdateRouteRules(route *v1alpha1.Route, configMap map[string]*v1alpha1.Configuration,
 	revMap map[string]*v1alpha1.Revision) ([]RevisionRoute, error) {
 	// grab a client that's specific to RouteRule.
@@ -789,7 +831,13 @@ func (c *Controller) createOrUpdateRouteRules(route *v1alpha1.Route, configMap m
 		glog.Errorf("Failed to check if should direct traffic to activator: %s", err)
 		return nil, err
 	}
-
+	// TODO: remove this once https://github.com/istio/istio/issues/5204 is fixed.
+	emptyRoutes, err := c.computeEmptyRevisionRoutes(route, configMap, revMap)
+	if err != nil {
+		glog.Errorf("Failed to get empty routes for %s : %q", route.Name, err)
+		return nil, err
+	}
+	revisionRoutes = append(revisionRoutes, emptyRoutes...)
 	// Create route rule for the route domain
 	routeRuleName := controller.GetRouteRuleName(route, nil)
 	routeRules, err := routeClient.Get(routeRuleName, metav1.GetOptions{})

pkg/controller/route/route_test.go

@@ -318,6 +318,90 @@ func TestCreateRouteCreatesStuff(t *testing.T) {
 	}
 }
 
+func TestCreateRouteFromConfigsWithMultipleRevs(t *testing.T) {
+	_, elaClient, controller, _, elaInformer := newTestController(t)
+
+	// A configuration and associated revision. Normally the revision would be
+	// created by the configuration controller.
+	config := getTestConfiguration()
+	latestReadyRev := getTestRevisionForConfig(config)
+	otherRev := &v1alpha1.Revision{
+		ObjectMeta: metav1.ObjectMeta{
+			SelfLink:  "/apis/ela/v1alpha1/namespaces/test/revisions/p-livebeef",
+			Name:      "p-livebeef",
+			Namespace: testNamespace,
+			Labels: map[string]string{
+				ela.ConfigurationLabelKey: config.Name,
+			},
+		},
+		Spec: *config.Spec.RevisionTemplate.Spec.DeepCopy(),
+		Status: v1alpha1.RevisionStatus{
+			ServiceName: "p-livebeef-service",
+		},
+	}
+	config.Status.LatestReadyRevisionName = latestReadyRev.Name
+	elaClient.ElafrosV1alpha1().Configurations(testNamespace).Create(config)
+	// Since updateRouteEvent looks in the lister, we need to add it to the informer
+	elaInformer.Elafros().V1alpha1().Configurations().Informer().GetIndexer().Add(config)
+	elaClient.ElafrosV1alpha1().Revisions(testNamespace).Create(latestReadyRev)
+	elaClient.ElafrosV1alpha1().Revisions(testNamespace).Create(otherRev)
+
+	// A route targeting both the config and standalone revision
+	route := getTestRouteWithTrafficTargets(
+		[]v1alpha1.TrafficTarget{
+			v1alpha1.TrafficTarget{
+				ConfigurationName: config.Name,
+				Percent:           100,
+			},
+		},
+	)
+	elaClient.ElafrosV1alpha1().Routes(testNamespace).Create(route)
+	// Since updateRouteEvent looks in the lister, we need to add it to the informer
+	elaInformer.Elafros().V1alpha1().Routes().Informer().GetIndexer().Add(route)
+
+	controller.updateRouteEvent(KeyOrDie(route))
+
+	routerule, err := elaClient.ConfigV1alpha2().RouteRules(testNamespace).Get(fmt.Sprintf("%s-istio", route.Name), metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("error getting routerule: %v", err)
+	}
+
+	expectedRouteSpec := v1alpha2.RouteRuleSpec{
+		Destination: v1alpha2.IstioService{
+			Name: fmt.Sprintf("%s-service", route.Name),
+		},
+		Match: v1alpha2.Match{
+			Request: v1alpha2.MatchRequest{
+				Headers: v1alpha2.Headers{
+					Authority: v1alpha2.MatchString{
+						Regex: regexp.QuoteMeta(
+							strings.Join([]string{route.Name, route.Namespace, defaultDomainSuffix}, "."),
+						),
+					},
+				},
+			},
+		},
+		Route: []v1alpha2.DestinationWeight{
+			v1alpha2.DestinationWeight{
+				Destination: v1alpha2.IstioService{
+					Name: fmt.Sprintf("%s-service.test", latestReadyRev.Name),
+				},
+				Weight: 100,
+			},
+			v1alpha2.DestinationWeight{
+				Destination: v1alpha2.IstioService{
+					Name: fmt.Sprintf("%s-service.test", otherRev.Name),
+				},
+				Weight: 0,
+			},
+		},
+	}
+
+	if diff := cmp.Diff(expectedRouteSpec, routerule.Spec); diff != "" {
+		t.Errorf("Unexpected rule spec diff (-want +got): %v", diff)
+	}
+}
+
 func TestCreateRouteWithMultipleTargets(t *testing.T) {
 	_, elaClient, controller, _, elaInformer := newTestController(t)
 	// A standalone revision

test/states.go

@@ -30,16 +30,12 @@ import (
 // traffic to and return true if 100% of the traffic is routing to revisionName.
 func AllRouteTrafficAtRevision(routeName string, revisionName string) func(r *v1alpha1.Route) (bool, error) {
 	return func(r *v1alpha1.Route) (bool, error) {
-		if len(r.Status.Traffic) > 0 {
-			if len(r.Status.Traffic) != 1 {
-				return true, fmt.Errorf("Expected Route to have only one configurated Traffic but had %d. Route: %v", len(r.Status.Traffic), r)
-			}
-			if r.Status.Traffic[0].RevisionName == revisionName {
-				if r.Status.Traffic[0].Percent != 100 {
-					return true, fmt.Errorf("Expected 100%% of traffic to go to Revision %s but actually is %d", revisionName, r.Status.Traffic[0].Percent)
-				}
-				if r.Status.Traffic[0].Name != routeName {
-					return true, fmt.Errorf("Expected traffic name to be %s but actually is %s", revisionName, r.Status.Traffic[0].Name)
+		for _, tt := range r.Status.Traffic {
+			if tt.RevisionName == revisionName {
+				if r.Status.Traffic[0].Percent == 100 {
+					if r.Status.Traffic[0].Name != routeName {
+						return true, fmt.Errorf("Expected traffic name to be %s but actually is %s", revisionName, r.Status.Traffic[0].Name)
+					}
 				}
 				return true, nil
 			}