cluster-api icon indicating copy to clipboard operation
cluster-api copied to clipboard

🌱 Implement privileged namespace security policy update for tilt-prepare

Open maxrantil opened this issue 1 year ago • 17 comments

What this PR does / why we need it: This PR creates a updateNamespacePodSecurityStandard function to ensure that the pod-security.kubernetes.io/enforce label is set to "privileged" for Namespace objects. This change is essential for compatibility with CAPIs tilt-prepare and Tiltfile when using CAPM3 (and other providers?), where accessing securityContext is required. Without this update, the existing pod-security policy restricts the usage of securityContext, hindering necessary operations. This aligns the behavior with CAPD, where the privileged policy is already set for Namespace objects.

/area provider/infrastructure-in-memory

maxrantil avatar Feb 20 '24 10:02 maxrantil

@maxrantil: The label(s) area/ cannot be applied, because the repository doesn't have them.

In response to this:

What this PR does / why we need it: This PR creates a updateNamespaceSecurityPolicy function to ensure that the pod-security.kubernetes.io/enforce label is set to "privileged" for Namespace objects. This change is essential for compatibility with CAPIs tilt-prepare and Tiltfile when using CAPM3 (and other providers?), where accessing securityContext is required. Without this update, the existing pod-security policy restricts the usage of securityContext, hindering necessary operations. This aligns the behavior with CAPD, where the privileged policy is already set for Namespace objects.

/area provider/infrastructure-in-memory

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.

k8s-ci-robot avatar Feb 20 '24 10:02 k8s-ci-robot

Hi @maxrantil. 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/test-infra repository.

k8s-ci-robot avatar Feb 20 '24 10:02 k8s-ci-robot

/ok-to-test

chrischdi avatar Feb 20 '24 10:02 chrischdi

/remove-area provider/infrastructure-in-memory

chrischdi avatar Feb 20 '24 10:02 chrischdi

/area devtools

chrischdi avatar Feb 20 '24 10:02 chrischdi

This change is essential for compatibility with CAPIs tilt-prepare and Tiltfile when using CAPM3 (and other providers?), where accessing securityContext is required. Without this update, the existing pod-security policy restricts the usage of securityContext, hindering necessary operations. This aligns the behavior with CAPD, where the privileged policy is already set for Namespace objects.

I just wanted to clarify this a bit. Tilt-prepare already removes the security context from the deployments, but this is not enough if the namespace has enforced pod security standards. The issue is not visible for CAPI/CAPD since CAPD anyway uses the privileged policy in order to mount the docker socket. Other providers will likely be able to run with a restricted policy for the namespace, except when using Tilt. This is why we want to make tilt-prepare change the label on the namespace.

lentzi90 avatar Feb 20 '24 11:02 lentzi90

This change is essential for compatibility with CAPIs tilt-prepare and Tiltfile when using CAPM3 (and other providers?), where accessing securityContext is required. Without this update, the existing pod-security policy restricts the usage of securityContext, hindering necessary operations. This aligns the behavior with CAPD, where the privileged policy is already set for Namespace objects.

I just wanted to clarify this a bit. Tilt-prepare already removes the security context from the deployments, but this is not enough if the namespace has enforced pod security standards. The issue is not visible for CAPI/CAPD since CAPD anyway uses the privileged policy in order to mount the docker socket. Other providers will likely be able to run with a restricted policy for the namespace, except when using Tilt. This is why we want to make tilt-prepare change the label on the namespace.

100% agree, this gets helpful when PSA is enforced in a clusters and we want to run a provider or the core CAPI providers using tilt. Core CAPI controllers should be affected to, only CAPD not because it requires privileged anyway.

Edit: it comes down to that the reason for wanting this in tilt-prepare is this line in tilt-prepare: https://github.com/kubernetes-sigs/cluster-api/blob/09f3520ff8e081485cde7840dff7bf08896a94a9/hack/tools/internal/tilt-prepare/main.go#L800

chrischdi avatar Feb 20 '24 11:02 chrischdi

Good point! So the reason it is currently working for CAPI is that there are no restrictions on the namespace. Probably the same for many other providers.

lentzi90 avatar Feb 20 '24 11:02 lentzi90

@chrischdi appreciate it a lot, thank you!

maxrantil avatar Feb 20 '24 13:02 maxrantil

LGTM label has been added.

Git tree hash: d232020f51f888b20f623983edefe5e19f4525dd

k8s-ci-robot avatar Feb 20 '24 13:02 k8s-ci-robot

/cc @killianmuldoon @JoelSpeed

maxrantil avatar Feb 26 '24 07:02 maxrantil

LGTM label has been added.

Git tree hash: 7cd7cabbf76b59c96494ba7f5bad36da33982cb3

k8s-ci-robot avatar Mar 08 '24 07:03 k8s-ci-robot

/lgtm

chrischdi avatar Mar 11 '24 09:03 chrischdi

LGTM label has been added.

Git tree hash: 92cf5b3fa87d43dd1aec4c273f8892485401cc79

k8s-ci-robot avatar Mar 11 '24 09:03 k8s-ci-robot

/cc @killianmuldoon @JoelSpeed would any of you please be willing to do a review?

maxrantil avatar Mar 13 '24 09:03 maxrantil

LGTM label has been added.

Git tree hash: dc11a973b8c001a38361c1b5656ab20d9dbd884b

k8s-ci-robot avatar Mar 20 '24 07:03 k8s-ci-robot

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: killianmuldoon

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:

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 avatar Mar 21 '24 11:03 k8s-ci-robot