Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(101-bundle-json.md): add clarification around contentDigest value #384

Merged
merged 3 commits into from
Sep 17, 2020

Conversation

vdice
Copy link
Member

@vdice vdice commented Aug 20, 2020

  • Clarifies which type of digest must be used for the contentDigest field of an invocation or bundle image

Proposed fix for #287

Signed-off-by: Vaughn Dice <vadice@microsoft.com>
101-bundle-json.md Outdated Show resolved Hide resolved
101-bundle-json.md Outdated Show resolved Hide resolved
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
Copy link
Contributor

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@SteveLasker
Copy link

Curious why the digest is required to be consistent with the tag.
While deploying unique tags is a best practice to have consistent deployments across a scale unit, locking to the digest blocks the "break glass" servicing capability. I get that some use docker-lock type rules until we have a better signing, or standard tag locking capability. Just suggesting this shouldn't be a hard requirement.

As a minor point, some of the references refer to "image":"example/microservice:1.2.3"
This is a bit confusing. Is this the default docker hub behavior where no registry = docker.io? Or, does the bundle support finding the image in the registry where the CNAB bundle was pulled from?
There's some later examples of: "image": "example.com/example/vote-frontend@sha256:aca460afa270d4c527981ef9ca4989346c56cf9b20217dcea37df1ece8120685",
Would suggest fully qualified names, if that's what it means, or better yet, support reference artifacts to the registry where the CNAB was pulled enabling better transportability.

For the image reference, the spec states:

The following OPTIONAL fields MAY be attached to an invocation image:
size: The image size in bytes. Implementations SHOULD verify this when a bundle is packaged as a thick bundle, and MAY verify it when the image is part of a thin bundle.

Would suggest requiring the full OCI Descriptor information, where digest, mediaType and size are required.

Copy link
Contributor

@chris-crone chris-crone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This is a clarification but it would, in theory, break runtimes that use a different contentDigest definition so I believe that rules this out as a simple errata. We'll need to bump the spec version.

@vdice
Copy link
Member Author

vdice commented Sep 16, 2020

@SteveLasker good points, all. Please feel free to create follow-up issues for spec amendments/proposals mentioned. However, the scope of this changeset is simply to clarify the expected value of the contentDigest field when the invocation image type is docker/oci.

Copy link
Member

@trishankatdatadog trishankatdatadog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, interesting. Would this break Signy's verification behaviour, @radu-matei? I suppose not unless you consider copying images between different registries?

Copy link
Member

@technosophos technosophos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Much clearer and precise this way.

@chris-crone
Copy link
Contributor

Hmm, interesting. Would this break Signy's verification behaviour, @radu-matei? I suppose not unless you consider copying images between different registries?

I remember having discussions about this with @radu-matei so I think he likely implemented Signy with this definition in mind

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@vdice vdice merged commit 59f80b0 into cnabio:master Sep 17, 2020
@vdice vdice deleted the content-digest-clarification branch September 17, 2020 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants