appc / docker2aci

library and CLI tool to convert Docker images to ACIs (archived, see https://github.com/rkt/rkt/issues/4024)
Apache License 2.0
186 stars 60 forks source link

repository2: get the correct layer index #188

Closed iaguis closed 8 years ago

iaguis commented 8 years ago

In da56c93f we introduced an optimization to avoid going through all the layers every time we need the index of a specific one.

However, this resulted in incorrect Image Manifests being generated.

In f8e82fab we attempted to fix this. We assumed that the mistake was that the index we used was supposed to be reversed (from the end instead of from the beginning of the layer list). This assumption was wrong. and was probably motivated by the name of the map (reverseLayers).

The problem was that there can be several layers with the same BlobSum (layerID) but different manifest and when we populated the map, we were storing the last one for a given BlobSum. This is not what we were doing previously, instead we were returning the first match.

Fix this by storing the first appearance of a BlobSum. Also, change the name of reverseLayers to layersIndex.

iaguis commented 8 years ago

This is hard to test because it only happens with API v2.1 and not with v2.2 and I'm not really sure how to reproduce it. The only image that shows this behavior I know of is quay.io/stackanetes/stackanetes-memcached:barcelona

iaguis commented 8 years ago

@dgonyeo can you have a look?

Maybe you can even think of a way to test this...

cgonyeo commented 8 years ago

I've been wanting to add emulation of a v2.1 registry to the new golang tests I added (https://github.com/appc/docker2aci/commit/b173254c583b41d9d0bfc6f0bb6d84d9223c1e93), but this would take some time to implement, and I don't expect to have any spare time for at least a few weeks.

The changes would probably be:

iaguis commented 8 years ago

I think I'll merge this without a test for now then.