Reference images by digest in docker driver

[HadrienPatte]

This is a new take on #166 with the same goal: reference images by digest in the docker driver, similarly to how it's currently done in the kubernetes driver.

[carolynvs]

@HadrienPatte I just wanted to catch up on the problem that you are fixing as I wasn't involved in the first go-around. Sounds like you'd like it so that when a CNAB tool uses the docker driver, and provides an image reference like this:

image: "example.com/myimg:v0.1.1"
digest: "sha256:abc123"

that you would like to see the driver interact with the image using the digest (when available), instead of the provided tag. So when the image is pulled, or the container created, it should use example.com/myimg@sha256:abc123.

Do I have that right?

[HadrienPatte]

@carolynvs That's exactly it! Do you have any feedbacks about the current approach? And do you foresee any potential issues with it?

[carolynvs]

I am not sure that this change would be 100% in alignment with the spec.

https://github.com/cnabio/cnab-spec/blob/main/101-bundle-json.md#invocation-images

Mostly because the runtime is supposed to use the digest to validate the pulled image. This would result in a change of behavior in the spec, where currently if they specified an invocation image that has been force pushed, the runtime would validate the digest and then fail to run the bundle. With this change, it would ignore the tag and pull using the digest and the bundle would pass.

To be clear, I'm not against this change at all, but this may not be something that we should implement in cnab-go until the question of "Should this be a spec change?" is answered. Personally I like having bundles always use digests and not allow for that error scenario above.

One way to accomplish your goal with how the spec works today, is for CNAB tools (like datadog or porter) to write out the bundle.json with the invocation image using a digest instead of a tag when the bundle is created. Porter actually does this for referenced images already but I just double checked and we don't for invocation images (but I'm totally going to add that now!). Did you try that already and find that doesn't work well?

So yeah, this is a good change, I'm just talking through how and where we should make the change. 😀

[HadrienPatte]

You're raising an interesting point. If this change is not 100% aligned with the CNAB spec as it is today, it means that the current kubernetes driver implementation is not spec compliant, as it already has this behavior?

Setting the invocation image image field to a fully digested ref in our tooling should indeed provide the same behavior when using the current cnab-go, but we'd like to have a deterministic behavior when running an installation of the same bundle with the kubernetes driver or the docker one.

[carolynvs]

Setting all image references to use a digest when you write out the bundle.json does yield deterministic bundle runs, right? I am really not trying to argue about changing the spec (happy to do it). I just want to make sure that if you need a fix now, you could use this workaround.

If you write out the bundle like this, where the image field uses the digest, then regardless of the driver or really the cnab tool, you are going to get consistent behavior at runtime.

{
    "description": "A very thorough test bundle",
    "invocationImages": [
        {
            "contentDigest": "sha256:5a527893243bc08e22c5a49484500d6319449ace2b52d7193d3c0a61f210e087",
            "image": "localhost:5000/whalegap-installer@sha256:5a527893243bc08e22c5a49484500d6319449ace2b52d7193d3c0a61f210e087",
            "imageType": "docker",
            "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
            "size": 2213
        }
    ],
    "name": "mybuns",
    "schemaVersion": "v1.0.0",
    "version": "0.1.2"
}

[carolynvs]

@HadrienPatte I was just checking in to see if the workaround I suggested makes sense? The CNAB General Meeting is tomorrow (https://cnab.io/community-meetings/) and we could discuss this more at the meeting if that would be helpful, especially if we want to try to clarify the spec.

[HadrienPatte]

Hi, thank you for suggesting this solution, I changed the way we generate bundle.json files to have a fully digested ref in the invocationImage.[0].image field, and with this we don't need the patch that this PR brings anymore.
I still think that using the provided contentDigest for more than just validation, and to enforce pulling and running an image with that digest would be a useful feature. If this requires a spec change, I'm happy to discuss this more with the CNAB community.

[carolynvs]

What is nice about your suggested change to the spec is that it prevents a bundle from getting into an "unrunnable state" which kind of defeats some of the purpose of making a bundle in the first place.

1. publish a bundle where the image reference uses a tag.
2. people use the bundle and it works. yay!
3. the developer makes a mistake and force pushes over the invocation image.
4. the same people who previously could run the bundle will now have the bundle fail because cnab-go will pull using the image tag, and then fail the digest comparison.

If we changed the spec to have images (both invocation and referenced images) always be by digest, then you couldn't get into this situation.

Next step would be to open an issue in https://github.com/cnabio/cnab-spec and propose your change to let others weigh in on it.

[HadrienPatte]

The proposed workaround works well for our use case, so I'm gonna close this PR as we don't need it anymore, thanks for the reviews :)