✨ feat: enforce restricted Pod Security Context Compliance in testing #4435

lunarwhite · 2024-12-19T04:44:01Z

Description

Main changes:

Updated the test scaffolds to enforce the restricted security policy:
- Labeled the manager namespace with pod-security.kubernetes.io/enforce=restricted.
- Changed the curl Pod creation command to adhere the securityContext Pod spec fields
Updated the tests under test/e2e:
- Changed the utils func LabelNamespacesToWarnAboutRestricted(), RemoveNamespaceLabelToWarnAboutRestricted() to LabelNamespacesToEnforceRestricted(), RemoveNamespaceLabelToEnforceRestricted().
- Made adjustments to ensure compatibility with the strict security policy enforced.
Changed to use curlimages/curl:latest instead of pinning a version.

k8s-ci-robot · 2024-12-19T04:44:08Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lunarwhite
Once this PR has been reviewed and has the lgtm label, please assign varshaprasad96 for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

k8s-ci-robot · 2024-12-19T04:44:11Z

Hi @lunarwhite. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

camilamacedo86 · 2024-12-19T08:14:17Z

testdata/project-v4-multigroup/test/e2e/e2e_test.go

+					"spec": {
+						"containers": [{
+							"name": "curl",
+							"image": "curlimages/curl:7.87.0",


Instead of pinning a version, do you think we could use the latest to make it easier the code be maintained and we do not need to bump new versions

WDYT?

Addressed. Ready for another review.

camilamacedo86 · 2024-12-19T08:14:54Z

/ok-to-test

camilamacedo86 · 2024-12-19T08:21:43Z

Hi @lunarwhite

Regards the CI errors:

In this case it is failing in the API diff because we are changing it at

sigs.k8s.io/kubebuilder/v4/test/e2e/utils
Incompatible changes:

(*TestContext).LabelNamespacesToWarnAboutRestricted: removed
(*TestContext).RemoveNamespaceLabelToWarnAboutRestricted: removed

But we do not really need to support its utils
That is for tests only, mainly us. we might want to make it internal in the future
Anyway, it seems fine. No worry about this failure.

⚠️ See that the tests are failing here: e2e deployimage tests.

Note that we have a plugin that scaffolds code to manage an image. We might consider enforcing tests only for go/v4 and providing warnings for other cases, as seen in the test scaffolds provided to end users.

The first step is to understand the error mentioned above. Additionally, ensure that the error string is being output correctly—it might currently be missing a conversion from []byte to string. If you need to know more about how it works see: https://book.kubebuilder.io/plugins/available/deploy-image-plugin-v1-alpha

lunarwhite · 2024-12-19T14:52:13Z

@camilamacedo86 Thanks for your hints!

The first step is to understand the error mentioned above.

The readable output would be like:

/workspaces/kubebuilder/test/e2e/deployimage/e2e-rorm/bin/controller-gen rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases
  cd config/manager && /workspaces/kubebuilder/test/e2e/deployimage/e2e-rorm/bin/kustomize edit set image controller=e2e-test/controller-manager:rorm
  /workspaces/kubebuilder/test/e2e/deployimage/e2e-rorm/bin/kustomize build config/default | kubectl apply -f -
  namespace/e2e-rorm-system created
  customresourcedefinition.apiextensions.k8s.io/foororms.barrorm.example.comrorm unchanged
  serviceaccount/e2e-rorm-controller-manager created
  role.rbac.authorization.k8s.io/e2e-rorm-leader-election-role created
  clusterrole.rbac.authorization.k8s.io/e2e-rorm-foororm-admin-role created
  clusterrole.rbac.authorization.k8s.io/e2e-rorm-foororm-editor-role created
  clusterrole.rbac.authorization.k8s.io/e2e-rorm-foororm-viewer-role created
  clusterrole.rbac.authorization.k8s.io/e2e-rorm-manager-role created
  clusterrole.rbac.authorization.k8s.io/e2e-rorm-metrics-auth-role created
  clusterrole.rbac.authorization.k8s.io/e2e-rorm-metrics-reader created
  rolebinding.rbac.authorization.k8s.io/e2e-rorm-leader-election-rolebinding created
  clusterrolebinding.rbac.authorization.k8s.io/e2e-rorm-manager-rolebinding created
  clusterrolebinding.rbac.authorization.k8s.io/e2e-rorm-metrics-auth-rolebinding created
  service/e2e-rorm-controller-manager-metrics-service created
  deployment.apps/e2e-rorm-controller-manager created

Are you suggesting we should keep the "Warning: would violate PodSecurity" checkpoint like:

	By("deploying the controller-manager")
	cmd := exec.Command("make", "deploy", "IMG="+kbc.ImageName)
++     	out, err := kbc.Run(cmd)
++	Expect(string(output)).NotTo(ContainSubstring("Warning: would violate PodSecurity"))
--	Expect(kbc.Run(cmd)).NotTo(ContainSubstring("Warning: would violate PodSecurity"))

Or just:

	By("deploying the controller-manager")
++	Expect(kbc.Make("deploy", "IMG="+kbc.ImageName)).To(Succeed())
--	cmd := exec.Command("make", "deploy", "IMG="+kbc.ImageName)
--	Expect(kbc.Run(cmd)).NotTo(ContainSubstring("Warning: would violate PodSecurity"))

Both of them passed locally.

camilamacedo86 · 2024-12-19T22:00:25Z

We need to have

out, err := kbc.Run(cmd)
++ Expect(string(output)).NotTo(ContainSubstring("Warning: would violate PodSecurity"))

To get the output in the logs.

camilamacedo86

/ok-to-test

mogsie · 2024-12-28T11:25:23Z

test/e2e/deployimage/plugin_cluster_test.go

@@ -93,7 +93,8 @@ func Run(kbc *utils.TestContext) {

 	By("deploying the controller-manager")
 	cmd := exec.Command("make", "deploy", "IMG="+kbc.ImageName)
-	Expect(kbc.Run(cmd)).NotTo(ContainSubstring("Warning: would violate PodSecurity"))
+	out, _ := kbc.Run(cmd)


This change ignores the error here. is that intentional?

Since it goes against idioms, maybe it should be commented?

Hi @mogsie

IHMO: We need to have like

_, err = utils.Run(cmd) Expect(err).NotTo(HaveOccurred(), "Failed to label namespace with restricted policy")

Instead of Expect(utils.Run(cmd)), which does not make the result/output clear, we risk having an error in the string format outputted when needed.

But if you see any other halls for improvements, can you please raise an issue for each case scenario so that it makes it easier for us to follow up wdyt?

Reported it as #4450

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 19, 2024

k8s-ci-robot requested review from camilamacedo86 and Kavinjsir December 19, 2024 04:44

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 19, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 19, 2024

lunarwhite mentioned this pull request Dec 19, 2024

Ensure Security Context Compliance in Tests and User-Scaffolded Tests for Restricted Environments #4423

Closed

camilamacedo86 reviewed Dec 19, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 19, 2024

camilamacedo86 self-requested a review December 19, 2024 08:21

lunarwhite force-pushed the scc branch from afffd18 to 922ba41 Compare December 20, 2024 07:12

feat: enforce restricted Pod Security Context Compliance in testing

0c45cf3

lunarwhite force-pushed the scc branch from 922ba41 to 0c45cf3 Compare December 20, 2024 07:21

camilamacedo86 reviewed Dec 20, 2024

View reviewed changes

camilamacedo86 merged commit ea2dee6 into kubernetes-sigs:master Dec 23, 2024
24 of 26 checks passed

camilamacedo86 mentioned this pull request Dec 23, 2024

Next Kubebuilder Release v4.4.0: Planning and Alignment #4427

Open

lunarwhite deleted the scc branch December 23, 2024 12:02

mogsie reviewed Dec 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ feat: enforce restricted Pod Security Context Compliance in testing #4435

✨ feat: enforce restricted Pod Security Context Compliance in testing #4435

lunarwhite commented Dec 19, 2024 •

edited

Loading

k8s-ci-robot commented Dec 19, 2024

k8s-ci-robot commented Dec 19, 2024

camilamacedo86 Dec 19, 2024

lunarwhite Dec 20, 2024

camilamacedo86 commented Dec 19, 2024

camilamacedo86 commented Dec 19, 2024 •

edited

Loading

lunarwhite commented Dec 19, 2024 •

edited

Loading

camilamacedo86 commented Dec 19, 2024

camilamacedo86 left a comment

mogsie Dec 28, 2024

camilamacedo86 Dec 29, 2024 •

edited

Loading

mogsie Dec 29, 2024

✨ feat: enforce restricted Pod Security Context Compliance in testing #4435

✨ feat: enforce restricted Pod Security Context Compliance in testing #4435

Conversation

lunarwhite commented Dec 19, 2024 • edited Loading

Description

k8s-ci-robot commented Dec 19, 2024

k8s-ci-robot commented Dec 19, 2024

camilamacedo86 Dec 19, 2024

Choose a reason for hiding this comment

lunarwhite Dec 20, 2024

Choose a reason for hiding this comment

camilamacedo86 commented Dec 19, 2024

camilamacedo86 commented Dec 19, 2024 • edited Loading

lunarwhite commented Dec 19, 2024 • edited Loading

camilamacedo86 commented Dec 19, 2024

camilamacedo86 left a comment

Choose a reason for hiding this comment

mogsie Dec 28, 2024

Choose a reason for hiding this comment

camilamacedo86 Dec 29, 2024 • edited Loading

Choose a reason for hiding this comment

mogsie Dec 29, 2024

Choose a reason for hiding this comment

lunarwhite commented Dec 19, 2024 •

edited

Loading

camilamacedo86 commented Dec 19, 2024 •

edited

Loading

lunarwhite commented Dec 19, 2024 •

edited

Loading

camilamacedo86 Dec 29, 2024 •

edited

Loading