conformance: skip check (by default) for zero layers in image manifest by rchincha · Pull Request #421 · opencontainers/distribution-spec

rchincha · 2023-05-25T19:03:42Z

As per image-spec, since this is a SHOULD (hence recommended) ...

https://raw.githubusercontent.com/opencontainers/image-spec/main/manifest.md

===

layers array of objects

Each item in the array MUST be a descriptor. For portability, layers SHOULD have at least one entry.

===

Fixes #410

sajayantony · 2023-05-25T20:15:58Z

I really hope to see zero layers come back as a valid manifest. Is it possible to keep these cases to see in the future which registries support these without failing conformance? This matrix should be automatable - opencontainers/image-spec#1025

rchincha · 2023-05-26T17:25:53Z

Perhaps, a way out could be to just enforce all MUSTs and warn on all SHOULDs, so we know without breaking the spirit of the conformance tests.

IINM, this particular test was enforcing that registries that don't accept zero layer manifests are not compliant, whereas the image-spec language has no such language. Also [this](https://github.com/opencontainers/image-spec/blob/main/schema/image-manifest-schema.json#LL30C1-L30C1} needs updating to 0 perhaps.

rchincha · 2023-05-26T19:13:44Z

A simpler variant of what we want to achieve.

sudo-bmitch

This needs another change to actually apply.

@sajayantony if we want add warnings, I'd be open to that as a separate PR.

conformance/setup.go

rchincha · 2023-06-07T17:41:51Z

Updated the PR with one more commit for a different option.

rchincha · 2023-06-08T22:05:42Z

@sudo-bmitch et al, the two commits can be squashed if we agree with the warning approach.

rchincha · 2023-06-17T05:08:51Z

Borrowed the Warn() from #375, also what appears to be a bugfix

jdolitsky · 2023-06-21T17:42:08Z

conformance/setup.go

+	fmt.Fprintf(colorable.NewColorableStderr(), "\n")
+	// print message
+	msgcolor := color.New(color.FgMagenta).FprintfFunc()
+	msgcolor(colorable.NewColorableStderr(), formatter.Fi(2, "\nWARNING: "+message))
+	fmt.Fprintf(colorable.NewColorableStderr(), "\n")
+	// print file:line
+	_, file, line, _ := runtime.Caller(1)
+	flcolor := color.New(color.FgWhite).FprintfFunc()
+	flcolor(colorable.NewColorableStderr(), formatter.Fi(2, "\n"))
+	flcolor(colorable.NewColorableStderr(), formatter.Fi(2, "%s:%d\n", file, line))


Wondering if we can simply do this warning with log/fmt package as not to bring in additional go mods?

We could. However, the colors give important visual feedback IMO.

https://github.com/onsi/ginkgo/blob/master/formatter/formatter_test.go#L51

https://github.com/onsi/ginkgo/blob/master/formatter/formatter.go#L60

^ let me try this instead?

••
WARNING: image manifest with no layers is not supported

/local/rchincha/go/src/github.com/opencontainers/distribution-spec/conformance/02_push_test.go:366 ••••••••

^ with the right colors still happens.

conformance/setup.go

jdolitsky · 2023-06-21T21:15:39Z

conformance/setup.go

+	// print message
+	fmt.Fprintf(os.Stderr, formatter.Fi(2, "\n{{magenta}}WARNING: %s\n{{/}}", message))
+	// print file:line
+	_, file, line, _ := runtime.Caller(1)
+	fmt.Fprintf(os.Stderr, formatter.Fi(2, "\n%s:%d\n", file, line))


some sort of linting issue on these

