fix: check ephemeral metadata is set before delete (#4089)

* fix: check ephemeral metadata is set before delete Signed-off-by: Jordan Rodgers <[email protected]> * add test case Signed-off-by: Jordan Rodgers <[email protected]> * address test comments Signed-off-by: Jordan Rodgers <[email protected]> --------- Signed-off-by: Jordan Rodgers <[email protected]>
argoproj · Feb 4, 2025 · 06f2d66 · 06f2d66
1 parent 8a4a81a
commit 06f2d66
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 14 deletions.
diff --git a/utils/replicaset/canary.go b/utils/replicaset/canary.go
@@ -640,18 +640,26 @@ func SyncEphemeralPodMetadata(metadata *metav1.ObjectMeta, existingPodMetadata,
 	if existingPodMetadata != nil {
 		for k := range existingPodMetadata.Annotations {
 			if desiredPodMetadata == nil || !isMetadataStillDesired(k, desiredPodMetadata.Annotations) {
-				if metadata.Annotations != nil {
-					delete(metadata.Annotations, k)
-					modified = true
+				if metadata.Annotations == nil {
+					continue
 				}
+				if _, ok := metadata.Annotations[k]; !ok {
+					continue
+				}
+				delete(metadata.Annotations, k)
+				modified = true
 			}
 		}
 		for k := range existingPodMetadata.Labels {
 			if desiredPodMetadata == nil || !isMetadataStillDesired(k, desiredPodMetadata.Labels) {
-				if metadata.Labels != nil {
-					delete(metadata.Labels, k)
-					modified = true
+				if metadata.Labels == nil {
+					continue
 				}
+				if _, ok := metadata.Labels[k]; !ok {
+					continue
+				}
+				delete(metadata.Labels, k)
+				modified = true
 			}
 		}
 	}

diff --git a/utils/replicaset/canary_test.go b/utils/replicaset/canary_test.go
@@ -1339,14 +1339,19 @@ func TestSyncEphemeralPodMetadata(t *testing.T) {
 			"ddd": "444",
 		},
 	}
-	{
-		// verify modified is false if there are no changes
+	t.Run("verify modified is false if there are no changes", func(t *testing.T) {
 		newMetadata, modified := SyncEphemeralPodMetadata(&meta, &existing, &existing)
 		assert.False(t, modified)
 		assert.Equal(t, meta, *newMetadata)
-	}
-	{
-		// verify we don't touch metadata that we did not inject ourselves
+	})
+	t.Run("verify modified is false if there are no actual deletions", func(t *testing.T) {
+		existingWithExtraLabel := existing.DeepCopy()
+		existingWithExtraLabel.Labels["foo"] = "bar"
+		newMetadata, modified := SyncEphemeralPodMetadata(&meta, existingWithExtraLabel, &existing)
+		assert.False(t, modified)
+		assert.Equal(t, meta, *newMetadata)
+	})
+	t.Run("verify we don't touch metadata that we did not inject ourselves", func(t *testing.T) {
 		desired := v1alpha1.PodTemplateMetadata{
 			Labels: map[string]string{
 				"aaa": "222",
@@ -1356,7 +1361,6 @@ func TestSyncEphemeralPodMetadata(t *testing.T) {
 			},
 		}
 		newMetadata, modified := SyncEphemeralPodMetadata(&meta, &existing, &desired)
-		assert.True(t, modified)
 		expected := metav1.ObjectMeta{
 			Labels: map[string]string{
 				"aaa":    "222",
@@ -1369,8 +1373,7 @@ func TestSyncEphemeralPodMetadata(t *testing.T) {
 		}
 		assert.True(t, modified)
 		assert.Equal(t, expected, *newMetadata)
-	}
-
+	})
 }
 
 func TestGetReplicasForScaleDown(t *testing.T) {