diff --git a/.chloggen/fix-disable-prometheus-annotations-removal.yaml b/.chloggen/fix-disable-prometheus-annotations-removal.yaml new file mode 100644 index 0000000000..cbbee38847 --- /dev/null +++ b/.chloggen/fix-disable-prometheus-annotations-removal.yaml @@ -0,0 +1,24 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action) +component: collector + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Honor `spec.observability.metrics.disablePrometheusAnnotations: true` on update by removing existing prometheus.io annotations from the pod template, not just stopping new ones from being added. + +# One or more tracking issues related to the change +issues: [5043] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: | + The pod-template mutate path preserves any annotation that exists on the + current resource but is absent from the desired render, so toggling + disablePrometheusAnnotations from false to true on an existing + OpenTelemetryCollector previously left the prometheus.io/scrape, port, and + path annotations stuck on the rolled pods. The mutate path now explicitly + strips a small fixed set of operator-managed keys (currently the + prometheus.io annotations) when they are absent from the desired pod + template, while continuing to preserve all other externally-set annotations. diff --git a/internal/manifests/mutate.go b/internal/manifests/mutate.go index 5f0ad14cef..52cf7b0890 100644 --- a/internal/manifests/mutate.go +++ b/internal/manifests/mutate.go @@ -386,11 +386,40 @@ func mutateIssuer(existing, desired *cmv1.Issuer) { existing.Spec = desired.Spec } +// operatorManagedPodAnnotationKeys is the set of pod-template annotation keys +// whose presence is exclusively governed by the operator. When mutating an +// existing pod template, any key in this set that is absent from the desired +// pod template is stripped from the existing pod template before the +// preserve-external-annotations merge runs. This lets operator-managed feature +// toggles (e.g. spec.observability.metrics.disablePrometheusAnnotations) take +// effect on already-running collectors without requiring a manual edit of the +// rendered Deployment, StatefulSet, or DaemonSet. +// +// Keys that are NOT in this set are preserved if previously set, matching the +// long-standing "external annotations on the pod template should survive a +// reconcile" contract enforced by the Ingress/Service mutate tests. +var operatorManagedPodAnnotationKeys = []string{ + "prometheus.io/scrape", + "prometheus.io/port", + "prometheus.io/path", +} + func mutatePodTemplate(existing, desired *corev1.PodTemplateSpec) error { if err := mergeWithOverride(&existing.Labels, desired.Labels); err != nil { return err } + // Strip operator-managed annotation keys from existing when they're absent + // from desired so that disabling the feature actually removes them. See the + // comment on operatorManagedPodAnnotationKeys for the rationale. + if existing.Annotations != nil { + for _, key := range operatorManagedPodAnnotationKeys { + if _, inDesired := desired.Annotations[key]; !inDesired { + delete(existing.Annotations, key) + } + } + } + if err := mergeWithOverride(&existing.Annotations, desired.Annotations); err != nil { return err } diff --git a/internal/manifests/mutate_test.go b/internal/manifests/mutate_test.go index 3a058d5a26..a1e26a97af 100644 --- a/internal/manifests/mutate_test.go +++ b/internal/manifests/mutate_test.go @@ -2625,3 +2625,117 @@ func TestGetMutateFunc_MutateNetworkPolicy(t *testing.T) { require.Exactly(t, got.Annotations, want.Annotations) require.Exactly(t, got.Spec, want.Spec) } + +// TestMutatePodTemplateStripsOperatorManagedAnnotations exercises the disable-after-create +// scenario described in https://github.com/open-telemetry/opentelemetry-operator/issues/5043. +// When the operator re-renders a Deployment with prometheus annotations disabled, the +// existing prometheus.io/* annotations on the pod template must be removed; merely omitting +// them from the desired pod template was not enough because the surrounding merge preserves +// any key only present on the existing side. +func TestMutatePodTemplateStripsOperatorManagedAnnotations(t *testing.T) { + const ( + userKept = "ops/internal-bookkeeping" + userValue = "should-be-preserved-across-reconciles" + nonProm = "non.prom/key-with-prefix-but-not-prom" + nonPromValue = "should-also-be-preserved" + sha256Key = "opentelemetry-operator-config/sha256" + oldHash = "old-hash" + newHash = "new-hash" + ) + + tests := []struct { + name string + existingTemplate corev1.PodTemplateSpec + desiredTemplate corev1.PodTemplateSpec + expectAnnotations map[string]string + }{ + { + name: "disable removes prometheus annotations but keeps user keys", + existingTemplate: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "prometheus.io/scrape": "true", + "prometheus.io/port": "8888", + "prometheus.io/path": "/metrics", + sha256Key: oldHash, + userKept: userValue, + nonProm: nonPromValue, + }, + }, + }, + desiredTemplate: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + sha256Key: newHash, + }, + }, + }, + expectAnnotations: map[string]string{ + sha256Key: newHash, + userKept: userValue, + nonProm: nonPromValue, + }, + }, + { + name: "enable keeps prometheus annotations when desired sets them", + existingTemplate: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + sha256Key: oldHash, + userKept: userValue, + }, + }, + }, + desiredTemplate: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "prometheus.io/scrape": "true", + "prometheus.io/port": "8888", + "prometheus.io/path": "/metrics", + sha256Key: newHash, + }, + }, + }, + expectAnnotations: map[string]string{ + "prometheus.io/scrape": "true", + "prometheus.io/port": "8888", + "prometheus.io/path": "/metrics", + sha256Key: newHash, + userKept: userValue, + }, + }, + { + name: "no operator-managed keys on either side leaves user annotations intact", + existingTemplate: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + userKept: userValue, + nonProm: nonPromValue, + }, + }, + }, + desiredTemplate: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + sha256Key: newHash, + }, + }, + }, + expectAnnotations: map[string]string{ + userKept: userValue, + nonProm: nonPromValue, + sha256Key: newHash, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + existing := tt.existingTemplate.DeepCopy() + desired := tt.desiredTemplate.DeepCopy() + err := mutatePodTemplate(existing, desired) + require.NoError(t, err) + require.Exactly(t, tt.expectAnnotations, existing.Annotations) + }) + } +}