Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Allow the Controller and Webhook Containers to run with the securityContext: readOnlyRootfilesystem: true #2282

Merged
merged 19 commits into from
Nov 7, 2024

Conversation

npgretz
Copy link
Contributor

@npgretz npgretz commented Oct 24, 2024

🛑 Important:

Please open an issue to discuss significant work before you start. We appreciate your contributions and don't want your efforts to go to waste!

For guidelines on how to contribute, please review the CONTRIBUTING.md document.

Purpose of this PR

Provide a clear and concise description of the changes. Explain the motivation behind these changes and link to relevant issues or discussions.

Close #2218

Proposed changes:

  • Mount an emptyDir onto the controller container to write Spark artifacts to
  • Mount an emptyDir onto the webhook container to write certficates to
  • Set the default securityContexts for the controller and webhook to readOnlyRootfilesystem: true

Change Category

Indicate the type of change by marking the applicable boxes:

  • Bugfix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that could affect existing functionality)
  • Documentation update

Rationale

Please see this issue for discussion around this feature.
This feature improves the default security posture of the Spark Operator Helm Chart and allows organizations that require readOnlyRootfilesystem: true to deploy the helm chart.

Checklist

Before submitting your PR, please review the following:

  • I have conducted a self-review of my own code.
  • I have updated documentation accordingly.
  • I have added tests that prove my changes are effective or that my feature works.
  • Existing unit tests pass locally with my changes.

Additional Notes

@ChenYi015
Copy link
Contributor

@npgretz Could you do a rebase as there is a merge conflict?

@npgretz npgretz force-pushed the read-only-filesystem branch from 1bbf105 to c11b7bf Compare October 24, 2024 13:42
@npgretz
Copy link
Contributor Author

npgretz commented Oct 24, 2024

@npgretz Could you do a rebase as there is a merge conflict?

Done. Thank you for making me aware!

@npgretz npgretz force-pushed the read-only-filesystem branch from 3f022dc to 4849592 Compare October 25, 2024 23:36
@npgretz
Copy link
Contributor Author

npgretz commented Oct 25, 2024

Sorry for the small mistakes holding up the merge; this is my first open-source contribution. I am just figuring out the process.

Copy link
Contributor

@ChenYi015 ChenYi015 left a comment

Choose a reason for hiding this comment

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

It would be better to use the K8s yaml style for values.yaml file, i.e., the array need not be indented.

@ChenYi015
Copy link
Contributor

@npgretz Thanks for the contribution to enhance the security of the Spark operator. Just a few small nits, could you address them and regenerate the helm README by running make helm-docs? Then everything will be fine for me.

@npgretz
Copy link
Contributor Author

npgretz commented Nov 5, 2024

It would be better to use the K8s yaml style for values.yaml file, i.e., the array need not be indented.

Thank you for your suggestions! Could I ask for clarification on this suggestion. Is this referring to the array of objects for the volumes and volumeMounts I added for the controller and webhook pods? I am not sure how I could create these arrays of objects without the indentation and have valid YAML.

I also did not find a reference for K8's YAML Style. Could you direct me to a resource?

Thank you, again!

@ChenYi015
Copy link
Contributor

@npgretz If you run kubectl get pod -o yaml to fetch the manifest of any pod, you will find the array objects are not indented:

$ kubectl get pod -n spark-operator spark-operator-controller-69844fbfb5-nqwts -o yaml
...
spec:
  containers:
  - args:
    - controller
    - start
    - --zap-log-level=info
    - --namespaces=default
    - --controller-threads=10
    - --enable-ui-service=true
 ...

Is this referring to the array of objects for the volumes and volumeMounts I added for the controller and webhook pods?

Yes, you can simply remove the indentation so that we can maintain a consistent YAML style.

npgretz and others added 10 commits November 6, 2024 15:11
…et the controller to readOnlyRootFilesystem

Signed-off-by: Nick Gretzon <npgretz@gmail.com>
… and set readOnlyRootFilesystem: true for the webhook pod

Signed-off-by: Nick Gretzon <npgretz@gmail.com>
Signed-off-by: Nick Gretzon <npgretz@gmail.com>
Signed-off-by: Nick Gretzon <npgretz@gmail.com>
Signed-off-by: Nick Gretzon <npgretz@gmail.com>
Signed-off-by: Nick Gretzon <npgretz@gmail.com>
…unts

Signed-off-by: Nick Gretzon <npgretz@gmail.com>
… values.yaml

