From f829ae798459cd79e8aca97cb93597aac53e396d Mon Sep 17 00:00:00 2001 From: Jordan Rodgers Date: Mon, 3 Feb 2025 18:18:56 -0800 Subject: [PATCH] fix: check ephemeral metadata is set before delete (#4089) * fix: check ephemeral metadata is set before delete Signed-off-by: Jordan Rodgers * add test case Signed-off-by: Jordan Rodgers * address test comments Signed-off-by: Jordan Rodgers --------- Signed-off-by: Jordan Rodgers --- utils/replicaset/canary.go | 20 ++++++++++++++------ utils/replicaset/canary_test.go | 19 +++++++++++-------- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/utils/replicaset/canary.go b/utils/replicaset/canary.go index f8fd8fe869..49c5ed4582 100644 --- 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 index dbe4ce5a03..a0d5900a4f 100755 --- 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) {