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

Jump to bottom

Patch in the VirtualService is not working when there is only one named route #3884

Open
2 tasks done
jeanmorais opened this issue Oct 9, 2024 · 0 comments · May be fixed by #4055
Open
2 tasks done

Patch in the VirtualService is not working when there is only one named route #3884

jeanmorais opened this issue Oct 9, 2024 · 0 comments · May be fixed by #4055
Labels
bug Something isn't working istio traffic-routing

Comments

@jeanmorais
Copy link

jeanmorais commented Oct 9, 2024 •
edited
Loading

Checklist:

  • I've included steps to reproduce the bug.
  • I've included the version of argo rollouts.

Describe the bug

When the VirtualService has only 1 HTTPRoute, with the defined name, and this name is not defined in Rollout (trafficRouting.istio.virtualService.routes), the patch is not performed.

To Reproduce

apiVersion: argoproj.io/v1alpha1
kind: Rollout
metadata:
  labels:
  name: canary-sample-app
  namespace: sample-canary
spec:
  replicas: 3
  selector:
    matchLabels:
      app: canary-sample-app
  strategy:
    canary:
      canaryMetadata:
        labels:
          canary: "true"
      maxSurge: 25%
      maxUnavailable: 0
      stableMetadata:
        labels:
          canary: "false"
      steps:
      - pause: {}
      - setWeight: 30
      - pause: {}
      - setWeight: 70
      - pause: {}
      trafficRouting:
        istio:
          destinationRule:
            canarySubsetName: canary
            name: canary-sample-app
            stableSubsetName: stable
          virtualService:
            name: canary-sample-app
---
apiVersion: networking.istio.io/v1beta1
kind: VirtualService
metadata:
  name: canary-sample-app
  namespace: sample-canary
spec:
  gateways:
  - canary-sample-app
  hosts:
  - canary-sample-app.domain.com
  http:
  - match:
    - uri:
        prefix: /
    name: default
    route:
    - destination:
        host: canary-sample-app
        port:
          number: 80
        subset: stable
      weight: 100
    - destination:
        host: canary-sample-app
        port:
          number: 80
        subset: canary
      weight: 0

The doc mentions that (optional if there is a single route in VirtualService, required otherwise):

      trafficRouting:
        istio:
          virtualService:
            name: rollout-vsvc   # required
            routes:
            - primary            # optional if there is a single route in VirtualService, required otherwise

Ref: https://argo-rollouts.readthedocs.io/en/stable/features/traffic-management/istio/#host-level-traffic-splitting

But it doesn't happen here:

argo-rollouts/rollout/trafficrouting/istio/istio.go

Lines 903 to 913 in 50300e5

// getHttpRouteIndexesToPatch returns array indices of the httpRoutes which need to be patched when updating weights
func getHttpRouteIndexesToPatch(routeNames []string, httpRoutes []VirtualServiceHTTPRoute) ([]int, error) {
//We have no routes listed in spec.strategy.canary.trafficRouting.istio.virtualService.routes so find index
//of the first empty named route
if len(routeNames) == 0 {
for i, route := range httpRoutes {
if route.Name == "" {
return []int{i}, nil
}
}
}

Maybe, the code should first check if the VirtualService has only one route, and then return it (I've tested it and it worked):

// something like this
	if len(routeNames) == 0 {
		if len(httpRoutes) == 1 {
			return []int{0}, nil
		}
		
		for i, route := range httpRoutes {
			if route.Name == "" {
				return []int{i}, nil
			}
		}
	}

Could you confirm if this is enough? I can open a PR with this fix.

Expected behavior

The weight field changed according to the promotion of the steps.

Version

v1.7.2

Message from the maintainers:

Impacted by this bug? Give it a 👍. We prioritize the issues with the most 👍.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working istio traffic-routing
Projects
None yet
Milestone
No milestone
2 participants
@jeanmorais @kostis-codefresh