Skip to content
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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lunarwhite
Copy link
Contributor

@lunarwhite lunarwhite commented Dec 19, 2024

Description

Closes #4423

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 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
Copy link
Contributor

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

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 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
Copy link
Contributor

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.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 19, 2024
"spec": {
"containers": [{
"name": "curl",
"image": "curlimages/curl:7.87.0",
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed. Ready for another review.

@camilamacedo86
Copy link
Member

/ok-to-test

@k8s-ci-robot 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
Copy link
Member

camilamacedo86 commented Dec 19, 2024

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

@camilamacedo86 camilamacedo86 self-requested a review December 19, 2024 08:21
@lunarwhite
Copy link
Contributor Author

lunarwhite commented Dec 19, 2024

@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
Copy link
Member

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.

Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/ok-to-test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure Security Context Compliance in Tests and User-Scaffolded Tests for Restricted Environments
3 participants