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

Return support for readOnlyRootFilesystem #120

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mbrancato
Copy link
Contributor

This adds the configurable readOnlyRootFilesystem back that was removed in v0.2.1. It has been moved from .spec.template.spec.securityContext (v1/PodSecurityContext) to .spec.template.spec.container[0].securityContext (v1/SecurityContext)

Copy link
Contributor

@jasonodonnell jasonodonnell left a comment

Choose a reason for hiding this comment

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

Hello @mbrancato,

Thanks for adding this to the container, however, this is causing a crash loop:

/bin/sh: can't create /tmp/storageconfig.hcl: Read-only file system

The helm chart does a bit of templating for the user to make it a little easier to configure (such as embedding runtime values like hostname or IP into some Vault config settings). With readonlyrootfilesystem, this causes issues. More work will need to be done here to support this feature properly.

@jasonodonnell
Copy link
Contributor

Thinking on this more, I think a memory volume could be added to /home/vault to give us a place to render the template and other caching things (such as login tokens temporarily).

@mbrancato
Copy link
Contributor Author

@jasonodonnell yes. I can look at doing spec.volumes.emptyDir.medium: Memory.

@mbrancato
Copy link
Contributor Author

I added a tmpfs mount for /tmp, if you know of other volumes needed I could add those. I do see folders under /vault/, but they are configMaps or using PVC. After making this change, I was able to launch and unseal a cluster using the chart.

@mbrancato
Copy link
Contributor Author

I noticed that /home/vault is used in some cases (I think dev mode). I've added this and fixed some tests needed.

@jasonodonnell
Copy link
Contributor

Hi @mbrancato, thanks for doing this!

I think it's probably best to have the /home/vault directory have the mounted volume and change the arg to render the configuration file there: https://github.com/hashicorp/vault-helm/blob/master/templates/_helpers.tpl#L117-L124.

This would remove the memory volume at /tmp. /home/vault is definitely required for things like token caching and isn't configurable.

@mbrancato
Copy link
Contributor Author

@jasonodonnell I've updated this to remove the use of /tmp in preference for /home/vault as ephemeral storage and tested locally.

@jasonodonnell
Copy link
Contributor

@mbrancato This has conflicts, can you resolve them?

@mbrancato
Copy link
Contributor Author

@jasonodonnell - I've rebased this and fixed the conflict

@jasonodonnell jasonodonnell added this to the v0.4.0 milestone Jan 23, 2020
@jasonodonnell
Copy link
Contributor

Looks good but needs conflicts to be resolved.

@tvoran tvoran added chart Area: helm chart enhancement New feature or request labels Mar 5, 2020
@tvoran tvoran modified the milestones: v0.4.0, v0.6.0 May 22, 2020
@jasonodonnell jasonodonnell modified the milestones: v0.6.0, v0.7.0 Jun 3, 2020
@samueltorres
Copy link

Hi,
Our internal container scanning tools is reporting this issue, and we really wanted this fixed. Is there any ETA for this PR to be merged ? :)

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@abhilashbs1981
Copy link

abhilashbs1981 commented Oct 5, 2023

Getting below error after making [readOnlyRootFilesystem] as true as for vault as below in values.yaml
container:
allowPrivilegeEscalation: false
readOnlyRootFilesystem: true

controlplane $ k logs -f vault-0
cp: can't create '/tmp/storageconfig.hcl': Read-only file system

@mchoy
Copy link

mchoy commented Nov 30, 2023

I am getting the same error after I set readOnlyRootFilesystem: true.
Is there a plan to merge this PR eventually?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chart Area: helm chart enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants