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.

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

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
Open
2 tasks done
Labels
bug Something isn't working

Comments

@jeanmorais
Copy link

jeanmorais commented Oct 9, 2024

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:

// 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 👍.

@jeanmorais jeanmorais added the bug Something isn't working label Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant