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

fix regression on Core.pluginDirectory when backend is builtin #15027

Merged
merged 10 commits into from
Apr 14, 2022

Conversation

fairclothjm
Copy link
Contributor

@fairclothjm fairclothjm commented Apr 13, 2022

Fix a regression on Core.pluginDirectory. When enabling a secrets/auth plugin, vault looks in the Core.pluginDirectory if the dev-plugin-dir flag or plugin_directory config is set and fails if it doesn’t find it.

The following error would occur when the dev-plugin-dir flag or plugin_directory config was set on a vault server:

$ vault server -dev -dev-root-token-id=root -dev-plugin-dir=$HOME/dev/plugins
$ vault version
Vault v1.11.0-dev1 ('2d03150157a76099af193264dd97cdc52c433f14')
$ vault auth enable approle
Error enabling approle auth: Error making API request.

URL: POST http://127.0.0.1:8200/v1/sys/auth/approle
Code: 400. Errors:

* error stating "/Users/jmf/dev/plugins/approle": stat /Users/jmf/dev/plugins/approle: no such file or directory

@calvn
Copy link
Contributor

calvn commented Apr 13, 2022

We should backport this all the way to 1.8.x as well.

@swenson
Copy link
Contributor

swenson commented Apr 13, 2022

Is there a good way to add a test for this regression?

@fairclothjm
Copy link
Contributor Author

Is there a good way to add a test for this regression?

@swenson I think it is a good idea. I will look into that.

@fairclothjm fairclothjm changed the title fix dev-plugin-dir when backend is builtin fix regression on Core.pluginDirectory when backend is builtin Apr 14, 2022
@fairclothjm
Copy link
Contributor Author

We should backport this all the way to 1.8.x as well.

@calvn Looks like those backports were either closed or reverted.

@vercel vercel bot temporarily deployed to Preview – vault-storybook April 14, 2022 13:46 Inactive
@vercel vercel bot temporarily deployed to Preview – vault April 14, 2022 13:46 Inactive
@vercel vercel bot temporarily deployed to Preview – vault April 14, 2022 13:55 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook April 14, 2022 13:55 Inactive
@vercel vercel bot temporarily deployed to Preview – vault April 14, 2022 13:57 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook April 14, 2022 13:57 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook April 14, 2022 14:23 Inactive
@vercel vercel bot temporarily deployed to Preview – vault April 14, 2022 14:23 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook April 14, 2022 16:30 Inactive
@vercel vercel bot temporarily deployed to Preview – vault April 14, 2022 16:30 Inactive
Copy link
Contributor

@swenson swenson left a comment

Choose a reason for hiding this comment

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

LGTM; thanks for adding the tests!

@vercel vercel bot temporarily deployed to Preview – vault-storybook April 14, 2022 17:08 Inactive
@vercel vercel bot temporarily deployed to Preview – vault April 14, 2022 17:08 Inactive
Copy link
Contributor

@calvn calvn left a comment

Choose a reason for hiding this comment

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

One question, but otherwise looks good! Thanks for adding the tests!

vault/auth_test.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – vault April 14, 2022 19:09 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook April 14, 2022 19:09 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook April 14, 2022 19:46 Inactive
@vercel vercel bot temporarily deployed to Preview – vault April 14, 2022 19:46 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook April 14, 2022 20:05 Inactive
@vercel vercel bot temporarily deployed to Preview – vault April 14, 2022 20:05 Inactive
@fairclothjm fairclothjm merged commit 59b48f1 into main Apr 14, 2022
@fairclothjm fairclothjm deleted the dev-plugin-dir-fix branch April 14, 2022 20:54
kitography pushed a commit that referenced this pull request Apr 24, 2022
* fix dev-plugin-dir when backend is builtin

* use builtinRegistry.Contains

* revert aa76337

* use correct plugin type for logical backend after revert

* fix factory func default setting after revert

* add ut coverage for builtin plugin with plugin directory set

* add coverage for secrets plugin type

* use totp in tests to avoid test import cycle in ssh package

* use nomad in tests to avoid test import cycle

* remove secrets mount tests due to unavoidable test import cycle
schultz-is pushed a commit that referenced this pull request Apr 27, 2022
* fix dev-plugin-dir when backend is builtin

* use builtinRegistry.Contains

* revert aa76337

* use correct plugin type for logical backend after revert

* fix factory func default setting after revert

* add ut coverage for builtin plugin with plugin directory set

* add coverage for secrets plugin type

* use totp in tests to avoid test import cycle in ssh package

* use nomad in tests to avoid test import cycle

* remove secrets mount tests due to unavoidable test import cycle
schultz-is pushed a commit that referenced this pull request May 2, 2022
* fix dev-plugin-dir when backend is builtin

* use builtinRegistry.Contains

* revert aa76337

* use correct plugin type for logical backend after revert

* fix factory func default setting after revert

* add ut coverage for builtin plugin with plugin directory set

* add coverage for secrets plugin type

* use totp in tests to avoid test import cycle in ssh package

* use nomad in tests to avoid test import cycle

* remove secrets mount tests due to unavoidable test import cycle
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.

3 participants