Error: setup.go:358:2: SA1006: printf-style function with dynamic format string and no further arguments should use print-style function instead (staticcheck) level=info msg="File cache stats: 1 entries of total size 12.4KiB" fmt.Fprintf(os.Stderr, formatter.Fi(2, "\n{{magenta}}WARNING: %s\n{{/}}", message)) ^ Error: setup.go:361:2: SA1006: printf-style function with dynamic format string and no further arguments should use print-style function instead (staticcheck) fmt.Fprintf(os.Stderr, formatter.Fi(2, "\n%s:%d\n", file, line)) ^ ```

As per image-spec, since this is a SHOULD (hence recommended) and now turned off by default. https://raw.githubusercontent.com/opencontainers/image-spec/main/manifest.md === layers array of objects Each item in the array MUST be a descriptor. For portability, layers SHOULD have at least one entry. === One can enable this test by setting env var: OCI_SKIP_EMPTY_LAYER_PUSH_TEST=0 Signed-off-by: Ramkumar Chinchani <rchincha@cisco.com>

emptyLayerManifest is being pushed but not deleted during teardown Signed-off-by: Ramkumar Chinchani <rchincha@cisco.com>

rchincha · 2023-06-21T21:20:28Z

zot/main passes with these changes

••••••••••••••••HTML report was created: /local/rchincha/go/src/github.com/opencontainers/distribution-spec/conformance/report.html
JUnit report was created: /local/rchincha/go/src/github.com/opencontainers/distribution-spec/conformance/junit.xml

Ran 63 of 68 Specs in 0.204 seconds
SUCCESS! -- 63 Passed | 0 Failed | 0 Pending | 5 Skipped

rchincha · 2023-06-21T21:22:51Z

zot/main instrumented to cause warn passes but with warning

+++ b/pkg/storage/common/common.go
@@ -94,6 +94,10 @@ func ValidateManifest(imgStore storageTypes.ImageStore, repo, reference, mediaTy
                                return "", zerr.ErrBadManifest
                        }
                }
+
+               if len(manifest.Layers) == 0 {
+                       return "", zerr.ErrBadManifest
+               }

••
WARNING: image manifest with no layers is not supported

/local/rchincha/go/src/github.com/opencontainers/distribution-spec/conformance/02_push_test.go:366                                                                    ••••••••

••••••••••••••••HTML report was created: /local/rchincha/go/src/github.com/opencontainers/distribution-spec/conformance/report.html
JUnit report was created: /local/rchincha/go/src/github.com/opencontainers/distribution-spec/conformance/junit.xml

Ran 63 of 68 Specs in 0.186 seconds
SUCCESS! -- 63 Passed | 0 Failed | 0 Pending | 5 Skipped

opencontainers#421) * conformance: always run but warn if empty layer manifest not supported As per image-spec, since this is a SHOULD (hence recommended) and now turned off by default. https://raw.githubusercontent.com/opencontainers/image-spec/main/manifest.md === layers array of objects Each item in the array MUST be a descriptor. For portability, layers SHOULD have at least one entry. === One can enable this test by setting env var: OCI_SKIP_EMPTY_LAYER_PUSH_TEST=0 Signed-off-by: Ramkumar Chinchani <rchincha@cisco.com> * conformance: delete the empty manifest in teardown emptyLayerManifest is being pushed but not deleted during teardown Signed-off-by: Ramkumar Chinchani <rchincha@cisco.com> --------- Signed-off-by: Ramkumar Chinchani <rchincha@cisco.com>

rchincha force-pushed the artifacts branch 2 times, most recently from bc70029 to 3752917 Compare May 25, 2023 19:06

rchincha changed the title ~~fix: remove check for zero layers~~ conformance: remove check for zero layers in image manifest May 25, 2023

rchincha force-pushed the artifacts branch 2 times, most recently from 028d12d to 7845d34 Compare May 26, 2023 19:16

rchincha marked this pull request as ready for review May 26, 2023 19:16

rchincha requested review from dmcgowan, jdolitsky, jonjohnsonjr, jzelinskie, mikebrow, stevvooe, sudo-bmitch and vbatts as code owners May 26, 2023 19:16

rchincha changed the title ~~conformance: remove check for zero layers in image manifest~~ conformance: skip check (by default) for zero layers in image manifest May 26, 2023

sudo-bmitch requested changes May 27, 2023

View reviewed changes

conformance/setup.go Outdated Show resolved Hide resolved

sudo-bmitch added this to the v1.1.0 milestone May 27, 2023

sudo-bmitch mentioned this pull request May 27, 2023

Conformance tests: delete emptylayer manifest too #307

Closed

rchincha force-pushed the artifacts branch 2 times, most recently from cf13c0b to b7be2d0 Compare May 31, 2023 18:17

rchincha requested a review from sudo-bmitch May 31, 2023 18:19

sudo-bmitch previously approved these changes Jun 1, 2023

View reviewed changes

rchincha dismissed sudo-bmitch’s stale review via fb8fae5 June 7, 2023 18:07

rchincha requested a review from sudo-bmitch June 7, 2023 21:44

rchincha force-pushed the artifacts branch from fb8fae5 to 05d66c7 Compare June 17, 2023 05:07

rchincha force-pushed the artifacts branch from 05d66c7 to 3431e8d Compare June 17, 2023 05:14

jdolitsky requested changes Jun 21, 2023

View reviewed changes

jdolitsky added the conformance label Jun 21, 2023

jdolitsky requested changes Jun 21, 2023

View reviewed changes

conformance/setup.go Show resolved Hide resolved

rchincha force-pushed the artifacts branch from f94cc29 to 10afab5 Compare June 21, 2023 20:56

jdolitsky reviewed Jun 21, 2023

View reviewed changes

conformance/setup.go Show resolved Hide resolved

rchincha force-pushed the artifacts branch from 10afab5 to edd6335 Compare June 21, 2023 21:12

jdolitsky previously approved these changes Jun 21, 2023

View reviewed changes

rchincha requested a review from jdolitsky June 21, 2023 21:13

jdolitsky reviewed Jun 21, 2023

View reviewed changes

Ramkumar Chinchani added 2 commits June 21, 2023 21:16

conformance: delete the empty manifest in teardown

1ea7ad4

emptyLayerManifest is being pushed but not deleted during teardown Signed-off-by: Ramkumar Chinchani <rchincha@cisco.com>

rchincha dismissed jdolitsky’s stale review via 1ea7ad4 June 21, 2023 21:17

rchincha force-pushed the artifacts branch from edd6335 to 1ea7ad4 Compare June 21, 2023 21:17

rchincha requested a review from jdolitsky June 21, 2023 21:20

jonjohnsonjr approved these changes Jun 21, 2023

View reviewed changes

jdolitsky approved these changes Jun 21, 2023

View reviewed changes

jdolitsky merged commit c76f05b into opencontainers:main Jun 21, 2023

jdolitsky mentioned this pull request Jun 27, 2023

tag 1.1.0-rc3 #433

Closed

8 tasks

jdolitsky mentioned this pull request Jul 6, 2023

tag 1.1.0-rc3 (part 2) #440

Merged

8 tasks

sudo-bmitch mentioned this pull request Feb 1, 2024

Release v1.1.0 #507

Merged

8 tasks

Conversation

rchincha commented May 25, 2023 • edited by sudo-bmitch Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sajayantony commented May 25, 2023

Uh oh!

rchincha commented May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchincha commented May 26, 2023

Uh oh!

sudo-bmitch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rchincha commented Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchincha commented Jun 8, 2023

Uh oh!

rchincha commented Jun 17, 2023

Uh oh!

jdolitsky Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

rchincha Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

rchincha Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jdolitsky Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

rchincha Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

rchincha commented Jun 21, 2023

Uh oh!

rchincha commented Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rchincha commented May 25, 2023 •

edited by sudo-bmitch

Loading

rchincha commented May 26, 2023 •

edited

Loading

rchincha commented Jun 7, 2023 •

edited

Loading

rchincha commented Jun 21, 2023 •

edited

Loading