fix: ensure pod security label on namespace #774

saumeya · 2024-08-28T09:58:56Z

What type of PR is this?

Uncomment only one /kind line, and delete the rest.
For example, > /kind bug would simply become: /kind bug

/kind bug
/kind cleanup
/kind failing-test
/kind enhancement
/kind documentation
/kind code-refactoring

What does this PR do / why we need it:

Have you updated the necessary documentation?

Documentation update is required by this PR.
Documentation has been updated.

Which issue(s) this PR fixes:

Fixes #?

Test acceptance criteria:

Unit Test
E2E Test

How to test changes / Special notes to the reviewer:

svghadi · 2024-08-29T12:59:22Z

/hold

We need to revisit this. Adding a restrictive pod security standard to an existing namespace (during an upgrade) will result in failure because the existing pods in the namespace won't be compliant with the restricted policy. The required policy configurations were added in argoproj-labs/argocd-operator#1288 and argoproj-labs/argocd-operator#1493, but these changes only work for fresh installs. Existing pods are not configured with the new policy settings unless the corresponding deployments are manually deleted.

svghadi

OpenShift automatically handles the securityContext if it is missing. After running some tests, my previous concerns have been addressed, and there shouldn’t be any issues during the upgrade. I also have PR #1533 to handle existing deployments, but it’s not a blocker for this PR.

/remove-hold

svghadi · 2024-09-02T09:59:37Z

controllers/gitopsservice_controller.go

+func ensurePodSecurityLabels(namespace *corev1.Namespace) (bool, *corev1.Namespace) {
+	for key := range namespace.Labels {
+		if strings.HasPrefix(key, "pod-security") {
+			return false, namespace
+		}
+	}
+
+	namespace.Labels["pod-security.kubernetes.io/enforce"] = "restricted"
+	namespace.Labels["pod-security.kubernetes.io/enforce-version"] = "v1.29"
+	namespace.Labels["pod-security.kubernetes.io/audit"] = "restricted"
+	namespace.Labels["pod-security.kubernetes.io/audit-version"] = "latest"
+	namespace.Labels["pod-security.kubernetes.io/warn"] = "restricted"
+	namespace.Labels["pod-security.kubernetes.io/warn-version"] = "latest"
+
+	return true, namespace
+}


Can we do a strict check here? The current logic won't detect changes if the label value is modified. Something like...

Suggested change

func ensurePodSecurityLabels(namespace *corev1.Namespace) (bool, *corev1.Namespace) {

for key := range namespace.Labels {

if strings.HasPrefix(key, "pod-security") {

return false, namespace

}

}

namespace.Labels["pod-security.kubernetes.io/enforce"] = "restricted"

namespace.Labels["pod-security.kubernetes.io/enforce-version"] = "v1.29"

namespace.Labels["pod-security.kubernetes.io/audit"] = "restricted"

namespace.Labels["pod-security.kubernetes.io/audit-version"] = "latest"

namespace.Labels["pod-security.kubernetes.io/warn"] = "restricted"

namespace.Labels["pod-security.kubernetes.io/warn-version"] = "latest"

return true, namespace

}

func ensurePodSecurityLabels(namespace *corev1.Namespace) (bool, *corev1.Namespace) {

pssLabels := map[string]string{

"pod-security.kubernetes.io/enforce": "restricted",

"pod-security.kubernetes.io/enforce-version": "v1.29",

"pod-security.kubernetes.io/audit": "restricted",

"pod-security.kubernetes.io/audit-version": "latest",

"pod-security.kubernetes.io/warn": "restricted",

"pod-security.kubernetes.io/warn-version": "latest",

}

changed := false

for pssKey, pssVal := range pssLabels {

if nsVal, exists := namespace.Labels[pssKey]; !exists || nsVal != pssVal {

namespace.Labels[pssKey] = pssVal

changed = true

}

}

return changed, namespace

}

svghadi · 2024-09-02T10:07:52Z

/remove-hold

Signed-off-by: saumeya <[email protected]>

svghadi

/lgtm

Thanks

iam-veeramalla · 2024-09-04T14:41:22Z

controllers/gitopsservice_controller.go

@@ -369,6 +377,15 @@ func (r *ReconcileGitopsService) reconcileDefaultArgoCDInstance(instance *pipeli
 				return reconcile.Result{}, err
 			}
 		}
+
+		needUpdate, updateNameSpace := ensurePodSecurityLabels(argocdNS)


Just a NIT, I would change the word needUpdate to needsUpdate.

iam-veeramalla

Thanks for the PR, LGTM except for the NIT.

iam-veeramalla · 2024-09-04T14:43:32Z

/lgtm
/approve

openshift-ci · 2024-09-04T14:43:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iam-veeramalla

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~~ [iam-veeramalla]

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

saumeya · 2024-09-04T16:10:09Z

/cherry-pick v1.14

saumeya · 2024-09-04T16:10:15Z

/cherry-pick v1.13

openshift-cherrypick-robot · 2024-09-04T16:10:18Z

@saumeya: only redhat-developer org members may request cherry picks. If you are already part of the org, make sure to change your membership to public. Otherwise you can still do the cherry-pick manually.

In response to this:

/cherry-pick v1.14

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-sigs/prow repository.

openshift-cherrypick-robot · 2024-09-04T16:10:24Z

@saumeya: only redhat-developer org members may request cherry picks. If you are already part of the org, make sure to change your membership to public. Otherwise you can still do the cherry-pick manually.

In response to this:

/cherry-pick v1.13

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-sigs/prow repository.

saumeya · 2024-09-04T16:11:18Z

/cherry-pick v1.14

saumeya · 2024-09-04T16:11:57Z

/cherry-pick v1.13

openshift-cherrypick-robot · 2024-09-04T16:12:00Z

