Skip to content

Commit eff720f

Browse files
shubham-pampattiwarclaude
shubham-pampattiwar
and
claude
committed
OADP-7869: Fix stale resourceAnnotations/resourceLabels not removed after DPA patch
When resourceAnnotations or resourceLabels were removed from the DPA spec, stale values persisted on all managed resources (Velero deployment, BSL, ConfigMaps, Services, etc.) because the apply functions only merged annotations in and never removed previously-applied keys. This fix introduces tracking annotations (oadp.openshift.io/managed-resource- annotation-keys and oadp.openshift.io/managed-resource-label-keys) that record which keys were applied by resourceAnnotations/resourceLabels. On each reconcile, previously-tracked keys are removed before applying the current set, ensuring cleanup when the DPA spec changes. Additionally fixes a secondary issue where the Velero deployment incorrectly merged metadata annotations into the pod template via AppendUniqueKeyTOfTMaps, causing stale annotations to persist on the pod template. Aligned with the node-agent pattern of passing only PodConfig annotations to the install library. Fixes: OADP-7869 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent c927a46 commit eff720f

12 files changed

Lines changed: 462 additions & 103 deletions

‎internal/controller/backup_repository.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,10 +42,8 @@ func (r *DataProtectionApplicationReconciler) updateBackupRepositoryCM(cm *corev
4242
oadpv1alpha1.OadpOperatorLabel: "True",
4343
}
4444

45-
// Apply user-provided resource labels (protected labels are filtered)
46-
cm.Labels = applyResourceLabels(r.dpa, cm.Labels)
47-
48-
// Apply user-provided resource annotations
45+
// Apply user-provided resource labels and annotations
46+
cm.Labels, cm.Annotations = applyResourceLabels(r.dpa, cm.Labels, cm.Annotations)
4947
cm.Annotations = applyResourceAnnotations(r.dpa, cm.Annotations)
5048

