Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
+174 −84
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions docs/getting-started/istio/index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like we are overusing the example in the getting-started page. Can we move this advanced use to https://argoproj.github.io/argo-rollouts/features/traffic-management/istio/ and leave the getting-started example straightfoward. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense. Will update!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR adds maxWeight prop at the level of the virtualService, therefore it will affect all the routes ArgoRollouts manage - which are specified in strategy.canary.trafficRouting.istio.virtualService.routes

While in our use case we'd want only specific routes to have less then 100 weight managed by ArgoRollouts.
Our Virtual Service has such routes

  • a route for the traffic originating in Mesh. Here we want e.g. 80% to be split between stable and canary in Mesh, and 20% to go to the legacy system.
  • a route for the traffic coming to Mesh Ingress Gateway from the Legacy system. Here we want 100% to be split between stable and canary in Mesh. (we don't want to bounce the traffic back to the legacy)

What do you think?

(maybe we can move the maxWeight to the level of the strategy.canary.trafficRouting.istio.virtualService.routes. Or remove maxWeight and configure ArgoRollouts to manage only the first 2 destinations in founds in a route...)

Copy link
Member Author

@agrawroh agrawroh Aug 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no straightforward way to do it at the per-route level. Currently, routes is nothing but an array of strings which take the HTTP route names. For us to do it at the route-level, we need a struct/object similar to what we have for tlsRoutes where we can add an optional maxWeight prop.

One proposal is to add an optional httpRoutes which will be an array of objects similar to the tlsRoutes and gradually deprecate the use of routes over time. In the beginning, we can enforce the use of only one i.e either routes or httpRoutes. I proposed this to @jessesuen as well.

@jessesuen @huikang Any suggestions on what would be the best way to move forward here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @agrawroh, I am interested in coming up with a solution for this as well. I would like to reopen this PR or something similar to support this feature.

@jessesuen @huikang are there any suggestions for moving forward ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dnguy078 Sure, that'd be great. Please feel free to take it over or let me know how I can help.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @agrawroh I am trying to solve an issue related to istio subset level in another PR as well. #3126 I was wondering if you could take a quick look :)

Copy link

@bakayolo bakayolo Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dnguy078 any update? This is a pretty big blocker for argo-rollouts adoption on our end.

Also note that even though this PR was not addressing this exact issue, it would have been incredibly helpful for single route virtual service.

# Optional if there is a single HTTP route in the VirtualService, otherwise required
routes:
- http-primary
Expand Down Expand Up @@ -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
Expand All @@ -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:
Expand Down
3 changes: 3 additions & 0 deletions manifests/crds/rollout-crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,9 @@ spec:
- name
- stableSubsetName
type: object
maxWeight:
format: int64
type: integer
virtualService:
properties:
name:
Expand Down
3 changes: 3 additions & 0 deletions manifests/install.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10228,6 +10228,9 @@ spec:
- name
- stableSubsetName
type: object
maxWeight:
format: int64
type: integer
virtualService:
properties:
name:
Expand Down
3 changes: 3 additions & 0 deletions manifests/namespace-install.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10228,6 +10228,9 @@ spec:
- name
- stableSubsetName
type: object
maxWeight:
format: int64
type: integer
virtualService:
properties:
name:
Expand Down
5 changes: 5 additions & 0 deletions pkg/apiclient/rollout/rollout.swagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 2 additions & 0 deletions pkg/apis/rollouts/v1alpha1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing // +optional

}

// IstioVirtualService holds information on the virtual service the rollout needs to modify
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/rollouts/validation/validation_references_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())

})
Expand Down
65 changes: 42 additions & 23 deletions rollout/trafficrouting/istio/istio.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
Expand Down
Loading