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

Move secret name or ID prefix resolving from client to daemon #29218

Merged
merged 1 commit into from Jan 27, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 1 addition & 5 deletions cli/command/secret/inspect.go
Expand Up @@ -33,13 +33,9 @@ func runSecretInspect(dockerCli *command.DockerCli, opts inspectOptions) error {
client := dockerCli.Client()
ctx := context.Background()

ids, err := getCliRequestedSecretIDs(ctx, client, opts.names)
if err != nil {
return err
}
getRef := func(id string) (interface{}, []byte, error) {
return client.SecretInspectWithRaw(ctx, id)
}

return inspect.Inspect(dockerCli.Out(), ids, opts.format, getRef)
return inspect.Inspect(dockerCli.Out(), opts.names, opts.format, getRef)
}
11 changes: 3 additions & 8 deletions cli/command/secret/remove.go
Expand Up @@ -33,20 +33,15 @@ func runSecretRemove(dockerCli *command.DockerCli, opts removeOptions) error {
client := dockerCli.Client()
ctx := context.Background()

ids, err := getCliRequestedSecretIDs(ctx, client, opts.names)
if err != nil {
return err
}

var errs []string

for _, id := range ids {
if err := client.SecretRemove(ctx, id); err != nil {
for _, name := range opts.names {
if err := client.SecretRemove(ctx, name); err != nil {
errs = append(errs, err.Error())
continue
}

fmt.Fprintln(dockerCli.Out(), id)
fmt.Fprintln(dockerCli.Out(), name)
}

if len(errs) > 0 {
Expand Down
67 changes: 60 additions & 7 deletions daemon/cluster/secrets.go
@@ -1,14 +1,63 @@
package cluster

import (
"fmt"
"strings"

apitypes "github.com/docker/docker/api/types"
types "github.com/docker/docker/api/types/swarm"
"github.com/docker/docker/daemon/cluster/convert"
swarmapi "github.com/docker/swarmkit/api"
"golang.org/x/net/context"
)

func getSecretByNameOrIDPrefix(ctx context.Context, state *nodeState, nameOrIDPrefix string) (*swarmapi.Secret, error) {
// attempt to lookup secret by full ID
if r, err := state.controlClient.GetSecret(ctx, &swarmapi.GetSecretRequest{
SecretID: nameOrIDPrefix,
}); err == nil {
return r.Secret, nil
}

// attempt to lookup secret by full name and partial ID
// Note here ListSecretRequest_Filters operate with `or`
r, err := state.controlClient.ListSecrets(ctx, &swarmapi.ListSecretsRequest{
Filters: &swarmapi.ListSecretsRequest_Filters{
Names: []string{nameOrIDPrefix},
IDPrefixes: []string{nameOrIDPrefix},
},
})
if err != nil {
return nil, err
}

// attempt to lookup secret by full name
for _, s := range r.Secrets {
if s.Spec.Annotations.Name == nameOrIDPrefix {
return s, nil
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we error out if len(r.Secrets) > 0 before this for loop?

Copy link
Member

Choose a reason for hiding this comment

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

Reason being if we get more than one, then it's an ambiguous result.
Also cleans up some of the stuff below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @cpuguy83 for the review. In swarmkit, the filter of secrets is slightly different from other filters (like services) in that it is operated as OR:
https://github.com/docker/docker/blob/v1.13.0-rc4/vendor/github.com/docker/swarmkit/manager/controlapi/secret.go#L121-L132

In the above case, we passes both Names and IDPrefixes so the result could be multiple like one for name and one for IDPrefix. This is considered as non-ambiguous as we have a full name match.

}
}
// attempt to lookup secret by partial ID (prefix)
// return error if more than one matches found (ambiguous)
n := 0
var found *swarmapi.Secret
for _, s := range r.Secrets {
if strings.HasPrefix(s.ID, nameOrIDPrefix) {
found = s
n++
}
}
if n > 1 {
return nil, fmt.Errorf("secret %s is ambiguous (%d matches found)", nameOrIDPrefix, n)
}
if found == nil {
return nil, fmt.Errorf("no such secret: %s", nameOrIDPrefix)
}
return found, nil
}

// GetSecret returns a secret from a managed swarm cluster
func (c *Cluster) GetSecret(id string) (types.Secret, error) {
func (c *Cluster) GetSecret(nameOrIDPrefix string) (types.Secret, error) {
c.mu.RLock()
defer c.mu.RUnlock()

Expand All @@ -20,12 +69,11 @@ func (c *Cluster) GetSecret(id string) (types.Secret, error) {
ctx, cancel := c.getRequestContext()
defer cancel()

r, err := state.controlClient.GetSecret(ctx, &swarmapi.GetSecretRequest{SecretID: id})
secret, err := getSecretByNameOrIDPrefix(ctx, &state, nameOrIDPrefix)
if err != nil {
return types.Secret{}, err
}

return convert.SecretFromGRPC(r.Secret), nil
return convert.SecretFromGRPC(secret), nil
}

// GetSecrets returns all secrets of a managed swarm cluster.
Expand Down Expand Up @@ -85,7 +133,7 @@ func (c *Cluster) CreateSecret(s types.SecretSpec) (string, error) {
}

// RemoveSecret removes a secret from a managed swarm cluster.
func (c *Cluster) RemoveSecret(id string) error {
func (c *Cluster) RemoveSecret(nameOrIDPrefix string) error {
c.mu.RLock()
defer c.mu.RUnlock()

Expand All @@ -97,11 +145,16 @@ func (c *Cluster) RemoveSecret(id string) error {
ctx, cancel := c.getRequestContext()
defer cancel()

secret, err := getSecretByNameOrIDPrefix(ctx, &state, nameOrIDPrefix)
if err != nil {
return err
}

req := &swarmapi.RemoveSecretRequest{
SecretID: id,
SecretID: secret.ID,
}

_, err := state.controlClient.RemoveSecret(ctx, req)
_, err = state.controlClient.RemoveSecret(ctx, req)
return err
}

Expand Down
2 changes: 1 addition & 1 deletion integration-cli/docker_cli_secret_create_test.go
Expand Up @@ -101,7 +101,7 @@ func (s *DockerSwarmSuite) TestSecretCreateResolve(c *check.C) {

// Remove based on ID prefix of the fake one should succeed
out, err = d.Cmd("secret", "rm", fake[:5])
c.Assert(out, checker.Contains, fake)
c.Assert(out, checker.Contains, fake[:5])
out, err = d.Cmd("secret", "ls")
c.Assert(err, checker.IsNil)
c.Assert(out, checker.Not(checker.Contains), name)
Expand Down