feat: Adding support for defining max destination weights in Istio VS

Signed-off-by: Rohit Agrawal <[email protected]>

docs/getting-started/istio/index.md

@@ -33,6 +33,8 @@ spec:
           virtualService:
             # Reference to a VirtualService which the controller updates with canary weights
             name: rollouts-demo-vsvc
+            # Optional if there are only two destinations in your routes or if you want to split 100% traffic between stable and canary services. If specified, this will be used as an upper bound for traffic between canary + stable services.
+            maxWeight: 80
             # Optional if there is a single HTTP route in the VirtualService, otherwise required
             routes:
             - http-primary
@@ -71,11 +73,14 @@ spec:
   - name: http-primary  # Should match spec.strategy.canary.trafficRouting.istio.virtualService.routes
     route:
     - destination:
-        host: rollouts-demo-stable  # Should match spec.strategy.canary.stableService
-      weight: 100
+        host: rollouts-demo-stable  # Should match rollout.spec.strategy.canary.stableService
+      weight: 80
     - destination:
         host: rollouts-demo-canary  # Should match spec.strategy.canary.canaryService
       weight: 0
+    - destination:
+        host: rollouts-demo-legacy
+      weight: 20
   tls:
   - match:
     - port: 3000  # Should match the port number of the route defined in spec.strategy.canary.trafficRouting.istio.virtualService.tlsRoutes
@@ -84,11 +89,14 @@ spec:
       - localhost
     route:
     - destination:
-        host: rollouts-demo-stable  # Should match spec.strategy.canary.stableService
-      weight: 100
+        host: rollouts-demo-stable  # Should match rollout.spec.strategy.canary.stableService
+      weight: 80
     - destination:
         host: rollouts-demo-canary  # Should match spec.strategy.canary.canaryService
       weight: 0