5149
if cm.Data == nil {

‎internal/controller/bsl.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -425,10 +425,8 @@ func (r *DataProtectionApplicationReconciler) updateBSLFromSpec(bsl *velerov1.Ba
425425
bsl.Labels["app.kubernetes.io/component"] = "bsl"
426426
bsl.Labels[oadpv1alpha1.OadpOperatorLabel] = "True"
427427

428-
// Apply user-provided resource labels (protected labels are filtered)
429-
bsl.Labels = applyResourceLabels(r.dpa, bsl.Labels)
430-
431-
// Apply user-provided resource annotations
428+
// Apply user-provided resource labels and annotations
429+
bsl.Labels, bsl.Annotations = applyResourceLabels(r.dpa, bsl.Labels, bsl.Annotations)
432430
bsl.Annotations = applyResourceAnnotations(r.dpa, bsl.Annotations)
433431

434432
return nil

‎internal/controller/kubevirt_datamover_controller.go‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -103,10 +103,10 @@ func (r *DataProtectionApplicationReconciler) buildKubevirtDatamoverDeployment(d
103103
return err
104104
}
105105

106-
// Apply user-provided resource labels (protected labels are filtered)
106+
// Apply user-provided resource labels and annotations
107107
// Note: NOT applied to Spec.Selector.MatchLabels as those are immutable after creation
108-
deploymentObject.Labels = applyResourceLabels(r.dpa, deploymentObject.Labels)
109-
deploymentObject.Spec.Template.Labels = applyResourceLabels(r.dpa, deploymentObject.Spec.Template.Labels)
108+
deploymentObject.Labels, deploymentObject.Annotations = applyResourceLabels(r.dpa, deploymentObject.Labels, deploymentObject.Annotations)
109+
deploymentObject.Spec.Template.Labels, deploymentObject.Spec.Template.Annotations = applyResourceLabels(r.dpa, deploymentObject.Spec.Template.Labels, deploymentObject.Spec.Template.Annotations)
110110

111111
// Re-assert selector labels to ensure template labels match selector
112112
// This prevents user resourceLabels from overriding selector-critical labels
@@ -119,7 +119,6 @@ func (r *DataProtectionApplicationReconciler) buildKubevirtDatamoverDeployment(d
119119
}
120120
}
121121

122-
// Apply user-provided resource annotations to both deployment and pod template
123122
deploymentObject.Annotations = applyResourceAnnotations(r.dpa, deploymentObject.Annotations)
124123
deploymentObject.Spec.Template.Annotations = applyResourceAnnotations(r.dpa, deploymentObject.Spec.Template.Annotations)
125124

‎internal/controller/monitor.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -64,10 +64,8 @@ func (r *DataProtectionApplicationReconciler) updateVeleroMetricsSVC(svc *corev1
6464

6565
svc.Labels = getDpaAppLabels(r.dpa)
6666

67-
// Apply user-provided resource labels (protected labels are filtered)
68-
svc.Labels = applyResourceLabels(r.dpa, svc.Labels)
69-
70-
// Apply user-provided resource annotations
67+
// Apply user-provided resource labels and annotations
68+
svc.Labels, svc.Annotations = applyResourceLabels(r.dpa, svc.Labels, svc.Annotations)
7169
svc.Annotations = applyResourceAnnotations(r.dpa, svc.Annotations)
7270

7371
return nil

‎internal/controller/nodeagent.go‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -195,10 +195,8 @@ func (r *DataProtectionApplicationReconciler) updateNodeAgentCM(cm *corev1.Confi
195195
oadpv1alpha1.OadpOperatorLabel: "True",
196196
}
197197

198-
// Apply user-provided resource labels (protected labels are filtered)
199-
cm.Labels = applyResourceLabels(r.dpa, cm.Labels)
200-
201-
// Apply user-provided resource annotations
198+
// Apply user-provided resource labels and annotations
199+
cm.Labels, cm.Annotations = applyResourceLabels(r.dpa, cm.Labels, cm.Annotations)
202200
cm.Annotations = applyResourceAnnotations(r.dpa, cm.Annotations)
203201

204202
if cm.Data == nil {
@@ -571,10 +569,10 @@ func (r *DataProtectionApplicationReconciler) customizeNodeAgentDaemonset(ds *ap
571569
}
572570
}
573571

574-
// Apply user-provided resource labels (protected labels are filtered)
572+
// Apply user-provided resource labels and annotations
575573
// Note: NOT applied to Spec.Selector.MatchLabels as those are immutable after creation
576-
ds.Labels = applyResourceLabels(dpa, ds.Labels)
577-
ds.Spec.Template.Labels = applyResourceLabels(dpa, ds.Spec.Template.Labels)
574+
ds.Labels, ds.Annotations = applyResourceLabels(dpa, ds.Labels, ds.Annotations)
575+
ds.Spec.Template.Labels, ds.Spec.Template.Annotations = applyResourceLabels(dpa, ds.Spec.Template.Labels, ds.Spec.Template.Annotations)
578576

579577
// Re-assert selector labels to ensure template labels match selector
580578
// This prevents user resourceLabels from overriding selector-critical labels
@@ -587,7 +585,6 @@ func (r *DataProtectionApplicationReconciler) customizeNodeAgentDaemonset(ds *ap
587585
}
588586
}
589587

590-
// Apply user-provided resource annotations to both daemonset and pod template
591588
ds.Annotations = applyResourceAnnotations(dpa, ds.Annotations)
592589
ds.Spec.Template.Annotations = applyResourceAnnotations(dpa, ds.Spec.Template.Annotations)
593590

‎internal/controller/nonadmin_controller.go‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -135,10 +135,10 @@ func (r *DataProtectionApplicationReconciler) buildNonAdminDeployment(deployment
135135
return err
136136
}
137137

138-
// Apply user-provided resource labels (protected labels are filtered)
138+
// Apply user-provided resource labels and annotations
139139
// Note: NOT applied to Spec.Selector.MatchLabels as those are immutable after creation
140-
deploymentObject.Labels = applyResourceLabels(r.dpa, deploymentObject.Labels)
141-
deploymentObject.Spec.Template.Labels = applyResourceLabels(r.dpa, deploymentObject.Spec.Template.Labels)
140+
deploymentObject.Labels, deploymentObject.Annotations = applyResourceLabels(r.dpa, deploymentObject.Labels, deploymentObject.Annotations)
141+
deploymentObject.Spec.Template.Labels, deploymentObject.Spec.Template.Annotations = applyResourceLabels(r.dpa, deploymentObject.Spec.Template.Labels, deploymentObject.Spec.Template.Annotations)
142142

143143
// Re-assert selector labels to ensure template labels match selector
144144
// This prevents user resourceLabels from overriding selector-critical labels
@@ -151,7 +151,6 @@ func (r *DataProtectionApplicationReconciler) buildNonAdminDeployment(deployment
151151
}
152152
}
153153

