Skip to content
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
ps syncer: own PSa labels previously owned by CPC
  • Loading branch information
stlaz committed Aug 15, 2023
commit f3b77341f493abbcc5f3051c05a09a48580ec739
75 changes: 74 additions & 1 deletion pkg/psalabelsyncer/podsecurity_label_sync_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ 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"
Expand All @@ -35,7 +37,6 @@ const (
controllerName = "pod-security-admission-label-synchronization-controller"
labelSyncControlLabel = "security.openshift.io/scc.podSecurityLabelSync"
currentPSaVersion = "v1.24"
managerName = "cluster-policy-controller" // for historical reasons we cannot use controllerName for server-side patching
)

var (
Expand Down Expand Up @@ -203,13 +204,85 @@ 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 _, labelMap := range []map[string]string{enforcementLabels, loggingLabels} {
for labelType, labelVersion := range labelMap {
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than do this, could we simply do an Apply with the force option set to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not possible. The field is owned by "cluster-policy-controller + Update". You cannot set the label to an empty value (fails validation). That means that the only action you can do with it is to remove it. But I don't think there is a way to set up a NamespaceApplyConfiguration that would express that intention.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not possible. The field is owned by "cluster-policy-controller + Update". You cannot set the label to an empty value (fails validation). That means that the only action you can do with it is to remove it. But I don't think there is a way to set up a NamespaceApplyConfiguration that would express that intention

Can't you set it to the current value with a force?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you set it to the current value, the "force" does not seem to matter at all. You will always get double ownership, which will still result in a conflict when you're trying to change it. Some fun corner cases I tested:

  1. cluster-policy-controller + apply to the same value
    a. the labels is now managed by cluster-policy-controller + update and cluster-policy-controller + apply
    b. you get the applyconfig and remove the ownership in an apply as the cluster-policy-controller
    c. the field is now again owned only by cluster-policy-controller + update
  2. this-controller + apply the same value
    a. the label is now managed by cluster-policy-controller + update and this-controller + apply
    b. try to change the label value as this-controller - you get a conflict

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
Expand Down