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.

Sign up for GitHub

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

Jump to bottom

image: add image layout validation #84

Merged
merged 1 commit into from
Jul 20, 2017
Merged

image: add image layout validation #84

merged 1 commit into from
Jul 20, 2017

Conversation

zhouhao3
Copy link

@zhouhao3 zhouhao3 commented Nov 15, 2016 •
edited
Loading

Increase the validation of the files contained in the layout.
Including blobs , oci-layout and index.json files.

Signed-off-by: zhouhao [email protected]

@zhouhao3 zhouhao3 force-pushed the test-layout branch 6 times, most recently from c9272dd to ce32d77 Compare November 15, 2016 06:16
@zhouhao3
Copy link
Author

@wking @xiekeyang @stevvooe @philips @coolljt0725 @vbatts PTAL

@xiekeyang
Copy link
Contributor

Autodetect is just distinguishing between types. In next steps(unpack or validation) objects will be found and determined. That can make process complete.
So this modification seems to be superfluous.

@coolljt0725
Copy link
Member

opencontainers/image-spec#459 has clarify that blobs and refs MUST exist, but for the oci-layout, the file name is not clear if it must be oci-layout at the moment.

@wking
Copy link
Contributor

wking commented Nov 17, 2016

I think the presence of oci-layout with a recognized version is enough to decide the target is attempting to be an image-layout. The presence of blobs and refs directories are part of determining whether that image-layout is valid/usable, but they probably don't need to be part of autodetection.

stevvooe
stevvooe reviewed Nov 18, 2016
View reviewed changes
image/autodetect.go Outdated
@@ -43,6 +50,11 @@ func Autodetect(path string) (string, error) {
}

if fi.IsDir() {
for _, file := range Layoutfile {
if _, err = os.Stat(path + file); os.IsNotExist(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You have to also make sure that each item is a directory or file (oci-layout must be a file).

Use filepath.Join here. Using + doesn't work correctly cross-platform.

stevvooe
stevvooe reviewed Nov 18, 2016
View reviewed changes
image/autodetect.go Outdated
@@ -43,6 +50,11 @@ func Autodetect(path string) (string, error) {
}

if fi.IsDir() {
for _, file := range Layoutfile {
if _, err = os.Stat(path + file); os.IsNotExist(err) {
return "", errors.New("unknown media type")
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this more specific.

stevvooe
stevvooe reviewed Nov 18, 2016
View reviewed changes
image/autodetect.go Outdated
@@ -34,6 +34,13 @@ const (
TypeConfig = "config"
)

// Layoutfile realize the image files that are included in the image layout type
var Layoutfile = []string{
"/blobs",
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for leading slash: use filepath.Join.

@zhouhao3 zhouhao3 force-pushed the test-layout branch 5 times, most recently from cb75f90 to 63b3fc4 Compare November 30, 2016 07:58
@Mashimiao
Copy link

If we just support image layout, image, manifest, manifestlist and image config, there is no need to add layout validation in autodetect. Because if the input is a directory, it should be considered as a image layout. Only image layout could be a directory in supported type validation
I think move the code in this PR into image.ValidateLayout() will be better.

vbatts
vbatts reviewed Feb 8, 2017
View reviewed changes
image/autodetect.go Outdated
var Layoutfile = []string{
"blobs",
"oci-layout",
"refs",
Copy link
Member

Choose a reason for hiding this comment

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

this is changed now. There is no longer a ./refs/ directory. And there is an index.json file. See the updated image-layout.md doc.

@zhouhao3 zhouhao3 force-pushed the test-layout branch 2 times, most recently from 8adac93 to 70d4a8e Compare February 17, 2017 06:36
@Mashimiao
Copy link

@q384566678 The Travis CI failed, I think you need to fix TestValidateLayout in image_test.go

@zhouhao3
Copy link
Author

The Travis CI failed, I think you need to fix TestValidateLayout in image_test.go

I'm doing, but there are some content to wait for image-spec certain.

@zhouhao3 zhouhao3 mentioned this pull request Jun 21, 2017
Merged
@zhouhao3 zhouhao3 force-pushed the test-layout branch 4 times, most recently from 3dfb290 to 148e4ba Compare June 26, 2017 02:47
@zhouhao3 zhouhao3 force-pushed the test-layout branch 2 times, most recently from 4b9bdae to fe62cfe Compare July 19, 2017 02:46
@zhouhao3 zhouhao3 changed the title autodetect.go: Increase the judgment condition image: add image layout validation Jul 19, 2017
@zhouhao3
Copy link
Author

Some methods in #50 are used to update the validation of layout.
@xiekeyang @coolljt0725 @cyphar @stevvooe @vbatts PTAL

@zhouhao3 zhouhao3 force-pushed the test-layout branch 3 times, most recently from a5f7c1c to 2773bda Compare July 19, 2017 06:21
@zhouhao3
Copy link
Author

reping @opencontainers/image-tools-maintainers I hope you can take a moment to review this PR, which is an important validation that is missing.

coolljt0725
coolljt0725 reviewed Jul 20, 2017
View reviewed changes
image/layout.go Outdated
"github.com/pkg/errors"
)

var blobsExist, indexExist, layoutExist bool
Copy link
Member

Choose a reason for hiding this comment

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

can we put these into layoutValidate?

Copy link
Author

Choose a reason for hiding this comment

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

ok, updated.

@coolljt0725
Copy link
Member

coolljt0725 commented Jul 20, 2017 •
edited by caniszczyk
Loading

LGTM tested, it works for me

Approved with PullApprove

@xiekeyang
Copy link
Contributor

xiekeyang commented Jul 20, 2017 •
edited by caniszczyk
Loading

LGTM

Approved with PullApprove

@xiekeyang xiekeyang merged commit dae3162 into opencontainers:master Jul 20, 2017
@zhouhao3 zhouhao3 deleted the test-layout branch July 20, 2017 06:28
@coolljt0725 coolljt0725 mentioned this pull request Jul 20, 2017
Closed
@xiekeyang xiekeyang mentioned this pull request Sep 8, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coolljt0725 coolljt0725 coolljt0725 left review comments

@stevvooe stevvooe stevvooe left review comments

@vbatts vbatts vbatts left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@zhouhao3 @xiekeyang @coolljt0725 @wking @Mashimiao @vbatts @stevvooe