From edeb5b6e0d3ac2bc6db33735a503cecccc2828de Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 30 Dec 2016 18:15:53 +0100 Subject: [PATCH] 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 --- command/service/parse.go | 2 +- command/service/update.go | 23 +++++++------- command/service/update_test.go | 57 ++++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 12 deletions(-) diff --git a/command/service/parse.go b/command/service/parse.go index ff3249e581..6af7e3bb8e 100644 --- a/command/service/parse.go +++ b/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() diff --git a/command/service/update.go b/command/service/update.go index 514b1bd510..6d13927dae 100644 --- a/command/service/update.go +++ b/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 diff --git a/command/service/update_test.go b/command/service/update_test.go index 08fe248769..a6df6b985e 100644 --- a/command/service/update_test.go +++ b/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") +}