Merge pull request #127 from stlaz/psa_managed_fields

AUTH-413: ps syncer: only sync labels if noone else is managing them

pkg/psalabelsyncer/podsecurity_label_sync_controller.go

@@ -2,17 +2,20 @@ package psalabelsyncer
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"strings"
 
 	"github.com/openshift/cluster-policy-controller/pkg/psalabelsyncer/nsexemptions"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/selection"
 	"k8s.io/apimachinery/pkg/util/sets"
+	corev1apply "k8s.io/client-go/applyconfigurations/core/v1"
 	corev1informers "k8s.io/client-go/informers/core/v1"
 	rbacv1informers "k8s.io/client-go/informers/rbac/v1"
 	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
@@ -36,12 +39,24 @@ const (
 	currentPSaVersion     = "v1.24"
 )
 
+var (
+	allPSaLabels = map[string]string{
+		psapi.EnforceLevelLabel: psapi.EnforceVersionLabel,
+		psapi.WarnLevelLabel:    psapi.WarnVersionLabel,
+		psapi.AuditLevelLabel:   psapi.AuditVersionLabel,
+	}
+	loggingLabels = map[string]string{
+		psapi.WarnLevelLabel:  psapi.WarnVersionLabel,
+		psapi.AuditLevelLabel: psapi.AuditVersionLabel,
+	}
+)
+
 // PodSecurityAdmissionLabelSynchronizationController watches over namespaces labelled with
 // "security.openshift.io/scc.podSecurityLabelSync: true" and configures the PodSecurity
 // admission namespace label to match the user account privileges in terms of being able
 // to use SCCs
 type PodSecurityAdmissionLabelSynchronizationController struct {
-	shouldEnforce bool
+	syncedLabels map[string]string
 
 	namespaceClient corev1client.NamespaceInterface
 
@@ -64,7 +79,7 @@ func NewEnforcingPodSecurityAdmissionLabelSynchronizationController(
 	eventRecorder events.Recorder,
 ) (factory.Controller, error) {
 	return newPodSecurityAdmissionLabelSynchronizationController(
-		true,
+		allPSaLabels,
 		namespaceClient,
 		namespaceInformer,
 		rbacInformers,
@@ -83,7 +98,7 @@ func NewAdvisingPodSecurityAdmissionLabelSynchronizationController(
 	eventRecorder events.Recorder,
 ) (factory.Controller, error) {
 	return newPodSecurityAdmissionLabelSynchronizationController(
-		false,
+		loggingLabels,
 		namespaceClient,
 		namespaceInformer,
 		rbacInformers,
@@ -94,7 +109,7 @@ func NewAdvisingPodSecurityAdmissionLabelSynchronizationController(
 }
 
 func newPodSecurityAdmissionLabelSynchronizationController(
-	shouldEnforce bool,
+	syncedLabels map[string]string,
 	namespaceClient corev1client.NamespaceInterface,
 	namespaceInformer corev1informers.NamespaceInformer,
 	rbacInformers rbacv1informers.Interface,
@@ -123,7 +138,7 @@ func newPodSecurityAdmissionLabelSynchronizationController(
 
 	syncCtx := factory.NewSyncContext(controllerName, eventRecorder.WithComponentSuffix(controllerName))
 	c := &PodSecurityAdmissionLabelSynchronizationController{
-		shouldEnforce: shouldEnforce,
+		syncedLabels: syncedLabels,
 
 		namespaceClient: namespaceClient,
 
@@ -191,13 +206,83 @@ func (c *PodSecurityAdmissionLabelSynchronizationController) sync(ctx context.Co
 		return nil
 	}
 
+	ns, err = forceHistoricalLabelsOwnership(ctx, c.namespaceClient, ns)
+	if err != nil {
+		return fmt.Errorf("failed to force ownership from cluster-policy-controller to %s: %w", controllerName, err)
+	}
+
 	if err := c.syncNamespace(ctx, controllerContext, ns); err != nil {
 		return fmt.Errorf(errFmt, qKey, err)
 	}
 
 	return nil
 }
 
+func forceHistoricalLabelsOwnership(ctx context.Context, nsClient corev1client.NamespaceInterface, ns *corev1.Namespace) (*corev1.Namespace, error) {
+	cpcOwnedLabelKeys := sets.New[string]()
+	for _, f := range ns.ManagedFields {
+		if f.Manager != "cluster-policy-controller" {
+			continue
+		}
+
+		newCPCLabels, err := managedLabels(f)
+		if err != nil {
+			return nil, err
+		}
+
+		cpcOwnedLabelKeys = cpcOwnedLabelKeys.Union(newCPCLabels)
+	}
+
+	if cpcOwnedLabelKeys.Len() == 0 {
+		return ns, nil
+	}
+
+	cpcOwnedPSaLabels := map[string]string{}
+	// filter out all the labels not owned by this controller
+	for labelType, labelVersion := range allPSaLabels {
+		if cpcOwnedLabelKeys.Has(labelType) {
+			cpcOwnedPSaLabels[labelType] = ns.Labels[labelType]
+		}
+		if cpcOwnedLabelKeys.Has(labelVersion) {
+			cpcOwnedPSaLabels[labelVersion] = ns.Labels[labelVersion]
+		}
+	}
+
+	// none of the labels CPC is managing are interesting to us
+	if len(cpcOwnedPSaLabels) == 0 {
+		return ns, nil
+	}
+
+	// we need to extract all our managed fields not to delete them on the apply below
+	ourOwned, err := corev1apply.ExtractNamespace(ns, controllerName)
+	if err != nil {
+		return nil, err
+	}
+
+	// add the PSa labels that were previously owned by CPC under this manager
+	ourOwned.WithLabels(cpcOwnedPSaLabels)
+
+	nsCopy := ns.DeepCopy()
+	for labelKey := range cpcOwnedPSaLabels {
+		delete(nsCopy.Labels, labelKey)
+	}
+
+	// previously, we were using Update to set the labels, Kube does not consider that as actually owning the fields, even
+	// though it shows up in managedFields and would cause conflicts on value change. Eh. Ugly.
+	//
+	// Writing custom logic that checks which fields are _really_ managed by a manager, caches the unstructured object and then
+	// conditionally removes the label from all those unstructured fields and shoves them back in the proper place
+	// in the object managedFields is tedious, ugly and super error-prone.
+	//
+	// Just remove the fields as the previous owner and quickly readd them as the new one.
+	if _, err = nsClient.Update(ctx, nsCopy, metav1.UpdateOptions{FieldManager: "cluster-policy-controller"}); err != nil {
+		return nil, fmt.Errorf("failed to share PSa label ownership with the previous owner: %w", err)
+	}
+
+	// take ownership of the fields since they should all be clear now
+	return nsClient.Apply(ctx, ourOwned, metav1.ApplyOptions{FieldManager: controllerName})
+}
+
 func (c *PodSecurityAdmissionLabelSynchronizationController) syncNamespace(ctx context.Context, controllerContext factory.SyncContext, ns *corev1.Namespace) error {
 	// We cannot safely determine the SCC level for an NS until it gets the UID annotation.
 	// No need to care about re-queueing the key, we should get the NS once it is updated
@@ -250,59 +335,49 @@ func (c *PodSecurityAdmissionLabelSynchronizationController) syncNamespace(ctx c
 		return fmt.Errorf("unknown PSa level for namespace %q", ns.Name)
 	}
 
-	nsCopy := ns.DeepCopy()
+	managedNamespaces, err := extractNSFieldsPerManager(ns)
+	if err != nil {
+		return fmt.Errorf("ns extraction failed: %w", err)
+	}
 
-	var changed bool
+	// we must extract the NS in case only some of the labels we're setting need
+	// updating to avoid hotlo
+	nsApplyConfig := corev1apply.Namespace(ns.Name)
+	if err != nil {
+		return fmt.Errorf("failed to extract field ownership for NS %q: %w", ns.Name, err)
+	}
 
-	if c.shouldEnforce {
-		if nsCopy.Labels[psapi.EnforceLevelLabel] != string(psaLevel) || nsCopy.Labels[psapi.EnforceVersionLabel] != currentPSaVersion {
-			changed = true
-			if nsCopy.Labels == nil {
-				nsCopy.Labels = map[string]string{}
+	var shouldUpdate bool
+	for typeLabel, versionLabel := range c.syncedLabels {
+		if manager := managedNamespaces.getManagerForLabel(typeLabel); len(manager) == 0 || manager == controllerName {
+			nsApplyConfig.WithLabels(map[string]string{
+				typeLabel: string(psaLevel),
+			})
+			if ns.Labels[typeLabel] != string(psaLevel) {
+				shouldUpdate = true
 			}
-
-			nsCopy.Labels[psapi.EnforceLevelLabel] = string(psaLevel)
-			nsCopy.Labels[psapi.EnforceVersionLabel] = currentPSaVersion
 		}
-
-		// cleanup audit and warn labels from version 4.11
-		// TODO: This can be removed in 4.13 and allow users set these as they wish
-		for typeLabel, versionLabel := range map[string]string{
-			psapi.WarnLevelLabel:  psapi.WarnVersionLabel,
-			psapi.AuditLevelLabel: psapi.AuditVersionLabel,
-		} {
-			if _, ok := nsCopy.Labels[typeLabel]; ok {
-				delete(nsCopy.Labels, typeLabel)
-				changed = true
-			}
-			if _, ok := nsCopy.Labels[versionLabel]; ok {
-				delete(nsCopy.Labels, versionLabel)
-				changed = true
+		if manager := managedNamespaces.getManagerForLabel(versionLabel); len(manager) == 0 || manager == controllerName {
+			nsApplyConfig.WithLabels(map[string]string{
+				versionLabel: currentPSaVersion,
+			})
+			if ns.Labels[versionLabel] != currentPSaVersion {
+				shouldUpdate = true
 			}
 		}
-	} else {
-		for typeLabel, versionLabel := range map[string]string{
-			psapi.WarnLevelLabel:  psapi.WarnVersionLabel,
-			psapi.AuditLevelLabel: psapi.AuditVersionLabel,
-		} {
-			if ns.Labels[typeLabel] != string(psaLevel) || ns.Labels[versionLabel] != currentPSaVersion {
-				changed = true
-				if nsCopy.Labels == nil {
-					nsCopy.Labels = map[string]string{}
-				}
-
-				nsCopy.Labels[typeLabel] = string(psaLevel)
-				nsCopy.Labels[versionLabel] = currentPSaVersion
+	}
 
-			}
-		}
+	if !shouldUpdate {
+		return nil
 	}
 
-	if changed {
-		_, err := c.namespaceClient.Update(ctx, nsCopy, metav1.UpdateOptions{})
-		if err != nil {
-			return fmt.Errorf("failed to update the namespace: %w", err)
+	_, err = c.namespaceClient.Apply(ctx, nsApplyConfig, metav1.ApplyOptions{FieldManager: controllerName})
+	if err != nil {
+		if apierrors.IsConflict(err) {
+			klog.Warning("someone else is already managing the PSa labels: %v", err)
+			return nil
 		}
+		return fmt.Errorf("failed to update the namespace: %w", err)
 	}
 
 	return nil
@@ -394,12 +469,43 @@ func isNSControlled(ns *corev1.Namespace) bool {
 		return false
 	}
 
+	if ns.Labels[labelSyncControlLabel] == "true" {
+		return true
+	}
+
 	// while "openshift-" namespaces should be considered controlled, there are some
 	// edge cases where users can also create them. Consider these a special case
 	// and delegate the decision to sync on the user who should know what they are
 	// doing when creating a NS that appears to be system-controlled.
 	if strings.HasPrefix(nsName, "openshift-") {
-		return ns.Labels[labelSyncControlLabel] == "true"
+		return false
+	}
+
+	extractedPerManager, err := extractNSFieldsPerManager(ns)
+	if err != nil {
+		klog.Errorf("ns extraction failed: %v", err)
+		return false
+	}
+
+	var owningAtLeastOneLabel bool
+	for _, labelName := range []string{
+		psapi.EnforceLevelLabel, psapi.EnforceVersionLabel,
+		psapi.WarnLevelLabel, psapi.WarnVersionLabel,
+		psapi.AuditLevelLabel, psapi.AuditVersionLabel,
+	} {
+		if _, ok := ns.Labels[labelName]; ok {
+			manager := extractedPerManager.getManagerForLabel(labelName)
+			if len(manager) > 0 && manager != "cluster-policy-controller" && manager != controllerName {
+				continue
+			}
+		}
+		// a label is either not set or is directly owned by us
+		owningAtLeastOneLabel = true
+
+	}
+
+	if !owningAtLeastOneLabel {
+		return false
 	}
 
 	return ns.Labels[labelSyncControlLabel] != "false"
@@ -415,3 +521,63 @@ func controlledNamespacesLabelSelector() (labels.Selector, error) {
 
 	return labels.NewSelector().Add(*labelRequirement), nil
 }
+
+// extractedNamespaces serves as a cache so that we don't have to re-extract the namespaces
+// for each label. It helps us prevent performance overhead from multiple deserializations.
+//
+// Maps a set of managed metadata.labels to their manager name.
+type extractedNamespaces map[string]sets.Set[string]
+
+// extractNSFieldsPerManager parses all the FieldsV1 in a Namespace `ns`,
+// extracts the information about label ownership and returns a structure that
+// maps all these labels in a set to their manager
+func extractNSFieldsPerManager(ns *corev1.Namespace) (extractedNamespaces, error) {
+	ret := extractedNamespaces{}
+	for _, fieldEntry := range ns.ManagedFields {
+		managedLabels, err := managedLabels(fieldEntry)
+		if err != nil {
+			return nil, fmt.Errorf("failed to extract managed fields for NS %q: %v", ns.Name, err)
+		}
+		if current, ok := ret[fieldEntry.Manager]; ok {
+			ret[fieldEntry.Manager] = current.Union(managedLabels)
+		} else {
+			ret[fieldEntry.Manager] = managedLabels
+		}
+	}
+	return ret, nil
+}
+
+func (n extractedNamespaces) getManagerForLabel(labelName string) string {
+	for manager, extractedNS := range n {
+		if _, managed := extractedNS[labelName]; managed {
+			return manager
+		}
+	}
+	return ""
+}
+
+// managedLabels extract the metadata.labels from the JSON in the managedEntry.FieldsV1
+// that describes the object's field ownership
+func managedLabels(fieldsEntry metav1.ManagedFieldsEntry) (sets.Set[string], error) {
+	managedUnstructured := map[string]interface{}{}
+	err := json.Unmarshal(fieldsEntry.FieldsV1.Raw, &managedUnstructured)
+	if err != nil {
+		return nil, fmt.Errorf("failed to unmarshal managed fields: %w", err)
+	}
+
+	labels, found, err := unstructured.NestedMap(managedUnstructured, "f:metadata", "f:labels")
+	if err != nil {
+		return nil, fmt.Errorf("failed to get labels from the managed fields: %w", err)
+	}
+
+	ret := sets.New[string]()
+	if !found {
+		return ret, nil
+	}
+
+	for l := range labels {
+		ret.Insert(strings.Replace(l, "f:", "", 1))
+	}
+
+	return ret, nil
+}