fixes: no name check in docker container command

[lifubang]

Signed-off-by: Lifubang lifubang@acmcoder.com

- What I did
fix a bug when run docker container client command like this:
docker stop redis/../redisps

The container redisps will be stopped.
It's very strange.

I have fix rm, kill, and stop commands. If this solution is acceptable, I will fix other container commands.

- How I did it
add container check when run these container client cmd.
The regex in names.go is from https://github.com/moby/moby/blob/master/daemon/names/names.go
If we can refer it, it'll be better.

- How to verify it
Before fix:

root@dockerdemo:~/gocode/src/github.com/docker/cli# build/docker ps
CONTAINER ID        IMAGE                              COMMAND                  CREATED             STATUS              PORTS               NAMES
4ada0f1dc848        redis:testps                       "/st 6379"               6 hours ago         Up 20 minutes       6379/tcp            redisps
d5555192178f        docker.acmcoder.com/public/redis   "docker-entrypoint.s…"   22 hours ago        Up 2 seconds        6379/tcp            redis
root@dockerdemo:~/gocode/src/github.com/docker/cli# build/docker stop redis/../redisps
stopid:redis/../redisps
redis/../redisps
root@dockerdemo:~/gocode/src/github.com/docker/cli# build/docker ps
CONTAINER ID        IMAGE                              COMMAND                  CREATED             STATUS              PORTS               NAMES
d5555192178f        docker.acmcoder.com/public/redis   "docker-entrypoint.s…"   22 hours ago        Up 26 seconds       6379/tcp            redis
root@dockerdemo:~/gocode/src/github.com/docker/cli#

After fix:

root@dockerdemo:~/gocode/src/github.com/docker/cli# build/docker kill redis/../redisps
container name redis/../redisps is invalid

- Description for the changelog
Add container name check when run docker container client command.

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

[codecov-io]

Codecov Report

Merging #1331 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1331      +/-   ##
==========================================
- Coverage   54.82%   54.79%   -0.04%     
==========================================
  Files         292      293       +1     
  Lines       19267    19279      +12     
==========================================
  Hits        10564    10564              
- Misses       8044     8056      +12     
  Partials      659      659

[silvin-lubecki]

Thank you @lifubang for submitting this PR! 😄
I have some comments and questions, but I think it mainly lacks some tests.

[silvin-lubecki]

Why trimming the prefix? Is a container "/aaaa" valid? @thaJeztah WDYT?

[lifubang]

Yes, docker daemon said it's valid.
see https://github.com/moby/moby/blob/master/daemon/names.go#L59

[thaJeztah]

Yes, the leading slash is automatically added, and for historic reasons; a container is named /containera, and a container linked to that container (using the legacy --link option on the default network) gets an internal name /containera/containerb (or something along those lines)

[thaJeztah]

I don't think it's allowed to manually create a container with a slash though; they should be added automatically by the daemon where needed

[lifubang]

Thank you for your wonderful review. I have some code optimizations. Thanks.

[thaJeztah]

Thanks! I'm not sure if this is the right thing to do though; the general design is to defer validation to the daemon; make the daemon / API server responsible for checking which values are valid, and what values are invalid.

The docker CLI can be used against a different (newer/older) version of the daemon, which may accept different characters for "object" names.

In addition, this patch would only handle containers, but (see moby/moby#29126) the same could apply to any other object.

The original issue reported here is not actualy due to an invalid name, but because of the way this name is sent to the daemon; the cli sends redis/../redisps as part of the URL, and this gets expanded/rewritten to redisps.

Here's some older discussions/issues about this; moby/moby#29126 (suggested fix with URLencode; moby/moby#29131, and some related PR's; moby/moby#31831, and moby/moby#32127).

I think the correct fix should be to URLencode values before sending, however (I can't find that discussions, but can dig some more) there were concerns about changing that on the client, as it could result in an incompatible change if the daemon version does not handle this.

[lifubang]

@thaJeztah That's really right. Thanks for your review. There are so many things need to be considered. I think URLencode is a simple, easy, but maybe a perfect way to fix this issue. I will try and think more.

[thaJeztah]

Thanks! Main thing to look into if URLEncoding is used, is what happens daemon-side, i.e.; the concern is to break compatibility with older daemons; perhaps it's handled automatically (haven't checked), but perhaps not. If it's not handled automatically, URLEncode could still be possible, but in that case it should be versioned (API 1.xx: don't url-encode, API > 1.xx use url-encode)

[lifubang]

Please see moby/moby#37857
I think we can fix docker/docker/client library first,
After 37857, then to fix docker/cli ?