Signed-off-by: Nick Gretzon <npgretz@gmail.com>
… rest of values.yaml"

This reverts commit dba97fc.

Signed-off-by: Nick Gretzon <npgretz@gmail.com>
…lumes and volumeMounts

Signed-off-by: Nick Gretzon <npgretz@gmail.com>
@npgretz npgretz force-pushed the read-only-filesystem branch from 22c8d39 to 247caf3 Compare November 6, 2024 21:11
@npgretz
Copy link
Contributor Author

npgretz commented Nov 6, 2024

@ChenYi015, thank for you for explaining that. I hadn't realized the format of the templated manifests was off. I have adjusted the deployment templates for the controller and webhook to correctly indent volumes and volumeMounts.

npgretz and others added 3 commits November 6, 2024 20:14
Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>
Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>
Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>
npgretz and others added 6 commits November 6, 2024 20:15
Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>
Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>
Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>
Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>
Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>
Signed-off-by: Nick Gretzon <npgretz@gmail.com>
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ChenYi015

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

@ChenYi015
Copy link
Contributor

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Nov 7, 2024
@google-oss-prow google-oss-prow bot merged commit 72107fd into kubeflow:master Nov 7, 2024
11 checks passed
@npgretz npgretz deleted the read-only-filesystem branch November 7, 2024 16:03
ChenYi015 pushed a commit to ChenYi015/spark-operator that referenced this pull request Dec 10, 2024
…ontext: readOnlyRootfilesystem: true (kubeflow#2282)

* create a tmp dir for the controller to write Spark artifacts to and set the controller to readOnlyRootFilesystem

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* mount a dir for the webhook container to generate its certificates in and set readOnlyRootFilesystem: true for the webhook pod

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* update the securityContext in the controller deployment test

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* update securityContext of the webhook container in the deployment_test

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* update README

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* remove -- so comments are not rendered in the README.md

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* recreate README.md after removal of comments for volumes and volumeMounts

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* make indentation for volumes and volumeMounts consistent with rest of values.yaml

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* Revert "make indentation for volumes and volumeMounts consistent with rest of values.yaml"

This reverts commit dba97fc.

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* fix indentation in webhook and controller deployment templates for volumes and volumeMounts

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* Update charts/spark-operator-chart/values.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/values.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/values.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/values.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/templates/controller/deployment.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/templates/controller/deployment.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/templates/webhook/deployment.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/templates/webhook/deployment.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* add additional securityContext to the controller deployment_test.yaml

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

---------

Signed-off-by: Nick Gretzon <npgretz@gmail.com>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>
Co-authored-by: Yi Chen <github@chenyicn.net>
(cherry picked from commit 72107fd)
@ChenYi015 ChenYi015 mentioned this pull request Dec 10, 2024
google-oss-prow bot pushed a commit that referenced this pull request Dec 11, 2024
* Allow setting automountServiceAccountToken (#2298)

* Allow setting automountServiceAccountToken on workloads and serviceAccounts

Signed-off-by: Aran Shavit <Aranshavit@gmail.com>

* update helm docs

Signed-off-by: Aran Shavit <Aranshavit@gmail.com>

---------

Signed-off-by: Aran Shavit <Aranshavit@gmail.com>
(cherry picked from commit 515d805)

* Fix: executor container security context does not work (#2306)

Signed-off-by: Yi Chen <github@chenyicn.net>
(cherry picked from commit 171e429)

* Fix: should not add emptyDir sizeLimit conf if it is nil (#2305)

Signed-off-by: Yi Chen <github@chenyicn.net>
(cherry picked from commit 763682d)

* Allow the Controller and Webhook Containers to run with the securityContext: readOnlyRootfilesystem: true (#2282)

* create a tmp dir for the controller to write Spark artifacts to and set the controller to readOnlyRootFilesystem

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* mount a dir for the webhook container to generate its certificates in and set readOnlyRootFilesystem: true for the webhook pod

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* update the securityContext in the controller deployment test

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* update securityContext of the webhook container in the deployment_test

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* update README

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* remove -- so comments are not rendered in the README.md

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* recreate README.md after removal of comments for volumes and volumeMounts

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* make indentation for volumes and volumeMounts consistent with rest of values.yaml

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* Revert "make indentation for volumes and volumeMounts consistent with rest of values.yaml"

This reverts commit dba97fc.

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* fix indentation in webhook and controller deployment templates for volumes and volumeMounts

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* Update charts/spark-operator-chart/values.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/values.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/values.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/values.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/templates/controller/deployment.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/templates/controller/deployment.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/templates/webhook/deployment.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/templates/webhook/deployment.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* add additional securityContext to the controller deployment_test.yaml

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

---------

Signed-off-by: Nick Gretzon <npgretz@gmail.com>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>
Co-authored-by: Yi Chen <github@chenyicn.net>
(cherry picked from commit 72107fd)

* Fix: should not add emptyDir sizeLimit conf on executor pods if it is nil (#2316)

Signed-off-by: Cian Gallagher <cian@ciangallagher.net>
(cherry picked from commit 2999546)

* Bump `volcano.sh/apis` to 1.10.0 (#2320)

Signed-off-by: Jacob Salway <jacob.salway@gmail.com>
(cherry picked from commit 22e4fb8)

* Truncate UI service name if over 63 characters (#2311)

* Truncate UI service name if over 63 characters

Signed-off-by: Jacob Salway <jacob.salway@gmail.com>

* Also truncate ingress name

Signed-off-by: Jacob Salway <jacob.salway@gmail.com>

---------

Signed-off-by: Jacob Salway <jacob.salway@gmail.com>
(cherry picked from commit 43c1888)

* Bump aquasecurity/trivy-action from 0.28.0 to 0.29.0 (#2332)

Bumps [aquasecurity/trivy-action](https://github.com/aquasecurity/trivy-action) from 0.28.0 to 0.29.0.
- [Release notes](https://github.com/aquasecurity/trivy-action/releases)
- [Commits](aquasecurity/trivy-action@0.28.0...0.29.0)

---
updated-dependencies:
- dependency-name: aquasecurity/trivy-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
(cherry picked from commit 270b09e)

* Bump github.com/onsi/ginkgo/v2 from 2.20.2 to 2.22.0 (#2335)

Bumps [github.com/onsi/ginkgo/v2](https://github.com/onsi/ginkgo) from 2.20.2 to 2.22.0.
- [Release notes](https://github.com/onsi/ginkgo/releases)
- [Changelog](https://github.com/onsi/ginkgo/blob/master/CHANGELOG.md)
- [Commits](onsi/ginkgo@v2.20.2...v2.22.0)

---
updated-dependencies:
- dependency-name: github.com/onsi/ginkgo/v2
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
(cherry picked from commit 40423d5)

* The webhook-key-name command-line param isn't taking effect (#2344)

Signed-off-by: C. H. Afzal <c-h-afzal@outlook.com>
(cherry picked from commit a261523)

* Robustness to driver pod taking time to create (#2315)

* Retry after driver pod now found if recent submission

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Add a test

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Make grace period configurable

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Update test

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Add an extra test with the driver pod

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Separate context to create and delete the driver pod

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Tidy

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Autoformat

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Update error message

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Add helm paramater

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Update internal/controller/sparkapplication/controller.go

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Newlines between helm tests

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

---------

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>
Co-authored-by: Yi Chen <github@chenyicn.net>
(cherry picked from commit d815e78)

* Use NSS_WRAPPER_PASSWD instead of /etc/passwd as in spark-operator image entrypoint.sh (#2312)

Signed-off-by: Aakcht <aakcht@gmail.com>
(cherry picked from commit 5dd91c4)

* Move sparkctl to cmd directory (#2347)

* Move spark-operator

Signed-off-by: Yi Chen <github@chenyicn.net>

* Move sparkctl to cmd directory

Signed-off-by: Yi Chen <github@chenyicn.net>

* Remove unnecessary app package/directory

Signed-off-by: Yi Chen <github@chenyicn.net>

---------

Signed-off-by: Yi Chen <github@chenyicn.net>
(cherry picked from commit 2375a30)

* Spark Operator Official Release v2.1.0

Signed-off-by: Yi Chen <github@chenyicn.net>

---------

Signed-off-by: Yi Chen <github@chenyicn.net>
Co-authored-by: Aran Shavit <Aranshavit@gmail.com>
Co-authored-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>
Co-authored-by: Cian (Keen) Gallagher <cg1830@gmail.com>
Co-authored-by: Jacob Salway <jacob.salway@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: C. H. Afzal <41106172+c-h-afzal@users.noreply.github.com>
Co-authored-by: Thomas Newton <thomas.w.newton@gmail.com>
Co-authored-by: Aakcht <aakcht@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Allow spark-operator-controller and spark-operator-webhook pods to run without root
2 participants