Chris,

Another thing I see is, it clobbers the existing image to <none>:<none> and also creates a new one.

Does this happen with you too ?

Shishir

Hrmmm, dunno, but that's very bad docker behaviour, and it very mean to clobber poor orphans, just sayin' :D  Are you sure it's clobbering them (replacing their ID) and not hiding the other ones (i.e. docker images --all)?

Chris, its not replacing the ID.

It just creates a new one, and clobbers the name of the existing {foobar} to <none>:<none>

Taking a look now.

Shishir

Oh ya, that's what I was reporting.  I'm not sure we can fault the behaviour itself, unless it's inconsistent with other tagging/naming operations.  The issue for me is more about avoiding surprises, making expectations and/or outcome clearer for users.  That could mean a documented rule of "docker always honours the last image naming request" and/or a warning message would be fine and appropriate as well.

Issue opened with Docker.
https://github.com/docker/docker/issues/11285

Working on the fix now.

Shishir

Chris,

Dan and I discussed a solution yesterday of throwing an error, if the incoming image name already matches to an existing name {foobar in this case}.
However naming the image part, {reading the repositories JSON} is at the very end i.e. the image layers would already have been loaded into /var/lib/docker/graph by that time, so it would be too late to throw an error.

I am working on an alternative solution. Here is the design.

If the user tries to load foobar:latest {ID: 5678} When a foobar:latest {ID: 1234} already exists, we throw a warning message 

Message: The image {e.g.foobar} you are trying to load has the same name to an existing image, but the IDs do not match. 

Changing the name from {e.g. foobar} to foobar_xxxx.

Here xxxx would be a randomly generated string.

In the end the original image would be foobar:latest and the new one would be 
foobar_xxxx:latest.

Does this work for you?

PS: I can read the JSON first and throw the error, but that would change the order of execution of how the `docker load` is working right now. I feel there must be a reason they are doing things in particular order. Load as compared to Import is different in the fact that it preserves all the metadeta while loading the tar ball.

Shishir

Shishir,

Yep, in general that sounds workable.  The only thing that makes me slightly nervous is renaming to a random name.  What happens if that name also already exists?

We ran into this problem with some docker autotest tests, and found we needed to verify the random name also doesn't exist.  However, in a situation where there are a LOT of images, the check can present a performance problem.

Though maybe it's just a corner-case, one way around it could be to skip the rename to random.  Instead just have the warning say it's leaving image ID 1234567 un-tagged (since the image ID is much less likely to clash than a random name).

Another idea is, back to the core of "limit surprises", maybe leave the existing behaviour, just make it part of the warning: "The image {e.g.foobar} you are trying to load has the same name to an existing image, but the IDs do not match.  Existing image with ID 1234567 has been un-tagged" (or something to that effect).

Just trying to think of ways so the solution doesn't add more complexity than the underlying causes :D

Chris,

Random name should be random. (Not security proof random :), but a good pseudo random)
That's where SEEDS come into picture. With a good dynamic seed, I should be able to create a random string.

If you see docker already uses this concept in their load functionality when they create a directory for untarring the tarball into /var/lib/docker/tmp.

They generate a directory at /var/lib/docker/tmp/docker-import-xxx where xxx is a random suffix generated by 
http://golang.org/pkg/io/ioutil/#TempDir

Shishir

