diff --git a/pkg/instrumentation/annotationmutator.go b/pkg/instrumentation/annotationmutator.go index 134916072..a6a46a779 100644 --- a/pkg/instrumentation/annotationmutator.go +++ b/pkg/instrumentation/annotationmutator.go @@ -35,7 +35,7 @@ func NewInsertAnnotationMutation(annotations map[string]string) AnnotationMutati } type removeAnnotationMutation struct { - remove []string + remove map[string]string } func (m *removeAnnotationMutation) Mutate(annotations map[string]string) map[string]string { @@ -43,7 +43,7 @@ func (m *removeAnnotationMutation) Mutate(annotations map[string]string) map[str if !m.shouldMutate(annotations) { return mutatedAnnotations } - for _, key := range m.remove { + for key := range m.remove { if value, ok := annotations[key]; ok { delete(annotations, key) mutatedAnnotations[key] = value @@ -52,18 +52,22 @@ func (m *removeAnnotationMutation) Mutate(annotations map[string]string) map[str return mutatedAnnotations } +// shouldMutate reports whether every managed key is present AND still carries the value the operator +// injected. A key present with a different value (e.g. an explicit "false" opt-out set by the user) means +// the annotation is not operator-owned, so the mutation must not remove it. func (m *removeAnnotationMutation) shouldMutate(annotations map[string]string) bool { - for _, key := range m.remove { - if _, ok := annotations[key]; !ok { + for key, managedValue := range m.remove { + if value, ok := annotations[key]; !ok || value != managedValue { return false } } return true } -// NewRemoveAnnotationMutation creates a new mutation that removes annotations. All provided annotation keys -// must be present for it to attempt to remove them. -func NewRemoveAnnotationMutation(annotations []string) AnnotationMutation { +// NewRemoveAnnotationMutation creates a new mutation that removes annotations. It only removes the provided +// keys when all of them are present and each still carries the value it maps to (the value the operator +// injected), so user-authored values are never destroyed. +func NewRemoveAnnotationMutation(annotations map[string]string) AnnotationMutation { return &removeAnnotationMutation{remove: annotations} } diff --git a/pkg/instrumentation/annotationmutator_test.go b/pkg/instrumentation/annotationmutator_test.go index 480b69b23..e3fb020d8 100644 --- a/pkg/instrumentation/annotationmutator_test.go +++ b/pkg/instrumentation/annotationmutator_test.go @@ -79,9 +79,9 @@ func TestMutateAnnotations(t *testing.T) { "keyB": "2", }, mutations: []AnnotationMutation{ - NewRemoveAnnotationMutation([]string{ - "keyA", - "keyC", + NewRemoveAnnotationMutation(map[string]string{ + "keyA": "1", + "keyC": "4", }), }, wantAnnotations: map[string]string{ @@ -96,9 +96,9 @@ func TestMutateAnnotations(t *testing.T) { "keyB": "2", }, mutations: []AnnotationMutation{ - NewRemoveAnnotationMutation([]string{ - "keyA", - "keyB", + NewRemoveAnnotationMutation(map[string]string{ + "keyA": "1", + "keyB": "2", }), }, wantAnnotations: map[string]string{}, @@ -113,11 +113,11 @@ func TestMutateAnnotations(t *testing.T) { "keyB": "2", }, mutations: []AnnotationMutation{ - NewRemoveAnnotationMutation([]string{ - "keyA", + NewRemoveAnnotationMutation(map[string]string{ + "keyA": "1", }), - NewRemoveAnnotationMutation([]string{ - "keyB", + NewRemoveAnnotationMutation(map[string]string{ + "keyB": "2", }), }, wantAnnotations: map[string]string{}, @@ -126,14 +126,33 @@ func TestMutateAnnotations(t *testing.T) { "keyB": "2", }, }, + "TestRemove/PreservesUserValue": { + // A managed key present with a value other than the one the operator injected + // (e.g. an explicit "false" opt-out) must not be removed. + annotations: map[string]string{ + "keyA": "false", + "keyB": "2", + }, + mutations: []AnnotationMutation{ + NewRemoveAnnotationMutation(map[string]string{ + "keyA": "true", + "keyB": "2", + }), + }, + wantAnnotations: map[string]string{ + "keyA": "false", + "keyB": "2", + }, + wantMutatedAnnotations: map[string]string{}, + }, "TestBoth": { annotations: map[string]string{ "keyA": "1", "keyB": "2", }, mutations: []AnnotationMutation{ - NewRemoveAnnotationMutation([]string{ - "keyA", + NewRemoveAnnotationMutation(map[string]string{ + "keyA": "1", }), NewInsertAnnotationMutation(map[string]string{ "keyA": "3", diff --git a/pkg/instrumentation/auto/annotation.go b/pkg/instrumentation/auto/annotation.go index 90ed4d310..99f8dce3a 100644 --- a/pkg/instrumentation/auto/annotation.go +++ b/pkg/instrumentation/auto/annotation.go @@ -231,7 +231,7 @@ func newMutatorBuilder(typeSet instrumentation.TypeSet) *mutatorBuilder { func buildMutations(instType instrumentation.Type) (instrumentation.AnnotationMutation, instrumentation.AnnotationMutation) { annotations := buildAnnotations(instType) return instrumentation.NewInsertAnnotationMutation(annotations), - instrumentation.NewRemoveAnnotationMutation(maps.Keys(annotations)) + instrumentation.NewRemoveAnnotationMutation(annotations) } // buildAnnotations creates an annotation map of the inject and auto-annotate keys.