Remove pinned builtin plugin versions from storage #18051
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
One other thought for database plugins, I could iterate through all of the connection configs during the call to vault/builtin/logical/database/rotation.go Lines 522 to 524 in 17bad82 It feels like that may end up cleaner, but I also worry whether that could cause an unacceptable increase to startup time for users with a very large number of databases. |
tomhjp
added
core/plugin
ecosystem/plugin
backport/1.12.x
and removed
pr/no-changelog
labels
benashz
reviewed
Nov 22, 2022
benashz
reviewed
Nov 22, 2022
benashz
reviewed
Nov 23, 2022
| "github.com/hashicorp/vault/sdk/helper/consts" | ||
| "github.com/hashicorp/vault/sdk/version" | ||
| ) | ||
|
|
||
| const ( | ||
| BuiltinMetadata = "builtin" |
ncabatoff
approved these changes
Nov 23, 2022
benashz
reviewed
Nov 23, 2022
|
Thanks! |
tomhjp
added a commit
that referenced
this pull request
Nov 23, 2022
* Removes _builtin_ versions from mount storage where it already exists * Stops new builtin versions being put into storage on mount creation/tuning * Stops the plugin catalog from returning a builtin plugin that has been overridden, so it more accurately reflects the plugins that are available to actually run Conflicts: vault/mount.go
tomhjp
added a commit
that referenced
this pull request
Nov 23, 2022
* Removes _builtin_ versions from mount storage where it already exists * Stops new builtin versions being put into storage on mount creation/tuning * Stops the plugin catalog from returning a builtin plugin that has been overridden, so it more accurately reflects the plugins that are available to actually run Conflicts: vault/mount.go Co-authored-by: Tom Proctor <tomhjp@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses bug described in the 1.12 upgrade guide: https://developer.hashicorp.com/vault/docs/upgrading/upgrade-to-1.12.x#pinning-to-builtin-plugin-versions-may-cause-failure-on-upgrade.
In a nutshell, when we try to start a plugin after upgrading, if it has a builtin version stored in the mount table and that version subsequently disappeared from catalog due to the upgrade, we won't be able to mount it. To fix this, the main idea is to stop storing version information for builtin plugins, and rely on the pre-existing behaviour that not specifying a version selects the builtin (unless it's been shadowed by an unversioned external plugin).
2 main changes, that need to be done for each of the 3 plugin types:
Stopping storage for new mounts is easy enough. Cleaning up existing auth and secrets mounts also seems relatively straightforward thanks to some existing logic for upgrading their storage.
Unfortunately, database plugins don't seem to have any such existing capability, instead this relies on fixing storage whenever it's next written to for each db plugin. That means databases also need an extra shim to stop mounting from failing, because we may mount a plugin before the config has been fixed in storage. We also delete builtin plugin versions from a config read response, so the out of date storage is invisible to users unless they poke around in raw endpoints.
The approach also means users who have a builtin version pinned, and an unversioned plugin of the same name registered, will start running the unversioned plugin on upgrading. That deserves a callout in the changelog, but I don't expect it to be problematic for many people, as most people using versions for builtins will probably also use versions for externals. Plus it's still a very new feature, and the delta between the builtin and external versions is likely small.
I've also updated the plugin catalog to not return a builtin if it has been shadowed by an unversioned external plugin of the same name.