From 8b53b2c2d4e613ba5ff7f763c391ef22ee7b06f5 Mon Sep 17 00:00:00 2001
From: csamant-salesforce
 <80490530+csamant-salesforce@users.noreply.github.com>
Date: Mon, 26 Jul 2021 10:34:14 -0700
Subject: [PATCH] fix: Abort rollout doesn't remove all canary pods for
 setCanaryScale (#1352)

Signed-off-by: Chinmoy Samant <chinmoy.samant@csamant-ltm1.internal.salesforce.com>
---
 ...-rollout-abort-delete-all-canary-pods.yaml | 90 +++++++++++++++++++
 test/e2e/istio_test.go                        | 29 ++++++
 utils/replicaset/canary.go                    |  4 +
 3 files changed, 123 insertions(+)
 create mode 100644 test/e2e/istio/istio-rollout-abort-delete-all-canary-pods.yaml

diff --git a/test/e2e/istio/istio-rollout-abort-delete-all-canary-pods.yaml b/test/e2e/istio/istio-rollout-abort-delete-all-canary-pods.yaml
new file mode 100644
index 0000000000..29e0dee1aa
--- /dev/null
+++ b/test/e2e/istio/istio-rollout-abort-delete-all-canary-pods.yaml
@@ -0,0 +1,90 @@
+apiVersion: v1
+kind: Service
+metadata:
+  name: istio-host-split-canary
+spec:
+  ports:
+    - port: 80
+      targetPort: http
+      protocol: TCP
+      name: http
+  selector:
+    app: istio-host-split
+
+---
+apiVersion: v1
+kind: Service
+metadata:
+  name: istio-host-split-stable
+spec:
+  ports:
+    - port: 80
+      targetPort: http
+      protocol: TCP
+      name: http
+  selector:
+    app: istio-host-split
+
+---
+apiVersion: networking.istio.io/v1alpha3
+kind: VirtualService
+metadata:
+  name: istio-host-split-vsvc
+spec:
+  hosts:
+    - istio-host-split
+  http:
+    - name: primary
+      route:
+        - destination:
+            host: istio-host-split-stable
+          weight: 100
+        - destination:
+            host: istio-host-split-canary
+          weight: 0
+
+---
+apiVersion: argoproj.io/v1alpha1
+kind: Rollout
+metadata:
+  name: istio-host-split
+spec:
+  replicas: 5
+  strategy:
+    canary:
+      canaryService: istio-host-split-canary
+      stableService: istio-host-split-stable
+      trafficRouting:
+        istio:
+          virtualService:
+            name: istio-host-split-vsvc
+            routes:
+              - primary
+      steps:
+        - setCanaryScale:
+            replicas: 2
+        - setWeight: 20
+        - pause: {}
+        - setCanaryScale:
+            replicas: 4
+        - setWeight: 40
+        - pause: {}
+  selector:
+    matchLabels:
+      app: istio-host-split
+  template:
+    metadata:
+      labels:
+        app: istio-host-split
+    spec:
+      containers:
+        - name: istio-host-split
+          image: nginx:1.19-alpine
+          ports:
+            - name: http
+              containerPort: 80
+              protocol: TCP
+          resources:
+            requests:
+              memory: 16Mi
+              cpu: 5m
diff --git a/test/e2e/istio_test.go b/test/e2e/istio_test.go
index 967abbbb03..2a01158be7 100644
--- a/test/e2e/istio_test.go
+++ b/test/e2e/istio_test.go
@@ -197,3 +197,32 @@ func (s *IstioSuite) TestIstioSubsetSplitSingleRoute() {
 		}).
 		ExpectRevisionPodCount("1", 1) // don't scale down old replicaset since it will be within scaleDownDelay
 }
+
+func (s *IstioSuite) TestIstioAbortUpdateDeleteAllCanaryPods() {
+	s.Given().
+		RolloutObjects("@istio/istio-rollout-abort-delete-all-canary-pods.yaml").
+		When().
+		ApplyManifests().
+		WaitForRolloutStatus("Healthy").
+		Then().
+		When().
+		UpdateSpec().
+		WaitForRolloutStatus("Paused").
+		Then().
+		ExpectRevisionPodCount("2", 2).
+		When().
+		PromoteRollout().
+		WaitForRolloutStatus("Paused").
+		Then().
+		When().
+		PromoteRollout().
+		WaitForRolloutStatus("Paused").
+		Then().
+		ExpectRevisionPodCount("2", 4).
+		When().
+		AbortRollout().
+		WaitForRolloutStatus("Degraded").
+		Then().
+		ExpectRevisionPodCount("2", 0).
+		ExpectRevisionPodCount("1", 5)
+}
diff --git a/utils/replicaset/canary.go b/utils/replicaset/canary.go
index 701c842a26..69ce4ca2cf 100644
--- a/utils/replicaset/canary.go
+++ b/utils/replicaset/canary.go
@@ -331,6 +331,10 @@ func GetCurrentSetWeight(rollout *v1alpha1.Rollout) int32 {
 // TrafficRouting is required to be set for SetCanaryScale to be applicable.
 // If MatchTrafficWeight is set after a previous SetCanaryScale step, it will likewise be ignored.
 func UseSetCanaryScale(rollout *v1alpha1.Rollout) *v1alpha1.SetCanaryScale {
+	// Return nil when rollout is aborted
+	if rollout.Status.Abort {
+		return nil
+	}
 	currentStep, currentStepIndex := GetCurrentCanaryStep(rollout)
 	if currentStep == nil {
 		return nil