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.

Sign up for GitHub

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

Jump to bottom

Support pod.spec.containers.securityContext specification #117

Merged
merged 1 commit into from
May 22, 2024
Merged

Support pod.spec.containers.securityContext specification #117

merged 1 commit into from
May 22, 2024

Conversation

LittleWat
Copy link
Contributor

This PR attempts to close #116

@cla-bot cla-bot bot added the cla-signed label Dec 20, 2023
@LittleWat
Copy link
Contributor Author

@hashhar sorry to ask you but could you kindly review this, please...? 🙏

@hashhar hashhar requested a review from mosabua January 9, 2024 06:16
@chgl
Copy link

chgl commented Jan 24, 2024

Not a maintainer, but I'm wondering if it makes sense to define a default, restrictive securityContext out-of-the-box. Trino should support running as non-root and without privilege (escalation) requirements. Most, if not all, of the bitnami charts do this, e.g. https://github.com/bitnami/charts/blob/main/bitnami/postgresql/values.yaml#L477.

@LittleWat LittleWat force-pushed the support-containerSecurityContext-specification branch 2 times, most recently from dd5e3ed to bf73293 Compare March 12, 2024 05:29
@LittleWat
Copy link
Contributor Author

@chgl thank you for your comments! That would be better!

Update this PR following the valeriano-manassero's implementation

@mosabua sorry to ask you but I fixed the commit and rebased this branch to the latest main branch. Could you review this when you have time, please...? 🙏

mosabua
mosabua requested changes Mar 13, 2024
View reviewed changes
charts/trino/README.md Outdated
@@ -49,6 +49,8 @@ The following table lists the configurable parameters of the Trino chart and the
| `sidecarContainers` | | `{}` |
| `securityContext.runAsUser` | | `1000` |
| `securityContext.runAsGroup` | | `1000` |
| `containerSecurityContext.allowPrivilegeEscalation` | Controls whether a process can gain more privileges than its parent process. | `false` |
Copy link
Member

Choose a reason for hiding this comment

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

I think this is generated so you can NOT insert it here .. instead it needs to be a comment in the yaml file itself.. check with other setups and maybe run frigate to confirm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I generated this README using the command frigate gen . > README.md so it would be okay

Copy link
Member

Choose a reason for hiding this comment

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

Thanks

charts/trino/README.md Outdated Show resolved Hide resolved
@LittleWat LittleWat force-pushed the support-containerSecurityContext-specification branch from bf73293 to bffbb5c Compare March 13, 2024 05:52
@LittleWat
Copy link
Contributor Author

@mosabua thank you for your review! I have amended the commit to update the description! could you check this again, please...?

mosabua
mosabua requested changes Mar 13, 2024
View reviewed changes
charts/trino/values.yaml Outdated
containerSecurityContext:
allowPrivilegeEscalation: false # Controls whether a process can gain more privileges than its parent process.
capabilities:
drop: # List of Linux kernel capabilities that are dropped from every container. You can confirm the options for "capabilities" here:　https://man7.org/linux/man-pages/man7/capabilities.7.html Please make sure to remove "CAP_" prefix which the kernel attaches to the names of permissions.
Copy link
Member

@mosabua mosabua Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Use

List of Linux kernel capabilities that are dropped from every container. Valid values are listed at https://man7.org/linux/man-pages/man7/capabilities.7.html Ensure to remove the "CAP_" prefix which the kernel attaches to the names of permissions.

Copy link
Member

Choose a reason for hiding this comment

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

Can you adjust to this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, sorry. Applied your suggestion! Thank you!

charts/trino/values.yaml Outdated
@@ -218,6 +218,13 @@ securityContext:
runAsUser: 1000
runAsGroup: 1000

# -- SecurityContext configuration for containers
containerSecurityContext:
allowPrivilegeEscalation: false # Controls whether a process can gain more privileges than its parent process.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
allowPrivilegeEscalation: false # Controls whether a process can gain more privileges than its parent process.
allowPrivilegeEscalation: false # Control whether a process can gain more privileges than its parent process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied!

charts/trino/values.yaml Show resolved Hide resolved
charts/trino/README.md Outdated
@@ -49,6 +49,8 @@ The following table lists the configurable parameters of the Trino chart and the
| `sidecarContainers` | | `{}` |
| `securityContext.runAsUser` | | `1000` |
| `securityContext.runAsGroup` | | `1000` |
| `containerSecurityContext.allowPrivilegeEscalation` | Controls whether a process can gain more privileges than its parent process. | `false` |
Copy link
Member

Choose a reason for hiding this comment

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

Thanks

@LittleWat LittleWat force-pushed the support-containerSecurityContext-specification branch 2 times, most recently from e4b9174 to 337b01c Compare March 15, 2024 01:38
@bond-
Copy link
Member

bond- commented Mar 21, 2024 •
edited
Loading

How about keeping all of the security context open to values.yaml

          securityContext:
            {{- toYaml .Values.securityContext | nindent 12 }}

EDIT: Ignore you did exactly that

@LittleWat
Copy link
Contributor Author

@bond- Thank you for your comment! yes, that's exactly what I did in the first place. But I set the default following the comment in this review by amending the commit 🙇

@nineinchnick
Copy link
Member

@LittleWat please rebase, and I'll review this once the new tests in the CI pass.

@nineinchnick nineinchnick mentioned this pull request May 22, 2024
Closed
@LittleWat LittleWat force-pushed the support-containerSecurityContext-specification branch from 337b01c to c51a86d Compare May 22, 2024 09:26
@LittleWat
Copy link
Contributor Author

@nineinchnick thanks! rebased!

nineinchnick
nineinchnick approved these changes May 22, 2024
View reviewed changes
Copy link
Member

@nineinchnick nineinchnick left a comment

Choose a reason for hiding this comment

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

Just two nitpicks and it'll be good to go!

charts/trino/values.yaml Outdated Show resolved Hide resolved
charts/trino/values.yaml Outdated Show resolved Hide resolved
@LittleWat LittleWat force-pushed the support-containerSecurityContext-specification branch from 5ca416c to 0fb7daa Compare May 22, 2024 12:23
@LittleWat
Support pod.spec.containers.securityContext specification
60045ad 
This PR attempts to close trinodb#116

Following [the valeriano-manassero's implementation](https://github.com/valeriano-manassero/helm-charts/blob/6382a14272927a908bc006d0f1370ba9dffc821f/valeriano-manassero/trino/values.yaml#L467-L471)

let me support `pod.spec.containers.securityContext`  specification
@LittleWat LittleWat force-pushed the support-containerSecurityContext-specification branch from 0fb7daa to 60045ad Compare May 22, 2024 12:27
@LittleWat
Copy link
Contributor Author

@nineinchnick thank you for your quick review! yes, that would be better! Fixed!

@nineinchnick nineinchnick merged commit 533ace5 into trinodb:main May 22, 2024
8 checks passed
@LittleWat LittleWat deleted the support-containerSecurityContext-specification branch May 22, 2024 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nineinchnick nineinchnick nineinchnick approved these changes

@mosabua mosabua mosabua requested changes

Assignees
No one assigned
Labels
cla-signed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support pod.spec.containers.securityContext specification
5 participants
@LittleWat @chgl @bond- @nineinchnick @mosabua