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

Update Helm Chart to Run as Non-Root User in OpenShift #5

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

Conversation

captainpro-eng
Copy link

This pull request updates the PrestoDB Helm chart to support deployment on OpenShift with the necessary configurations to run as a non-root user. The changes include:

  • Adding securityContext and podSecurityContext configurations in the Helm chart to specify non-root user settings.
  • Updating values.yaml with default values for runAsUser, runAsNonRoot, and fsGroup to ensure compatibility with OpenShift's security constraints.

Issue:

In the current Helm chart, when deploying in OpenShift as a non-root user, the container is unable to create the required data directory due to permission restrictions. To address this, we need to apply privileged permissions in the container's security context (securityContext.privileged: true) to allow the creation of the data directory and proper access management.

These updates allow the Helm chart to be used in environments where running containers as a root user is restricted, such as OpenShift.

- Renamed existing `securityContext` to `podSecurityContext` for the all depolyment pod spec.
- Applied `securityContext` at the container level.

Refactor security context in presto Helm chart

- Renamed existing `securityContext` to `podSecurityContext` for the all depolyment pod spec.
- Applied `securityContext` at the container level.
@captainpro-eng
Copy link
Author

@dnskr please review it.

@dnskr dnskr self-requested a review August 28, 2024 19:54
Copy link
Collaborator

@dnskr dnskr left a comment

Choose a reason for hiding this comment

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

Thank you for submitting the PR!

Just to acknowledge the fact: this PR introduces breaking changes, because it moves securityContext property value from pod to container level and introduces podSecurityContext to replace it. It is fine for development versions 0.x to change "public api". However, could you please explain why you chose this approach and the naming?

I don't see values.yaml changes mentioned in the description. Did you miss to include them in the commit?

BTW, did you try to mitigate the issue by changing the path?
For instance, you could set node.dataDir to /tmp/node/data or similar.

@@ -2,7 +2,7 @@ apiVersion: v2
name: presto
description: The official Helm chart for Presto
type: application
version: 0.3.0
version: 0.3.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
version: 0.3.1
version: 0.3.0

Please, do not bump the chart version, because it triggers the release workflow. Usually we collect few changes for the next release.

Copy link
Author

Choose a reason for hiding this comment

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

@dnskr Thanks for suggestion the changing the path. it is working.

Changes of values.yaml need to commit.

I chose different names for podSecurityContext and securityContext because some properties, such as privileged, only apply at the container level and not at the pod level, ensuring that settings are accurately and appropriately applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants