From b2e4c7f3b5b30fc6940768ec41836d708a48b463 Mon Sep 17 00:00:00 2001 From: Evan Hazlett Date: Thu, 3 Nov 2016 14:09:13 -0400 Subject: [PATCH] review updates - use Filters instead of Filter for secret list - UID, GID -> string - getSecrets -> getSecretsByName - updated test case for secrets with better source - use golang.org/x/context instead of context - for grpc conversion allocate with make - check for nil with task.Spec.GetContainer() Signed-off-by: Evan Hazlett --- api/server/router/swarm/cluster_routes.go | 8 ++--- api/types/container/secret.go | 4 +-- api/types/types.go | 2 +- cli/command/secret/inspect.go | 2 +- cli/command/secret/remove.go | 2 +- cli/command/secret/utils.go | 4 +-- cli/command/service/opts_test.go | 34 ++++++++++---------- cli/command/service/parse.go | 4 +-- client/secret_list.go | 4 +-- client/secret_list_test.go | 2 +- daemon/cluster/convert/container.go | 4 +-- daemon/cluster/executor/container/adapter.go | 21 ++++-------- daemon/cluster/secrets.go | 3 +- daemon/container_operations_unix.go | 11 ++++++- 14 files changed, 52 insertions(+), 53 deletions(-) diff --git a/api/server/router/swarm/cluster_routes.go b/api/server/router/swarm/cluster_routes.go index b04d066191..3c98e3ee26 100644 --- a/api/server/router/swarm/cluster_routes.go +++ b/api/server/router/swarm/cluster_routes.go @@ -267,14 +267,13 @@ func (sr *swarmRouter) getSecrets(ctx context.Context, w http.ResponseWriter, r if err := httputils.ParseForm(r); err != nil { return err } - filter, err := filters.FromParam(r.Form.Get("filters")) + filters, err := filters.FromParam(r.Form.Get("filters")) if err != nil { return err } - secrets, err := sr.backend.GetSecrets(basictypes.SecretListOptions{Filter: filter}) + secrets, err := sr.backend.GetSecrets(basictypes.SecretListOptions{Filters: filters}) if err != nil { - logrus.Errorf("Error getting secrets: %v", err) return err } @@ -289,7 +288,6 @@ func (sr *swarmRouter) createSecret(ctx context.Context, w http.ResponseWriter, id, err := sr.backend.CreateSecret(secret) if err != nil { - logrus.Errorf("Error creating secret %s: %v", id, err) return err } @@ -300,7 +298,6 @@ func (sr *swarmRouter) createSecret(ctx context.Context, w http.ResponseWriter, func (sr *swarmRouter) removeSecret(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { if err := sr.backend.RemoveSecret(vars["id"]); err != nil { - logrus.Errorf("Error removing secret %s: %v", vars["id"], err) return err } @@ -310,7 +307,6 @@ func (sr *swarmRouter) removeSecret(ctx context.Context, w http.ResponseWriter, func (sr *swarmRouter) getSecret(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { secret, err := sr.backend.GetSecret(vars["id"]) if err != nil { - logrus.Errorf("Error getting secret %s: %v", vars["id"], err) return err } diff --git a/api/types/container/secret.go b/api/types/container/secret.go index da86577f9c..13a50d352c 100644 --- a/api/types/container/secret.go +++ b/api/types/container/secret.go @@ -6,7 +6,7 @@ type ContainerSecret struct { Name string Target string Data []byte - UID int - GID int + UID string + GID string Mode os.FileMode } diff --git a/api/types/types.go b/api/types/types.go index fe60755c84..ceb18ddd89 100644 --- a/api/types/types.go +++ b/api/types/types.go @@ -520,5 +520,5 @@ type SecretCreateResponse struct { // SecretListOptions holds parameters to list secrets type SecretListOptions struct { - Filter filters.Args + Filters filters.Args } diff --git a/cli/command/secret/inspect.go b/cli/command/secret/inspect.go index c5b0aa6a3d..25da79f16d 100644 --- a/cli/command/secret/inspect.go +++ b/cli/command/secret/inspect.go @@ -35,7 +35,7 @@ func runSecretInspect(dockerCli *command.DockerCli, opts inspectOptions) error { ctx := context.Background() // attempt to lookup secret by name - secrets, err := getSecrets(client, ctx, []string{opts.name}) + secrets, err := getSecretsByName(client, ctx, []string{opts.name}) if err != nil { return err } diff --git a/cli/command/secret/remove.go b/cli/command/secret/remove.go index 9396b9b179..d277eceba2 100644 --- a/cli/command/secret/remove.go +++ b/cli/command/secret/remove.go @@ -32,7 +32,7 @@ func runSecretRemove(dockerCli *command.DockerCli, opts removeOptions) error { ctx := context.Background() // attempt to lookup secret by name - secrets, err := getSecrets(client, ctx, opts.ids) + secrets, err := getSecretsByName(client, ctx, opts.ids) if err != nil { return err } diff --git a/cli/command/secret/utils.go b/cli/command/secret/utils.go index 40aa4a6d77..d1a7d97c44 100644 --- a/cli/command/secret/utils.go +++ b/cli/command/secret/utils.go @@ -9,13 +9,13 @@ import ( "github.com/docker/docker/client" ) -func getSecrets(client client.APIClient, ctx context.Context, names []string) ([]swarm.Secret, error) { +func getSecretsByName(client client.APIClient, ctx context.Context, names []string) ([]swarm.Secret, error) { args := filters.NewArgs() for _, n := range names { args.Add("names", n) } return client.SecretList(ctx, types.SecretListOptions{ - Filter: args, + Filters: args, }) } diff --git a/cli/command/service/opts_test.go b/cli/command/service/opts_test.go index 551dfc239c..3df3a4fd5d 100644 --- a/cli/command/service/opts_test.go +++ b/cli/command/service/opts_test.go @@ -108,45 +108,45 @@ func TestHealthCheckOptionsToHealthConfigConflict(t *testing.T) { } func TestSecretOptionsSimple(t *testing.T) { - var opt SecretOpt + var opt opts.SecretOpt - testCase := "source=/foo,target=testing" + testCase := "source=foo,target=testing" assert.NilError(t, opt.Set(testCase)) reqs := opt.Value() assert.Equal(t, len(reqs), 1) req := reqs[0] - assert.Equal(t, req.source, "/foo") - assert.Equal(t, req.target, "testing") + assert.Equal(t, req.Source, "foo") + assert.Equal(t, req.Target, "testing") } func TestSecretOptionsCustomUidGid(t *testing.T) { - var opt SecretOpt + var opt opts.SecretOpt - testCase := "source=/foo,target=testing,uid=1000,gid=1001" + testCase := "source=foo,target=testing,uid=1000,gid=1001" assert.NilError(t, opt.Set(testCase)) reqs := opt.Value() assert.Equal(t, len(reqs), 1) req := reqs[0] - assert.Equal(t, req.source, "/foo") - assert.Equal(t, req.target, "testing") - assert.Equal(t, req.uid, "1000") - assert.Equal(t, req.gid, "1001") + assert.Equal(t, req.Source, "foo") + assert.Equal(t, req.Target, "testing") + assert.Equal(t, req.UID, "1000") + assert.Equal(t, req.GID, "1001") } func TestSecretOptionsCustomMode(t *testing.T) { - var opt SecretOpt + var opt opts.SecretOpt - testCase := "source=/foo,target=testing,uid=1000,gid=1001,mode=0444" + testCase := "source=foo,target=testing,uid=1000,gid=1001,mode=0444" assert.NilError(t, opt.Set(testCase)) reqs := opt.Value() assert.Equal(t, len(reqs), 1) req := reqs[0] - assert.Equal(t, req.source, "/foo") - assert.Equal(t, req.target, "testing") - assert.Equal(t, req.uid, "1000") - assert.Equal(t, req.gid, "1001") - assert.Equal(t, req.mode, os.FileMode(0444)) + assert.Equal(t, req.Source, "foo") + assert.Equal(t, req.Target, "testing") + assert.Equal(t, req.UID, "1000") + assert.Equal(t, req.GID, "1001") + assert.Equal(t, req.Mode, os.FileMode(0444)) } diff --git a/cli/command/service/parse.go b/cli/command/service/parse.go index cbf2745dce..4728c773c4 100644 --- a/cli/command/service/parse.go +++ b/cli/command/service/parse.go @@ -1,13 +1,13 @@ package service import ( - "context" "fmt" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/filters" swarmtypes "github.com/docker/docker/api/types/swarm" "github.com/docker/docker/client" + "golang.org/x/net/context" ) // parseSecrets retrieves the secrets from the requested names and converts @@ -39,7 +39,7 @@ func parseSecrets(client client.APIClient, requestedSecrets []*types.SecretReque } secrets, err := client.SecretList(ctx, types.SecretListOptions{ - Filter: args, + Filters: args, }) if err != nil { return nil, err diff --git a/client/secret_list.go b/client/secret_list.go index 5e9d2b5098..7e9d5ec167 100644 --- a/client/secret_list.go +++ b/client/secret_list.go @@ -14,8 +14,8 @@ import ( func (cli *Client) SecretList(ctx context.Context, options types.SecretListOptions) ([]swarm.Secret, error) { query := url.Values{} - if options.Filter.Len() > 0 { - filterJSON, err := filters.ToParam(options.Filter) + if options.Filters.Len() > 0 { + filterJSON, err := filters.ToParam(options.Filters) if err != nil { return nil, err } diff --git a/client/secret_list_test.go b/client/secret_list_test.go index 174963c7ee..1ac11cddb3 100644 --- a/client/secret_list_test.go +++ b/client/secret_list_test.go @@ -45,7 +45,7 @@ func TestSecretList(t *testing.T) { }, { options: types.SecretListOptions{ - Filter: filters, + Filters: filters, }, expectedQueryParams: map[string]string{ "filters": `{"label":{"label1":true,"label2":true}}`, diff --git a/daemon/cluster/convert/container.go b/daemon/cluster/convert/container.go index 1a6121c240..b5ce27dc61 100644 --- a/daemon/cluster/convert/container.go +++ b/daemon/cluster/convert/container.go @@ -78,7 +78,7 @@ func containerSpecFromGRPC(c *swarmapi.ContainerSpec) types.ContainerSpec { } func secretReferencesToGRPC(sr []*types.SecretReference) []*swarmapi.SecretReference { - refs := []*swarmapi.SecretReference{} + refs := make([]*swarmapi.SecretReference, 0, len(sr)) for _, s := range sr { refs = append(refs, &swarmapi.SecretReference{ SecretID: s.SecretID, @@ -97,7 +97,7 @@ func secretReferencesToGRPC(sr []*types.SecretReference) []*swarmapi.SecretRefer return refs } func secretReferencesFromGRPC(sr []*swarmapi.SecretReference) []*types.SecretReference { - refs := []*types.SecretReference{} + refs := make([]*types.SecretReference, 0, len(sr)) for _, s := range sr { target := s.GetFile() if target == nil { diff --git a/daemon/cluster/executor/container/adapter.go b/daemon/cluster/executor/container/adapter.go index 02c327f8a1..87ddaac455 100644 --- a/daemon/cluster/executor/container/adapter.go +++ b/daemon/cluster/executor/container/adapter.go @@ -5,7 +5,6 @@ import ( "encoding/json" "fmt" "io" - "strconv" "strings" "syscall" "time" @@ -219,7 +218,11 @@ func (c *containerAdapter) create(ctx context.Context) error { } } - secrets := []*containertypes.ContainerSecret{} + container := c.container.task.Spec.GetContainer() + if container == nil { + return fmt.Errorf("unable to get container from task spec") + } + secrets := make([]*containertypes.ContainerSecret, 0, len(container.Secrets)) for _, s := range c.container.task.Spec.GetContainer().Secrets { sec := c.secrets.Get(s.SecretID) if sec == nil { @@ -233,23 +236,13 @@ func (c *containerAdapter) create(ctx context.Context) error { logrus.Warnf("secret target was not a file: secret=%s", s.SecretID) continue } - // convert uid / gid string to int - uid, err := strconv.Atoi(target.UID) - if err != nil { - return err - } - - gid, err := strconv.Atoi(target.GID) - if err != nil { - return err - } secrets = append(secrets, &containertypes.ContainerSecret{ Name: name, Target: target.Name, Data: sec.Spec.Data, - UID: uid, - GID: gid, + UID: target.UID, + GID: target.GID, Mode: target.Mode, }) } diff --git a/daemon/cluster/secrets.go b/daemon/cluster/secrets.go index ca795192be..27114152f4 100644 --- a/daemon/cluster/secrets.go +++ b/daemon/cluster/secrets.go @@ -29,7 +29,7 @@ func (c *Cluster) GetSecrets(options apitypes.SecretListOptions) ([]types.Secret return nil, c.errNoManager() } - filters, err := newListSecretsFilters(options.Filter) + filters, err := newListSecretsFilters(options.Filters) if err != nil { return nil, err } @@ -97,6 +97,7 @@ func (c *Cluster) RemoveSecret(id string) error { } // UpdateSecret updates a secret in a managed swarm cluster. +// Note: this is not exposed to the CLI but is available from the API only func (c *Cluster) UpdateSecret(id string, version uint64, spec types.SecretSpec) error { c.RLock() defer c.RUnlock() diff --git a/daemon/container_operations_unix.go b/daemon/container_operations_unix.go index ac6b6ad1ad..d1b77134be 100644 --- a/daemon/container_operations_unix.go +++ b/daemon/container_operations_unix.go @@ -191,7 +191,16 @@ func (daemon *Daemon) setupSecretDir(c *container.Container) (setupErr error) { return errors.Wrap(err, "error injecting secret") } - if err := os.Chown(fPath, s.UID, s.GID); err != nil { + uid, err := strconv.Atoi(s.UID) + if err != nil { + return err + } + gid, err := strconv.Atoi(s.GID) + if err != nil { + return err + } + + if err := os.Chown(fPath, uid, gid); err != nil { return errors.Wrap(err, "error setting ownership for secret") } }