feat(helm): support restricted pod security for CRD upgrade hook Job

[KHARSHAVARDHAN-eng]

Description

Fixes #3070.

This PR updates the CRD upgrade hook Job so it can run in Kubernetes namespaces enforcing the restricted Pod Security Standard.

Changes

• Run the kubectl hook image as a non-root user by default.
• Add configurable hook.podSecurityContext and hook.securityContext values.
• Add restricted Pod Security compliant defaults, including:
  • runAsNonRoot: true
  • seccompProfile.type: RuntimeDefault
  • readOnlyRootFilesystem: true
  • allowPrivilegeEscalation: false
  • privileged: false
  • capabilities.drop: [ALL]
• Add Helm unit tests for default and custom security contexts.
• Regenerate the Helm chart README documentation.

Testing

• ./bin/helm unittest charts/spark-operator-chart --strict --file "tests/**/*_test.yaml"
  • 235/235 tests passed
• Verified the rendered Helm template with default and custom security context values.
• Ran git diff --check successfully.

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details

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

[github-actions]

🎉 Welcome to the Kubeflow Spark Operator! 🎉

Thanks for opening your first PR! We're happy to have you as part of our community 🚀

Here's what happens next:

• If you haven't already, please check out our Contributing Guide for repo-specific guidelines and the Kubeflow Contributor Guide for general community standards.
• Our team will review your PR soon!

Join the community:

• Slack: Join our #kubeflow-spark-on-kubernetes Slack channel.

Feel free to ask questions in the comments if you need any help or clarification!
Thanks again for contributing to Kubeflow! 🙏

[sxivansx]

Pulled the branch. Renders restricted-compliant, unittest 235/235, README regen looks right. USER 65534:65534 is the correct fix for the Dockerfile.

One problem though. runAsNonRoot: true with no runAsUser fails against any image built before this PR. I pulled the config blobs from ghcr:

kubectl:2.5.2       User=''
kubectl:2.5.0-rc.0  User=''

Both UID 0, and no released tag has a USER line. Kubelet rejects that with container has runAsNonRoot and image will run as root, so the Job never starts and helm upgrade fails.

Hits anyone pinning hook.image.tag, pulling through a mirror, or using their own kubectl image. Chart appVersion is still 2.5.0-rc.0, so the default points at a root image too until a new one ships.

Neither gate catches this. helm unittest only checks rendered YAML, and integration never exercises hook.upgradeCrd.

runAsUser: 65534 in the default securityContext fixes it and still passes restricted.

@danish9039 suggested runAsNonRoot alone above. I think that's backwards: runAsNonRoot alone is what breaks other images, runAsUser is what makes it portable. Unless there's a reason not to pin the UID?

Minor: --set hook.securityContext=null now drops all the hardening that used to be hardcoded. Matches how controller.securityContext works so probably intentional, just flagging it.

[KHARSHAVARDHAN-eng]

Thanks for catching this! @sxivansx I've addressed the concern by adding runAsUser: 65534 to the default hook.securityContext, so the hook doesn't rely on the image metadata to satisfy runAsNonRoot.

I've also updated the corresponding Helm unit test and chart documentation.

@danish9039.. Please take a look and looking a any further feedback!!!!!

[sxivansx]

Verified. Pinning hook.image.tag=2.5.2 now renders runAsUser: 65534, so the kubelet has an explicit UID and won't reject the pod. Still restricted-compliant, and unittest is 235/235.

Old images work too: kubectl is 0755 and the CRDs are world-readable, so 65534 can read and exec both without the image needing its own USER.

LGTM from me, though I'm not a maintainer so it still needs a reviewer.

[tariq-hasan]

Hi @KHARSHAVARDHAN-eng thanks so much for working on this and @sxivansx for guiding the review! I have added a few comments.

Also @KHARSHAVARDHAN-eng please follow these instructions to sign your commits: https://github.com/kubeflow/spark-operator/pull/3129/checks?check_run_id=99506904919.

[tariq-hasan]

I can understand runAsUser was set to help enable backward-compatibility but it would end up doing more harm than good - since we are forcing all new installations to run as this user when that may not be the intention to begin with.

To align with controller and webhook let's drop runAsUser. For platform admins that do need compatibility with older images they are free to add this option as needed - we can add a note above the key for this?

Suggested change

	# -- Security context for the Helm hook Job pod.
	podSecurityContext: {}
	# -- Security context for the Helm hook Job container.
	securityContext:
	readOnlyRootFilesystem: true
	privileged: false
	allowPrivilegeEscalation: false
	runAsNonRoot: true
	runAsUser: 65534
	capabilities:
	drop:
	- ALL
	seccompProfile:
	type: RuntimeDefault
	# -- Security context for the Helm hook Job pod.
	podSecurityContext: {}
	# -- Security context for the Helm hook Job container.
	securityContext:
	readOnlyRootFilesystem: true
	privileged: false
	allowPrivilegeEscalation: false
	runAsNonRoot: true
	capabilities:
	drop:
	- ALL
	seccompProfile:
	type: RuntimeDefault

The general principle here is regarding portability. We'd want to ship a chart that expresses security properties but without exact implementation details. In this case, imposing a concrete user identity would conflict with platform-level UID allocation such as in vendor-based platforms such as OpenShift. From that point of view it makes perfect sense to add runAsNonRoot: true and retain a default identity in the image but enable platform admins to override with appropriate UIDs or security policies as necessary.

This would then enable a clean abstraction boundary and separation of responsibilities between what the chart, image and platform individually accomplish - chart expresses security properties, image supplies the identity, platform overrides.

[tariq-hasan]

For consistency should we use 185 as UID and GID? Granted 185 exists in the controller image since that's native to the Spark image itself but I am wondering if we can retain the same identity here for consistency here as well.

spark-operator/Dockerfile

Lines 58 to 72 in 761686e

	ARG SPARK_UID=185
	ARG SPARK_GID=185
	USER root
	RUN apt-get update \
	&& apt-get install -y catatonit \
	&& rm -rf /var/lib/apt/lists/*
	RUN mkdir -p /etc/k8s-webhook-server/serving-certs /home/spark && \
	chmod -R g+rw /etc/k8s-webhook-server/serving-certs && \
	chown -R spark /etc/k8s-webhook-server/serving-certs /home/spark
	USER ${SPARK_UID}:${SPARK_GID}

[tariq-hasan]

Should we also add a negative test for hook.securityContext: null?

[tariq-hasan]

/ok-to-test
/retest

[danish9039]

@KHARSHAVARDHAN-eng Please add an integration test that enables hook.upgradeCrd in a restricted namespace and verifies the hook completes.

[tariq-hasan]

@KHARSHAVARDHAN-eng Please address the review comments so we can move forward with the PR.