use rfc code in bundle validate #451

liangchenye · 2017-08-29T14:34:55Z

Signed-off-by: Liang Chenye [email protected]

liangchenye · 2017-08-29T14:37:43Z

This is a big change to bundle validation. It uses RFC error.
It helps to sort out "which spec item do we already follow'.

wking

Overall, I think this PR is a tremendous improvement, although I ran out of time before reviewing all the changes. Besides the issues I raised inline, golint has many concerns, which should all be easy to fix.

wking · 2017-08-29T16:28:37Z

validate/error.go

-			return fmt.Sprintf(referenceTemplate, version, "config-linux.md#default-filesystems"), nil
-		},
-	},
+	// NonRFCError represents that an error is not a rfc2119 error


I don't think we need to duplicate this comment here (you already have it on line 19).

wking · 2017-08-29T16:32:23Z

validate/error.go

+	// Root
+	RootOnNonHyperV: errorTemplate{Level: rfc2119.Required, Reference: rootRef},
+	RootOnHyperV:    errorTemplate{Level: rfc2119.Must, Reference: rootRef},
+	// TODO


Probably expand this comment to:

TODO: Add tests for these spec conditions

or some such.

wking · 2017-08-29T16:34:32Z

validate/error.go

+	PathFormatOnWindows: errorTemplate{Level: rfc2119.Must, Reference: rootRef},
+	PathName:            errorTemplate{Level: rfc2119.Should, Reference: rootRef},
+	PathExistence:       errorTemplate{Level: rfc2119.Must, Reference: rootRef},
+	ReadonlyFilesystem:  errorTemplate{Level: rfc2119.Must, Reference: rootRef},


I think you intend this for the spec's:

readonly (bool, OPTIONAL) If true then the root filesystem MUST be read-only inside the container, defaults to false.

But that is a runtime requirement which is tested by validation and runtimetest. It doesn't belong in validate, which is about config/bundle validation.

Yes, it is same with DefaultFilesystems. I prefer to put the error definition of runtime test and bundle test together. Maybe I should also move the validate/error.go file to the error directory?

wking · 2017-08-29T16:36:04Z

validate/error.go

+// FindError finds an error from a source error (mulitple error) and
+// returns the error code if founded.
+// If the source error is nil or empty, return NonErr.
+// If the source error is not a multiple error, return NonRFCErr.


NonErr → NonError and NonRFCErr → NonRFCError.

wking · 2017-08-29T16:41:51Z

validate/validate.go

+			NewError(PathExistence, fmt.Sprintf("The root path %q is not a directory.", rootfsPath), rspec.Version))
 	}

 	rootParent := filepath.Dir(absRootPath)


We will need a guard for the case where absRootPath was not initialized because the Abs call failed above. Although that issue predates this PR and can be handled in a separate PR if you like.

From the golang document, it seems only a terrible system failure will cause an Abs Error.
I think we can just return when we catch an Abs error. I'll add it in the following update.

wking · 2017-08-29T16:42:54Z

