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

Add -mount flag to kv list command #19378

Merged
merged 15 commits into from
Mar 20, 2023
Merged

Add -mount flag to kv list command #19378

merged 15 commits into from
Mar 20, 2023

Conversation

dhuckins
Copy link
Contributor

resolves #17399

Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>
Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>
c.UI.Error(err.Error())
return 2
}
// If true, we're working with "-mount=secret foo" syntax.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dhuckins dhuckins changed the title Dh/kv list mount Add -mount flag to kv list command Feb 27, 2023
Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>
Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>
Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>
Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>
Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>
Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>
Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>
Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>
@dhuckins dhuckins marked this pull request as ready for review March 6, 2023 18:20
@dhuckins dhuckins requested a review from yhyakuna as a code owner March 6, 2023 18:20
@dhuckins dhuckins requested a review from a team March 6, 2023 18:20
Copy link
Collaborator

@lursu lursu 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 about test coverage but otherwise LGTM.

command/kv_test.go Show resolved Hide resolved
@dhuckins dhuckins requested review from digivava and a team March 6, 2023 19:52
Copy link
Contributor

@averche averche left a comment

Choose a reason for hiding this comment

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

A couple small nits, otherwise looks good.

Comment on lines +57 to +58
path and secret path, with /data/ automatically appended between KV
v2 secrets.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
path and secret path, with /data/ automatically appended between KV
v2 secrets.`,
path and secret path, with /data/ automatically inserted between the two
paths for KV-v2 secrets.`,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is copied from kv get

vault/command/kv_get.go

Lines 72 to 76 in e4e9612

Usage: `Specifies the path where the KV backend is mounted. If specified,
the next argument will be interpreted as the secret path. If this flag is
not specified, the next argument will be interpreted as the combined mount
path and secret path, with /data/ automatically appended between KV
v2 secrets.`,

so going to leave for now

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps adjusting both descriptions for clarity would help.

Comment on lines +135 to +138
// v1
if mountFlagSyntax {
fullPath = path.Join(mountPath, partialPath)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit hard to follow these if blocks, perhaps they can be rearranged a bit for clarity? In particular we are doing path.Join(...) for kv-v1 here yet for kv-v2 in the previous if block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just copied the logic from kv put

each command in kv ... does this a little differently, going to look to consolidate the logic in a subsequent ticket

Copy link
Collaborator

@digivava digivava left a comment

Choose a reason for hiding this comment

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

Thanks for taking a look at this! The reason that I didn't have a -mount flag for this command is not that I forgot it so much as it behaved a little differently from the others so I just kinda skipped it, but I think it's tripped up a few people so it's definitely worth adding.

Here's the thing about this command: the kv list command is the only one that lets you pass just the mount path itself with no secret path at all. When you do this, it shows all the secrets at that mount path, and indeed I think this is a common use case for the command.

For example, let's say I have some secrets I've created inside the secret mount path.
If I run your code with the traditional way of passing the path:

$ vault kv list secret
Keys
----
foo
bar

vs with the -mount flag:

$ vault kv list -mount=secret
Not enough arguments (expected 1, got 0)

So basically, if you can fix it so that version of the command doesn't throw an error, and instead returns the same thing as the version without the flag, then I think this will be good to go!

@dhuckins
Copy link
Contributor Author

dhuckins commented Mar 7, 2023

@digivava
thank you for the context!

So basically, if you can fix it so that version of the command doesn't throw an error, and instead returns the same thing as the version without the flag, then I think this will be good to go!

Lets see if there's a way to pull that off, thank you!

Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>
Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>
@averche
Copy link
Contributor

averche commented Mar 7, 2023

$ vault kv list secret
Keys
----
foo
bar

vs with the -mount flag:

$ vault kv list -mount=secret
Not enough arguments (expected 1, got 0)

Would it be reasonable to require a "/" or a "." parameter to be provided for this edge case:

vault kv list -mount=secret /

Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>
Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>
Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>
@dhuckins dhuckins requested a review from digivava March 16, 2023 17:00
Comment on lines +637 to +646
{
// this is behavior that should be tested
// `kv` here is an explicit mount
// `my-prefix` is not
// the current kv code will ignore `my-prefix`
name: "ignore_multi_part_mounts",
args: []string{"-mount", "kv/my-prefix"},
outStrings: []string{"my-prefix"},
code: 0,
},
Copy link
Contributor Author

@dhuckins dhuckins Mar 16, 2023

Choose a reason for hiding this comment

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

@digivava when you have a moment, can you give this PR another pass please?

Copy link
Collaborator

@digivava digivava left a comment

Choose a reason for hiding this comment

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

This seems to work as expected now! Thank you!

@dhuckins dhuckins merged commit 1723525 into main Mar 20, 2023
@dhuckins dhuckins deleted the dh/kv-list-mount branch March 20, 2023 20:26
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.

5 participants