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

Move secret name or ID prefix resolving from client to daemon #29218

Merged
merged 1 commit into from Jan 27, 2017

Conversation

Projects
None yet
9 participants
@yongtang
Member

yongtang commented Dec 7, 2016

- What I did

This fix is a follow up for comment:
#28896 (comment)

Currently secret name or ID prefix resolving is done at the client side, which means different behavior of API and CMD.

/cc @thaJeztah @ehazlett @allencloud

- How I did it
This fix is a follow up for comment:
#28896 (comment)

Currently secret name or ID prefix resolving is done at the client side, which means different behavior of API and CMD.

- How to verify it

All existing tests should pass.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

x240-khu

This fix is related to #28896, #28884 and may be related to #29125.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

@ehazlett

This comment has been minimized.

Show comment
Hide comment
@ehazlett

ehazlett Dec 7, 2016

Contributor

nice -- i had started something to work on 29125 but this is better.

LGTM (nam)

Contributor

ehazlett commented Dec 7, 2016

nice -- i had started something to work on 29125 but this is better.

LGTM (nam)

Show outdated Hide outdated daemon/cluster/secrets.go Outdated
Show outdated Hide outdated daemon/cluster/secrets.go Outdated

@thaJeztah thaJeztah added this to the 1.13.0 milestone Dec 8, 2016