154-
// Apply user-provided resource annotations to both deployment and pod template
155154
deploymentObject.Annotations = applyResourceAnnotations(r.dpa, deploymentObject.Annotations)
156155
deploymentObject.Spec.Template.Annotations = applyResourceAnnotations(r.dpa, deploymentObject.Spec.Template.Annotations)
157156

‎internal/controller/registry.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -746,10 +746,8 @@ func (r *DataProtectionApplicationReconciler) patchRegistrySecret(secret *corev1
746746
return err
747747
}
748748

749-
// Apply user-provided resource labels (protected labels are filtered)
750-
secret.Labels = applyResourceLabels(r.dpa, secret.Labels)
751-
752-
// Apply user-provided resource annotations
749+
// Apply user-provided resource labels and annotations
750+
secret.Labels, secret.Annotations = applyResourceLabels(r.dpa, secret.Labels, secret.Annotations)
753751
secret.Annotations = applyResourceAnnotations(r.dpa, secret.Annotations)
754752

755753
// when updating the spec fields we update each field individually

‎internal/controller/repository_maintenance.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,8 @@ func (r *DataProtectionApplicationReconciler) updateRepositoryMaintenanceCM(cm *
3636
oadpv1alpha1.OadpOperatorLabel: "True",
3737
}
3838

39-
// Apply user-provided resource labels (protected labels are filtered)
40-
cm.Labels = applyResourceLabels(r.dpa, cm.Labels)
41-
42-
// Apply user-provided resource annotations
39+
// Apply user-provided resource labels and annotations
40+
cm.Labels, cm.Annotations = applyResourceLabels(r.dpa, cm.Labels, cm.Annotations)
4341
cm.Annotations = applyResourceAnnotations(r.dpa, cm.Annotations)
4442

