feat: don't alter well-known labels and annotations (#1170)

* feat: don't touch well-known labels and annotations

These are reserved for kubernetes own usage and should
not be altered by users.

Apply the same logic alreayd used for PVC to all
resources managed by this controller for both labels
and annotations.

* refactor: use map range for all map copying

Author: sgaist

internal/controller/amaltheasession_controller_test.go

@@ -161,6 +161,68 @@ var _ = Describe("AmaltheaSession Controller", func() {
 			})
 			Expect(err).NotTo(HaveOccurred())
 		})
+
+		It("should not touch well known labels/annotations", func(ctx SpecContext) {
+			// Run reconcile once to start
+			controllerReconciler := newReconciler()
+			_, err := controllerReconciler.Reconcile(ctx, reconcile.Request{
+				NamespacedName: typeNamespacedName,
+			})
+			Expect(err).NotTo(HaveOccurred())
+
+			// Adding new labels and annotations
+			newLabels := map[string]string{
+				"renku.io/test":                "newLabel",
+				"app.kubernetes.io/created-by": "other-manager",
+				"app.kubernetes.io/name":       "other-name",
+			}
+			expectedLabels := map[string]string{
+				"renku.io/test":                "newLabel",
+				"app.kubernetes.io/created-by": "controller-manager",
+				"app.kubernetes.io/name":       "AmaltheaSession",
+			}
+
+			// Adding well-known annotations to simulate the system doing it
+			// These annotations are reserved and shall not be set/modified
+			// by other components
+			newAnnotations := map[string]string{
+				"renku.io/test": "newAnnotation",
+				"applyset.kubernetes.io/additional-namespaces":       "namespace1,namespace2",
+				"kube-scheduler-simulator.sigs.k8s.io/selected-node": "testnode",
+			}
+			expectedAnnotations := map[string]string{
+				"renku.io/test": "newAnnotation",
+				"applyset.kubernetes.io/additional-namespaces":       "namespace1,namespace2",
+				"kube-scheduler-simulator.sigs.k8s.io/selected-node": "testnode",
+			}
+
+			newSession0 := amaltheasession.DeepCopy()
+			newSession0.Spec.Template.Metadata.Labels = newLabels
+			newSession0.Spec.Template.Metadata.Annotations = newAnnotations
+			newSession0.Spec.ReconcileStrategy = amaltheadevv1alpha1.Always
+			err = k8sClient.Patch(ctx, newSession0, client.MergeFrom(amaltheasession))
+			Expect(err).NotTo(HaveOccurred())
+			_, err = controllerReconciler.Reconcile(ctx, reconcile.Request{
+				NamespacedName: typeNamespacedName,
+			})
+			Expect(err).NotTo(HaveOccurred())
+			childrenMetadataCheck(ctx, k8sClient, typeNamespacedName, expectedLabels, expectedAnnotations, false)
+
+			// Altering well-known annotations should fail
+			newAnnotations = map[string]string{
+				"applyset.kubernetes.io/additional-namespaces":       "namespace3,namespace4",
+				"kube-scheduler-simulator.sigs.k8s.io/selected-node": "other-node",
+			}
+
+			newSession0.Spec.Template.Metadata.Annotations = newAnnotations
+			err = k8sClient.Patch(ctx, newSession0, client.MergeFrom(amaltheasession))
+			Expect(err).NotTo(HaveOccurred())
+			_, err = controllerReconciler.Reconcile(ctx, reconcile.Request{
+				NamespacedName: typeNamespacedName,
+			})
+			Expect(err).NotTo(HaveOccurred())
+			childrenMetadataCheck(ctx, k8sClient, typeNamespacedName, expectedLabels, expectedAnnotations, false)
+		})
 	})
 
 	Context("When reconciling a resource without ingress", func() {

internal/controller/children.go

@@ -70,6 +70,30 @@ func isFailedOrHibernated(as *amaltheadevv1alpha1.AmaltheaSession) bool {
 		as.Status.State == amaltheadevv1alpha1.Hibernated
 }
 
+// Returns a map ensuring that the labels/annotations desired do not
+// overwrite well-known values.
+// Reference: https://kubernetes.io/docs/reference/labels-annotations-taints/
+func cleanWellKnown(current map[string]string, desired map[string]string) map[string]string {
+	preserved := map[string]string{}
+	for key, value := range current {
+		domain, _, _ := strings.Cut(key, "/")
+		if domain == "kubernetes.io" || domain == "k8s.io" || strings.HasSuffix(domain, ".kubernetes.io") ||
+			strings.HasSuffix(domain, ".k8s.io") {
+			preserved[key] = value
+		}
+	}
+
+	clean := map[string]string{}
+	for key, value := range desired {
+		clean[key] = value
+	}
+
+	for key, value := range preserved {
+		clean[key] = value
+	}
+	return clean
+}
+
 func (c ChildResource[T]) Reconcile(ctx context.Context, clnt client.Client, cr *amaltheadevv1alpha1.AmaltheaSession) ChildResourceUpdate[T] { //nolint:gocyclo
 	logger := log.FromContext(ctx)
 	if c.Current == nil {
@@ -102,8 +126,8 @@ func (c ChildResource[T]) Reconcile(ctx context.Context, clnt client.Client, cr
 				fallthrough
 			case amaltheadevv1alpha1.Always:
 				current.Spec = desired.Spec
-				current.Labels = desired.Labels
-				current.Annotations = desired.Annotations
+				current.Labels = cleanWellKnown(current.Labels, desired.Labels)
+				current.Annotations = cleanWellKnown(current.Annotations, desired.Annotations)
 			default:
 				return fmt.Errorf("attempting to reconcile ingress with unknown strategy %s", strategy)
 			}
@@ -160,8 +184,8 @@ func (c ChildResource[T]) Reconcile(ctx context.Context, clnt client.Client, cr
 				current.Spec.Template.Spec.InitContainers = desired.Spec.Template.Spec.InitContainers
 				current.Spec.Template.Spec.Volumes = desired.Spec.Template.Spec.Volumes
 				current.Spec.Selector = desired.Spec.Selector
-				current.Labels = desired.Labels
-				current.Annotations = desired.Annotations
+				current.Labels = cleanWellKnown(current.Labels, desired.Labels)
+				current.Annotations = cleanWellKnown(current.Annotations, desired.Annotations)
 				current.Spec.Template.Labels = desired.Spec.Template.Labels
 				current.Spec.Template.Annotations = desired.Spec.Template.Annotations
 			default:
@@ -197,25 +221,8 @@ func (c ChildResource[T]) Reconcile(ctx context.Context, clnt client.Client, cr
 					// NOTE: If the desired storage class is nil then the current spec contains the name for the default storage class
 					current.Spec.StorageClassName = desired.Spec.StorageClassName
 				}
-				// Do not touch reserved labels and annotations
-				// Reference: https://kubernetes.io/docs/reference/labels-annotations-taints/
-				// TODO: handle labels
-				current.Labels = desired.Labels
-				preservedAnnotations := map[string]string{}
-				for key := range current.Annotations {
-					domain, _, _ := strings.Cut(key, "/")
-					if domain == "kubernetes.io" || domain == "k8s.io" || strings.HasSuffix(domain, ".kubernetes.io") ||
-						strings.HasSuffix(domain, ".k8s.io") {
-						preservedAnnotations[key] = current.Annotations[key]
-					}
-				}
-				current.Annotations = desired.Annotations
-				if current.Annotations == nil {
-					current.Annotations = map[string]string{}
-				}
-				for key := range preservedAnnotations {
-					current.Annotations[key] = preservedAnnotations[key]
-				}
+				current.Labels = cleanWellKnown(current.Labels, desired.Labels)
+				current.Annotations = cleanWellKnown(current.Annotations, desired.Annotations)
 			default:
 				return fmt.Errorf("attempting to reconcile PVC with unknown strategy %s", strategy)
 			}
@@ -246,8 +253,8 @@ func (c ChildResource[T]) Reconcile(ctx context.Context, clnt client.Client, cr
 			case amaltheadevv1alpha1.Always:
 				current.Spec.Ports = desired.Spec.Ports
 				current.Spec.Selector = desired.Spec.Selector
-				current.Labels = desired.Labels
-				current.Annotations = desired.Annotations
+				current.Labels = cleanWellKnown(current.Labels, desired.Labels)
+				current.Annotations = cleanWellKnown(current.Annotations, desired.Annotations)
 			default:
 				return fmt.Errorf("attempting to reconcile service with unknown strategy %s", strategy)
 			}
@@ -293,8 +300,8 @@ func (c ChildResource[T]) Reconcile(ctx context.Context, clnt client.Client, cr
 				}
 				current.Data = desired.Data
 				current.StringData = preservedStringData
-				current.Labels = desired.Labels
-				current.Annotations = desired.Annotations
+				current.Labels = cleanWellKnown(current.Labels, desired.Labels)
+				current.Annotations = cleanWellKnown(current.Annotations, desired.Annotations)
 			default:
 				return fmt.Errorf("attempting to reconcile secret with unknown strategy %s", strategy)
 			}
@@ -362,8 +369,8 @@ func (c ChildResource[T]) Reconcile(ctx context.Context, clnt client.Client, cr
 				current.Spec.Template.Spec.Containers = desired.Spec.Template.Spec.Containers
 				current.Spec.Template.Spec.InitContainers = desired.Spec.Template.Spec.InitContainers
 				current.Spec.Template.Spec.Volumes = desired.Spec.Template.Spec.Volumes
-				current.Labels = desired.Labels
-				current.Annotations = desired.Annotations
+				current.Labels = cleanWellKnown(current.Labels, desired.Labels)
+				current.Annotations = cleanWellKnown(current.Annotations, desired.Annotations)
 				current.Spec.Template.Annotations = desired.Spec.Template.Annotations
 			default:
 				return fmt.Errorf("attempting to reconcile batchjob with unknown strategy %s", strategy)