Update order of '--secret-rm' and '--secret-add'

When using both `--secret-rm` and `--secret-add` on `docker service update`,
`--secret-rm` was always performed last. This made it impossible to update
a secret that was already in use on a service (for example, to change
it's permissions, or mount-location inside the container).

This patch changes the order in which `rm` and `add` are performed,
allowing updating a secret in a single `docker service update`.

Before this change, the `rm` was always performed "last", so the secret
was always removed:

    $ echo "foo" | docker secret create foo -f -
    foo

    $ docker service create --name myservice --secret foo nginx:alpine
    62xjcr9sr0c2hvepdzqrn3ssn

    $ docker service update --secret-rm foo --secret-add source=foo,target=foo2 myservice
    myservice

    $ docker service inspect --format '{{ json .Spec.TaskTemplate.ContainerSpec.Secrets }}' myservice | jq .
    null

After this change, the `rm` is performed _first_, allowing users to
update a secret without updating the service _twice_;

    $ echo "foo" | docker secret create foo -f -
    1bllmvw3a1yaq3eixqw3f7bjl

    $ docker service create --name myservice --secret foo nginx:alpine
    lr6s3uoggli1x0hab78glpcxo

    $ docker service update --secret-rm foo --secret-add source=foo,target=foo2 myservice
    myservice

    $ docker service inspect --format '{{ json .Spec.TaskTemplate.ContainerSpec.Secrets }}' myservice | jq .

    [
      {
        "File": {
          "Name": "foo2",
          "UID": "0",
          "GID": "0",
          "Mode": 292
        },
        "SecretID": "tn9qiblgnuuut11eufquw5dev",
        "SecretName": "foo"
      }
    ]

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

cli/command/service/parse.go

@@ -12,7 +12,7 @@ import (
 
 // parseSecrets retrieves the secrets from the requested names and converts
 // them to secret references to use with the spec
-func parseSecrets(client client.APIClient, requestedSecrets []*types.SecretRequestOption) ([]*swarmtypes.SecretReference, error) {
+func parseSecrets(client client.SecretAPIClient, requestedSecrets []*types.SecretRequestOption) ([]*swarmtypes.SecretReference, error) {
 	secretRefs := make(map[string]*swarmtypes.SecretReference)
 	ctx := context.Background()
 

cli/command/service/update.go

@@ -6,8 +6,6 @@ import (
 	"strings"
 	"time"
 
-	"golang.org/x/net/context"
-
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/container"
 	mounttypes "github.com/docker/docker/api/types/mount"
@@ -21,6 +19,7 @@ import (
 	shlex "github.com/flynn-archive/go-shlex"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
+	"golang.org/x/net/context"
 )
 
 func newUpdateCommand(dockerCli *command.DockerCli) *cobra.Command {
@@ -431,7 +430,16 @@ func updateEnvironment(flags *pflag.FlagSet, field *[]string) {
 	*field = removeItems(*field, toRemove, envKey)
 }
 
-func getUpdatedSecrets(apiClient client.APIClient, flags *pflag.FlagSet, secrets []*swarm.SecretReference) ([]*swarm.SecretReference, error) {
+func getUpdatedSecrets(apiClient client.SecretAPIClient, flags *pflag.FlagSet, secrets []*swarm.SecretReference) ([]*swarm.SecretReference, error) {
+	newSecrets := []*swarm.SecretReference{}
+
+	toRemove := buildToRemoveSet(flags, flagSecretRemove)
+	for _, secret := range secrets {
+		if _, exists := toRemove[secret.SecretName]; !exists {
+			newSecrets = append(newSecrets, secret)
+		}
+	}
+
 	if flags.Changed(flagSecretAdd) {
 		values := flags.Lookup(flagSecretAdd).Value.(*opts.SecretOpt).Value()
 
@@ -439,14 +447,7 @@ func getUpdatedSecrets(apiClient client.APIClient, flags *pflag.FlagSet, secrets
 		if err != nil {
 			return nil, err
 		}
-		secrets = append(secrets, addSecrets...)
-	}
-	toRemove := buildToRemoveSet(flags, flagSecretRemove)
-	newSecrets := []*swarm.SecretReference{}
-	for _, secret := range secrets {
-		if _, exists := toRemove[secret.SecretName]; !exists {
-			newSecrets = append(newSecrets, secret)
-		}
+		newSecrets = append(newSecrets, addSecrets...)
 	}
 
 	return newSecrets, nil

cli/command/service/update_test.go

@@ -6,10 +6,12 @@ import (
 	"testing"
 	"time"
 
+	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/container"
 	mounttypes "github.com/docker/docker/api/types/mount"
 	"github.com/docker/docker/api/types/swarm"
 	"github.com/docker/docker/pkg/testutil/assert"
+	"golang.org/x/net/context"
 )
 
 func TestUpdateServiceArgs(t *testing.T) {
@@ -382,3 +384,58 @@ func TestValidatePort(t *testing.T) {
 		assert.Error(t, err, e)
 	}
 }
+
+type secretAPIClientMock struct {
+	listResult []swarm.Secret
+}
+
+func (s secretAPIClientMock) SecretList(ctx context.Context, options types.SecretListOptions) ([]swarm.Secret, error) {
+	return s.listResult, nil
+}
+func (s secretAPIClientMock) SecretCreate(ctx context.Context, secret swarm.SecretSpec) (types.SecretCreateResponse, error) {
+	return types.SecretCreateResponse{}, nil
+}
+func (s secretAPIClientMock) SecretRemove(ctx context.Context, id string) error {
+	return nil
+}
+func (s secretAPIClientMock) SecretInspectWithRaw(ctx context.Context, name string) (swarm.Secret, []byte, error) {
+	return swarm.Secret{}, []byte{}, nil
+}
+
+// TestUpdateSecretUpdateInPlace tests the ability to update the "target" of an secret with "docker service update"
+// by combining "--secret-rm" and "--secret-add" for the same secret.
+func TestUpdateSecretUpdateInPlace(t *testing.T) {
+	apiClient := secretAPIClientMock{
+		listResult: []swarm.Secret{
+			{
+				ID:   "tn9qiblgnuuut11eufquw5dev",
+				Spec: swarm.SecretSpec{Annotations: swarm.Annotations{Name: "foo"}},
+			},
+		},
+	}
+
+	flags := newUpdateCommand(nil).Flags()
+	flags.Set("secret-add", "source=foo,target=foo2")
+	flags.Set("secret-rm", "foo")
+
+	secrets := []*swarm.SecretReference{
+		{
+			File: &swarm.SecretReferenceFileTarget{
+				Name: "foo",
+				UID:  "0",
+				GID:  "0",
+				Mode: 292,
+			},
+			SecretID:   "tn9qiblgnuuut11eufquw5dev",
+			SecretName: "foo",
+		},
+	}
+
+	updatedSecrets, err := getUpdatedSecrets(apiClient, flags, secrets)
+
+	assert.Equal(t, err, nil)
+	assert.Equal(t, len(updatedSecrets), 1)
+	assert.Equal(t, updatedSecrets[0].SecretID, "tn9qiblgnuuut11eufquw5dev")
+	assert.Equal(t, updatedSecrets[0].SecretName, "foo")
+	assert.Equal(t, updatedSecrets[0].File.Name, "foo2")
+}