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

cli: Improve error handling for plugin commands #24250

Merged
merged 3 commits into from
Nov 28, 2023
Merged

Conversation

tomhjp
Copy link
Contributor

@tomhjp tomhjp commented Nov 24, 2023

vault plugin info and vault plugin deregister have accepted either 1 or 2 positional args since we introduced types for plugins way back in v1.0.0 (#5536). This PR drops CLI support for type-less plugins on those commands, but the API can still be used to query any extremely old legacy plugins (although based on the plugin catalog's UpgradePlugins function, run during unseal, I don't think it should be possible for v1.0.0+ versions of Vault to have functioning type-less plugins). In return, we can get a much nicer UX on the CLI. It also improves the error reporting for vault plugin reload, which was simply buggy.

Examples of previous bad behaviour:

# The type is missing, but it's hard to give a good error message when it's valid to not include it.
$ vault plugin info github
No value found for plugin "github"

# Plugin not actually deleted because a type-less artifactory plugin does not exist
$ vault plugin deregister artifactory
Success! Deregistered plugin (if it was registered): artifactory
$ vault plugin list | grep artifactory
artifactory

# reload takes flags instead of positional arguments
$ vault plugin reload artifactory
Not enough arguments (expected 1, got 1)

To exercise all the possible vault plugin deregister outcomes and get a feel for the UX:

# Setup Vault dev server with plugin_directory config
tee local/config.hcl <<EOF
plugin_directory = "$(pwd)/local/config.hcl
EOF
VAULT_DEV_ROOT_TOKEN_ID=root vault server -dev -config=local/config.hcl
export VAULT_ADDR=http://127.0.0.1:8200
export VAULT_TOKEN=root
vault policy write deregister-only -<<EOF
$(vault plugin deregister -output-policy auth github)
EOF

# Build a plugin
go build -o local/plugins/github builtin/credential/github/cmd/github/main.go
SHA256="(shasum -a 256 local/plugins/github | cut -f1 -d' ')"
vault plugin register -sha256=$SHA256 auth github
$ vault plugin deregister auth github
Success! Deregistered auth plugin: github

$ vault plugin deregister auth github
Plugin "github" (type: "auth") is a builtin plugin
# Exit code 2

$ vault plugin deregister auth github2
Plugin "github2" (type: "auth", version "") does not exist in the catalog
# Exit code 0

$ VAULT_TOKEN="$(vault token create -policy=deregister-only -field=token)" vault plugin deregister auth github
Success! Deregistered auth plugin (if it was registered): github

@tomhjp tomhjp added this to the 1.16.0-rc1 milestone Nov 24, 2023
@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Nov 24, 2023
Copy link

Build Results:
All builds succeeded! ✅

Copy link

github-actions bot commented Nov 24, 2023

CI Results:
All Go tests succeeded! ✅

@benashz benashz self-requested a review November 24, 2023 16:58
Copy link
Contributor

@benashz benashz left a comment

Choose a reason for hiding this comment

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

Aside from some minor nits, it seems good to me. I guess its better to get this breaking change out sooner rather than later, and it is definitely a good step forward.

command/plugin_info.go Outdated Show resolved Hide resolved
@@ -206,7 +206,7 @@ func TestPluginDeregisterCommand_Run(t *testing.T) {
t.Errorf("expected %d to be %d", code, exp)
}

expected := "Success! Deregistered plugin (if it was registered): "
expected := "does not exist in the catalog"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I wonder if we should change the test below to be a bit more specific by using strings.HasSuffix rather than strings.Contains.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm already pretty uncomfortable with how brittle these tests are tbh (string-matching on error messages that can be arbitrarily updated), and it doesn't feel like that meaningfully improves the intent of the test, but it does make it more brittle. Unless you disagree strongly, I'd prefer to leave it as-is.

command/plugin_deregister.go Show resolved Hide resolved
@tomhjp
Copy link
Contributor Author

tomhjp commented Nov 28, 2023

Thanks!

@tomhjp tomhjp merged commit 51d99fc into main Nov 28, 2023
109 checks passed
@tomhjp tomhjp deleted the plugin-cli-updates branch November 28, 2023 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core/cli core/plugin hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants