Skip to content

Fix duplicate volume claim name#178

Open
spwoodcock wants to merge 3 commits into
guerzon:mainfrom
spwoodcock:patch-1
Open

Fix duplicate volume claim name#178
spwoodcock wants to merge 3 commits into
guerzon:mainfrom
spwoodcock:patch-1

Conversation

@spwoodcock

@spwoodcock spwoodcock commented Oct 12, 2025

Copy link
Copy Markdown

Love this chart! One small problem with it during my migration.

This PR

Fixes #139

Closes #145 which has a breaking change.

But honestly, either approach would work for now, as existingVolumeClaim is currently a broken option anyway.

This PR updates the pvc for attachments, as I guess the idea was to mount the data and attachments separately, not to duplicate the data from a single pvc in two places?

Workarounds without this PR

  • As the same pvc is mounted in two paths, the only fix for this for now is to set attachmentsPath to somewhere else, such as /attachments, but ignore this path and actually use /data/attachments.

@guerzon

guerzon commented Oct 12, 2025

Copy link
Copy Markdown
Owner

Thanks for the PR @spwoodcock, please also bump the chart version.

@spwoodcock

spwoodcock commented Oct 12, 2025

Copy link
Copy Markdown
Author

My bad, it was late and I didn't look at this too thoroughly.

I managed to fix my config with the workaround.
But I checked the vaultwarden code & there is no way to configure a separate attachments directory:
https://github.com/dani-garcia/vaultwarden/blob/a2ad1dc7c3d28834749d4b14206838d795236c27/src/config.rs#L408
It's always located under ${DATA_FOLDER}/attachments.

So the config in this helm chart is invalid:

  • The option for storage.data and storage.attachments won't work, as we can't tell the server where the attachments are.
  • The options for existingVolumeClaim.attachmentsPath and existingVolumeClaim.dataPath default to attachments inside the data folder, which causes a recursion issue (see Fix recursing volume mounts when using storage.existingVolumeClaim #145).

I'll update this PR to include a fix - removing redundant variables from the chart & incrementing a breaking change.
(honestly, I doubt it is really 'breaking' because the current config doesn't work & I would imagine everyone deploying uses either (1) a workaround for existing volume (2) thinks their attachments are stored separately, when in fact they aren't).

Btw, I appreciate your work on this - easy oversight to make - very thankful the chart exists though 😄

@spwoodcock

Copy link
Copy Markdown
Author

I made a small non-breaking change to this effect:

  • Remove existingVolumeClaim.attachmentsDir as this doesn't do anything.
  • Incremented chart version with a patch.
  • Those with the variable already set won't see any different (it will just prevent the volume recursion issue).

I can make another PR removing the attachment PVCs entirely (breaking).

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.

Using storage.existingVolumeClaim in v0.31.1 can't find existing attachments.

2 participants