(In reply to smahajan from comment #10)
> That's where SEEDS come into picture. With a good dynamic seed, I should be
> able to create a random string.

Seeds aren't the issue, it's the string-length that's the problem.  In other words, docker-import-xxx is only as safe as it's frequency of use versus the total number of random 'xxx' character combinations.  It's an ancient attack vector, to exploit this.  Even the standard libraries goes to extra lengths to guarantee random-name temp directories can't clash (see man 3 mkstemp it's quite incredible).  But...this is another problem, and for this issue it's probably a corner case.

One idea could be to "borrow" the assumed-uniqueness of the 12-character short ID as the "random" part.  e.g. Existing container foobar with id 123456789012 gets renamed to 'foobar_123456789012:latest'.  Then you're not responsible for any clashes because that ID already clashed :D

(looking at docs, Go's TempDir looks safe, similar to 'mkdtemp', but I'm not a Go authority).

Yes I agree its a corner case. 
But I think probability of having a collision would be quite quite rare.

Say for ex ... if my random generated ID is 10 letters.
N each letter is being picked up from a 52 character set {small letters (a-z), caps letters (A-Z)}.

I am looking at {52 C 10} = 52! / (10! * 42!) combinations  ... Which is a pretty large number to find a collision. What I m trying to say is, the whole point of randomness is, there is a very low probability of having a collision, to cover a corner case like this. 

I think we should not change the existing ID, my implementation was never touching the existing image name and tag. For the new one we can prefix the ID to the image name so image name becomes foobar_5678 in your example.

Again only if you are not convinced by my randomness hypothesis :)
otherwise we do my solution, and I get to play around with some cool stuff in goalng !

Shishir

Chris,

I promise it won't be something like this :))
https://www.xkcd.com/221/

Shishir

Can you scan the entire image before writing to the graph driver to look for conflicts and error out?  Or is this costly an operation?

(In reply to smahajan from comment #13)
> I am looking at {52 C 10} = 52! / (10! * 42!) combinations

You forgot order is important also, it's actually 10^52 but...disk space is cheap, especially for empty directories.  Even so, no reason hold up for a corner-case, we both agree it is such, and I'm sure there are more pressing issues needing attention.  I'm fine with whatever solution is lowest risk and easiest to maintain (including docs) :D

(In reply to Daniel Walsh from comment #15)
> Can you scan the entire image before writing to the graph driver to look for
> conflicts and error out?  Or is this costly an operation?

FWIW, this was expensive in docker autotest at the time (we were running a TON of iterations).  Here, the check is as expensive as the number of things to check (likely high) times the frequency of the checking (likely low), so not likely, but possibly bad.

In any case, back to the main point of this issue: It's not fundamentally technical in nature, though it could be made so.  Docker can keep doing what it does, I'm just saying let's not surprise users in cases of possible ambiguity.  Toss an error, warning, document it, change the behaviour, or all at once,  My motivation is trying to help avoid the support call: "Docker ate my image".

Closing the bugzilla in favor of PR
https://github.com/docker/docker/pull/11293

Shishir

Great, thanks for all your help on this.

Shishir it is not modified until it is in a package Chris can test.  A pull request is only good once it is merged.

oops ! my bad ... I need to check, might have a few more wrong MODIFIED then.

Shishir

Merged upstream. Will be available in the next docker release.

Shishir

(In reply to smahajan from comment #22)
> Merged upstream. Will be available in the next docker release.
> 
> Shishir

Hi Shishir,

I don't find the patch in graph/tags.go in docker-1.6.0-9.el7.x86_64, will it be included in 1.7 or rhel 1.6 version?

And an upstream test in  
#./docker -v
Docker version 1.7.0-dev, build 6cba310-dirty,

seems like,
#docker load < foobar.tar
doesn't trigger this (upstream graph/tags.go line 267)
if !force {
     return fmt.Errorf("Conflict: Tag %s is already set to image %s, if you want to replace it, please use -f option", tag, old)
			}

which "force" gets a True value through my debug.

Hi Luwen,

You are looking at the wrong code. This is an existing code in upstream.

This https://github.com/docker/docker/blob/master/graph/tags.go#L271-274 fixes our issue.

You should see a warning message: The image %s:%s already exists, renaming the old one with ID %s to empty string.

Let me know if you still don't see it.

Shishir

(In reply to smahajan from comment #24)
> Hi Luwen,
> 
> You are looking at the wrong code. This is an existing code in upstream.
> 
> This https://github.com/docker/docker/blob/master/graph/tags.go#L271-274
> fixes our issue.
> 
> You should see a warning message: The image %s:%s already exists, renaming
> the old one with ID %s to empty string.
> 
> Let me know if you still don't see it.
> 
> Shishir

Hi Shishir,

I saw the warning yesterday, just wondering if the `force` part will be useful :).
Okay, i am going to verified this once this patch goes into build. 
CC lsm to let him know.

Hi Lokesh,

I remember this patch is in the 1.7 branch, not the 1.6.
Also not found it in https://github.com/rhatdan/docker/blob/rhel7-1.6/graph/tags.go#L270, should we remove it from errata ?

Yes it should not be in errata.

FIxed in docker-1.7

dropped from advisory

In docker-1.7.1-107.el7.x86_64

# docker load < foobar.tar
The image foobar:latest already exists, renaming the old one with ID ace883dfcafa to empty string
# docker images
REPOSITORY                         TAG                 IMAGE ID            CREATED             VIRTUAL SIZE
<none>                             <none>              ace883dfcafa        20 seconds ago      158.2 MB

Move to verified

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHBA-2015-1536.html