// attempt to lookup secret by full name
for _, s := range r.Secrets {
if s.Spec.Annotations.Name == nameOrIDPrefix {
return s, nil

This comment has been minimized.

@cpuguy83

cpuguy83 Dec 21, 2016

Contributor

Shouldn't we error out if len(r.Secrets) > 0 before this for loop?

@cpuguy83

cpuguy83 Dec 21, 2016

Contributor

Shouldn't we error out if len(r.Secrets) > 0 before this for loop?

This comment has been minimized.

@cpuguy83

cpuguy83 Dec 21, 2016

Contributor

Reason being if we get more than one, then it's an ambiguous result.
Also cleans up some of the stuff below.

@cpuguy83

cpuguy83 Dec 21, 2016

Contributor

Reason being if we get more than one, then it's an ambiguous result.
Also cleans up some of the stuff below.

This comment has been minimized.

@yongtang

yongtang Dec 21, 2016

Member

Thanks @cpuguy83 for the review. In swarmkit, the filter of secrets is slightly different from other filters (like services) in that it is operated as OR:
https://github.com/docker/docker/blob/v1.13.0-rc4/vendor/github.com/docker/swarmkit/manager/controlapi/secret.go#L121-L132

In the above case, we passes both Names and IDPrefixes so the result could be multiple like one for name and one for IDPrefix. This is considered as non-ambiguous as we have a full name match.

@yongtang

yongtang Dec 21, 2016

Member

Thanks @cpuguy83 for the review. In swarmkit, the filter of secrets is slightly different from other filters (like services) in that it is operated as OR:
https://github.com/docker/docker/blob/v1.13.0-rc4/vendor/github.com/docker/swarmkit/manager/controlapi/secret.go#L121-L132

In the above case, we passes both Names and IDPrefixes so the result could be multiple like one for name and one for IDPrefix. This is considered as non-ambiguous as we have a full name match.

@cpuguy83

LGTM

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang
Member

yongtang commented Dec 22, 2016

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Jan 14, 2017

Contributor

Code LGTM

@yongtang: This has API impact, so api/swagger.yaml needs to be updated.

@thaJeztah: Are we comfortable making an API change like this to endpoints that have been added in 1.13?

Contributor

aaronlehmann commented Jan 14, 2017

Code LGTM

@yongtang: This has API impact, so api/swagger.yaml needs to be updated.

@thaJeztah: Are we comfortable making an API change like this to endpoints that have been added in 1.13?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 14, 2017

Member

Are we comfortable making an API change like this to endpoints that have been added in 1.13?

Ideally, this would've been part of 1.13, but I don't think that's still realistic, so;

  • I think this change is fully backward compatible with the current 1.13 API (i.e., providing a full ID still returns the same information, correct?
  • We could gate the "fuzzy" search (name, partial ID) for the 1.13 (API 1.25) version, or just keep it as "undocumented" enhancement. would there be situations where this could have an unwanted side effects for 1.13 clients?

For the documentation; I'm not sure how to define a change for a specific version in the swagger.yml. @dnephin @bfirsh perhaps you can assist with that?

Otherwise; good to go 😄 - I'm changing the milestone to 1.14, unless we see reasons/options to include this in 1.13 (@vieux?)

Member

thaJeztah commented Jan 14, 2017

Are we comfortable making an API change like this to endpoints that have been added in 1.13?

Ideally, this would've been part of 1.13, but I don't think that's still realistic, so;

  • I think this change is fully backward compatible with the current 1.13 API (i.e., providing a full ID still returns the same information, correct?
  • We could gate the "fuzzy" search (name, partial ID) for the 1.13 (API 1.25) version, or just keep it as "undocumented" enhancement. would there be situations where this could have an unwanted side effects for 1.13 clients?

For the documentation; I'm not sure how to define a change for a specific version in the swagger.yml. @dnephin @bfirsh perhaps you can assist with that?

Otherwise; good to go 😄 - I'm changing the milestone to 1.14, unless we see reasons/options to include this in 1.13 (@vieux?)

@thaJeztah thaJeztah modified the milestones: 1.14.0, 1.13.0 Jan 14, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 14, 2017

Member

Actually; if this goes to 1.14, the CLI should still do "client side" lookup when talking to a 1.13 daemon. 😞

Member

thaJeztah commented Jan 14, 2017

Actually; if this goes to 1.14, the CLI should still do "client side" lookup when talking to a 1.13 daemon. 😞

@bfirsh

This comment has been minimized.

Show comment
Hide comment
@bfirsh

bfirsh Jan 14, 2017

Contributor

@thaJeztah Each API version has its own swagger.yml. For this change, there is no issue because there is no swagger.yml representation of 1.24 or lower.

At some point we'll have to make a copy of swagger.yml for 1.25 docs when 1.26 becomes the current docs (if that hasn't happened already?). The original intention was to keep these old versions in the docs repository, because in theory they should not be updated again and any changes would just be bug fixes. For simplicity of contribution, however, perhaps it makes sense for them to live in this repo somewhere.

Contributor

bfirsh commented Jan 14, 2017

@thaJeztah Each API version has its own swagger.yml. For this change, there is no issue because there is no swagger.yml representation of 1.24 or lower.

At some point we'll have to make a copy of swagger.yml for 1.25 docs when 1.26 becomes the current docs (if that hasn't happened already?). The original intention was to keep these old versions in the docs repository, because in theory they should not be updated again and any changes would just be bug fixes. For simplicity of contribution, however, perhaps it makes sense for them to live in this repo somewhere.

@dnephin

LGTM

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Jan 16, 2017

Member

Actually; if this goes to 1.14, the CLI should still do "client side" lookup when talking to a 1.13 daemon.

I don't think that's necessary. We can just make that an error or warning, and inform the user that it won't work as expected (only works with exact IDs in that case).

Member

dnephin commented Jan 16, 2017

Actually; if this goes to 1.14, the CLI should still do "client side" lookup when talking to a 1.13 daemon.

I don't think that's necessary. We can just make that an error or warning, and inform the user that it won't work as expected (only works with exact IDs in that case).

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 16, 2017

Member

@dnephin the point is that basic functionality of docker secret won't work if we don't fall back to the old behavior; using the client from this PR, against docker 1.13

Client:
 Version:      1.14.0-dev
 API version:  1.25 (downgraded from 1.26)
 Go version:   go1.7.3
 Git commit:   a097d6c
 Built:        Mon Jan 16 21:30:03 2017
 OS/Arch:      linux/amd64

Server:
 Version:      1.13.0-rc6
 API version:  1.25 (minimum version 1.12)
 Go version:   go1.7.3
 Git commit:   2f2d055
 Built:        Wed Jan 11 21:47:55 2017
 OS/Arch:      linux/amd64
 Experimental: false

Create a secret

$ echo "foo" | docker secret create foo
veltdcwwvv5qdewcxs85n8tj2

$ docker secret ls

ID                          NAME                CREATED             UPDATED
veltdcwwvv5qdewcxs85n8tj2   foo                 4 minutes ago       4 minutes ago

then try to either inspect or rm it;

$ docker secret inspect foo
[]
Error: no such secret: foo
$ docker secret rm foo
Error response from daemon: rpc error: code = 5 desc = could not find secret foo
Member

thaJeztah commented Jan 16, 2017

@dnephin the point is that basic functionality of docker secret won't work if we don't fall back to the old behavior; using the client from this PR, against docker 1.13

Client:
 Version:      1.14.0-dev
 API version:  1.25 (downgraded from 1.26)
 Go version:   go1.7.3
 Git commit:   a097d6c
 Built:        Mon Jan 16 21:30:03 2017
 OS/Arch:      linux/amd64

Server:
 Version:      1.13.0-rc6
 API version:  1.25 (minimum version 1.12)
 Go version:   go1.7.3
 Git commit:   2f2d055
 Built:        Wed Jan 11 21:47:55 2017
 OS/Arch:      linux/amd64
 Experimental: false

Create a secret

$ echo "foo" | docker secret create foo
veltdcwwvv5qdewcxs85n8tj2

$ docker secret ls

ID                          NAME                CREATED             UPDATED
veltdcwwvv5qdewcxs85n8tj2   foo                 4 minutes ago       4 minutes ago

then try to either inspect or rm it;

$ docker secret inspect foo
[]
Error: no such secret: foo
$ docker secret rm foo
Error response from daemon: rpc error: code = 5 desc = could not find secret foo
@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Jan 16, 2017

Member

@thaJeztah I understand that. So we can add a warning about it in the 1.14 client. When the 1.14 downgrades it can warn about having to use ids.

Member

dnephin commented Jan 16, 2017

@thaJeztah I understand that. So we can add a warning about it in the 1.14 client. When the 1.14 downgrades it can warn about having to use ids.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 16, 2017

Member

I'd like to get input from @vieux on that; I don't think it's good to have support for talking to older daemons, but at the same time break that functionality. I think we'll only be supporting going one or two versions back, in which case, keeping the fallback mechanism should still be an option. (but that's just my 0.02c)

Member

thaJeztah commented Jan 16, 2017

I'd like to get input from @vieux on that; I don't think it's good to have support for talking to older daemons, but at the same time break that functionality. I think we'll only be supporting going one or two versions back, in which case, keeping the fallback mechanism should still be an option. (but that's just my 0.02c)

@LK4D4 LK4D4 removed the version/1.13 label Jan 27, 2017

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 27, 2017

Contributor

@yongtang needs rebase, sorry :)

Contributor

LK4D4 commented Jan 27, 2017

@yongtang needs rebase, sorry :)

Move secret name or ID prefix resolving from client to daemon
This fix is a follow up for comment:
#28896 (comment)

Currently secret name or ID prefix resolving is done at the client
side, which means different behavior of API and CMD.

This fix moves the resolving from client to daemon, with exactly the
same rule:
- Full ID
- Full Name
- Partial ID (prefix)

All existing tests should pass.

This fix is related to #288896, #28884 and may be related to #29125.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 27, 2017

Member

Thanks @LK4D4 the PR has been rebased.

Member

yongtang commented Jan 27, 2017

Thanks @LK4D4 the PR has been rebased.

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 27, 2017

Contributor

This already has 2 LGTMs

Contributor

LK4D4 commented Jan 27, 2017

This already has 2 LGTMs

@LK4D4 LK4D4 merged commit 3c32e17 into moby:master Jan 27, 2017

3 of 4 checks passed

windowsRS1 Jenkins build Docker-PRs-WoW-RS1 9685 has failed
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 30045 has succeeded
Details
janky Jenkins build Docker-PRs 38657 has succeeded
Details

@yongtang yongtang deleted the yongtang:28884-secret-inspect-follow-up branch Jan 27, 2017

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

Merge pull request moby#29218 from yongtang/28884-secret-inspect-foll…
…ow-up

Move secret name or ID prefix resolving from client to daemon
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment