diff --git a/pkg/controller/route/route.go b/pkg/controller/route/route.go index 3b0b52ec9335..c74dda325d0e 100644 --- a/pkg/controller/route/route.go +++ b/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{}) diff --git a/pkg/controller/route/route_test.go b/pkg/controller/route/route_test.go index c96c71fb0993..3c1a2400cb0b 100644 --- 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", 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 diff --git a/test/states.go b/test/states.go index a9cd281896f2..dac766fbbd4b 100644 --- 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 }