review changes

- fix lint issues
- use errors pkg for wrapping errors
- cleanup on error when setting up secrets mount
- fix erroneous import
- remove unneeded switch for secret reference mode
- return single mount for secrets instead of slice

Signed-off-by: Evan Hazlett <ejhazlett@gmail.com>

api/types/swarm/secret.go

@@ -11,7 +11,7 @@ type Secret struct {
 
 type SecretSpec struct {
 	Annotations
-	Data []byte `json",omitempty"`
+	Data []byte `json:",omitempty"`
 }
 
 type SecretReferenceMode int

cli/command/service/parse.go

@@ -3,6 +3,7 @@ package service
 import (
 	"context"
 	"fmt"
+	"path/filepath"
 	"strings"
 
 	"github.com/docker/docker/api/types"
@@ -31,6 +32,13 @@ func parseSecretString(secretString string) (string, string, error) {
 	} else {
 		targetName = secretName
 	}
+
+	// ensure target is a filename only; no paths allowed
+	tDir, _ := filepath.Split(targetName)
+	if tDir != "" {
+		return "", "", fmt.Errorf("target must not have a path")
+	}
+
 	return secretName, targetName, nil
 }
 

container/container_unix.go

@@ -22,8 +22,8 @@ import (
 	"golang.org/x/sys/unix"
 )
 
-// DefaultSHMSize is the default size (64MB) of the SHM which will be mounted in the container
 const (
+	// DefaultSHMSize is the default size (64MB) of the SHM which will be mounted in the container
 	DefaultSHMSize           int64 = 67108864
 	containerSecretMountPath       = "/run/secrets"
 )
@@ -178,6 +178,7 @@ func (container *Container) NetworkMounts() []Mount {
 	return mounts
 }
 
+// SecretMountPath returns the path of the secret mount for the container
 func (container *Container) SecretMountPath() string {
 	return filepath.Join(container.Root, "secrets")
 }
@@ -267,19 +268,19 @@ func (container *Container) IpcMounts() []Mount {
 	return mounts
 }
 
-// SecretMounts returns the list of Secret mounts
-func (container *Container) SecretMounts() []Mount {
-	var mounts []Mount
+// SecretMount returns the list of Secret mounts
+func (container *Container) SecretMount() Mount {
+	var mount Mount
 
 	if len(container.Secrets) > 0 {
-		mounts = append(mounts, Mount{
+		mount = Mount{
 			Source:      container.SecretMountPath(),
 			Destination: containerSecretMountPath,
 			Writable:    false,
-		})
+		}
 	}
 
-	return mounts
+	return mount
 }
 
 // UnmountSecrets unmounts the local tmpfs for secrets

daemon/cluster/convert/container.go

@@ -79,12 +79,9 @@ func containerSpecFromGRPC(c *swarmapi.ContainerSpec) types.ContainerSpec {
 func secretReferencesToGRPC(sr []*types.SecretReference) []*swarmapi.SecretReference {
 	refs := []*swarmapi.SecretReference{}
 	for _, s := range sr {
-		var mode swarmapi.SecretReference_Mode
-		switch s.Mode {
-		case types.SecretReferenceSystem:
+		mode := swarmapi.SecretReference_FILE
+		if s.Mode == types.SecretReferenceSystem {
 			mode = swarmapi.SecretReference_SYSTEM
-		default:
-			mode = swarmapi.SecretReference_FILE
 		}
 		refs = append(refs, &swarmapi.SecretReference{
 			SecretID:   s.SecretID,

daemon/cluster/convert/secret.go

@@ -1,7 +1,6 @@
 package convert
 
 import (
-	"github.com/Sirupsen/logrus"
 	swarmtypes "github.com/docker/docker/api/types/swarm"
 	swarmapi "github.com/docker/swarmkit/api"
 	"github.com/docker/swarmkit/protobuf/ptypes"
@@ -9,7 +8,6 @@ import (
 
 // SecretFromGRPC converts a grpc Secret to a Secret.
 func SecretFromGRPC(s *swarmapi.Secret) swarmtypes.Secret {
-	logrus.Debugf("%+v", s)
 	secret := swarmtypes.Secret{
 		ID:         s.ID,
 		Digest:     s.Digest,
@@ -33,14 +31,12 @@ func SecretFromGRPC(s *swarmapi.Secret) swarmtypes.Secret {
 }
 
 // SecretSpecToGRPC converts Secret to a grpc Secret.
-func SecretSpecToGRPC(s swarmtypes.SecretSpec) (swarmapi.SecretSpec, error) {
-	spec := swarmapi.SecretSpec{
+func SecretSpecToGRPC(s swarmtypes.SecretSpec) swarmapi.SecretSpec {
+	return swarmapi.SecretSpec{
 		Annotations: swarmapi.Annotations{
 			Name:   s.Name,
 			Labels: s.Labels,
 		},
 		Data: s.Data,
 	}
-
-	return spec, nil
 }

daemon/cluster/executor/container/attachment.go

@@ -2,9 +2,9 @@ package container
 
 import (
 	executorpkg "github.com/docker/docker/daemon/cluster/executor"
+	"github.com/docker/swarmkit/agent/exec"
 	"github.com/docker/swarmkit/api"
 	"golang.org/x/net/context"
-	"src/github.com/docker/swarmkit/agent/exec"
 )
 
 // networkAttacherController implements agent.Controller against docker's API.

daemon/cluster/secrets.go

@@ -63,10 +63,7 @@ func (c *Cluster) CreateSecret(s types.SecretSpec) (string, error) {
 	ctx, cancel := c.getRequestContext()
 	defer cancel()
 
-	secretSpec, err := convert.SecretSpecToGRPC(s)
-	if err != nil {
-		return "", err
-	}
+	secretSpec := convert.SecretSpecToGRPC(s)
 
 	r, err := c.node.client.CreateSecret(ctx,
 		&swarmapi.CreateSecretRequest{Spec: &secretSpec})
@@ -111,10 +108,7 @@ func (c *Cluster) UpdateSecret(id string, version uint64, spec types.SecretSpec)
 	ctx, cancel := c.getRequestContext()
 	defer cancel()
 
-	secretSpec, err := convert.SecretSpecToGRPC(spec)
-	if err != nil {
-		return err
-	}
+	secretSpec := convert.SecretSpecToGRPC(spec)
 
 	if _, err := c.client.UpdateSecret(ctx,
 		&swarmapi.UpdateSecretRequest{

daemon/container_operations_unix.go

@@ -13,6 +13,7 @@ import (
 	"time"
 
 	"github.com/Sirupsen/logrus"
+	"github.com/cloudflare/cfssl/log"
 	containertypes "github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/container"
 	"github.com/docker/docker/daemon/links"
@@ -25,6 +26,7 @@ import (
 	"github.com/opencontainers/runc/libcontainer/devices"
 	"github.com/opencontainers/runc/libcontainer/label"
 	"github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/pkg/errors"
 )
 
 func u32Ptr(i int64) *uint32     { u := uint32(i); return &u }
@@ -146,36 +148,58 @@ func (daemon *Daemon) setupSecretDir(c *container.Container) error {
 	localMountPath := c.SecretMountPath()
 	logrus.Debugf("secrets: setting up secret dir: %s", localMountPath)
 
+	var setupErr error
+
+	defer func(err error) {
+		if err != nil {
+			// cleanup
+			_ = detachMounted(localMountPath)
+
+			if err := os.RemoveAll(localMountPath); err != nil {
+				log.Errorf("error cleaning up secret mount: %s", err)
+			}
+		}
+	}(setupErr)
+
 	// create tmpfs
 	if err := os.MkdirAll(localMountPath, 0700); err != nil {
-		return fmt.Errorf("error creating secret local mount path: %s", err)
+		setupErr = errors.Wrap(err, "error creating secret local mount path")
 	}
 	if err := mount.Mount("tmpfs", localMountPath, "tmpfs", "nodev"); err != nil {
-		return fmt.Errorf("unable to setup secret mount: %s", err)
+		setupErr = errors.Wrap(err, "unable to setup secret mount")
 	}
 
 	for _, s := range c.Secrets {
-		fPath := filepath.Join(localMountPath, s.Target)
+		// ensure that the target is a filename only; no paths allowed
+		tDir, tPath := filepath.Split(s.Target)
+		if tDir != "" {
+			setupErr = fmt.Errorf("error creating secret: secret must not have a path")
+		}
+
+		fPath := filepath.Join(localMountPath, tPath)
 		if err := os.MkdirAll(filepath.Dir(fPath), 0700); err != nil {
-			return fmt.Errorf("error creating secret mount path: %s", err)
+			setupErr = errors.Wrap(err, "error creating secret mount path")
 		}
 
-		logrus.Debugf("injecting secret: name=%s path=%s", s.Name, fPath)
+		logrus.WithFields(logrus.Fields{
+			"name": s.Name,
+			"path": fPath,
+		}).Debug("injecting secret")
 		if err := ioutil.WriteFile(fPath, s.Data, s.Mode); err != nil {
-			return fmt.Errorf("error injecting secret: %s", err)
+			setupErr = errors.Wrap(err, "error injecting secret")
 		}
 
 		if err := os.Chown(fPath, s.Uid, s.Gid); err != nil {
-			return fmt.Errorf("error setting ownership for secret: %s", err)
+			setupErr = errors.Wrap(err, "error setting ownership for secret")
 		}
 	}
 
 	// remount secrets ro
 	if err := mount.Mount("tmpfs", localMountPath, "tmpfs", "remount,ro"); err != nil {
-		return fmt.Errorf("unable to remount secret dir as readonly: %s", err)
+		setupErr = errors.Wrap(err, "unable to remount secret dir as readonly")
 	}
 
-	return nil
+	return setupErr
 }
 
 func killProcessDirectly(container *container.Container) error {

daemon/oci_linux.go

@@ -718,7 +718,8 @@ func (daemon *Daemon) createSpec(c *container.Container) (*specs.Spec, error) {
 	}
 	ms = append(ms, tmpfsMounts...)
 
-	ms = append(ms, c.SecretMounts()...)
+	ms = append(ms, c.SecretMount())
+
 	sort.Sort(mounts(ms))
 	if err := setMounts(daemon, &s, c, ms); err != nil {
 		return nil, fmt.Errorf("linux mounts: %v", err)

daemon/secrets.go

@@ -5,8 +5,9 @@ import (
 	containertypes "github.com/docker/docker/api/types/container"
 )
 
+// SetContainerSecrets sets the container secrets needed
 func (daemon *Daemon) SetContainerSecrets(name string, secrets []*containertypes.ContainerSecret) error {
-	if !secretsSupported() {
+	if !secretsSupported() && len(secrets) > 0 {
 		logrus.Warn("secrets are not supported on this platform")
 		return nil
 	}

integration-cli/daemon_swarm.go

@@ -284,6 +284,16 @@ func (d *SwarmDaemon) listServices(c *check.C) []swarm.Service {
 	return services
 }
 
+func (d *SwarmDaemon) listSecrets(c *check.C) []swarm.Service {
+	status, out, err := d.SockRequest("GET", "/secrets", nil)
+	c.Assert(err, checker.IsNil, check.Commentf(string(out)))
+	c.Assert(status, checker.Equals, http.StatusOK, check.Commentf("output: %q", string(out)))
+
+	secrets := []swarm.Secret{}
+	c.Assert(json.Unmarshal(out, &secrets), checker.IsNil)
+	return services
+}
+
 func (d *SwarmDaemon) getSwarm(c *check.C) swarm.Swarm {
 	var sw swarm.Swarm
 	status, out, err := d.SockRequest("GET", "/swarm", nil)

integration-cli/docker_api_swarm_test.go

@@ -1263,3 +1263,27 @@ func (s *DockerSwarmSuite) TestAPISwarmServicesUpdateWithName(c *check.C) {
 	c.Assert(status, checker.Equals, http.StatusOK, check.Commentf("output: %q", string(out)))
 	waitAndAssert(c, defaultReconciliationTimeout, d.checkActiveContainerCount, checker.Equals, instances)
 }
+
+func (s *DockerSwarmSuite) TestAPISwarmSecretsEmptyList(c *check.C) {
+	d := s.AddDaemon(c, true, true)
+
+	secrets := d.listSecrets(c)
+	c.Assert(secrets, checker.NotNil)
+	c.Assert(len(secrets), checker.Equals, 0, check.Commentf("secrets: %#v", secrets))
+}
+
+//func (s *DockerSwarmSuite) TestAPISwarmServicesCreate(c *check.C) {
+//	d := s.AddDaemon(c, true, true)
+//
+//	instances := 2
+//	id := d.createService(c, simpleTestService, setInstances(instances))
+//	waitAndAssert(c, defaultReconciliationTimeout, d.checkActiveContainerCount, checker.Equals, instances)
+//
+//	service := d.getService(c, id)
+//	instances = 5
+//	d.updateService(c, service, setInstances(instances))
+//	waitAndAssert(c, defaultReconciliationTimeout, d.checkActiveContainerCount, checker.Equals, instances)
+//
+//	d.removeService(c, service.ID)
+//	waitAndAssert(c, defaultReconciliationTimeout, d.checkActiveContainerCount, checker.Equals, 0)
+//}