-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AUTH-413: ps syncer: only sync labels if noone else is managing them #127
Merged
openshift-merge-robot
merged 6 commits into
openshift:master
from
stlaz:psa_managed_fields
Aug 16, 2023
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
bf80f8c
ps syncer: only sync labels if noone else is managing them
stlaz f3b7734
ps syncer: own PSa labels previously owned by CPC
stlaz b49f55e
ps syncer: add unit tests
stlaz 5485d35
review comments + simplify isNSControlled() logic
stlaz df62a40
address comments
stlaz f5ce53d
ps syncer: don't mutate NS client cache between tests
stlaz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stlaz just realized that this comment got somehow truncated, just FYI |
||
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 | ||
stlaz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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. | ||
stlaz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// | ||
// 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) { | ||
stlaz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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") | ||
deads2k marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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 | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you set it to the current value with a force?
There was a problem hiding this comment.
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:
cluster-policy-controller
+ apply to the same valuea. the labels is now managed by
cluster-policy-controller + update
andcluster-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
this-controller
+ apply the same valuea. the label is now managed by
cluster-policy-controller + update
andthis-controller + apply
b. try to change the label value as this-controller - you get a conflict