address sonarqube

Signed-off-by: Andrew Brown <[email protected]>

Author: andrewjamesbrown

rollout/replicaset.go

@@ -416,48 +416,46 @@ func getIstioDestinationRuleSpec(ro *v1alpha1.Rollout) *v1alpha1.IstioDestinatio
 	return ro.Spec.Strategy.Canary.TrafficRouting.Istio.DestinationRule
 }
 
+// subsetReferencesHash checks if a single subset's labels contain the given pod template hash.
+func subsetReferencesHash(subset map[string]interface{}, rsPodHash string) bool {
+	labels, found, err := unstructured.NestedStringMap(subset, "labels")
+	if err != nil || !found {
+		return false
+	}
+	hash, ok := labels[v1alpha1.DefaultRolloutUniqueLabelKey]
+	return ok && hash == rsPodHash
+}
+
 // isReplicaSetReferencedByIstioDestinationRule checks if the given pod template hash is still
 // referenced by any subset in the Istio DestinationRule. This prevents scaling down a ReplicaSet
 // that is still receiving traffic via Istio subset-level routing.
 func (c *rolloutContext) isReplicaSetReferencedByIstioDestinationRule(rsPodHash string) bool {
 	dRuleSpec := getIstioDestinationRuleSpec(c.rollout)
-	if dRuleSpec == nil {
+	if dRuleSpec == nil || c.IstioController == nil || c.IstioController.DestinationRuleLister == nil {
 		return false
 	}
 
-	if c.IstioController == nil || c.IstioController.DestinationRuleLister == nil {
-		return false
-	}
 	dRuleUn, err := c.IstioController.DestinationRuleLister.Namespace(c.rollout.Namespace).Get(dRuleSpec.Name)
 	if err != nil {
 		if k8serrors.IsNotFound(err) {
 			return false
 		}
-		// If we can't get the DestinationRule, err on the side of caution
+		// For unexpected errors, err on the side of caution and assume it's still referenced.
 		c.log.Warnf("Failed to get DestinationRule %s: %v", dRuleSpec.Name, err)
 		return true
 	}
 
-	// Extract subsets from the DestinationRule
 	subsets, found, err := unstructured.NestedSlice(dRuleUn.UnstructuredContent(), "spec", "subsets")
 	if err != nil || !found {
 		return false
 	}
 
 	for _, subsetObj := range subsets {
 		subset, ok := subsetObj.(map[string]interface{})
-		if !ok {
-			continue
-		}
-		labels, found, err := unstructured.NestedStringMap(subset, "labels")
-		if err != nil || !found {
-			continue
-		}
-		if hash, ok := labels[v1alpha1.DefaultRolloutUniqueLabelKey]; ok && hash == rsPodHash {
+		if ok && subsetReferencesHash(subset, rsPodHash) {
 			c.log.Infof("ReplicaSet with hash %s is still referenced by DestinationRule %s", rsPodHash, dRuleSpec.Name)
 			return true
 		}
 	}
-
 	return false
 }

rollout/replicaset_test.go

@@ -590,6 +590,8 @@ func TestIsReplicaSetReferenced(t *testing.T) {
 }
 
 func TestIsReplicaSetReferencedByIstioDestinationRule(t *testing.T) {
+	const testDestRuleName = "test-destrule"
+
 	newRSWithPodTemplateHash := func(hash string) *appsv1.ReplicaSet {
 		return &appsv1.ReplicaSet{
 			ObjectMeta: metav1.ObjectMeta{
@@ -686,47 +688,47 @@ spec:
 		},
 		{
 			name:            "destination rule references hash - should return true",
-			rollout:         newRolloutWithIstioDestinationRule("test-destrule"),
-			destinationRule: newDestinationRuleYAML("test-destrule", "abc123", "def456"),
+			rollout:         newRolloutWithIstioDestinationRule(testDestRuleName),
+			destinationRule: newDestinationRuleYAML(testDestRuleName, "abc123", "def456"),
 			rsHash:          "abc123",
 			expectedResult:  true,
 		},
 		{
 			name:            "destination rule does not reference hash - should return false",
-			rollout:         newRolloutWithIstioDestinationRule("test-destrule"),
-			destinationRule: newDestinationRuleYAML("test-destrule", "abc123", "def456"),
+			rollout:         newRolloutWithIstioDestinationRule(testDestRuleName),
+			destinationRule: newDestinationRuleYAML(testDestRuleName, "abc123", "def456"),
 			rsHash:          "xyz789",
 			expectedResult:  false,
 		},
 		{
 			name:            "destination rule not found - should return false",
 			rollout:         newRolloutWithIstioDestinationRule("non-existent-destrule"),
-			destinationRule: newDestinationRuleYAML("test-destrule", "abc123", "def456"),
+			destinationRule: newDestinationRuleYAML(testDestRuleName, "abc123", "def456"),
 			rsHash:          "abc123",
 			expectedResult:  false,
 		},
 		{
 			name:              "no istio controller - should return false",
-			rollout:           newRolloutWithIstioDestinationRule("test-destrule"),
+			rollout:           newRolloutWithIstioDestinationRule(testDestRuleName),
 			noIstioController: true,
 			rsHash:            "abc123",
 			expectedResult:    false,
 		},
 		{
 			name:            "canary subset references hash - should return true",
-			rollout:         newRolloutWithIstioDestinationRule("test-destrule"),
-			destinationRule: newDestinationRuleYAML("test-destrule", "stable-hash", "canary-hash"),
+			rollout:         newRolloutWithIstioDestinationRule(testDestRuleName),
+			destinationRule: newDestinationRuleYAML(testDestRuleName, "stable-hash", "canary-hash"),
 			rsHash:          "canary-hash",
 			expectedResult:  true,
 		},
 		{
 			name:    "destination rule with no subsets - should return false",
-			rollout: newRolloutWithIstioDestinationRule("test-destrule"),
+			rollout: newRolloutWithIstioDestinationRule(testDestRuleName),
 			destinationRule: `
 apiVersion: networking.istio.io/v1alpha3
 kind: DestinationRule
 metadata:
-  name: test-destrule
+  name: ` + testDestRuleName + `
   namespace: default
 spec:
   host: test-service
@@ -736,12 +738,12 @@ spec:
 		},
 		{
 			name:    "destination rule with subset missing labels - should return false",
-			rollout: newRolloutWithIstioDestinationRule("test-destrule"),
+			rollout: newRolloutWithIstioDestinationRule(testDestRuleName),
 			destinationRule: `
 apiVersion: networking.istio.io/v1alpha3
 kind: DestinationRule
 metadata:
-  name: test-destrule
+  name: ` + testDestRuleName + `
   namespace: default
 spec:
   host: test-service