New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jan 9, 2017

Conversation

Projects
None yet
6 participants
@thaJeztah
Member

thaJeztah commented Dec 31, 2016

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

@@ -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) {

This comment has been minimized.

@thaJeztah

thaJeztah Dec 31, 2016

Member

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

@thaJeztah

thaJeztah Dec 31, 2016

Member

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

@vdemeester

LGTM 🐸

@runcom

This comment has been minimized.

Show comment
Hide comment
@runcom

runcom Dec 31, 2016

Member

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

Member

runcom commented Dec 31, 2016

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

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>
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 31, 2016

Member

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

nice catch; updated 😄

Member

thaJeztah commented Dec 31, 2016

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

nice catch; updated 😄

@thaJeztah thaJeztah changed the title from Update order of '--secret-rm' and '--secret-=add' to Update order of '--secret-rm' and '--secret-add' Dec 31, 2016

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Jan 3, 2017

Contributor

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

Contributor

aaronlehmann commented Jan 3, 2017

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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 3, 2017

Member

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.

Member

thaJeztah commented Jan 3, 2017

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

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Jan 9, 2017

Contributor

This can probably be merged unless there are outstanding concerns.

Contributor

aaronlehmann commented Jan 9, 2017

This can probably be merged unless there are outstanding concerns.

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Jan 9, 2017

Collaborator

LGTM

Collaborator

tiborvass commented Jan 9, 2017

LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Jan 9, 2017

30709bc3c9b060baf771c0b2e2626f95-snow-white-high-five

@thaJeztah thaJeztah merged commit cd82a31 into moby:master Jan 9, 2017

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 29018 has succeeded
Details
janky Jenkins build Docker-PRs 37610 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 9016 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Jan 9, 2017

@thaJeztah thaJeztah deleted the thaJeztah:fix-secret-rm-consistency branch Jan 9, 2017

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#29802 from thaJeztah/fix-secret-rm-consistency
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