Skip to content

refactor(pass): accept a store factory in Root for lazy keychain init #554

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

Closed
joe0BAB wants to merge 1 commit into from
Closed

refactor(pass): accept a store factory in Root for lazy keychain init #554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions plugins/pass/command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ import (
"github.com/docker/secrets-engine/store"
)

// StoreFactory opens the backing keychain store on demand. Subcommands invoke
// it from their RunE so that commands which never touch the store (version,
// help, completion) do not pay the keychain-init cost — and do not hang in
// headless environments where no D-Bus session bus is reachable.
type StoreFactory func() (store.Store, error)

// Note: We use a custom help template to make it more brief.
const helpTemplate = `Docker Pass CLI - Manage your local secrets.
{{if .UseLine}}
Expand All @@ -56,8 +62,13 @@ var rootExample string
//go:embed long.md
var rootLong string

// Root returns the root command for the docker-pass CLI plugin
func Root(ctx context.Context, s store.Store, info commands.VersionInfo) *cobra.Command {
// Root returns the root command for the docker-pass CLI plugin.
//
// newStore is invoked lazily by subcommands that need keychain access. Commands
// that do not touch the store (version, help, completion) never call it, so the
// binary stays usable in headless environments where opening the keychain would
// hang on a missing D-Bus session bus.
func Root(ctx context.Context, newStore StoreFactory, info commands.VersionInfo) *cobra.Command {
cmd := &cobra.Command{
Use: "pass set|get|ls|rm|run",
Short: "Manage your local OS keychain secrets.",
Expand All @@ -80,10 +91,10 @@ func Root(ctx context.Context, s store.Store, info commands.VersionInfo) *cobra.
return []string{"--help"}, cobra.ShellCompDirectiveNoFileComp
})

cmd.AddCommand(wrapRunEWithSpan(commands.SetCommand(s)))
cmd.AddCommand(wrapRunEWithSpan(commands.ListCommand(s)))
cmd.AddCommand(wrapRunEWithSpan(commands.RmCommand(s)))
cmd.AddCommand(wrapRunEWithSpan(commands.GetCommand(s)))
cmd.AddCommand(wrapRunEWithSpan(commands.SetCommand(newStore)))
cmd.AddCommand(wrapRunEWithSpan(commands.ListCommand(newStore)))
cmd.AddCommand(wrapRunEWithSpan(commands.RmCommand(newStore)))
cmd.AddCommand(wrapRunEWithSpan(commands.GetCommand(newStore)))
cmd.AddCommand(wrapRunEWithSpan(commands.RunCommand()))
cmd.AddCommand(commands.VersionCommand(info))

Expand Down
48 changes: 26 additions & 22 deletions plugins/pass/command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,18 +38,22 @@ var mockInfo = commands.VersionInfo{
Commit: "abc",
}

func staticFactory(s store.Store) StoreFactory {
Comment thread
joe0BAB marked this conversation as resolved.
return func() (store.Store, error) { return s, nil }
}

func Test_rootCommand(t *testing.T) {
t.Parallel()
t.Run("version", func(t *testing.T) {
mock := teststore.NewMockStore()
out, err := executeCommand(Root(t.Context(), mock, mockInfo), "version")
out, err := executeCommand(Root(t.Context(), staticFactory(mock), mockInfo), "version")
assert.NoError(t, err)
assert.Equal(t, "Version: v88\nCommit: abc\n", out)
})
t.Run("set", func(t *testing.T) {
t.Run("ok", func(t *testing.T) {
mock := teststore.NewMockStore()
out, err := executeCommand(Root(t.Context(), mock, mockInfo), "set", "foo=bar=bar=bar")
out, err := executeCommand(Root(t.Context(), staticFactory(mock), mockInfo), "set", "foo=bar=bar=bar")
assert.NoError(t, err)
assert.Empty(t, out)
s, err := mock.Get(t.Context(), secrets.MustParseID("foo"))
Expand All @@ -62,7 +66,7 @@ func Test_rootCommand(t *testing.T) {
})
t.Run("from STDIN", func(t *testing.T) {
mock := teststore.NewMockStore()
out, err := executeCommandWithStdin(Root(t.Context(), mock, mockInfo), "my\nmultiline\nvalue", "set", "foo")
out, err := executeCommandWithStdin(Root(t.Context(), staticFactory(mock), mockInfo), "my\nmultiline\nvalue", "set", "foo")
assert.NoError(t, err)
assert.Empty(t, out)
s, err := mock.Get(t.Context(), secrets.MustParseID("foo"))
Expand All @@ -75,7 +79,7 @@ func Test_rootCommand(t *testing.T) {
})
t.Run("with --metadata flag", func(t *testing.T) {
mock := teststore.NewMockStore()
out, err := executeCommand(Root(t.Context(), mock, mockInfo), "set", "foo=bar", "--metadata", "name=bob", "--metadata", "expiry=2027-03-01")
out, err := executeCommand(Root(t.Context(), staticFactory(mock), mockInfo), "set", "foo=bar", "--metadata", "name=bob", "--metadata", "expiry=2027-03-01")
assert.NoError(t, err)
assert.Empty(t, out)
s, err := mock.Get(t.Context(), secrets.MustParseID("foo"))
Expand All @@ -89,7 +93,7 @@ func Test_rootCommand(t *testing.T) {
})
t.Run("from STDIN JSON with value and metadata", func(t *testing.T) {
mock := teststore.NewMockStore()
out, err := executeCommandWithStdin(Root(t.Context(), mock, mockInfo), `{"secret":"bar","metadata":{"name":"bob"}}`, "set", "foo")
out, err := executeCommandWithStdin(Root(t.Context(), staticFactory(mock), mockInfo), `{"secret":"bar","metadata":{"name":"bob"}}`, "set", "foo")
assert.NoError(t, err)
assert.Empty(t, out)
s, err := mock.Get(t.Context(), secrets.MustParseID("foo"))
Expand All @@ -103,7 +107,7 @@ func Test_rootCommand(t *testing.T) {
})
t.Run("from STDIN JSON merged with --metadata flag wins on collision", func(t *testing.T) {
mock := teststore.NewMockStore()
out, err := executeCommandWithStdin(Root(t.Context(), mock, mockInfo), `{"secret":"bar","metadata":{"name":"bob","extra":"thing"}}`, "set", "foo", "--metadata", "name=alice")
out, err := executeCommandWithStdin(Root(t.Context(), staticFactory(mock), mockInfo), `{"secret":"bar","metadata":{"name":"bob","extra":"thing"}}`, "set", "foo", "--metadata", "name=alice")
assert.NoError(t, err)
assert.Empty(t, out)
s, err := mock.Get(t.Context(), secrets.MustParseID("foo"))
Expand All @@ -117,20 +121,20 @@ func Test_rootCommand(t *testing.T) {
})
t.Run("invalid --metadata flag (no =)", func(t *testing.T) {
mock := teststore.NewMockStore()
_, err := executeCommand(Root(t.Context(), mock, mockInfo), "set", "foo=bar", "--metadata", "invalid")
_, err := executeCommand(Root(t.Context(), staticFactory(mock), mockInfo), "set", "foo=bar", "--metadata", "invalid")
assert.ErrorContains(t, err, "invalid metadata pair (expected key=value): invalid")
})
t.Run("store error", func(t *testing.T) {
errSave := errors.New("save error")
mock := teststore.NewMockStore(teststore.WithStoreSaveErr(errSave))
out, err := executeCommand(Root(t.Context(), mock, mockInfo), "set", "foo=bar")
out, err := executeCommand(Root(t.Context(), staticFactory(mock), mockInfo), "set", "foo=bar")
assert.ErrorIs(t, errSave, err)
assert.Equal(t, "Error: "+errSave.Error()+"\n", out)
})
t.Run("invalid id", func(t *testing.T) {
errSave := errors.New("save error")
mock := teststore.NewMockStore(teststore.WithStoreSaveErr(errSave))
out, err := executeCommand(Root(t.Context(), mock, mockInfo), "set", "/foo=bar")
out, err := executeCommand(Root(t.Context(), staticFactory(mock), mockInfo), "set", "/foo=bar")
errInvalidID := secrets.ErrInvalidID{ID: "/foo"}
assert.ErrorIs(t, err, errInvalidID)
assert.Equal(t, "Error: "+errInvalidID.Error()+"\n", out)
Expand All @@ -144,7 +148,7 @@ func Test_rootCommand(t *testing.T) {
}),
teststore.WithStoreSaveErr(errors.New("save should not be called when --force is set")),
)
out, err := executeCommand(Root(t.Context(), mock, mockInfo), "set", "foo=new", "--force")
out, err := executeCommand(Root(t.Context(), staticFactory(mock), mockInfo), "set", "foo=new", "--force")
assert.NoError(t, err)
assert.Empty(t, out)
s, err := mock.Get(t.Context(), secrets.MustParseID("foo"))
Expand All @@ -158,7 +162,7 @@ func Test_rootCommand(t *testing.T) {
t.Run("--force surfaces upsert error", func(t *testing.T) {
errUpsert := errors.New("upsert error")
mock := teststore.NewMockStore(teststore.WithStoreUpsertErr(errUpsert))
out, err := executeCommand(Root(t.Context(), mock, mockInfo), "set", "foo=bar", "--force")
out, err := executeCommand(Root(t.Context(), staticFactory(mock), mockInfo), "set", "foo=bar", "--force")
assert.ErrorIs(t, errUpsert, err)
assert.Equal(t, "Error: "+errUpsert.Error()+"\n", out)
})
Expand All @@ -169,14 +173,14 @@ func Test_rootCommand(t *testing.T) {
store.MustParseID("foo"): pass.NewPassValue([]byte("bar")),
store.MustParseID("baz"): pass.NewPassValue([]byte("0")),
}))
out, err := executeCommand(Root(t.Context(), mock, mockInfo), "list")
out, err := executeCommand(Root(t.Context(), staticFactory(mock), mockInfo), "list")
assert.NoError(t, err)
assert.Equal(t, "baz\nfoo\n", out)
})
t.Run("store error", func(t *testing.T) {
errGetAll := errors.New("get error")
mock := teststore.NewMockStore(teststore.WithStoreGetAllErr(errGetAll))
out, err := executeCommand(Root(t.Context(), mock, mockInfo), "list")
out, err := executeCommand(Root(t.Context(), staticFactory(mock), mockInfo), "list")
assert.ErrorIs(t, errGetAll, err)
assert.Equal(t, "Error: "+errGetAll.Error()+"\n", out)
})
Expand All @@ -187,7 +191,7 @@ func Test_rootCommand(t *testing.T) {
store.MustParseID("foo"): pass.NewPassValue([]byte("bar")),
store.MustParseID("baz"): pass.NewPassValue([]byte("0")),
}))
out, err := executeCommand(Root(t.Context(), mock, mockInfo), "rm", "foo", "baz")
out, err := executeCommand(Root(t.Context(), staticFactory(mock), mockInfo), "rm", "foo", "baz")
assert.NoError(t, err)
assert.Equal(t, "RM: baz\nRM: foo\n", out)
l, err := mock.GetAllMetadata(t.Context())
Expand All @@ -199,7 +203,7 @@ func Test_rootCommand(t *testing.T) {
store.MustParseID("foo"): pass.NewPassValue([]byte("bar")),
store.MustParseID("baz"): pass.NewPassValue([]byte("0")),
}))
out, err := executeCommand(Root(t.Context(), mock, mockInfo), "rm", "--all")
out, err := executeCommand(Root(t.Context(), staticFactory(mock), mockInfo), "rm", "--all")
assert.NoError(t, err)
assert.Equal(t, "RM: baz\nRM: foo\n", out)
l, err := mock.GetAllMetadata(t.Context())
Expand All @@ -209,26 +213,26 @@ func Test_rootCommand(t *testing.T) {
t.Run("store error", func(t *testing.T) {
errRemove := errors.New("remove error")
mock := teststore.NewMockStore(teststore.WithStoreDeleteErr(errRemove))
out, err := executeCommand(Root(t.Context(), mock, mockInfo), "rm", "foo")
out, err := executeCommand(Root(t.Context(), staticFactory(mock), mockInfo), "rm", "foo")
assert.ErrorIs(t, err, errRemove)
assert.Equal(t, "ERR: foo: remove error\nError: "+errRemove.Error()+"\n", out)
})
t.Run("invalid id", func(t *testing.T) {
mock := teststore.NewMockStore()
out, err := executeCommand(Root(t.Context(), mock, mockInfo), "rm", "/foo")
out, err := executeCommand(Root(t.Context(), staticFactory(mock), mockInfo), "rm", "/foo")
errInvalidID := secrets.ErrInvalidID{ID: "/foo"}
assert.ErrorIs(t, err, errInvalidID)
assert.Equal(t, "Error: "+errInvalidID.Error()+"\n", out)
})
t.Run("cannot mix --all with explicit list", func(t *testing.T) {
mock := teststore.NewMockStore()
out, err := executeCommand(Root(t.Context(), mock, mockInfo), "rm", "--all", "foo")
out, err := executeCommand(Root(t.Context(), staticFactory(mock), mockInfo), "rm", "--all", "foo")
assert.ErrorContains(t, err, "either provide a secret name or use --all to remove all secrets")
assert.Equal(t, "Error: either provide a secret name or use --all to remove all secrets\n", out)
})
t.Run("no args or --all", func(t *testing.T) {
mock := teststore.NewMockStore()
out, err := executeCommand(Root(t.Context(), mock, mockInfo), "rm")
out, err := executeCommand(Root(t.Context(), staticFactory(mock), mockInfo), "rm")
assert.ErrorContains(t, err, "either provide a secret name or use --all to remove all secrets")
assert.Equal(t, "Error: either provide a secret name or use --all to remove all secrets\n", out)
})
Expand All @@ -238,14 +242,14 @@ func Test_rootCommand(t *testing.T) {
mock := teststore.NewMockStore(teststore.WithStore(map[store.ID]store.Secret{
store.MustParseID("foo"): pass.NewPassValue([]byte("bar")),
}))
out, err := executeCommand(Root(t.Context(), mock, mockInfo), "get", "foo")
out, err := executeCommand(Root(t.Context(), staticFactory(mock), mockInfo), "get", "foo")
assert.NoError(t, err)
assert.Equal(t, "ID: foo\nValue: **********\n", out)
})
t.Run("store error", func(t *testing.T) {
errGet := errors.New("get error")
mock := teststore.NewMockStore(teststore.WithStoreGetErr(errGet))
out, err := executeCommand(Root(t.Context(), mock, mockInfo), "get", "foo")
out, err := executeCommand(Root(t.Context(), staticFactory(mock), mockInfo), "get", "foo")
assert.ErrorIs(t, err, errGet)
assert.Equal(t, "Error: "+errGet.Error()+"\n", out)
})
Expand Down Expand Up @@ -280,7 +284,7 @@ func Test_rootCommandTelemetry(t *testing.T) {
mock := teststore.NewMockStore(teststore.WithStore(map[store.ID]store.Secret{
store.MustParseID("baz"): pass.NewPassValue([]byte("bar")),
}))
_, err := executeCommand(Root(t.Context(), mock, mockInfo), tc.args...)
_, err := executeCommand(Root(t.Context(), staticFactory(mock), mockInfo), tc.args...)
assert.NoError(t, err)

var rm metricdata.ResourceMetrics
Expand Down
6 changes: 5 additions & 1 deletion plugins/pass/commands/get.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
"github.com/docker/secrets-engine/store"
)

func GetCommand(kc store.Store) *cobra.Command {
func GetCommand(newStore func() (store.Store, error)) *cobra.Command {
cmd := &cobra.Command{
Use: "get NAME",
Args: cobra.ExactArgs(1),
Expand All @@ -34,6 +34,10 @@ func GetCommand(kc store.Store) *cobra.Command {
if err != nil {
return err
}
kc, err := newStore()
if err != nil {
return err
}
s, err := kc.Get(cmd.Context(), id)
if err != nil {
return err
Expand Down
6 changes: 5 additions & 1 deletion plugins/pass/commands/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,18 @@ import (
"github.com/docker/secrets-engine/store"
)

func ListCommand(kc store.Store) *cobra.Command {
func ListCommand(newStore func() (store.Store, error)) *cobra.Command {
cmd := &cobra.Command{
Use: "ls",
Aliases: []string{"list"},
Short: "List all secrets from local keychain.",
Long: "Lists the names of all secrets stored in the local OS keychain.",
Args: cobra.NoArgs,
RunE: func(cmd *cobra.Command, _ []string) error {
kc, err := newStore()
if err != nil {
return err
}
l, err := kc.GetAllMetadata(cmd.Context())
if err != nil {
return err
Expand Down
6 changes: 5 additions & 1 deletion plugins/pass/commands/rm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ type rmOpts struct {
All bool
}

func RmCommand(kc store.Store) *cobra.Command {
func RmCommand(newStore func() (store.Store, error)) *cobra.Command {
opts := rmOpts{}
cmd := &cobra.Command{
Use: "rm name1 name2 ...",
Expand All @@ -48,6 +48,10 @@ func RmCommand(kc store.Store) *cobra.Command {
if err != nil {
return err
}
kc, err := newStore()
if err != nil {
return err
}
return runRm(cmd.Context(), cmd.OutOrStdout(), kc, idList, opts)
},
}
Expand Down
7 changes: 6 additions & 1 deletion plugins/pass/commands/set.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ type stdinPayload struct {
Metadata map[string]string `json:"metadata,omitempty"`
}

func SetCommand(kc store.Store) *cobra.Command {
func SetCommand(newStore func() (store.Store, error)) *cobra.Command {
opts := setOpts{}
cmd := &cobra.Command{
Use: "set id[=value]",
Expand Down Expand Up @@ -98,6 +98,11 @@ func SetCommand(kc store.Store) *cobra.Command {
return err
}
}

kc, err := newStore()
if err != nil {
return err
}
if opts.force {
return kc.Upsert(cmd.Context(), id, pv)
}
Expand Down
Loading