Skip to content
This repository has been archived by the owner on Aug 14, 2020. It is now read-only.

Manifest differ with same image #245

Open
gfyrag opened this issue Apr 23, 2017 · 10 comments
Open

Manifest differ with same image #245

gfyrag opened this issue Apr 23, 2017 · 10 comments

Comments

@gfyrag
Copy link

gfyrag commented Apr 23, 2017

When converting a remote image, i ended up with two differents manifest (Not all the times).
There is just labels which are not write with the same order ("os" and "arch" in my case).
The results is two aci files with differents signatures.

Using rocket, i discover this when fetching "latest" image multiple times.
This produce multiple images with the same name in the store but with sha256 hash differing.

I don't know if this can be enforced or if it can be considered as a bug...

@lucab
Copy link
Contributor

lucab commented Apr 23, 2017

golang maps are unordered collections. JSON as well does not guarantee any ordering for dict entries. If we need to maintain the hash-identity property on converted artifacts, we should switch to some canonical JSON form.

@euank
Copy link
Contributor

euank commented Apr 25, 2017

Over in OCI land, the image spec has a "SHOULD use canonical json" here.

I believe docker also uses that, so we can both use their library and act more like a typical docker client by doing so.

cc @dgonyeo

@lucab
Copy link
Contributor

lucab commented Apr 25, 2017

@euank thanks for the reference. Just FYI, I got dubious datapoints when looking at docker before.

But canonical/json seems fine, well contained and easy to drop-in. @gfyrag would you like to try tackling your issue by switching to the Canonical encoder provided by the above package?

@gfyrag
Copy link
Author

gfyrag commented Apr 25, 2017

@lucab Ok, i'm not familiar with the code but i'll give it a try this week end.

@lucab
Copy link
Contributor

lucab commented Apr 28, 2017

@gfyrag great! No hurry, take time to learn and make the process enjoyable, and feel free to ask.

Just for reference, this is how we typically add dependencies (i.e. via an helper script around glide).

And the package you are looking for should be around here (from a quick skim, but please double-check before starting).

gfyrag added a commit to omwave/docker2aci that referenced this issue Jun 12, 2017
Manifest was encoded using official encoding/json which does not canonical encode data.
Use library from github.com/docker/go/canonical/json instead.
gfyrag added a commit to omwave/docker2aci that referenced this issue Jun 12, 2017
Manifest was encoded using official encoding/json which does not canonical encode data.
Use library from github.com/docker/go/canonical/json instead.
@gfyrag
Copy link
Author

gfyrag commented Jun 12, 2017

@lucab Hello. Here the PR #247 (After a very long delay...)

I integrated canonical/json package. I just change the code where the manifest is write.
Maybe change other parts ?

Also, the glide-update script has updated some other dependencies automatically. Is it an expected behavior with glide (not familiar with this tool)?

@lucab
Copy link
Contributor

lucab commented Jun 12, 2017

Hi and great to see you coming back to this!

I integrated canonical/json package. I just change the code where the manifest is write.
Maybe change other parts ?

From a very quick look I think that should be enough. Empirically you should also see your random fetch-conversion skews addressed.

Also, the glide-update script has updated some other dependencies automatically. Is it an expected behavior with glide (not familiar with this tool)?

Yes, those are transitive dependencies that are not pinned by us or by our direct dependency. They thus have some degree of freedom.

@euank
Copy link
Contributor

euank commented Jun 12, 2017

It looks like we were jumping to conclusions a little.

I was still able to reproduce the issue with the above docker2aci fix. The bit that actually differs is:

@@ -4,16 +4,16 @@
 	"name": "registry-1.docker.io/euank/manylabels",
 	"labels": [
 		{
-			"name": "os",
-			"value": "linux"
-		},
-		{
 			"name": "arch",
 			"value": "amd64"
 		},
 		{
 			"name": "version",
 			"value": "busybox"
+		},
+		{
+			"name": "os",
+			"value": "linux"
 		}
 	],
 	"app": {

Notice that labels is actually a slice objects, not a map. Because it's a slice, json marshalling doesn't change the order, it just happened to already be wrong.

Where this actually happens is in LabelsFromMap which is called by docker2aci.

Just changing that function to sort the labels ends up with a consistent sha512 in my reproduction.

If you're curious, I'm reproducing this with this bash script in a docker2aci repo checkout.

I'll send a PR to to the appc repo shortly with what I think fixes this.

euank added a commit to euank/spec that referenced this issue Jun 12, 2017
Prior to this change, the slice of labels produced by LabelsFromMap
could have any arbitrary ordering.

See appc/docker2aci#245 for one of the issues
this causes in practice.

I also renamed the type 'labels' to 'labelsSlice' since, as it was, it
was being constantly shadowed.

This is not accompanied with any spec change since labels in the appc
spec are already a slice, and thus already have a consistent ordering.
It's unfortunate that a round trip of ToMap/FromMap won't be consistent,
but I don't think any callers actually do that currently.
@lucab
Copy link
Contributor

lucab commented Jul 4, 2017

Thanks for tracking down the underlying issue. I still believe your original comment about using canonical json helps in general, but I'm not sure if we still need it. #247 is still open, so we should either land it or close it. @euank what do you think?

@euank
Copy link
Contributor

euank commented Jul 6, 2017

@lucab Even though it turns out not to help in this specific case, canonical-json does feel more appropriate for how the manifest is used.

I don't have a strong preference

indradhanush pushed a commit to kinvolk-archives/appc-spec that referenced this issue Jan 24, 2018
Prior to this change, the slice of labels produced by LabelsFromMap
could have any arbitrary ordering.

See appc/docker2aci#245 for one of the issues
this causes in practice.

I also renamed the type 'labels' to 'labelsSlice' since, as it was, it
was being constantly shadowed.

This is not accompanied with any spec change since labels in the appc
spec are already a slice, and thus already have a consistent ordering.
It's unfortunate that a round trip of ToMap/FromMap won't be consistent,
but I don't think any callers actually do that currently.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants