X Tutup
Skip to content

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

Merged
thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah:fix-secret-rm-consistency
Jan 9, 2017
Merged

Update order of '--secret-rm' and '--secret-add'#29802
thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah:fix-secret-rm-consistency

Conversation

@thaJeztah
Copy link
Member

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"
  }
]

listen-very-carefully-i-shall-say-this-only-once

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed this to a more fine-grained interface; only SecretAPIClient interface was expected - made it a bit easier to create a mock 😄

@thaJeztah thaJeztah force-pushed the fix-secret-rm-consistency branch from 759662f to b349a93 Compare December 31, 2016 13:26
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐸

@runcom
Copy link
Member

runcom commented Dec 31, 2016

Commit message has a typo s/-=add/-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>
@thaJeztah thaJeztah force-pushed the fix-secret-rm-consistency branch from b349a93 to e919534 Compare December 31, 2016 13:56
@thaJeztah
Copy link
Member Author

commit message has a typo s/-=add/-add

nice catch; updated 😄

@thaJeztah thaJeztah changed the title Update order of '--secret-rm' and '--secret-=add' Update order of '--secret-rm' and '--secret-add' Dec 31, 2016
@aaronlehmann
Copy link

I remember pointing this out on one of the earlier PRs, and getting the response that there would be a --replace-update flag to handle this case. But I guess we didn't add that?

Anyway, LGTM. Just wondering if there was a reason for it to behave this way.

cc @ehazlett

@thaJeztah
Copy link
Member Author

I remember pointing this out on one of the earlier PRs, and getting the response that there would be a --replace-update flag to handle this case. But I guess we didn't add that?

I missed that particular comment I guess, but there's indeed an open PR for that; #28461. That's still an option for future enhancement, but the basic options should work as well, and having rm always trump other options doesn't really make sense, and (IMO) is surprising behavior.

@aaronlehmann
Copy link

This can probably be merged unless there are outstanding concerns.

@tiborvass
Copy link
Contributor

LGTM

@thaJeztah
Copy link
Member Author

30709bc3c9b060baf771c0b2e2626f95-snow-white-high-five

@thaJeztah thaJeztah merged commit cd82a31 into moby:master Jan 9, 2017
@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Jan 9, 2017
@thaJeztah thaJeztah deleted the fix-secret-rm-consistency branch January 9, 2017 20:34
dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017
Update order of '--secret-rm' and '--secret-add'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

X Tutup