feat: allow VirtualService HTTPRoute to be inferred if there is singl…

…e route (argoproj#1273)

Signed-off-by: Jesse Suen <[email protected]>

docs/features/specification.md

@@ -274,7 +274,7 @@ spec:
           virtualService: 
             name: rollout-vsvc  # required
             routes:
-            - primary # At least one route is required
+            - primary # optional if there is a single route in VirtualService, required otherwise
 
         # NGINX Ingress Controller routing configuration
         nginx:

docs/features/traffic-management/istio.md

@@ -17,7 +17,7 @@ stable ReplicaSet. Istio provides two approaches for weighted traffic splitting,
 are available as options in Argo Rollouts:
 
 1. [Host-level traffic splitting](#host-level-traffic-splitting)
-2. [Subset-lvel traffic splitting](#subset-level-traffic-splitting)
+2. [Subset-level traffic splitting](#subset-level-traffic-splitting)
 
 ## Host-level Traffic Splitting
 
@@ -49,7 +49,7 @@ spec:
           virtualService: 
             name: rollout-vsvc   # required
             routes:
-            - primary            # required
+            - primary            # optional if there is a single route in VirtualService, required otherwise
       steps:
       - setWeight: 5
       - pause:
@@ -60,7 +60,7 @@ The VirtualService must contain an HTTP route with a name referenced in the Roll
 two route destinations with `host` values that match the `canaryService` and `stableService` 
 referenced in the Rollout.  If the VirtualService is defined in a different namespace than the rollout,
 its name should be `rollout-vsvc.<vsvc namespace name>`. Note that Istio requires that all weights add to
-100, so the initial weights can be be 100% to stable, and 0% to canary.
+100, so the initial weights can be 100% to stable, and 0% to canary.
 
 ```yaml
 apiVersion: networking.istio.io/v1alpha3
@@ -85,7 +85,7 @@ spec:
 
 Finally, a canary and stable Service should be deployed. The selector of these Services will be
 modified by the Rollout during an update to target the canary and stable ReplicaSet pods.
-Note that if virtualservice and destionation host reside in different namespaces (e.g., virtualservice and rollout are not in the same namespace), we should use FQDN as the destination host like `stable-svc.<namespace>`.
+Note that if the VirtualService and destination host resides in different namespaces (e.g., VirtualService and Rollout are not in the same namespace), the namespace should be included in the destination host (e.g. `stable-svc.<namespace>`).
 
 ```yaml
 apiVersion: v1
@@ -127,7 +127,7 @@ During the lifecycle of a Rollout update, Argo Rollouts will continuously:
 
 !!! note
 
-    Rollout does not make any other assumptions about the fields within the Virtual Service or the Istio mesh. The user could specify additional configurations for the virtual service like URI rewrite rules on the primary route or any other route if desired. The user can also create specific destination rules for each of the services. 
+    Rollout does not make any other assumptions about the fields within the VirtualService or the Istio mesh. The user could specify additional configurations for the VirtualService like URI rewrite rules on the primary route or any other route if desired. The user can also create specific DestinationRules for each of the services. 
 
 
 ## Subset-level Traffic Splitting
@@ -162,7 +162,7 @@ spec:
           virtualService: 
             name: rollout-vsvc        # required
             routes:
-            - primary                 # required
+            - primary                 # optional if there is a single route in VirtualService, required otherwise
           destinationRule:
             name: rollout-destrule    # required
             canarySubsetName: canary  # required
@@ -173,7 +173,7 @@ spec:
           duration: 10m
 ```
 
-A single service should be defined, which target the Rollout pods. Note that unlike the first
+A single service should be defined, which targets the Rollout pods. Note that unlike the first
 approach, where traffic splitting is against multiple Services which are modified to contain the
 rollout-pod-template-hash of the canary/stable ReplicaSets, this Service is not modified by
 the rollout controller.
@@ -195,8 +195,8 @@ spec:
 
 The VirtualService must contain an HTTP route with a name referenced in the Rollout, containing
 two route destinations with `subset` values that match the `canarySubsetName` and `stableSubsetName` 
-referenced in the Rollout. Note that Istio require that all weights add to 100, so the initial
-weights can be be 100% to stable, and 0% to canary.
+referenced in the Rollout. Note that Istio requires that all weights add to 100, so the initial
+weights can be 100% to stable, and 0% to canary.
 
 ```yaml
 apiVersion: networking.istio.io/v1alpha3
@@ -254,11 +254,11 @@ splitting.
 ### DNS requirements
 
 With host-level splitting, the VirtualService requires different `host` values to split among the
-two destinations. However, using two host values implies that there are different DNS names. For
-north-south traffic, which reach the service through the Istio Gateway, having multiple DNS names to
-reach the canary vs. stable pods may not matter. However, for east-west traffic that happen inside
-the cluster, it forces microservice-to-microservice communication to choose whether to hit the
-stable or the canary DNS name, go through the gateway, or add DNS entries for the virtualservices.
+two destinations. However, using two host values implies the use of different DNS names (one for
+the canary, the other for the stable). For north-south traffic, which reaches the Service through
+the Istio Gateway, having multiple DNS names to reach the canary vs. stable pods may not matter.
+However, for east-west or intra-cluster traffic, it forces microservice-to-microservice communication to choose whether to hit the
+stable or the canary DNS name, go through the gateway, or add DNS entries for the VirtualServices.
 In this situation, the DestinationRule subset traffic splitting would be a better option for
 intra-cluster canarying.
 
@@ -358,7 +358,7 @@ other controllers (e.g. Argo Rollouts) controller manage them instead.
 
 An early design alternative was that instead of the controller modifying a referenced VirtualService, the Rollout controller would create, manage, and own a Virtual Service. While this approach is GitOps friendly, it introduces other issues:
 
-*  To provide the same flexibility as referencing VirtualService within a Rollout, the Rollout needs to inline a large portion of the Istio spec. However, networking is outside the responsibility of the Rollout and makes the Rollout spec unnecessary complicated.
+*  To provide the same flexibility as referencing VirtualService within a Rollout, the Rollout needs to inline a large portion of the Istio spec. However, networking is outside the responsibility of the Rollout and makes the Rollout spec unnecessarily complicated.
 * If Istio introduces a feature, that feature will not be available in Argo Rollouts until implemented within Argo Rollouts.
 
 Both of these issues adds more complexity to the users and Argo Rollouts developers compared to referencing a Virtual Service.

docs/getting-started/istio/index.md

@@ -34,7 +34,7 @@ spec:
             # Reference to a VirtualService which the controller updates with canary weights
             name: rollouts-demo-vsvc
             routes:
-            - primary # At least one route is required
+            - primary # optional if there is a single route in VirtualService, required otherwise
 ...
 ```
 

manifests/crds/rollout-crd.yaml

@@ -525,7 +525,6 @@ spec:
                                     type: array
                                 required:
                                 - name
-                                - routes
                                 type: object
                             required:
                             - virtualService

manifests/install.yaml

@@ -10214,7 +10214,6 @@ spec:
                                     type: array
                                 required:
                                 - name
-                                - routes
                                 type: object
                             required:
                             - virtualService

manifests/namespace-install.yaml

@@ -10214,7 +10214,6 @@ spec:
                                     type: array
                                 required:
                                 - name
-                                - routes
                                 type: object
                             required:
                             - virtualService

pkg/apiclient/rollout/rollout.swagger.json

@@ -823,7 +823,7 @@
           "items": {
             "type": "string"
           },
-          "title": "Routes list of routes within VirtualService to edit"
+          "title": "Routes are list of routes within VirtualService to edit. If omitted, VirtualService must have a single route"
         }
       },
       "title": "IstioVirtualService holds information on the virtual service the rollout needs to modify"

pkg/apis/rollouts/v1alpha1/types.go

@@ -360,8 +360,8 @@ type IstioTrafficRouting struct {
 type IstioVirtualService struct {
 	// Name holds the name of the VirtualService
 	Name string `json:"name" protobuf:"bytes,1,opt,name=name"`
-	// Routes list of routes within VirtualService to edit
-	Routes []string `json:"routes" protobuf:"bytes,2,rep,name=routes"`
+	// Routes are list of routes within VirtualService to edit. If omitted, VirtualService must have a single route
+	Routes []string `json:"routes,omitempty" protobuf:"bytes,2,rep,name=routes"`
 }
 
 // IstioDestinationRule is a reference to an Istio DestinationRule to modify and shape traffic

pkg/apis/rollouts/validation/validation.go

@@ -50,8 +50,6 @@ const (
 	ScaleDownLimitLargerThanRevisionLimit = "This rollout's revision history limit can not be smaller than the rollout's scale down limit"
 	// InvalidTrafficRoutingMessage indicates that both canary and stable service must be set to use Traffic Routing
 	InvalidTrafficRoutingMessage = "Canary service and Stable service must to be set to use Traffic Routing"
-	// InvalidIstioRoutesMessage indicates that rollout does not have a route specified for the istio Traffic Routing
-	InvalidIstioRoutesMessage = "Istio virtual service must have at least 1 route specified"
 	// InvalidAnalysisArgsMessage indicates that arguments provided in analysis steps are refrencing un-supported metadatafield.
 	//supported fields are "metadata.annotations", "metadata.labels", "metadata.name", "metadata.namespace", "metadata.uid"
 	InvalidAnalysisArgsMessage = "Analyses arguments must refer to valid object metadata supported by downwardAPI"
@@ -213,9 +211,6 @@ func ValidateRolloutStrategyCanary(rollout *v1alpha1.Rollout, fldPath *field.Pat
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("canaryService"), canary.CanaryService, InvalidTrafficRoutingMessage))
 		}
 	}
-	if canary.TrafficRouting != nil && canary.TrafficRouting.Istio != nil && len(canary.TrafficRouting.Istio.VirtualService.Routes) == 0 {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("trafficRouting").Child("istio").Child("virtualService").Child("routes"), "[]", InvalidIstioRoutesMessage))
-	}
 
 	if canary.ScaleDownDelaySeconds != nil && canary.TrafficRouting == nil {
 		allErrs = append(allErrs, field.Invalid(fldPath.Child("scaleDownDelaySeconds"), *canary.ScaleDownDelaySeconds, InvalidCanaryScaleDownDelay))

rollout/trafficrouting/istio/istio.go

@@ -83,25 +83,21 @@ func (patches virtualServicePatches) patchVirtualService(httpRoutes []interface{
 func (r *Reconciler) generateVirtualServicePatches(httpRoutes []VirtualServiceHTTPRoute, desiredWeight int64) virtualServicePatches {
 	canarySvc := r.rollout.Spec.Strategy.Canary.CanaryService
 	stableSvc := r.rollout.Spec.Strategy.Canary.StableService
-	routes := map[string]bool{}
-	for _, r := range r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Routes {
-		routes[r] = true
-	}
 	canarySubset := ""
 	stableSubset := ""
 	if r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.DestinationRule != nil {
 		canarySubset = r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.DestinationRule.CanarySubsetName
 		stableSubset = r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.DestinationRule.StableSubsetName
 	}
 
+	// err can be ignored because we already called ValidateHTTPRoutes earlier
+	routeIndexesToPatch, _ := getRouteIndexesToPatch(r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Routes, httpRoutes)
+
 	patches := virtualServicePatches{}
-	for i := range httpRoutes {
-		route := httpRoutes[i]
-		if !routes[route.Name] {
-			continue
-		}
+	for _, routeIdx := range routeIndexesToPatch {
+		route := httpRoutes[routeIdx]
 		for j := range route.Route {
-			destination := httpRoutes[i].Route[j]
+			destination := route.Route[j]
 
 			var host string
 			if idx := strings.Index(destination.Destination.Host, "."); idx > 0 {
@@ -115,7 +111,7 @@ func (r *Reconciler) generateVirtualServicePatches(httpRoutes []VirtualServiceHT
 			if (host != "" && host == canarySvc) || (subset != "" && subset == canarySubset) {
 				if weight != desiredWeight {
 					patch := virtualServicePatch{
-						routeIndex:       i,
+						routeIndex:       routeIdx,
 						destinationIndex: j,
 						weight:           desiredWeight,
 					}
@@ -125,7 +121,7 @@ func (r *Reconciler) generateVirtualServicePatches(httpRoutes []VirtualServiceHT
 			if (host != "" && host == stableSvc) || (subset != "" && subset == stableSubset) {
 				if weight != 100-desiredWeight {
 					patch := virtualServicePatch{
-						routeIndex:       i,
+						routeIndex:       routeIdx,
 						destinationIndex: j,
 						weight:           100 - desiredWeight,
 					}
@@ -466,6 +462,7 @@ func (r *Reconciler) SetWeight(desiredWeight int32) error {
 	}
 	_, err = client.Update(ctx, modifiedVsvc, metav1.UpdateOptions{})
 	if err == nil {
+		r.log.Debugf("UpdatedVirtualService: %s", modifiedVsvc)
 		r.recorder.Eventf(r.rollout, record.EventOptions{EventReason: "UpdatedVirtualService"}, "VirtualService `%s` set to desiredWeight '%d'", vsvcName, desiredWeight)
 	}
 	return err
@@ -475,34 +472,48 @@ func (r *Reconciler) VerifyWeight(desiredWeight int32) (bool, error) {
 	return true, nil
 }
 
+// getRouteIndexesToPatch returns array indices of the httpRoutes which need to be patched when updating weights
+func getRouteIndexesToPatch(routeNames []string, httpRoutes []VirtualServiceHTTPRoute) ([]int, error) {
+	var routeIndexesToPatch []int
+	if len(routeNames) == 0 {
+		if len(httpRoutes) != 1 {
+			return nil, fmt.Errorf("VirtualService spec.http[] must have exactly one route when omitting spec.strategy.canary.trafficRouting.istio.virtualService.routes")
+		}
+		routeIndexesToPatch = append(routeIndexesToPatch, 0)
+	} else {
+		for _, routeName := range routeNames {
+			foundRoute := false
+			for i, route := range httpRoutes {
+				if route.Name == routeName {
+					routeIndexesToPatch = append(routeIndexesToPatch, i)
+					foundRoute = true
+					break
+				}
+			}
+			if !foundRoute {
+				return nil, fmt.Errorf("Route '%s' is not found", routeName)
+			}
+		}
+	}
+	return routeIndexesToPatch, nil
+}
+
 // validateHTTPRoutes ensures that all the routes in the rollout exist and they only have two destinations
 func ValidateHTTPRoutes(r *v1alpha1.Rollout, httpRoutes []VirtualServiceHTTPRoute) error {
-	routes := r.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Routes
 	stableSvc := r.Spec.Strategy.Canary.StableService
 	canarySvc := r.Spec.Strategy.Canary.CanaryService
 
-	routesPatched := map[string]bool{}
-	for _, route := range routes {
-		routesPatched[route] = false
-	}
-
-	for _, route := range httpRoutes {
-		// check if the httpRoute is in the list of routes from the rollout
-		if _, ok := routesPatched[route.Name]; ok {
-			routesPatched[route.Name] = true
-			err := validateVirtualServiceHTTPRouteDestinations(route, stableSvc, canarySvc, r.Spec.Strategy.Canary.TrafficRouting.Istio.DestinationRule)
-			if err != nil {
-				return err
-			}
-		}
+	routeIndexesToPatch, err := getRouteIndexesToPatch(r.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Routes, httpRoutes)
+	if err != nil {
+		return err
 	}
-
-	for i := range routesPatched {
-		if !routesPatched[i] {
-			return fmt.Errorf("Route '%s' is not found", i)
+	for _, routeIndex := range routeIndexesToPatch {
+		route := httpRoutes[routeIndex]
+		err := validateVirtualServiceHTTPRouteDestinations(route, stableSvc, canarySvc, r.Spec.Strategy.Canary.TrafficRouting.Istio.DestinationRule)
+		if err != nil {
+			return err
 		}
 	}
-
 	return nil
 }