Reference images by digest in docker driver

[jlegrone]

This builds on the work in #114 by supporting the same functionality for the docker driver.

[glyn]

I think it would be better to use a full SHA 256 hash to avoid any confusion. Maybe just pad it with zeros?

[glyn]

Approved with a small suggestion.

[silvin-lubecki]

I think returning false here is not accurate, as i.Image can be a digested reference itself. We should test if the image name is a digested reference here and return the result.

[jlegrone]

I'm on the fence about this. The spec states that

If the contentDigest is not present, the runtime SHOULD report an error so the user is aware that there is no contentDigest provided.

If an image contains what we interpret to be a digest, then some runtimes could certainly opt to use this in lieu of an explicit contentDigest. But having the boolean correspond to whether contentDigest has actually been set seems most in line with the spec as written.

[silvin-lubecki]

We're missing a case here:

"digested reference": {
    expected: "foo@sha256:7ea254518cab82f53938523eab0dc6601a3dd1edf700f1f6235e66e27cbc7986
    hasDigest: true,
    image: BaseImage{
        Image: "foo@sha256:7ea254518cab82f53938523eab0dc6601a3dd1edf700f1f6235e66e27cbc7986",
    },
},

[silvin-lubecki]

Regarding @glyn comment on this isssue #145 (comment) I don't think we should use the contentDigest field as it is proposed in this PR.

Regardless of which definition we choose, it seems clear that we can't simply append the contentDigest to the end of the image field to create a digested image reference because the image field may already be digested.

@glyn WDYT?

[glyn]

Regarding @glyn comment on this isssue #145 (comment) I don't think we should use the contentDigest field as it is proposed in this PR.

Regardless of which definition we choose, it seems clear that we can't simply append the contentDigest to the end of the image field to create a digested image reference because the image field may already be digested.

@glyn WDYT?

Of course, I agree with my own comment. But are you saying that the current PR proposes a particular interpretation of contentDigest?

[silvin-lubecki]

Of course, I agree with my own comment. But are you saying that the current PR proposes a particular interpretation of contentDigest?

Yes I think here https://github.com/cnabio/cnab-go/pull/166/files#diff-187c9d266554ba3ff27d602db2a12221R105

return fmt.Sprintf("%s@%s", i.Image, i.Digest), true

i.Digest is the contentDigest (this field is unfortunately badly named). I guess it is exactly what your are referring to in your previous comment we can't simply append the contentDigest to the end of the image field to create a digested image reference. Or am I totally wrong? 🤔

[glyn]

That's correct, but even if the image field is not digested, it only makes sense to add the contentDigest on the end if contentDigest is a repo digest (and not an image id). We won't know for sure which it is until cnabio/cnab-spec#287 has been decided.

[radu-matei]

/brig run

[silvin-lubecki]

Agreed, that's why I think we should put that PR on hold.

[vdice]

Attempt to clarify the spec (address cnabio/cnab-spec#287) has been issued in cnabio/cnab-spec#384.

Meanwhile, this PR looks great to me. It looks like there are some unit test suggestions remaining and a rebase is needed. These changes will come in handy for digest validation in #227.

[vdice]

cnabio/cnab-spec#384 has been merged.

@jlegrone any chance this branch can be rebased when convenient?

[carolynvs]

@jlegrone I'm closing this PR since it's been 2 years and has drifted. It sounds like from #269 that DataDog may not need this change anymore? If you want to resubmit this PR, rebased on the latest release, I'm happy to help get that pushed through.