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

stlaz · 2023-08-01T14:07:10Z

/cc @deads2k
/cc @ibihim

pkg/psalabelsyncer/podsecurity_label_sync_controller.go

liouk · 2023-08-08T15:07:14Z

pkg/psalabelsyncer/podsecurity_label_sync_controller.go

-			nsCopy.Labels[psapi.EnforceVersionLabel] = currentPSaVersion
+	managedNamespaces, err := extractNSFieldsPerManager(ns)
+	if err != nil {
+		if err != nil {


Duplicate if statement.

Huh, that must have happened somewhere through rebasing. Good thing you noticed

This probably trickled through some commit somewhere as it's still here :)

pkg/psalabelsyncer/podsecurity_label_sync_controller.go

liouk · 2023-08-14T10:53:57Z

/lgtm
/hold

holding for @deads2k to review

deads2k · 2023-08-14T20:49:56Z

pkg/psalabelsyncer/podsecurity_label_sync_controller.go

+	// 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.


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

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.

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?

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

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

pkg/psalabelsyncer/podsecurity_label_sync_controller.go

deads2k · 2023-08-14T20:56:07Z

pkg/psalabelsyncer/podsecurity_label_sync_controller.go

-	if changed {
-		_, err := c.namespaceClient.Update(ctx, nsCopy, metav1.UpdateOptions{})
+	if len(nsApplyConfig.Labels) > 0 {
+		_, err := c.namespaceClient.Apply(ctx, nsApplyConfig, metav1.ApplyOptions{FieldManager: controllerName})
 		if err != nil {


reduce nesting with

if is conflict log return nil if err != nil return err

I'd like to keep the block that contains error cases like this, unless we decide to handle more errors and introduce a switch here

I'd like to keep the block that contains error cases like this, unless we decide to handle more errors and introduce a switch here

Non-standard with both golang and kube. Sort of like using == "" versus len(foo) == 0.

deads2k · 2023-08-14T20:57:53Z

worthy of an e2e test once merged.

liouk · 2023-08-15T13:20:03Z

pkg/psalabelsyncer/podsecurity_label_sync_controller.go

-			nsCopy.Labels[psapi.EnforceVersionLabel] = currentPSaVersion
+	managedNamespaces, err := extractNSFieldsPerManager(ns)
+	if err != nil {
+		if err != nil {


This probably trickled through some commit somewhere as it's still here :)

openshift-ci · 2023-08-15T17:28:05Z

@stlaz: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

stlaz · 2023-08-16T07:44:02Z

/hold cancel

openshift-ci-robot · 2023-08-16T07:47:13Z

@stlaz: This pull request references AUTH-413 which is a valid jira issue.

In response to this:

/cc @deads2k
/cc @ibihim

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

liouk · 2023-08-16T14:18:01Z

/lgtm

openshift-ci · 2023-08-16T14:20:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liouk, stlaz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [stlaz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

liouk · 2023-08-29T13:16:39Z

pkg/psalabelsyncer/podsecurity_label_sync_controller.go


-	var changed bool
+	// we must extract the NS in case only some of the labels we're setting need
+	// updating to avoid hotlo


@stlaz just realized that this comment got somehow truncated, just FYI

openshift-ci bot requested review from deads2k and ibihim August 1, 2023 14:07

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2023

deads2k reviewed Aug 1, 2023

View reviewed changes

pkg/psalabelsyncer/podsecurity_label_sync_controller.go Outdated Show resolved Hide resolved

deads2k reviewed Aug 1, 2023

View reviewed changes

pkg/psalabelsyncer/podsecurity_label_sync_controller.go Outdated Show resolved Hide resolved

deads2k reviewed Aug 1, 2023

View reviewed changes

pkg/psalabelsyncer/podsecurity_label_sync_controller.go Outdated Show resolved Hide resolved

stlaz force-pushed the psa_managed_fields branch 10 times, most recently from 0ea5307 to 145b9af Compare August 8, 2023 14:00

deads2k reviewed Aug 8, 2023

View reviewed changes

pkg/psalabelsyncer/podsecurity_label_sync_controller.go Show resolved Hide resolved

deads2k reviewed Aug 8, 2023

View reviewed changes

pkg/psalabelsyncer/podsecurity_label_sync_controller.go Show resolved Hide resolved

deads2k reviewed Aug 8, 2023

View reviewed changes

pkg/psalabelsyncer/podsecurity_label_sync_controller.go Show resolved Hide resolved

liouk reviewed Aug 8, 2023

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 14, 2023

openshift-ci bot assigned liouk Aug 14, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2023

deads2k reviewed Aug 14, 2023

View reviewed changes

pkg/psalabelsyncer/podsecurity_label_sync_controller.go Outdated Show resolved Hide resolved

deads2k reviewed Aug 14, 2023

View reviewed changes

pkg/psalabelsyncer/podsecurity_label_sync_controller.go Show resolved Hide resolved

deads2k reviewed Aug 14, 2023

View reviewed changes

pkg/psalabelsyncer/podsecurity_label_sync_controller.go Outdated Show resolved Hide resolved

deads2k reviewed Aug 14, 2023

View reviewed changes

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2023

stlaz force-pushed the psa_managed_fields branch from 3d0cd0b to 4f24f35 Compare August 15, 2023 13:14

liouk suggested changes Aug 15, 2023

View reviewed changes

stlaz force-pushed the psa_managed_fields branch 2 times, most recently from 52c0faf to 202b52b Compare August 15, 2023 14:58

stlaz added 6 commits August 15, 2023 17:03

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

bf80f8c

ps syncer: own PSa labels previously owned by CPC

f3b7734

ps syncer: add unit tests

b49f55e

review comments + simplify isNSControlled() logic

5485d35

address comments

df62a40

ps syncer: don't mutate NS client cache between tests

f5ce53d

stlaz force-pushed the psa_managed_fields branch from 202b52b to f5ce53d Compare August 15, 2023 15:03

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 16, 2023

stlaz changed the title ~~ps syncer: only sync labels if noone else is managing them~~ AUTH-413: ps syncer: only sync labels if noone else is managing them Aug 16, 2023

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 16, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2023

openshift-merge-robot merged commit f31cb0b into openshift:master Aug 16, 2023
7 checks passed

stlaz mentioned this pull request Aug 17, 2023

psalabelsyncer: synchronize also audit and warn labels when enforcing #123

Closed

liouk reviewed Aug 29, 2023

View reviewed changes

liouk mentioned this pull request Sep 13, 2023

AUTH-428: add PSa labelsyncer managed labels e2e tests openshift/origin#28257

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

stlaz commented Aug 1, 2023

liouk Aug 8, 2023

stlaz Aug 9, 2023

liouk Aug 15, 2023

stlaz Aug 15, 2023

liouk commented Aug 14, 2023

deads2k Aug 14, 2023

stlaz Aug 15, 2023

deads2k Aug 15, 2023

deads2k Aug 15, 2023

stlaz Aug 16, 2023

deads2k Aug 14, 2023

stlaz Aug 15, 2023

deads2k Aug 15, 2023

deads2k commented Aug 14, 2023

liouk Aug 15, 2023

openshift-ci bot commented Aug 15, 2023

stlaz commented Aug 16, 2023

openshift-ci-robot commented Aug 16, 2023 •

edited by openshift-ci bot

Loading

liouk commented Aug 16, 2023

openshift-ci bot commented Aug 16, 2023

liouk Aug 29, 2023

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

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

Conversation

stlaz commented Aug 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liouk commented Aug 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Aug 14, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Aug 15, 2023

stlaz commented Aug 16, 2023

openshift-ci-robot commented Aug 16, 2023 • edited by openshift-ci bot Loading

liouk commented Aug 16, 2023

openshift-ci bot commented Aug 16, 2023

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 16, 2023 •

edited by openshift-ci bot

Loading