@saumeya: #774 failed to apply on top of branch "v1.14":

Patch is empty.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To record the empty patch as an empty commit, run "git am --allow-empty".
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick v1.14

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-sigs/prow repository.

openshift-cherrypick-robot · 2024-09-04T16:12:39Z

@saumeya: new pull request created: #777

In response to this:

/cherry-pick v1.13

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-sigs/prow repository.

saumeya requested a review from svghadi August 28, 2024 09:59

openshift-ci bot requested a review from chetan-rns August 28, 2024 09:59

saumeya force-pushed the ensure-pod-security-label branch from 3e24e11 to 0b17b20 Compare August 28, 2024 10:11

openshift-ci bot added the do-not-merge/hold label Aug 29, 2024

svghadi reviewed Sep 2, 2024

View reviewed changes

openshift-ci bot removed the do-not-merge/hold label Sep 2, 2024

saumeya added 3 commits September 3, 2024 16:57

fix: ensure pod security label on namespace

17fad5b

Signed-off-by: saumeya <[email protected]>

fix:

27e5651

Signed-off-by: saumeya <[email protected]>

review comments

ff7bcbe

Signed-off-by: saumeya <[email protected]>

saumeya force-pushed the ensure-pod-security-label branch from 0b17b20 to ff7bcbe Compare September 3, 2024 11:35

svghadi approved these changes Sep 3, 2024

View reviewed changes

openshift-ci bot assigned svghadi Sep 3, 2024

openshift-ci bot added the lgtm label Sep 3, 2024

iam-veeramalla reviewed Sep 4, 2024

View reviewed changes

openshift-ci bot assigned iam-veeramalla Sep 4, 2024

openshift-ci bot added the approved label Sep 4, 2024

openshift-merge-bot bot merged commit b50e9e1 into redhat-developer:master Sep 4, 2024
20 checks passed

openshift-cherrypick-robot mentioned this pull request Sep 4, 2024

[v1.13] fix: ensure pod security label on namespace #777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ensure pod security label on namespace #774

fix: ensure pod security label on namespace #774

saumeya commented Aug 28, 2024

svghadi commented Aug 29, 2024

svghadi left a comment •

edited

Loading

svghadi Sep 2, 2024 •

edited

Loading

saumeya Sep 3, 2024

svghadi commented Sep 2, 2024

svghadi left a comment

iam-veeramalla Sep 4, 2024

iam-veeramalla left a comment

iam-veeramalla commented Sep 4, 2024

openshift-ci bot commented Sep 4, 2024

saumeya commented Sep 4, 2024

saumeya commented Sep 4, 2024

openshift-cherrypick-robot commented Sep 4, 2024

openshift-cherrypick-robot commented Sep 4, 2024

saumeya commented Sep 4, 2024

saumeya commented Sep 4, 2024

openshift-cherrypick-robot commented Sep 4, 2024

openshift-cherrypick-robot commented Sep 4, 2024

-func ensurePodSecurityLabels(namespace *corev1.Namespace) (bool, *corev1.Namespace) {
-	for key := range namespace.Labels {
-		if strings.HasPrefix(key, "pod-security") {
-			return false, namespace
-		}
-	}
-	namespace.Labels["pod-security.kubernetes.io/enforce"] = "restricted"
-	namespace.Labels["pod-security.kubernetes.io/enforce-version"] = "v1.29"
-	namespace.Labels["pod-security.kubernetes.io/audit"] = "restricted"
-	namespace.Labels["pod-security.kubernetes.io/audit-version"] = "latest"
-	namespace.Labels["pod-security.kubernetes.io/warn"] = "restricted"
-	namespace.Labels["pod-security.kubernetes.io/warn-version"] = "latest"
-	return true, namespace
-}
+func ensurePodSecurityLabels(namespace *corev1.Namespace) (bool, *corev1.Namespace) {
+	pssLabels := map[string]string{
+		"pod-security.kubernetes.io/enforce": "restricted",
+		"pod-security.kubernetes.io/enforce-version": "v1.29",
+		"pod-security.kubernetes.io/audit": "restricted",
+		"pod-security.kubernetes.io/audit-version": "latest",
+		"pod-security.kubernetes.io/warn": "restricted",
+		"pod-security.kubernetes.io/warn-version": "latest",
+	}
+	changed := false
+	for pssKey, pssVal := range pssLabels {
+		if nsVal, exists := namespace.Labels[pssKey]; !exists || nsVal != pssVal {
+			namespace.Labels[pssKey] = pssVal
+			changed = true
+		}
+	}
+	return changed, namespace
+}

fix: ensure pod security label on namespace #774

fix: ensure pod security label on namespace #774

Conversation

saumeya commented Aug 28, 2024

svghadi commented Aug 29, 2024

svghadi left a comment • edited Loading

Choose a reason for hiding this comment

svghadi Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

saumeya Sep 3, 2024

Choose a reason for hiding this comment

svghadi commented Sep 2, 2024

svghadi left a comment

Choose a reason for hiding this comment

iam-veeramalla Sep 4, 2024

Choose a reason for hiding this comment

iam-veeramalla left a comment

Choose a reason for hiding this comment

iam-veeramalla commented Sep 4, 2024

openshift-ci bot commented Sep 4, 2024

saumeya commented Sep 4, 2024

saumeya commented Sep 4, 2024

openshift-cherrypick-robot commented Sep 4, 2024

openshift-cherrypick-robot commented Sep 4, 2024

saumeya commented Sep 4, 2024

saumeya commented Sep 4, 2024

openshift-cherrypick-robot commented Sep 4, 2024

openshift-cherrypick-robot commented Sep 4, 2024

svghadi left a comment •

edited

Loading

svghadi Sep 2, 2024 •

edited

Loading