Also provision empty route rules for inactive revisions.

This appears to make Istio happy, and if we switch traffic to them later we won't cause Istio to throw spurious 503s. See istio/istio#5204.
knative · May 8, 2018 · 53cf4ec · 53cf4ec
1 parent 2b3ee4f
commit 53cf4ec
Show file tree

Hide file tree

Showing 3 changed files with 127 additions and 35 deletions.
diff --git a/pkg/controller/route/route.go b/pkg/controller/route/route.go
@@ -352,7 +352,6 @@ func (c *Controller) updateRouteEvent(key string) error {
 
 	// Call syncTrafficTargetsAndUpdateRouteStatus, which also updates the Route.Status
 	// to contain the domain we will use for Ingress creation.
-
 	if _, err = c.syncTrafficTargetsAndUpdateRouteStatus(route); err != nil {
 		return err
 	}
@@ -387,14 +386,12 @@ func (c *Controller) syncTrafficTargetsAndUpdateRouteStatus(route *v1alpha1.Rout
 	if err := c.extendRevisionsWithIndirectTrafficTargets(route, configMap, revMap); err != nil {
 		return nil, err
 	}
-
 	if err := c.deleteLabelForOutsideOfGivenConfigurations(route, configMap); err != nil {
 		return nil, err
 	}
 	if err := c.setLabelForGivenConfigurations(route, configMap); err != nil {
 		return nil, err
 	}
-
 	if err := c.deleteLabelForOutsideOfGivenRevisions(route, revMap); err != nil {
 		return nil, err
 	}
@@ -503,7 +500,6 @@ func (c *Controller) getDirectTrafficTargets(route *v1alpha1.Route) (
 			revMap[revName] = rev
 		}
 	}
-
 	return configMap, revMap, nil
 }
 
@@ -725,43 +721,59 @@ func (c *Controller) computeRevisionRoutes(
 	glog.Infof("Figuring out routes for Route: %s", route.Name)
 	ns := route.Namespace
 	elaNS := controller.GetElaNamespaceName(ns)
-	ret := []RevisionRoute{}
-
+	revClient := c.elaclientset.ElafrosV1alpha1().Revisions(ns)
+	revRoutes := []RevisionRoute{}
 	for _, tt := range route.Spec.Traffic {
-		var rev *v1alpha1.Revision
+		var targetRev *v1alpha1.Revision
 		var err error
+		configName := tt.ConfigurationName
 		revName := tt.RevisionName
 		if tt.ConfigurationName != "" {
 			// Get the configuration's LatestReadyRevisionName
-			revName = configMap[tt.ConfigurationName].Status.LatestReadyRevisionName
-			if revName == "" {
-				glog.Errorf("Configuration %s is not ready. Should skip updating route rules",
-					tt.ConfigurationName)
+			latestReadyRevName := configMap[tt.ConfigurationName].Status.LatestReadyRevisionName
+			if latestReadyRevName == "" {
+				glog.Errorf("Configuration %s is not ready. Should skip updating route rules", configName)
 				return nil, nil
 			}
-			// Revision has been already fetched indirectly in extendRevisionsWithIndirectTrafficTargets
-			rev = revMap[revName]
-
+			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 rev.Name == latestReadyRevName {
+					// This is the latest active revision, which we will deal with later.
+					targetRev = rev.DeepCopy()
+				} else if _, ok := revMap[rev.Name]; !ok {
+					// 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,
+					})
+				}
+			}
 		} else {
 			// Direct revision has already been fetched
-			rev = revMap[revName]
+			targetRev = revMap[revName]
 		}
-		//TODO(grantr): What should happen if revisionName is empty?
-
-		if rev == nil {
-			// For safety, which should never happen.
+		if targetRev == nil {
+			// If we are provided the correct revMap map, this should not happen.
 			glog.Errorf("Failed to fetch Revision %s: %s", revName, err)
 			return nil, err
 		}
-		rr := RevisionRoute{
+		revRoutes = append(revRoutes, RevisionRoute{
 			Name:         tt.Name,
-			RevisionName: rev.Name,
-			Service:      fmt.Sprintf("%s.%s", rev.Status.ServiceName, elaNS),
+			RevisionName: targetRev.Name,
+			Service:      fmt.Sprintf("%s.%s", targetRev.Status.ServiceName, elaNS),
 			Weight:       tt.Percent,
-		}
-		ret = append(ret, rr)
+		})
 	}
-	return ret, nil
+	return revRoutes, nil
 }
 
 func (c *Controller) createOrUpdateRouteRules(route *v1alpha1.Route, configMap map[string]*v1alpha1.Configuration,

diff --git a/pkg/controller/route/route_test.go b/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", otherRev.Name),
+				},
+				Weight: 0,
+			},
+			v1alpha2.DestinationWeight{
+				Destination: v1alpha2.IstioService{
+					Name: fmt.Sprintf("%s-service.test", latestReadyRev.Name),
+				},
+				Weight: 100,
+			},
+		},
+	}
+
+	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

diff --git a/test/states.go b/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
 			}