+    - destination:
+        host: rollouts-demo-legacy
+      weight: 20
 ```
 
 Run the following commands to deploy:

manifests/crds/rollout-crd.yaml

@@ -530,6 +530,9 @@ spec:
                                 - name
                                 - stableSubsetName
                                 type: object
+                              maxWeight:
+                                format: int64
+                                type: integer
                               virtualService:
                                 properties:
                                   name:

manifests/install.yaml

@@ -10228,6 +10228,9 @@ spec:
                                 - name
                                 - stableSubsetName
                                 type: object
+                              maxWeight:
+                                format: int64
+                                type: integer
                               virtualService:
                                 properties:
                                   name:

manifests/namespace-install.yaml

@@ -10228,6 +10228,9 @@ spec:
                                 - name
                                 - stableSubsetName
                                 type: object
+                              maxWeight:
+                                format: int64
+                                type: integer
                               virtualService:
                                 properties:
                                   name:

pkg/apiclient/rollout/rollout.swagger.json

@@ -833,6 +833,11 @@
         "destinationRule": {
           "$ref": "#/definitions/github.com.argoproj.argo_rollouts.pkg.apis.rollouts.v1alpha1.IstioDestinationRule",
           "title": "DestinationRule references an Istio DestinationRule to modify to shape traffic"
+        },
+        "maxWeight": {
+          "type": "string",
+          "format": "int64",
+          "description": "Max weight that will be split between canary and stable services. If unset, it defaults to 100."
         }
       },
       "title": "IstioTrafficRouting configuration for Istio service mesh to enable fine grain configuration"

pkg/apis/rollouts/v1alpha1/types.go

@@ -368,6 +368,8 @@ type IstioTrafficRouting struct {
 	VirtualService IstioVirtualService `json:"virtualService" protobuf:"bytes,1,opt,name=virtualService"`
 	// DestinationRule references an Istio DestinationRule to modify to shape traffic
 	DestinationRule *IstioDestinationRule `json:"destinationRule,omitempty" protobuf:"bytes,2,opt,name=destinationRule"`
+	// Max weight that will be split between canary and stable services. If unset, it defaults to 100.
+	MaxWeight int64 `json:"maxWeight,omitempty" protobuf:"bytes,3,opt,name=maxWeight"`
 }
 
 // IstioVirtualService holds information on the virtual service the rollout needs to modify

pkg/apis/rollouts/validation/validation_references_test.go

@@ -379,7 +379,7 @@ func TestValidateVirtualService(t *testing.T) {
 		vsvc := unstructured.StrToUnstructuredUnsafe(failCaseVsvc)
 		allErrs := ValidateVirtualService(ro, *vsvc)
 		assert.Len(t, allErrs, 1)
-		expectedErr := field.Invalid(field.NewPath("spec", "strategy", "canary", "trafficRouting", "istio", "virtualService", "name"), "istio-vsvc-name", "Istio VirtualService has invalid HTTP routes. Error: Stable Service 'stable' not found in route")
+		expectedErr := field.Invalid(field.NewPath("spec", "strategy", "canary", "trafficRouting", "istio", "virtualService", "name"), "istio-vsvc-name", "Istio VirtualService has invalid HTTP routes. Error: Stable Service 'stable' not found in the route.")
 		assert.Equal(t, expectedErr.Error(), allErrs[0].Error())
 
 	})

rollout/trafficrouting/istio/istio.go

@@ -101,11 +101,21 @@ func (patches virtualServicePatches) patchVirtualService(httpRoutes []interface{
 	return nil
 }
 
+func getSanitizedMaxWeight(maxWeight int64) int64 {
+	// Ignore any invalid values for maxWeight by setting it to the default (100)
+	sanitizedMaxWeight := maxWeight
+	if sanitizedMaxWeight <= 0 || sanitizedMaxWeight >= 100 {
+		sanitizedMaxWeight = 100
+	}
+	return sanitizedMaxWeight
+}
+
 func (r *Reconciler) generateVirtualServicePatches(httpRoutes []VirtualServiceHTTPRoute, tlsRoutes []VirtualServiceTLSRoute, desiredWeight int64) virtualServicePatches {
 	canarySvc := r.rollout.Spec.Strategy.Canary.CanaryService
 	stableSvc := r.rollout.Spec.Strategy.Canary.StableService
 	canarySubset := ""
 	stableSubset := ""
+	maxWeight := getSanitizedMaxWeight(r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.MaxWeight)
 	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
@@ -127,19 +137,19 @@ func (r *Reconciler) generateVirtualServicePatches(httpRoutes []VirtualServiceHT
 		if len(httpRoutes) <= routeIdx {
 			break
 		}
-		patches = processRoutes(Http, routeIdx, httpRoutes[routeIdx].Route, desiredWeight, svcSubsets, patches)
+		patches = processRoutes(Http, routeIdx, httpRoutes[routeIdx].Route, desiredWeight, maxWeight, svcSubsets, patches)
 	}
 	// Process TLS Routes
 	for _, routeIdx := range tlsRouteIndexesToPatch {
 		if len(tlsRoutes) <= routeIdx {
 			break
 		}
-		patches = processRoutes(Tls, routeIdx, tlsRoutes[routeIdx].Route, desiredWeight, svcSubsets, patches)
+		patches = processRoutes(Tls, routeIdx, tlsRoutes[routeIdx].Route, desiredWeight, maxWeight, svcSubsets, patches)
 	}
 	return patches
 }
 
-func processRoutes(routeType string, routeIdx int, destinations []VirtualServiceRouteDestination, desiredWeight int64, svcSubsets svcSubsets, patches virtualServicePatches) virtualServicePatches {
+func processRoutes(routeType string, routeIdx int, destinations []VirtualServiceRouteDestination, desiredWeight, maxWeight int64, svcSubsets svcSubsets, patches virtualServicePatches) virtualServicePatches {
 	for idx, destination := range destinations {
 		host := getHost(destination)
 		subset := destination.Destination.Subset
@@ -148,7 +158,7 @@ func processRoutes(routeType string, routeIdx int, destinations []VirtualService
 			patches = appendPatch(routeIdx, routeType, weight, desiredWeight, idx, patches)
 		}
 		if (host != "" && host == svcSubsets.stableSvc) || (subset != "" && subset == svcSubsets.stableSubset) {
-			patches = appendPatch(routeIdx, routeType, weight, 100-desiredWeight, idx, patches)
+			patches = appendPatch(routeIdx, routeType, weight, maxWeight-desiredWeight, idx, patches)
 		}
 	}
 	return patches
@@ -659,7 +669,7 @@ func ValidateHTTPRoutes(r *v1alpha1.Rollout, httpRoutes []VirtualServiceHTTPRout
 	}
 	for _, routeIndex := range routeIndexesToPatch {
 		route := httpRoutes[routeIndex]
-		err := validateVirtualServiceRouteDestinations(route.Route, stableSvc, canarySvc, r.Spec.Strategy.Canary.TrafficRouting.Istio.DestinationRule)
+		err := validateVirtualServiceRouteDestinations(route.Route, stableSvc, canarySvc, r.Spec.Strategy.Canary.TrafficRouting.Istio.MaxWeight, r.Spec.Strategy.Canary.TrafficRouting.Istio.DestinationRule)
 		if err != nil {
 			return err
 		}
@@ -681,7 +691,7 @@ func ValidateTlsRoutes(r *v1alpha1.Rollout, tlsRoutes []VirtualServiceTLSRoute)
 	}
 	for _, routeIndex := range routeIndexesToPatch {
 		route := tlsRoutes[routeIndex]
-		err := validateVirtualServiceRouteDestinations(route.Route, stableSvc, canarySvc, r.Spec.Strategy.Canary.TrafficRouting.Istio.DestinationRule)
+		err := validateVirtualServiceRouteDestinations(route.Route, stableSvc, canarySvc, r.Spec.Strategy.Canary.TrafficRouting.Istio.MaxWeight, r.Spec.Strategy.Canary.TrafficRouting.Istio.DestinationRule)
 		if err != nil {
 			return err
 		}
@@ -694,50 +704,59 @@ func ValidateTlsRoutes(r *v1alpha1.Rollout, tlsRoutes []VirtualServiceTLSRoute)
 
 // validateVirtualServiceRouteDestinations ensures there are two destinations within a route and
 // verifies that there is both a canary and a stable host or subset specified
-func validateVirtualServiceRouteDestinations(hr []VirtualServiceRouteDestination, stableSvc, canarySvc string, dRule *v1alpha1.IstioDestinationRule) error {
-	if len(hr) != 2 {
-		return fmt.Errorf("Route does not have exactly two route destinations.")
-	}
+func validateVirtualServiceRouteDestinations(hr []VirtualServiceRouteDestination, stableSvc, canarySvc string, maxWeight int64, dRule *v1alpha1.IstioDestinationRule) error {
 	hasStableSvc := false
 	hasCanarySvc := false
 	hasStableSubset := false
 	hasCanarySubset := false
+
+	var fixedWeight int64 = 0
 	for _, r := range hr {
 		host := getHost(r)
-
-		if stableSvc != "" && host == stableSvc {
-			hasStableSvc = true
-		}
-
-		if canarySvc != "" && host == canarySvc {
-			hasCanarySvc = true
-		}
 		if dRule != nil {
 			if dRule.StableSubsetName != "" && r.Destination.Subset == dRule.StableSubsetName {
 				hasStableSubset = true
+				continue
 			}
 			if dRule.CanarySubsetName != "" && r.Destination.Subset == dRule.CanarySubsetName {
 				hasCanarySubset = true
+				continue
 			}
+		} else if stableSvc != "" && host == stableSvc {
+			hasStableSvc = true
+			continue
+		} else if canarySvc != "" && host == canarySvc {
+			hasCanarySvc = true
+			continue
 		}
+		// This VS route is not a stable/canary route.
+		fixedWeight += r.Weight
 	}
-	return validateDestinationRule(dRule, hasCanarySubset, hasStableSubset, hasCanarySvc, hasStableSvc, canarySvc, stableSvc)
+
+	err := validateDestinationRule(dRule, hasCanarySubset, hasStableSubset, hasCanarySvc, hasStableSvc, canarySvc, stableSvc)
+	if err != nil {
+		return err
+	} else if 100-getSanitizedMaxWeight(maxWeight) != fixedWeight {
+		// Assert whether fixed wight is equal to the difference of 100 - maxWeight
+		return fmt.Errorf("VirtualService destination weights doesn't add upto 100.")
+	}
+	return nil
 }
 
 func validateDestinationRule(dRule *v1alpha1.IstioDestinationRule, hasCanarySubset, hasStableSubset, hasCanarySvc, hasStableSvc bool, canarySvc, stableSvc string) error {
 	if dRule != nil {
 		if !hasCanarySubset {
-			return fmt.Errorf("Canary DestinationRule subset '%s' not found in route", dRule.CanarySubsetName)
+			return fmt.Errorf("Canary DestinationRule subset '%s' not found in the route.", dRule.CanarySubsetName)
 		}
 		if !hasStableSubset {
-			return fmt.Errorf("Stable DestinationRule subset '%s' not found in route", dRule.StableSubsetName)
+			return fmt.Errorf("Stable DestinationRule subset '%s' not found in the route.", dRule.StableSubsetName)
 		}
 	} else {
 		if !hasCanarySvc {
-			return fmt.Errorf("Canary Service '%s' not found in route", canarySvc)
+			return fmt.Errorf("Canary Service '%s' not found in the route.", canarySvc)
 		}
 		if !hasStableSvc {
-			return fmt.Errorf("Stable Service '%s' not found in route", stableSvc)
+			return fmt.Errorf("Stable Service '%s' not found in the route.", stableSvc)
 		}
 	}
 	return nil