Skip to content

Conversation

@zhouhao3
Copy link

I think we should return an error here, if it is to use WARING, then the validate function will return nil, and ultimately make the verification results are wrong
Signed-off-by: zhouhao [email protected]

@wking
Copy link
Contributor

wking commented Nov 14, 2016

On Sun, Nov 13, 2016 at 10:35:56PM -0800, Zhou Hao wrote:

I think we should return an error here…

Previous discussion in opencontainers/image-spec#287 (maybe moved to #15) and 1. I don't see
spec language requiring an image-layout to contain refs, and I'd
rather avoid a requirement like that. I'm fine with a flag to turn
warnings to errors in this case (--common-use-cases? ;), but I don't
think we want this to be part of formal compliance testing.

@zhouhao3
Copy link
Author

zhouhao@zz:~/image-tools$ ./oci-image-validate --type imageLayout ../busybox-b
WARNING: no descriptors found
../busybox-b:OK

If not change, we are in accordance with the above command to verify an empty folder, it will only display warning, but it will still be OK, this is wrong

@wking
Copy link
Contributor

wking commented Nov 15, 2016

On Mon, Nov 14, 2016 at 05:27:12PM -0800, Zhou Hao wrote:

zhouhao@zz:~/image-tools$ ./oci-image-validate --type imageLayout ../busybox-b
WARNING: no descriptors found
../busybox-b:OK

If not change, we are in accordance with the above command to verify
an empty folder, it will only display warning, but it will still be
OK, this is wrong

You can have warnings in a valid image (that's the difference between
warnings and errors). GCC has -Werror to turn all warnings into
errors, and I'm fine with something similar in image-tools. But
without a way to toggle the error-ness of these warnings, I don't
think we should be erroring out where there is no spec requirement
being violated.

@xiekeyang
Copy link
Contributor

I debug it. the Warning happens when oci layout lack refs descriptor.
And layout spec declare that

The image layout has two top level directories:
"blobs" contains content-addressable blobs. A blob has no schema and should be considered opaque.
"refs" contains descriptors. Commonly pointing to an image manifest or an image manifest list.

So I think it should return error instead warning in this case.

LGTM

@wking
Copy link
Contributor

wking commented Nov 16, 2016

On Tue, Nov 15, 2016 at 07:08:13PM -0800, xiekeyang wrote:

And layout spec declare that

The image layout has two top level directories:
"blobs" contains content-addressable blobs. A blob has no schema and should be considered opaque.
"refs" contains descriptors. Commonly pointing to an image manifest or an image manifest list.

So I think it should return error instead warning in this case.

Are you interpreting “contains descriptors” as “MUST contain at least
one descriptor”? I don't read it that way. If you do think there's
a case for that interpretation, it's probably worth making that change
via an image-spec PR, instead of adding strict image-tools tests based
on an ambiguous spec requirement.

@Mashimiao
Copy link

@xiekeyang If LGTM is not the beginning of comment, it doesn't make any sense. you need re-LGTM

@xiekeyang
Copy link
Contributor

@wking

Are you interpreting “contains descriptors” as “MUST contain at least
one descriptor”? ...

I think we are discussing if ref is necessary for OCI image. IMO, I thinks it is necessary, as the identifier. Right?

wking added a commit to wking/image-spec that referenced this pull request Nov 16, 2016
The previous wording ("has" and "contains") was not clear enough to
avoid confusion [1].  I consider this PR to be a spec clarification,
and not a spec change, but others will probably disagree [2] (which is
why I think we need the clarification).

If you cared about running images from the layout, you'd need "and
there MUST be at least one unpackable ref" language.  And then you
have to match the oci-layout version with the media types that were
unpackable when it was current (or is validity in the eye of the
validator?)... This is a bowl that I do not want to fathom ;).

Maybe folks are just using an image-layout to ship some missing blobs
(and have refs empty).  I don't see any incentive to image-authors to
publish ref-less blobs and then pretend they are runnable, so I don't
see a need to get into the business of restricting refs.

