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.

Sign up for GitHub

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

Jump to bottom

Add deprecation status to list commands #16849

Merged
merged 10 commits into from
Aug 31, 2022
Merged

Add deprecation status to list commands #16849

merged 10 commits into from
Aug 31, 2022

Conversation

mpalmi
Copy link
Contributor

@mpalmi mpalmi commented Aug 23, 2022

This PR adds the ability to query builtin deprecation status by adding a new column to the auth, secrets, and plugins lists. This will be further augmented in a follow-up PR which will add the status information to API endpoints as well.

@mpalmi mpalmi changed the title Vault 7137/add status to list commands Add deprecation status to list commands Aug 23, 2022
@mpalmi
Copy link
Contributor Author

mpalmi commented Aug 24, 2022

@tomhjp brought up a good point out of band. This lookup is happening client-side, so will not reflect the state as it exists in the server. Going to update the deprecation lookups to happen server-side and do a bit more testing around this.

@ccapurso
Copy link
Contributor

@tomhjp brought up a good point out of band. This lookup is happening client-side, so will not reflect the state as it exists in the server. Going to update the deprecation lookups to happen server-side and do a bit more testing around this.

I totally missed that. Thanks for the update!

mpalmi
mpalmi commented Aug 25, 2022
View reviewed changes
vault/logical_system_test.go Outdated Show resolved Hide resolved
mpalmi
mpalmi commented Aug 25, 2022
View reviewed changes
vault/logical_system.go Outdated Show resolved Hide resolved
@mpalmi mpalmi requested a review from a team August 25, 2022 15:21
@mpalmi mpalmi force-pushed the VAULT-7137/add-status-to-list-commands branch from 895540d to aebc75e Compare August 25, 2022 18:39
@mpalmi
Copy link
Contributor Author

mpalmi commented Aug 26, 2022

@tomhjp the plugin list updates are still using the client-side lookup, which needs to be fixed.

I have the changes to the backend handling, but plugin_list.go still needs some work.

Should I go ahead and add both version and deprecation status as a part of this PR, or are you comfortable with the following for now:

  1. Add endpoint handling functionality so the API can reflect deprecation status
  2. Omit plugin_list.go updates until the version changes are merged

This is just my inclination for the sake of getting the PR merged, but more than happy to add version information to plugin list.

tomhjp
tomhjp reviewed Aug 26, 2022
View reviewed changes
Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

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

I'm fine with a phased approach 👍

vault/logical_system.go Outdated Show resolved Hide resolved
vault/logical_system.go Outdated Show resolved Hide resolved
command/plugin_list.go Outdated Show resolved Hide resolved
vault/mount.go Outdated
Comment on lines 678 to 696
if c.builtinRegistry.Contains(entry.Type, consts.PluginTypeSecrets) {
pluginTypes = append(pluginTypes, consts.PluginTypeSecrets)
}

if c.builtinRegistry.Contains(entry.Type, consts.PluginTypeDatabase) {
pluginTypes = append(pluginTypes, consts.PluginTypeDatabase)
}
Copy link
Contributor

@tomhjp tomhjp Aug 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

These seem like unsafe assumptions to me. e.g. If we are processing an externally registered k8s auth plugin, it will get labelled as a secrets engine because there is also a built-in kubernetes secret engine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, and this actually makes for a good test case. I'll see about adding that to the PR.

@mpalmi mpalmi removed the request for review from austingebauer August 29, 2022 13:48
@mpalmi mpalmi force-pushed the VAULT-7137/add-status-to-list-commands branch from 8023fec to fe223f2 Compare August 29, 2022 13:50
@mpalmi mpalmi removed the request for review from akshya96 August 29, 2022 13:51
@mpalmi mpalmi force-pushed the VAULT-7137/add-status-to-list-commands branch from fe223f2 to cf8ab35 Compare August 30, 2022 11:14
tomhjp
tomhjp approved these changes Aug 31, 2022
View reviewed changes
Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

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

LGTM

@mpalmi mpalmi force-pushed the VAULT-7137/add-status-to-list-commands branch from 9bbbdf4 to dd67781 Compare August 31, 2022 18:51
@mpalmi mpalmi merged commit 4099ca7 into main Aug 31, 2022
@mpalmi mpalmi deleted the VAULT-7137/add-status-to-list-commands branch August 31, 2022 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fairclothjm fairclothjm fairclothjm approved these changes

@tomhjp tomhjp tomhjp approved these changes

@VioletHynes VioletHynes VioletHynes approved these changes

@ccapurso ccapurso ccapurso approved these changes

@taoism4504 taoism4504 Awaiting requested review from taoism4504

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mpalmi @ccapurso @VioletHynes @tomhjp @fairclothjm