add OCIError to differentiate 'MUST/SHOULD' and add reference support #354

liangchenye · 2017-03-30T16:40:48Z

this is an implementation of #353 .

wking · 2017-03-30T16:56:46Z

cmd/runtimetest/main.go

@@ -642,6 +644,12 @@ func validate(context *cli.Context) error {
 			err := v.test(spec)
 			t.Ok(err == nil, v.description)
 			if err != nil {
+				if logLevel >= logrus.ErrorLevel {
+					// Skip 'SHOULD', 'MAY' ...


I don't like overloading logrus.ErrorLevel to control this, since “SHOULD violations are fatal” is not the same as “I want to see debug logging”. Can we add a setting to the Validator structure to control this for config validation, and a command-line parameter to runtimetest to control this for runtime validation?

Yes, overloading logrus is a little bit tricky.
I'll use another flag like 'rfc-level'.

Maybe compliance-level would be better than rfc-level? There are many RFCs, and what we're interested in here is how well the tested-thing complies with the specification.

I'll update with all your good suggestions tomorrow. It is a little bit late here.

wking · 2017-03-30T17:00:00Z

validate/error.go

+	Err       error
+}
+
+const ReferencePrefix = "https://github.com/opencontainers/runtime-spec/blob/master/"


Instead of floating this with master, we should be using the ociVersion of the config we're checking against. This is another reason that we should only be supporting tagged spec releases.

The current version is: '-rc5-dev' and we don't have a matched release.
We can change it after v.1.0.0 release, for example: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config-linux.md

The current version is: '-rc5-dev'…

The current runtime-spec version supported by runtime-tools' master is -rc1-dev. If we make the adjustment I'm asking for in the in-flight #349, we can target -rc5. I see no purpose to targeting -rc5-dev.

wking · 2017-03-30T17:01:49Z

The OCIError structure looks reasonable to me. I think we should use it for MUST-level violations as well.

liangchenye · 2017-03-30T17:14:20Z

All the 'MUST-level' errors need to be filled. We can add all ErrorCode. It also helps to track #66.

liangchenye · 2017-03-31T13:45:50Z

Updated:
add a 'compliance-level' option to runtimetest.
add man page and complete bash

The tagged release and other 'MUST' error will be added in the following commits.

PTAL @wking @mrunalp @hqhq @Mashimiao

wking · 2017-03-31T17:57:03Z

validate/error.go

@@ -0,0 +1,69 @@
+package validate


How would you feel about pulling this out into its own package? I think image-spec and image-tools would also benefit from this approach to handling compliance levels?

I think image-tool could use the same OCIError struct. But OCIErrorCode can not be shared between runtime-tool and image-tool. I think we can keep it here.

But OCIErrorCode can not be shared between runtime-tool and image-tool. I think we can keep it here.

Yeah. A stand-alone module with all the tooling and a public code-> string map, and then the validating consumer populates that map (and spec URL) for whatever it's testing.

The runtime-tool and image-tool are separated, where should we have such a 'stand-alone module', "opencontainers/certification"?

The runtime-tool and image-tool are separated, where should we have such a 'stand-alone module', ...

I was thinking a separate sub-package in this repo. Or maybe in runtime-spec, since both image repos currently have some runtime-spec connections. It seems too tiny for its own repo, and opencontainers/certification seems more focused on policy than implementation.

wking · 2017-03-31T17:58:03Z

validate/error.go

+	Err       error
+}
+
+//FIXME: change to tagged spec releases


In order to support this future change, we probably want to include a SpecVersion field in the OCIError structure.

How about add a field (not in this PR) like this :
SpecVersions []string

So each OCI Error will be associate with one or more spec versions.
In runtimetest, we can add another option:
spec-version.
So if an OCI Error does not have the wanted spec-version, we can ignore it.

So each OCI Error will be associate with one or more spec versions.

I think it's better to only test against the spec for the target version. But if we're passing around that target version so we can decide what conditions to test, there's probably no need to stash it on Error. I'll withdraw my suggestion, and we can leave the struct fields alone.

With #349 landed we can fix this FIXME.

wking · 2017-03-31T17:58:54Z

validate/error.go

+)
+
+// OCIError represents an error with compliance level and OCI reference
+type OCIError struct {


This type is already OCI-namespaced via the package path (github.com/opencontainers/runtime-tools/validate). I think we can just go with Error as the struct name.

wking · 2017-03-31T18:07:16Z

validate/error.go

+	case "SHOULD":
+		return ComplianceShould
+	case "MUST":
+		return ComplianceMust


This doesn't seem quite right. RFC 2119 has the following levels:

MUST-level (MUST, MUST NOT, REQUIRED, SHALL, SHALL NOT).

SHOULD-level (SHOULD, SHOULD NOT, RECOMMENDED, NOT RECOMMENDED).

MAY-level (MAY, OPTIONAL).

You've listed yours in the inverse order from the RFC (and left off NOT RECOMMENDED?), and that means that your ComplianceMustNot is less than your ComplianceMust, even though both should be in the MUST-level. I recommend either:

a. Reorder your constants to put ComplianceShould at the bottom of the SHOULD-level constants and ComplianceMust at the bottom of the MUST-level constants.
b. Add a function that maps the constants to their level (e.g. ComplianceLevel(ComplianceShallNot) → ComplianceMust) and use that when deciding whether errors are fatal.
c. Adjust this case here to select whatever the lowest constant is for that level (e.g. "MUST" → ComplianceShall).

updated with solution C.

Mashimiao · 2017-04-11T05:23:01Z

LGTM

liangchenye · 2017-04-11T13:33:45Z

@hqhq @mrunalp PTAL

wking · 2017-04-11T16:32:40Z

completions/bash/oci-runtime-tool

+__oci-runtime-tool_complete_compliance_level() {
+	COMPREPLY=( $( compgen -W "
+		must
+		should


Missing may (which is documented here). Although I'm not sure what MAY-level validation looks like, and would be fine dropping it from --compliance-level.

Lost 'may' in previous PR. We have 'MAY' in spec. In this case, we 'may' print 'MAY' information when platform.os is linux but the linux item is not set.

In this case, we 'may' print 'MAY' information when platform.os is linux but the linux item is not set.

So would this be all unset OPTIONAL child properties? Or more case-by case at the runtime-tools maintainers descretion? Either was, a fatal level seems odd. If something deserves occasional fatal warnings, it should probably be SHOULD in the spec.

wking · 2017-04-11T16:43:56Z

validate/error.go

+	Err       error
+}
+
+//FIXME: change to tagged spec releases


With #349 landed we can fix this FIXME.

wking · 2017-04-11T16:47:10Z

cmd/runtimetest/main.go

+
+	for fs, fstype := range defaultFS {
+		if !(mountsMap[fs] == fstype) {
+			return ociErr.NewError(ociErr.DefaultFilesystems, fmt.Sprintf("%v must exist and expected type is %v", fs, fstype))


“must” → “SHOULD”.

wking · 2017-04-11T16:50:08Z

cmd/runtimetest/main.go

 	var validationErrors error
 	for _, v := range defaultValidations {
 		err := v.test(spec)
 		t.Ok(err == nil, v.description)
 		if err != nil {
+			if e, ok := err.(*ociErr.Error); ok && e.Level < complianceLevel {
+				continue


We may want to print these as warnings even if they are non-fatal. With TAP, I'd recommend using diagnostics. I'm not sure what the best approach is with Go's testing framework, but we should probably figure that out ;).

I opened #362 to keep track of this.

wking · 2017-04-11T16:51:22Z

validate/error.go

+	case "REQUIRED":
+		return ComplianceMust
+	default:
+		return ComplianceMust


I think the default should be to raise an “unrecognized level” error. Silently defaulting to MUST seems confusing.

you are right. I should raise error here.

wking · 2017-04-11T16:54:41Z

validate/error.go

+// NewError creates an Error by ErrorCode and message
+func NewError(code ErrorCode, msg string) error {
+	err := ociErrors[code]
+	err.Err = errors.New(msg)


Is this making a new Error? It looks like it's attaching a new error to a pre-existing Error (possibly clobbering a previous error). Or maybe we will only ever report one error for a given ErrorCode and we want that clobbering? Or maybe I'm misreading this and we are creating a new Error ;).

I'm creating a new Error :) So the OCIError could be used as a normal Error.

liangchenye · 2017-04-12T08:27:23Z

Updated by

add 'may' in completion file
reference with 'released' version
return error when ParseLevel with unrecognized string

PTAL @wking @Mashimiao @mrunalp @hqhq

Signed-off-by: liangchenye <[email protected]>

liangchenye · 2017-07-06T08:05:14Z

rebased and fixed the glint error. PTAL @Mashimiao @hqhq @mrunalp

Mashimiao · 2017-07-07T05:26:16Z

LGTM

liangchenye · 2017-07-14T07:23:59Z

@hqhq @mrunalp PTAL

liangchenye · 2017-07-28T03:46:58Z

@q384566678 PTAL

zhouhao3 · 2017-07-28T05:25:56Z

cmd/runtimetest/main.go

@@ -30,6 +32,13 @@ const PrGetNoNewPrivs = 39
 const specConfig = "config.json"

 var (
+	defaultFS = map[string]string{
+		"/proc":    "proc",


proc -> procfs

proc here is right, there is no procfs such type in Linux

https://github.com/opencontainers/runtime-spec/blame/master/config-linux.md#L15
In the spec is defined.

we call it procfs but the real type in Linux is proc. I will submit a PR to fix it in runtime-spec

zhouhao3 · 2017-07-28T05:56:39Z

LGTM

Mashimiao · 2017-07-28T06:27:26Z

LGTM

As discussed in [1]. This makes it easier for other projects (e.g. image-tools) to use the same tooling if they want. Some components of the old validate/error.go were runtime-spec-specific (e.g. the reference template and ociErrors), so they've stayed in the validate package. I've also expanded NewError to take an explicit version (as requested in [2]). That allows us to link to the proper spec even if we're capable of validating several spec versions (e.g. 1.0 and 1.1 configurations or runtimes). [1]: opencontainers#354 (comment) [2]: opencontainers#354 (comment) Signed-off-by: W. Trevor King <[email protected]>

The man-page entry and Bash completions for this option were added in 4029999 (oci error: add error level and reference, 2017-03-31, opencontainers#354), but the option was left unimplemented. The implementation in this commit is very similar to the existing runtimetest implementation from 4029999, except we warn instead of silently skipping non-fatal spec violations. The warning (vs. error) on invalid level arguments follows the runtimetest implementation from 6316a4e (use released version as reference; improve Parse error, 2017-04-12, opencontainers#354). I'd personally prefer hard errors on invalid level arguments, but didn't want to break runtime-tools consistency over that. Signed-off-by: W. Trevor King <[email protected]>

wking reviewed Mar 30, 2017

View reviewed changes

liangchenye force-pushed the master branch from 2541823 to cc26e8c Compare March 31, 2017 13:41

wking reviewed Mar 31, 2017

View reviewed changes

Mashimiao modified the milestone: 0.6 Apr 11, 2017

wking reviewed Apr 11, 2017

View reviewed changes

liangchenye force-pushed the master branch from bfd87c8 to 68c9de2 Compare April 12, 2017 07:46

liangchenye mentioned this pull request Apr 12, 2017

Way to tell error messages with different compliance levels. #362

Closed

liangchenye force-pushed the master branch from 68c9de2 to 70d8c60 Compare April 12, 2017 09:46

wking mentioned this pull request May 11, 2017

Indicate Yes, No, AND not-implemented opencontainers/oci-conformance#32

Closed

This was referenced May 19, 2017

schema: add checkPlatform opencontainers/image-spec#491

Merged

Fix the schema validation API opencontainers/image-spec#686

Closed

liangchenye added 3 commits July 6, 2017 15:25

oci error: add error level and reference

4029999

Signed-off-by: liangchenye <[email protected]>

add defaultFS and use OCIError

6b1d25d

Signed-off-by: liangchenye <[email protected]>

sort compliance level according to RFC2119

1259e3e

Signed-off-by: liangchenye <[email protected]>

liangchenye force-pushed the master branch 3 times, most recently from 63a13b3 to ccf57b5 Compare July 6, 2017 07:54

use released version as reference; improve Parse error

6316a4e

Signed-off-by: liangchenye <[email protected]>

liangchenye force-pushed the master branch from ccf57b5 to 6316a4e Compare July 6, 2017 08:02

zhouhao3 reviewed Jul 28, 2017

View reviewed changes

Mashimiao merged commit 3a21ae6 into opencontainers:master Jul 28, 2017

wking mentioned this pull request Jul 28, 2017

error: Pull the RFC 2119 error representation into its own package #437

Merged

This was referenced Jul 28, 2017

Reference spec when reporting violations #16

Closed

validate: enhance linux devices validation #297

Merged

Mashimiao removed this from the v0.6.0 milestone Sep 15, 2017

wking mentioned this pull request Oct 2, 2017

cmd/oci-runtime-tool: Implement --compliance-level #492

Merged

add OCIError to differentiate 'MUST/SHOULD' and add reference support #354

add OCIError to differentiate 'MUST/SHOULD' and add reference support #354

Conversation

liangchenye commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Mar 30, 2017 via email

liangchenye commented Mar 30, 2017

liangchenye commented Mar 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mashimiao commented Apr 11, 2017 • edited by caniszczyk Loading

liangchenye commented Apr 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liangchenye commented Apr 12, 2017

liangchenye commented Jul 6, 2017

Mashimiao commented Jul 7, 2017 • edited by caniszczyk Loading

liangchenye commented Jul 14, 2017

liangchenye commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouhao3 commented Jul 28, 2017 • edited by caniszczyk Loading

Mashimiao commented Jul 28, 2017 • edited by caniszczyk Loading

Mashimiao commented Apr 11, 2017 •

edited by caniszczyk

Loading

Mashimiao commented Jul 7, 2017 •

edited by caniszczyk

Loading

zhouhao3 commented Jul 28, 2017 •

edited by caniszczyk

Loading

Mashimiao commented Jul 28, 2017 •

edited by caniszczyk

Loading