Or maybe they're shipping some missing refs (and have blobs empty).
Maybe they expect all blobs to be fetched via the descriptor's 'urls'.
Those sound fine to me too, so I don't think we should be in the
business of restricting blobs (and we already have "The blobs
directory MAY be missing referenced blobs...").

I am fine with validation code *warning* users about either case
(e.g. "this image-layout has no refs" or "refs a, b, and c require
blobs which are not stored in this image-layout"), but I don't think
the spec should block either of those.

[1]: opencontainers#287
[2]: opencontainers/image-tools#83 (comment)

Signed-off-by: W. Trevor King <[email protected]>
@wking
Copy link
Contributor

wking commented Nov 16, 2016

On Tue, Nov 15, 2016 at 10:51:37PM -0800, xiekeyang wrote:

I think we are discussing if ref is necessary for OCI image. IMO,
I thinks it is necessary, as the identifier. Right?

I've filed opencontainers/image-spec#459 to add clarity to the
image-layout requirements. If that looks good to you, maybe our
difference is what the image-layout is getting validated as. I've
been resisting errors here because I was thinking we were validating
it as an image-layout. Your “OCI image” comment suggests you are
validating it as an OCI Image 1. I agree that you need a ref,
manifest, and config, and (currently) at least one layer to be a valid
OCI Image.

When you give validation an explicit list of refs, it's fairly clear
you want those validated as OCI Images.

When you just give it an image-layout, it's not clear (to me) that you
are requiring it to contain an OCI Image. I'd expect you to validate
the image-layout, and then validate any refs that point to manifests
or manifest lists as OCI Images.

@xiekeyang
Copy link
Contributor

As most folks accept opencontainers/image-spec#459, this PR should be closed.

@xiekeyang xiekeyang closed this Nov 17, 2016
@wking
Copy link
Contributor

wking commented Nov 17, 2016

On Wed, Nov 16, 2016 at 06:32:32PM -0800, xiekeyang wrote:

As most folks accept opencontainers/image-spec#459, this PR should
be closed.

I think there may still be room for improved validation here if we can
figure out a UI that easily distinguishes between validating an
image-layout and requiring an OCI Image to be contained within that
image-layout 1. --has-at-least-one-oci-image is a bit heavy handed,
but maybe something along those lines would cover the use-case you
were getting at here?

@zhouhao3 zhouhao3 deleted the test-out branch December 6, 2016 06:22
dattgoswami9lk5g added a commit to dattgoswami9lk5g/bremlinr that referenced this pull request Jun 6, 2022
The previous wording ("has" and "contains") was not clear enough to
avoid confusion [1].  I consider this PR to be a spec clarification,
and not a spec change, but others will probably disagree [2] (which is
why I think we need the clarification).

If you cared about running images from the layout, you'd need "and
there MUST be at least one unpackable ref" language.  And then you
have to match the oci-layout version with the media types that were
unpackable when it was current (or is validity in the eye of the
validator?)... This is a bowl that I do not want to fathom ;).

Maybe folks are just using an image-layout to ship some missing blobs
(and have refs empty).  I don't see any incentive to image-authors to
publish ref-less blobs and then pretend they are runnable, so I don't
see a need to get into the business of restricting refs.

Or maybe they're shipping some missing refs (and have blobs empty).
Maybe they expect all blobs to be fetched via the descriptor's 'urls'.
Those sound fine to me too, so I don't think we should be in the
business of restricting blobs (and we already have "The blobs
directory MAY be missing referenced blobs...").

I am fine with validation code *warning* users about either case
(e.g. "this image-layout has no refs" or "refs a, b, and c require
blobs which are not stored in this image-layout"), but I don't think
the spec should block either of those.

[1]: opencontainers/image-spec#287
[2]: opencontainers/image-tools#83 (comment)

Signed-off-by: W. Trevor King <[email protected]>
7c00d pushed a commit to 7c00d/J1nHyeockKim that referenced this pull request Jun 6, 2022
The previous wording ("has" and "contains") was not clear enough to
avoid confusion [1].  I consider this PR to be a spec clarification,
and not a spec change, but others will probably disagree [2] (which is
why I think we need the clarification).

If you cared about running images from the layout, you'd need "and
there MUST be at least one unpackable ref" language.  And then you
have to match the oci-layout version with the media types that were
unpackable when it was current (or is validity in the eye of the
validator?)... This is a bowl that I do not want to fathom ;).

Maybe folks are just using an image-layout to ship some missing blobs
(and have refs empty).  I don't see any incentive to image-authors to
publish ref-less blobs and then pretend they are runnable, so I don't
see a need to get into the business of restricting refs.

Or maybe they're shipping some missing refs (and have blobs empty).
Maybe they expect all blobs to be fetched via the descriptor's 'urls'.
Those sound fine to me too, so I don't think we should be in the
business of restricting blobs (and we already have "The blobs
directory MAY be missing referenced blobs...").

I am fine with validation code *warning* users about either case
(e.g. "this image-layout has no refs" or "refs a, b, and c require
blobs which are not stored in this image-layout"), but I don't think
the spec should block either of those.

[1]: opencontainers/image-spec#287
[2]: opencontainers/image-tools#83 (comment)

Signed-off-by: W. Trevor King <[email protected]>
7c00d added a commit to 7c00d/J1nHyeockKim that referenced this pull request Jun 6, 2022
The previous wording ("has" and "contains") was not clear enough to
avoid confusion [1].  I consider this PR to be a spec clarification,
and not a spec change, but others will probably disagree [2] (which is
why I think we need the clarification).

If you cared about running images from the layout, you'd need "and
there MUST be at least one unpackable ref" language.  And then you
have to match the oci-layout version with the media types that were
unpackable when it was current (or is validity in the eye of the
validator?)... This is a bowl that I do not want to fathom ;).

Maybe folks are just using an image-layout to ship some missing blobs
(and have refs empty).  I don't see any incentive to image-authors to
publish ref-less blobs and then pretend they are runnable, so I don't
see a need to get into the business of restricting refs.

Or maybe they're shipping some missing refs (and have blobs empty).
Maybe they expect all blobs to be fetched via the descriptor's 'urls'.
Those sound fine to me too, so I don't think we should be in the
business of restricting blobs (and we already have "The blobs
directory MAY be missing referenced blobs...").

I am fine with validation code *warning* users about either case
(e.g. "this image-layout has no refs" or "refs a, b, and c require
blobs which are not stored in this image-layout"), but I don't think
the spec should block either of those.

[1]: opencontainers/image-spec#287
[2]: opencontainers/image-tools#83 (comment)

Signed-off-by: W. Trevor King <[email protected]>
laventuraw added a commit to laventuraw/Kihara-tony0 that referenced this pull request Jun 6, 2022
The previous wording ("has" and "contains") was not clear enough to
avoid confusion [1].  I consider this PR to be a spec clarification,
and not a spec change, but others will probably disagree [2] (which is
why I think we need the clarification).

If you cared about running images from the layout, you'd need "and
there MUST be at least one unpackable ref" language.  And then you
have to match the oci-layout version with the media types that were
unpackable when it was current (or is validity in the eye of the
validator?)... This is a bowl that I do not want to fathom ;).

Maybe folks are just using an image-layout to ship some missing blobs
(and have refs empty).  I don't see any incentive to image-authors to
publish ref-less blobs and then pretend they are runnable, so I don't
see a need to get into the business of restricting refs.

Or maybe they're shipping some missing refs (and have blobs empty).
Maybe they expect all blobs to be fetched via the descriptor's 'urls'.
Those sound fine to me too, so I don't think we should be in the
business of restricting blobs (and we already have "The blobs
directory MAY be missing referenced blobs...").

I am fine with validation code *warning* users about either case
(e.g. "this image-layout has no refs" or "refs a, b, and c require
blobs which are not stored in this image-layout"), but I don't think
the spec should block either of those.

[1]: opencontainers/image-spec#287
[2]: opencontainers/image-tools#83 (comment)

Signed-off-by: W. Trevor King <[email protected]>
tomalopbsr0tt added a commit to tomalopbsr0tt/fabiojosej that referenced this pull request Oct 6, 2022
The previous wording ("has" and "contains") was not clear enough to
avoid confusion [1].  I consider this PR to be a spec clarification,
and not a spec change, but others will probably disagree [2] (which is
why I think we need the clarification).

If you cared about running images from the layout, you'd need "and
there MUST be at least one unpackable ref" language.  And then you
have to match the oci-layout version with the media types that were
unpackable when it was current (or is validity in the eye of the
validator?)... This is a bowl that I do not want to fathom ;).

Maybe folks are just using an image-layout to ship some missing blobs
(and have refs empty).  I don't see any incentive to image-authors to
publish ref-less blobs and then pretend they are runnable, so I don't
see a need to get into the business of restricting refs.

Or maybe they're shipping some missing refs (and have blobs empty).
Maybe they expect all blobs to be fetched via the descriptor's 'urls'.
Those sound fine to me too, so I don't think we should be in the
business of restricting blobs (and we already have "The blobs
directory MAY be missing referenced blobs...").

I am fine with validation code *warning* users about either case
(e.g. "this image-layout has no refs" or "refs a, b, and c require
blobs which are not stored in this image-layout"), but I don't think
the spec should block either of those.

[1]: opencontainers/image-spec#287
[2]: opencontainers/image-tools#83 (comment)

Signed-off-by: W. Trevor King <[email protected]>
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.

4 participants