validate/validate.go

 	}
 	if version != rspec.Version {
-		msgs = append(msgs, fmt.Sprintf("internal error: validate currently only handles version %s, but the supplied configuration targets %s", rspec.Version, version))
+		errs = multierror.Append(errs, fmt.Errorf("internal error: validate currently only handles version %s, but the supplied configuration targets %s", rspec.Version, version))


This is clearly an internal error because it is a non-RFC error. I think we should drop “internal error: ” from the value.

wking · 2017-08-29T16:47:18Z

validate/validate.go

 	for _, hook := range hooks {
 		if !filepath.IsAbs(hook.Path) {
-			msgs = append(msgs, fmt.Sprintf("The %s hook %v: is not absolute path", hookType, hook.Path))
+			errs = multierror.Append(errs, fmt.Errorf("The %s hook %v: is not absolute path", hookType, hook.Path))


The MUST for this check is here. For poststart and poststop the requirement is indirect, via this and this. Perhaps we should expand Error.Reference to an array of strings so we can represent trails like that? Although if we're sticking to Markdown links, this is as specific as we can get at the moment, and it covers all the lines I linked earlier.

I opened an issue to track this: #452.

wking · 2017-08-29T16:49:03Z

validate/validate.go

 	process := v.spec.Process
 	if !filepath.IsAbs(process.Cwd) {
-		msgs = append(msgs, fmt.Sprintf("cwd %q is not an absolute path", process.Cwd))
+		errs = multierror.Append(errs, fmt.Errorf("cwd %q is not an absolute path", process.Cwd))


The MUST for this check is here.

This PR is quite big. I think we could change other errors in the following PRs.

wking · 2017-08-29T16:53:05Z

validate/validate.go

 	for _, env := range process.Env {
 		if !envValid(env) {
-			msgs = append(msgs, fmt.Sprintf("env %q should be in the form of 'key=value'. The left hand side must consist solely of letters, digits, and underscores '_'.", env))
+			errs = multierror.Append(errs, fmt.Errorf("env %q should be in the form of 'key=value'. The left hand side must consist solely of letters, digits, and underscores '_'.", env))


The spec punts to POSIX here, and POSIX has:

These strings have the form name=value; names shall not contain the character '='.

which I read as a MUST-level requirement. POSIX also has:

Environment variable names used by the utilities in the Shell and Utilities volume of POSIX.1-2008 consist solely of uppercase letters, digits, and the ( '_' ) from the characters defined in Portable Character Set and do not begin with a digit. Other characters may be permitted by an implementation; applications shall tolerate the presence of such names.

which I read as a SHOULD-level requirement for names, because OCI runtimes are not “used by the utilities in the Shell and Utilities volume of POSIX.1-2008”.

liangchenye · 2017-08-30T04:08:19Z

move validate/error.go to error/runtime_spec.go
move error/error.go to error/rfc2119.go
change msg []string to error in bundle validation checks
use rfc runtime spec error in CheckRoot and CheckVersion
update unittest and add assert pkg which is very useful
fix golint error and other errors reported by @wking

The next plan:

replace most previous errors (except inner error) to rfc runtime spec error
complete unittest

PTAL @wking @q384566678 @Mashimiao @mrunalp @hqhq

Signed-off-by: Liang Chenye <[email protected]>

Mashimiao · 2017-08-30T06:35:45Z

LGTM
I'm OK with such changes and look forward to following changes.

zhouhao3 · 2017-08-30T07:01:30Z

LGTM

…kage With 8f4d367 (error: Pull the RFC 2119 error representation into its own package, 2017-07-28, opencontainers#437), I'd created a package that was completely independent of runtime-spec. As I pointed out in that commit message, this made it possible for image-tools and other projects to reuse the generic RFC 2119 handling (which they care about) without involving the runtime-spec-specific error enumeration and such (which they don't care about). In 2520212 (add stretchr/testify/assert pkgs; use rfc code in bundle validation, 2017-08-29, opencontainers#451), some runtime-spec-specific functionality leaked into the error package. I'd recommended keeping configuration and runtime requirements separate [1], because you're unlikely to be testing both of those at once. But Liang wanted them collected [2,3]. And the NewError and FindError utilities would be the same regardless of target, so that's a good argument for keeping them together. This commit moves the runtime-spec-specific functionality into a new package where both config and runtime validators can share it, but where it won't pollute the generic RFC 2119 error package. I've also changed NewError to take an error argument instead of a string message, because creating an error from a string is easy (e.g. with fmt.Errorf(...)), and using an error allows you to preserve any additional structured information from a system error (e.g. as returned by GetMounts). [1]: opencontainers#451 (comment) [2]: opencontainers#451 (comment) [3]: opencontainers#451 (comment) Signed-off-by: W. Trevor King <[email protected]>

liangchenye force-pushed the runtimeenv branch from a1f3098 to c4acfbb Compare August 29, 2017 14:36

liangchenye force-pushed the runtimeenv branch from c4acfbb to af6946f Compare August 29, 2017 14:39

wking reviewed Aug 29, 2017

View reviewed changes

liangchenye mentioned this pull request Aug 30, 2017

Error Reference cannot tell the specific line on the spec #452

Open

liangchenye force-pushed the runtimeenv branch 3 times, most recently from 597bf92 to 7950d0b Compare August 30, 2017 03:37

liangchenye mentioned this pull request Aug 30, 2017

add 'Runtime' struct to make test easier; add create test #447

Merged

liangchenye force-pushed the runtimeenv branch 2 times, most recently from 7fd6597 to be46cca Compare August 30, 2017 03:57

liangchenye mentioned this pull request Aug 30, 2017

runtimespec VS ocitools #66

Closed

76 tasks

add stretchr/testify/assert pkgs; use rfc code in bundle validation

2520212

Signed-off-by: Liang Chenye <[email protected]>

liangchenye force-pushed the runtimeenv branch from be46cca to 2520212 Compare August 30, 2017 06:29

zhouhao3 merged commit 68ec7ee into opencontainers:master Aug 30, 2017

wking mentioned this pull request Aug 30, 2017

specerror: Pull runtime-spec-specific error handling into its own package #455

Merged

wking mentioned this pull request Aug 31, 2017

filepath: Add a stand-alone package for explicit-OS path logic #445

Merged

use rfc code in bundle validate #451

use rfc code in bundle validate #451

Uh oh!

Conversation

liangchenye commented Aug 29, 2017

Uh oh!

liangchenye commented Aug 29, 2017

Uh oh!

wking left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liangchenye commented Aug 30, 2017

Uh oh!

Mashimiao commented Aug 30, 2017 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhouhao3 commented Aug 30, 2017 • edited by caniszczyk 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!

4 participants

Mashimiao commented Aug 30, 2017 •

edited by caniszczyk

Loading

zhouhao3 commented Aug 30, 2017 •

edited by caniszczyk

Loading