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

use strict security context #155

Merged
merged 1 commit into from
Jun 1, 2023
Merged

use strict security context #155

merged 1 commit into from
Jun 1, 2023

Conversation

uhthomas
Copy link
Contributor

@uhthomas uhthomas commented Jun 1, 2023

Use a strict security context by default to comply with the restricted pod security policy.

Fixes #154

itzg
itzg reviewed Jun 1, 2023
View reviewed changes
Copy link
Owner

@itzg itzg left a comment

Choose a reason for hiding this comment

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

Thanks! Just need to bump the chart versions and it's good to go.

@uhthomas uhthomas force-pushed the 154 branch 2 times, most recently from 1616da9 to 8e31d84 Compare June 1, 2023 15:38
itzg
itzg reviewed Jun 1, 2023
View reviewed changes
Copy link
Owner

@itzg itzg left a comment

Choose a reason for hiding this comment

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

Do you mind reverting the auto whitespace changes?

@itzg
Copy link
Owner

itzg commented Jun 1, 2023

...and maybe related https://github.com/itzg/minecraft-server-charts/actions/runs/5146374308/jobs/9265335257?pr=155#step:7:61

@uhthomas
Copy link
Contributor Author

uhthomas commented Jun 1, 2023

Do you mind reverting the auto whitespace changes?

Can do, all of them or just some? I noticed this project doesn't seem to have a standard.

@uhthomas uhthomas force-pushed the 154 branch 3 times, most recently from a93b339 to 9bf3c22 Compare June 1, 2023 15:49
@itzg
Copy link
Owner

itzg commented Jun 1, 2023

Do you mind reverting the auto whitespace changes?

Can do, all of them or just some? I noticed this project doesn't seem to have a standard.

Ones you adjusted are good. Thanks.

Yeah, this is an inherited project, so I'm sure it's quite inconsistent.

@uhthomas
Copy link
Contributor Author

uhthomas commented Jun 1, 2023 •
edited
Loading

Looks like CI fails because there is nothing mounted to the required volume /opt/rcon-web-admin/db as stated in https://github.com/itzg/docker-rcon-web-admin.

Is it okay for this to be an ephemeral directory, or does it require persistence?

@uhthomas
use strict security context
3c403f9 
Use a strict security context by default to comply with the restricted pod
security policy.

Fixes itzg#154
@itzg
Copy link
Owner

itzg commented Jun 1, 2023

Looks like CI fails because there is nothing mounted to the required volume /opt/rcon-web-admin/db as stated in https://github.com/itzg/docker-rcon-web-admin.

Is it okay for this to be an ephemeral directory, or does it require persistence?

Ephemeral is totally fine. I guess that chart hasn't been re-built in a long time.

@uhthomas
Copy link
Contributor Author

uhthomas commented Jun 1, 2023

Looks like CI fails because there is nothing mounted to the required volume /opt/rcon-web-admin/db as stated in https://github.com/itzg/docker-rcon-web-admin.
Is it okay for this to be an ephemeral directory, or does it require persistence?

Ephemeral is totally fine. I guess that chart hasn't been re-built in a long time.

Thanks, all looks good now I think!

@itzg
Copy link
Owner

itzg commented Jun 1, 2023

Ready for me to merge?

@uhthomas
Copy link
Contributor Author

uhthomas commented Jun 1, 2023

Ready for me to merge?

Yep, all looks good to me.

@itzg itzg merged commit 0cb2e83 into itzg:master Jun 1, 2023
itzg
itzg reviewed Jul 12, 2023
View reviewed changes
charts/minecraft-bedrock/values.yaml
Comment on lines +27 to +29
runAsGroup: 3000
runAsNonRoot: true
fsGroup: 2000
Copy link
Owner

Choose a reason for hiding this comment

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

@uhthomas I had missed this before: why did you choose group ID 3000 to run and group ID for the filesystem? It seems to be causing issues like #164

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is standard as recommended by the official Kubernetes documentation.

https://kubernetes.io/docs/tasks/configure-pod-container/security-context/

uid=1000 gid=3000 groups=2000

The idea is that there is as little permission as possible.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for following up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itzg itzg itzg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use strict security context
2 participants
@uhthomas @itzg