[bug] prevent panic when finding out rollout strategy (#210)

* prevent panic when finding out rollout strategy Co-authored-by: Anubhav Aeron <anubhav_aeron@intuit.com> Signed-off-by: sa <sushanth_a@intuit.com>
istio-ecosystem · Jul 21, 2022 · a356f44 · a356f44
1 parent 833f921
commit a356f44
Show file tree

Hide file tree

Showing 2 changed files with 89 additions and 6 deletions.
diff --git a/admiral/pkg/clusters/serviceentry.go b/admiral/pkg/clusters/serviceentry.go
@@ -137,11 +137,7 @@ func modifyServiceEntryForNewServiceOrPod(event admiral.EventType, env string, s
 		localFqdn := serviceInstance.Name + common.Sep + serviceInstance.Namespace + common.DotLocalDomainSuffix
 		rc := remoteRegistry.RemoteControllers[sourceCluster]
 		var meshPorts map[string]uint32
-		isBlueGreenStrategy := false
-
-		if len(sourceRollouts) > 0 {
-			isBlueGreenStrategy = sourceRollouts[sourceCluster].Spec.Strategy.BlueGreen != nil
-		}
+		blueGreenStrategy := isBlueGreenStrategy(sourceRollouts[sourceCluster])
 
 		if len(sourceDeployments) > 0 {
 			meshPorts = GetMeshPorts(sourceCluster, serviceInstance, sourceDeployments[sourceCluster])
@@ -159,7 +155,7 @@ func modifyServiceEntryForNewServiceOrPod(event admiral.EventType, env string, s
 				//replace istio ingress-gateway address with local fqdn, note that ingress-gateway can be empty (not provisoned, or is not up)
 				if ep.Address == clusterIngress || ep.Address == "" {
 					// Update endpoints with locafqdn for active and preview se of bluegreen rollout
-					if isBlueGreenStrategy {
+					if blueGreenStrategy {
 						oldPorts := ep.Ports
 						updateEndpointsForBlueGreen(sourceRollouts[sourceCluster], sourceWeightedServices[sourceCluster], cnames, ep, sourceCluster, key)
 
@@ -761,3 +757,12 @@ func generateServiceEntry(event admiral.EventType, admiralCache *AdmiralCache, m
 
 	return tmpSe
 }
+
+func isBlueGreenStrategy(rollout *argo.Rollout) bool {
+	if rollout != nil && &rollout.Spec != (&argo.RolloutSpec{}) && rollout.Spec.Strategy != (argo.RolloutStrategy{}) {
+		if rollout.Spec.Strategy.BlueGreen != nil {
+			return true
+		}
+	}
+	return false
+}
diff --git a/admiral/pkg/clusters/serviceentry_test.go b/admiral/pkg/clusters/serviceentry_test.go
@@ -1339,3 +1339,81 @@ func isLower(s string) bool {
 	}
 	return true
 }
+
+func TestIsBlueGreenStrategy(t *testing.T) {
+	var (
+		emptyRollout                 *argo.Rollout
+		rolloutWithBlueGreenStrategy = &argo.Rollout{
+			Spec: argo.RolloutSpec{
+				Strategy: argo.RolloutStrategy{
+					BlueGreen: &argo.BlueGreenStrategy{
+						ActiveService: "active",
+					},
+				},
+			},
+		}
+		rolloutWithCanaryStrategy = &argo.Rollout{
+			Spec: argo.RolloutSpec{
+				Strategy: argo.RolloutStrategy{
+					Canary: &argo.CanaryStrategy{
+						CanaryService: "canaryservice",
+					},
+				},
+			},
+		}
+		rolloutWithNoStrategy = &argo.Rollout{
+			Spec: argo.RolloutSpec{},
+		}
+		rolloutWithEmptySpec = &argo.Rollout{}
+	)
+	cases := []struct {
+		name           string
+		rollout        *argo.Rollout
+		expectedResult bool
+	}{
+		{
+			name: "Given argo rollout is configured with blue green rollout strategy" +
+				"When isBlueGreenStrategy is called" +
+				"Then it should return true",
+			rollout:        rolloutWithBlueGreenStrategy,
+			expectedResult: true,
+		},
+		{
+			name: "Given argo rollout is configured with canary rollout strategy" +
+				"When isBlueGreenStrategy is called" +
+				"Then it should return false",
+			rollout:        rolloutWithCanaryStrategy,
+			expectedResult: false,
+		},
+		{
+			name: "Given argo rollout is configured without any rollout strategy" +
+				"When isBlueGreenStrategy is called" +
+				"Then it should return false",
+			rollout:        rolloutWithNoStrategy,
+			expectedResult: false,
+		},
+		{
+			name: "Given argo rollout is nil" +
+				"When isBlueGreenStrategy is called" +
+				"Then it should return false",
+			rollout:        emptyRollout,
+			expectedResult: false,
+		},
+		{
+			name: "Given argo rollout has an empty Spec" +
+				"When isBlueGreenStrategy is called" +
+				"Then it should return false",
+			rollout:        rolloutWithEmptySpec,
+			expectedResult: false,
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			result := isBlueGreenStrategy(c.rollout)
+			if result != c.expectedResult {
+				t.Errorf("expected: %t, got: %t", c.expectedResult, result)
+			}
+		})
+	}
+}