4543
if cm.Data == nil {

‎internal/controller/velero.go‎

Lines changed: 89 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"fmt"
55
"os"
66
"reflect"
7+
"slices"
78
"strconv"
89
"strings"
910

@@ -51,6 +52,9 @@ const (
5152
caCertMountPath = "/etc/velero/ca-certs"
5253
caBundleFileName = "ca-bundle.pem"
5354
caBundleConfigMapName = "velero-ca-bundle"
55+
56+
managedAnnotationKeysAnnotation = "oadp.openshift.io/managed-resource-annotation-keys"
57+
managedLabelKeysAnnotation = "oadp.openshift.io/managed-resource-label-keys"
5458
)
5559

5660
var (
@@ -171,14 +175,9 @@ func (r *DataProtectionApplicationReconciler) buildVeleroDeployment(veleroDeploy
171175
// get resource requirements for velero deployment
172176
// ignoring err here as it is checked in validator.go
173177
veleroResourceReqs, _ := r.getVeleroResourceReqs()
174-
veleroAnnotations := make(map[string]string)
178+
var podAnnotations map[string]string
175179
if dpa.Spec.Configuration != nil && dpa.Spec.Configuration.Velero != nil && dpa.Spec.Configuration.Velero.PodConfig != nil {
176-
veleroAnnotations = dpa.Spec.Configuration.Velero.PodConfig.Annotations
177-
}
178-
179-
podAnnotations, err := common.AppendUniqueKeyTOfTMaps(veleroAnnotations, veleroDeployment.Annotations)
180-
if err != nil {
181-
return fmt.Errorf("error appending pod annotations: %v", err)
180+
podAnnotations = dpa.Spec.Configuration.Velero.PodConfig.Annotations
182181
}
183182

184183
uploaderType := ""
@@ -188,7 +187,7 @@ func (r *DataProtectionApplicationReconciler) buildVeleroDeployment(veleroDeploy
188187

189188
// Filter out resourceLabels before passing to install.Deployment
190189
// This ensures matchLabels don't contain resourceLabels (which would cause immutable selector errors on reconcile)
191-
labelsForInstall := filterOutResourceLabels(dpa, veleroDeployment.Labels)
190+
labelsForInstall := filterOutResourceLabels(dpa, veleroDeployment.Labels, veleroDeployment.Annotations)
192191

193192
installDeployment := install.Deployment(veleroDeployment.Namespace,
194193
install.WithResources(veleroResourceReqs),
@@ -244,7 +243,7 @@ func (r *DataProtectionApplicationReconciler) customizeVeleroDeployment(veleroDe
244243
}
245244
// Filter out resourceLabels from veleroDeployment.Labels before adding to matchLabels
246245
// matchLabels are immutable, so we must not include resourceLabels which are user-configurable
247-
labelsForMatchLabels := filterOutResourceLabels(dpa, veleroDeployment.Labels)
246+
labelsForMatchLabels := filterOutResourceLabels(dpa, veleroDeployment.Labels, veleroDeployment.Annotations)
248247
veleroDeployment.Spec.Selector.MatchLabels, err = common.AppendUniqueKeyTOfTMaps(veleroDeployment.Spec.Selector.MatchLabels, labelsForMatchLabels, getDpaAppLabels(dpa))
249248
if err != nil {
250249
return fmt.Errorf("velero deployment selector label: %v", err)
@@ -263,8 +262,8 @@ func (r *DataProtectionApplicationReconciler) customizeVeleroDeployment(veleroDe
263262

264263
// Apply user-provided resource labels (protected labels are filtered)
265264
// Note: NOT applied to Spec.Selector.MatchLabels as those are immutable after creation
266-
veleroDeployment.Labels = applyResourceLabels(dpa, veleroDeployment.Labels)
267-
veleroDeployment.Spec.Template.Labels = applyResourceLabels(dpa, veleroDeployment.Spec.Template.Labels)
265+
veleroDeployment.Labels, veleroDeployment.Annotations = applyResourceLabels(dpa, veleroDeployment.Labels, veleroDeployment.Annotations)
266+
veleroDeployment.Spec.Template.Labels, veleroDeployment.Spec.Template.Annotations = applyResourceLabels(dpa, veleroDeployment.Spec.Template.Labels, veleroDeployment.Spec.Template.Annotations)
268267

269268
// Apply user-provided resource annotations to both deployment and pod template
270269
veleroDeployment.Annotations = applyResourceAnnotations(dpa, veleroDeployment.Annotations)
@@ -840,33 +839,59 @@ func isProtectedLabel(key string) bool {
840839
return false
841840
}
842841

843-
// applyResourceLabels merges DPA resourceLabels with core labels.
844-
// Core labels take precedence - protected labels from user input are filtered out.
845-
// Returns a new map containing core labels plus non-protected user labels.
846-
func applyResourceLabels(dpa *oadpv1alpha1.DataProtectionApplication, coreLabels map[string]string) map[string]string {
847-
if dpa == nil || dpa.Spec.ResourceLabels == nil {
848-
return coreLabels
842+
// applyResourceLabels applies DPA resourceLabels to a resource's labels,
843+
// using a tracking annotation to clean up stale keys when labels are removed from the DPA spec.
844+
// Returns updated labels and annotations (annotations carry the tracking key).
845+
func applyResourceLabels(dpa *oadpv1alpha1.DataProtectionApplication, coreLabels map[string]string, annotations map[string]string) (map[string]string, map[string]string) {
846+
labelResult := make(map[string]string)
847+
for k, v := range coreLabels {
848+
labelResult[k] = v
849849
}
850850

851-
// Start with core labels
852-
result := make(map[string]string)
853-
for k, v := range coreLabels {
854-
result[k] = v
851+
if tracked, ok := annotations[managedLabelKeysAnnotation]; ok {
852+
for _, k := range strings.Split(tracked, ",") {
853+
if k = strings.TrimSpace(k); k != "" && !isProtectedLabel(k) {
854+
delete(labelResult, k)
855+
}
856+
}
857+
if annotations != nil {
858+
annCopy := make(map[string]string)
859+
for k, v := range annotations {
860+
annCopy[k] = v
861+
}
862+
delete(annCopy, managedLabelKeysAnnotation)
863+
annotations = annCopy
864+
}
865+
}
866+
867+
if dpa == nil || len(dpa.Spec.ResourceLabels) == 0 {
868+
return labelResult, annotations
855869
}
856870

857-
// Add user labels, skipping protected ones
871+
var keys []string
858872
for k, v := range dpa.Spec.ResourceLabels {
859873
if !isProtectedLabel(k) {
860-
result[k] = v
874+
labelResult[k] = v
875+
keys = append(keys, k)
861876
}
862877
}
863878

864-
return result
879+
if len(keys) > 0 {
880+
annResult := make(map[string]string)
881+
for k, v := range annotations {
882+
annResult[k] = v
883+
}
884+
slices.Sort(keys)
885+
annResult[managedLabelKeysAnnotation] = strings.Join(keys, ",")
886+
return labelResult, annResult
887+
}
888+
889+
return labelResult, annotations
865890
}
866891

867-
// filterOutResourceLabels removes resourceLabels from the given labels map.
868-
// This is used to get labels that are safe for matchLabels (which are immutable).
869-
func filterOutResourceLabels(dpa *oadpv1alpha1.DataProtectionApplication, labels map[string]string) map[string]string {
892+
// filterOutResourceLabels removes resourceLabels (current and previously-tracked) from the given
893+
// labels map. This is used to get labels that are safe for matchLabels (which are immutable).
894+
func filterOutResourceLabels(dpa *oadpv1alpha1.DataProtectionApplication, labels map[string]string, annotations map[string]string) map[string]string {
870895
if labels == nil {
871896
return nil
872897
}
@@ -876,8 +901,6 @@ func filterOutResourceLabels(dpa *oadpv1alpha1.DataProtectionApplication, labels
876901
result[k] = v
877902
}
878903

879-
// Remove resourceLabels if present, but skip protected labels
880-
// Protected labels should always be preserved in matchLabels
881904
if dpa != nil && dpa.Spec.ResourceLabels != nil {
882905
for k := range dpa.Spec.ResourceLabels {
883906
if !isProtectedLabel(k) {
@@ -886,14 +909,24 @@ func filterOutResourceLabels(dpa *oadpv1alpha1.DataProtectionApplication, labels
886909
}
887910
}
888911

912+
if tracked, ok := annotations[managedLabelKeysAnnotation]; ok {
913+
for _, k := range strings.Split(tracked, ",") {
914+
if k = strings.TrimSpace(k); k != "" && !isProtectedLabel(k) {
915+
delete(result, k)
916+
}
917+
}
918+
}
919+
889920
return result
890921
}
891922

892-
// applyResourceAnnotations merges DPA resourceAnnotations with existing annotations.
893-
// User-provided annotations are added to existing annotations. User annotations take precedence
894-
// for any conflicting keys.
923+
// applyResourceAnnotations applies DPA resourceAnnotations to a resource's annotations,
924+
// using a tracking annotation to clean up stale keys when annotations are removed from the DPA spec.
895925
func applyResourceAnnotations(dpa *oadpv1alpha1.DataProtectionApplication, existingAnnotations map[string]string) map[string]string {
896-
if dpa == nil || dpa.Spec.ResourceAnnotations == nil {
926+
_, hasTracking := existingAnnotations[managedAnnotationKeysAnnotation]
927+
hasNewAnnotations := dpa != nil && len(dpa.Spec.ResourceAnnotations) > 0
928+
929+
if !hasTracking && !hasNewAnnotations {
897930
return existingAnnotations
898931
}
899932

@@ -902,8 +935,31 @@ func applyResourceAnnotations(dpa *oadpv1alpha1.DataProtectionApplication, exist
902935
result[k] = v
903936
}
904937

938+
if tracked, ok := result[managedAnnotationKeysAnnotation]; ok {
939+
for _, k := range strings.Split(tracked, ",") {
940+
if k = strings.TrimSpace(k); k != "" {
941+
delete(result, k)
942+
}
943+
}
944+
delete(result, managedAnnotationKeysAnnotation)
945+
}
946+
947+
if !hasNewAnnotations {
948+
return result
949+
}
950+
951+
var keys []string
905952
for k, v := range dpa.Spec.ResourceAnnotations {
953+
if k == managedAnnotationKeysAnnotation || k == managedLabelKeysAnnotation {
954+
continue
955+
}
906956
result[k] = v
957+
keys = append(keys, k)
958+
}
959+
960+
if len(keys) > 0 {
961+
slices.Sort(keys)
962+
result[managedAnnotationKeysAnnotation] = strings.Join(keys, ",")
907963
}
908964

909965
return result

0 commit comments

Comments
 (0)