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

Increase column width of vault_key on mysql #14231

Merged
merged 1 commit into from Feb 24, 2022
Merged

Conversation

drawks
Copy link
Contributor

@drawks drawks commented Feb 23, 2022

As noted in #14114 the vault_key column is too narrow in the default schema. This fix will fix new installations, existing installations will still need to modify their schema in place.

@hsimon-hashicorp
Copy link
Contributor

Thanks, @drawks! Can you also add a changelog entry? :)

@ccapurso
Copy link
Contributor

Hi, @drawks. Thank you for the contribution! The increased width of 3072 is a reasonable change given how MySQL allocates for VARBINARY columns. Is the new width necessary or is it an attempt to provide so much headroom to completely avoid the issue?

@drawks
Copy link
Contributor Author

drawks commented Feb 23, 2022

@ccapurso 3072 is just the max width for that column type. AFAICT the actual data width of the column is not logically bounded and increases relative to the number of "directory parts" of the key. In my use case a width of ~600 is sufficient, but I imagine there are others with arbitrarily deep paths in their kv stores and an upgrade from KVv1 to KVv2 is not recoverable if the column is too narrow so I just opted for the maximum width.

@ccapurso
Copy link
Contributor

Great, thanks for the context!

CHANGELOG.md Outdated Show resolved Hide resolved
@ccapurso
Copy link
Contributor

This looks good overall, just one comment on the changelog entry which needs to be addressed prior to merging.

* resolves The default schema used in the mysql backend is insufficient for KVv2 storage hashicorp#14114
* increases column width of vault_key from 512 to 3072 in mysql physical backend
* updates changelog
Copy link
Contributor

@ccapurso ccapurso left a comment

Choose a reason for hiding this comment

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

Looks good! I will merge once CircleCI checks pass.

@ypid-work
Copy link

ypid-work commented Jul 14, 2023

existing installations will still need to modify their schema in place.

In that case there should be either:

  • A database migration script that does this change for existing setups.
  • Mentioned in the changelog/upgrade guide that the following needs to be executed:
ALTER TABLE `vault` CHANGE `vault_key` `vault_key` VARBINARY(3072) NOT NULL;

I am not that familiar with Vault but that is how other projects do it. If I miss something please excuse me and feel free to educate me.

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

Successfully merging this pull request may close these issues.

The default schema used in the mysql backend is insufficient for KVv2 storage
4 participants