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

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]>

Author: wking

cmd/runtimetest/main.go

@@ -22,7 +22,8 @@ import (
 	"github.com/urfave/cli"
 
 	"github.com/opencontainers/runtime-tools/cmd/runtimetest/mount"
-	rerr "github.com/opencontainers/runtime-tools/error"
+	rfc2119 "github.com/opencontainers/runtime-tools/error"
+	"github.com/opencontainers/runtime-tools/specerror"
 )
 
 // PrGetNoNewPrivs isn't exposed in Golang so we define it ourselves copying the value from
@@ -313,7 +314,7 @@ func validateRootFS(spec *rspec.Spec) error {
 	if spec.Root.Readonly {
 		err := testWriteAccess("/")
 		if err == nil {
-			return rerr.NewError(rerr.ReadonlyFilesystem, "Rootfs should be readonly", rspec.Version)
+			return specerror.NewError(specerror.ReadonlyFilesystem, fmt.Errorf("rootfs must be readonly"), rspec.Version)
 		}
 	}
 
@@ -325,7 +326,7 @@ func validateDefaultFS(spec *rspec.Spec) error {
 
 	mountInfos, err := mount.GetMounts()
 	if err != nil {
-		rerr.NewError(rerr.DefaultFilesystems, err.Error(), spec.Version)
+		specerror.NewError(specerror.DefaultFilesystems, err, spec.Version)
 	}
 
 	mountsMap := make(map[string]string)
@@ -335,7 +336,7 @@ func validateDefaultFS(spec *rspec.Spec) error {
 
 	for fs, fstype := range defaultFS {
 		if !(mountsMap[fs] == fstype) {
-			return rerr.NewError(rerr.DefaultFilesystems, fmt.Sprintf("%v SHOULD exist and expected type is %v", fs, fstype), rspec.Version)
+			return specerror.NewError(specerror.DefaultFilesystems, fmt.Errorf("%v SHOULD exist and expected type is %v", fs, fstype), rspec.Version)
 		}
 	}
 
@@ -779,17 +780,17 @@ func run(context *cli.Context) error {
 	t.Header(0)
 
 	complianceLevelString := context.String("compliance-level")
-	complianceLevel, err := rerr.ParseLevel(complianceLevelString)
+	complianceLevel, err := rfc2119.ParseLevel(complianceLevelString)
 	if err != nil {
-		complianceLevel = rerr.Must
+		complianceLevel = rfc2119.Must
 		logrus.Warningf("%s, using 'MUST' by default.", err.Error())
 	}
 	var validationErrors error
 	for _, v := range defaultValidations {
 		err := v.test(spec)
 		t.Ok(err == nil, v.description)
 		if err != nil {
-			if e, ok := err.(*rerr.Error); ok && e.Level < complianceLevel {
+			if e, ok := err.(*specerror.Error); ok && e.Err.Level < complianceLevel {
 				continue
 			}
 			validationErrors = multierror.Append(validationErrors, err)
@@ -801,7 +802,7 @@ func run(context *cli.Context) error {
 			err := v.test(spec)
 			t.Ok(err == nil, v.description)
 			if err != nil {
-				if e, ok := err.(*rerr.Error); ok && e.Level < complianceLevel {
+				if e, ok := err.(*specerror.Error); ok && e.Err.Level < complianceLevel {
 					continue
 				}
 				validationErrors = multierror.Append(validationErrors, err)

error/rfc2199.go error/error.goerror/rfc2199.go renamed to error/error.go

@@ -48,7 +48,6 @@ type Error struct {
 	Level     Level
 	Reference string
 	Err       error
-	ErrCode   int
 }
 
 // ParseLevel takes a string level and returns the OCI compliance level constant.

error/runtime_spec.go specerror/error.goerror/runtime_spec.go renamed to specerror/error.go

@@ -1,15 +1,18 @@
-package error
+// Package specerror implements runtime-spec-specific tooling for
+// tracking RFC 2119 violations.
+package specerror
 
 import (
-	"errors"
 	"fmt"
 
 	"github.com/hashicorp/go-multierror"
+	rfc2119 "github.com/opencontainers/runtime-tools/error"
 )
 
 const referenceTemplate = "https://github.com/opencontainers/runtime-spec/blob/v%s/%s"
 
-// SpecErrorCode represents the compliance content.
+// SpecErrorCode represents the spec violation, enumerating both
+// configuration violations and runtime violations.
 type SpecErrorCode int
 
 const (
@@ -53,10 +56,19 @@ const (
 )
 
 type errorTemplate struct {
-	Level     Level
+	Level     rfc2119.Level
 	Reference func(version string) (reference string, err error)
 }
 
+// Error represents a runtime-spec violation.
+type Error struct {
+	// Err holds the RFC 2119 violation.
+	Err rfc2119.Error
+
+	// ErrCode is a matchable holds a SpecErrorCode
+	ErrCode SpecErrorCode
+}
+
 var (
 	containerFormatRef = func(version string) (reference string, err error) {
 		return fmt.Sprintf(referenceTemplate, version, "bundle.md#container-format"), nil
@@ -78,56 +90,63 @@ var (
 var ociErrors = map[SpecErrorCode]errorTemplate{
 	// Bundle.md
 	// Container Format
-	ConfigFileExistence:  {Level: Must, Reference: containerFormatRef},
-	ArtifactsInSingleDir: {Level: Must, Reference: containerFormatRef},
+	ConfigFileExistence:  {Level: rfc2119.Must, Reference: containerFormatRef},
+	ArtifactsInSingleDir: {Level: rfc2119.Must, Reference: containerFormatRef},
 
 	// Config.md
 	// Specification Version
-	SpecVersion: {Level: Must, Reference: specVersionRef},
+	SpecVersion: {Level: rfc2119.Must, Reference: specVersionRef},
 	// Root
-	RootOnNonHyperV: {Level: Required, Reference: rootRef},
-	RootOnHyperV:    {Level: Must, Reference: rootRef},
+	RootOnNonHyperV: {Level: rfc2119.Required, Reference: rootRef},
+	RootOnHyperV:    {Level: rfc2119.Must, Reference: rootRef},
 	// TODO: add tests for 'PathFormatOnWindows'
-	PathFormatOnWindows: {Level: Must, Reference: rootRef},
-	PathName:            {Level: Should, Reference: rootRef},
-	PathExistence:       {Level: Must, Reference: rootRef},
-	ReadonlyFilesystem:  {Level: Must, Reference: rootRef},
-	ReadonlyOnWindows:   {Level: Must, Reference: rootRef},
+	PathFormatOnWindows: {Level: rfc2119.Must, Reference: rootRef},
+	PathName:            {Level: rfc2119.Should, Reference: rootRef},
+	PathExistence:       {Level: rfc2119.Must, Reference: rootRef},
+	ReadonlyFilesystem:  {Level: rfc2119.Must, Reference: rootRef},
+	ReadonlyOnWindows:   {Level: rfc2119.Must, Reference: rootRef},
 
 	// Config-Linux.md
 	// Default Filesystems
-	DefaultFilesystems: {Level: Should, Reference: defaultFSRef},
+	DefaultFilesystems: {Level: rfc2119.Should, Reference: defaultFSRef},
 
 	// Runtime.md
 	// Create
-	CreateWithID:       {Level: Must, Reference: runtimeCreateRef},
-	CreateWithUniqueID: {Level: Must, Reference: runtimeCreateRef},
-	CreateNewContainer: {Level: Must, Reference: runtimeCreateRef},
+	CreateWithID:       {Level: rfc2119.Must, Reference: runtimeCreateRef},
+	CreateWithUniqueID: {Level: rfc2119.Must, Reference: runtimeCreateRef},
+	CreateNewContainer: {Level: rfc2119.Must, Reference: runtimeCreateRef},
+}
+
+// Error returns the error message with specification reference.
+func (err *Error) Error() string {
+	return err.Err.Error()
 }
 
 // NewError creates an Error referencing a spec violation.  The error
-// can be cast to a *runtime-tools.error.Error for extracting
-// structured information about the level of the violation and a
-// reference to the violated spec condition.
+// can be cast to an *Error for extracting structured information
+// about the level of the violation and a reference to the violated
+// spec condition.
 //
 // A version string (for the version of the spec that was violated)
 // must be set to get a working URL.
-func NewError(code SpecErrorCode, msg string, version string) (err error) {
+func NewError(code SpecErrorCode, err error, version string) error {
 	template := ociErrors[code]
-	reference, err := template.Reference(version)
-	if err != nil {
-		return err
+	reference, err2 := template.Reference(version)
+	if err2 != nil {
+		return err2
 	}
 	return &Error{
-		Level:     template.Level,
-		Reference: reference,
-		Err:       errors.New(msg),
-		ErrCode:   int(code),
+		Err: rfc2119.Error{
+			Level:     template.Level,
+			Reference: reference,
+			Err:       err,
+		},
+		ErrCode: code,
 	}
 }
 
 // FindError finds an error from a source error (multiple error) and
-// returns the error code if founded.
+// returns the error code if found.
 // If the source error is nil or empty, return NonError.
 // If the source error is not a multiple error, return NonRFCError.
 func FindError(err error, code SpecErrorCode) SpecErrorCode {
@@ -141,7 +160,7 @@ func FindError(err error, code SpecErrorCode) SpecErrorCode {
 		}
 		for _, e := range merr.Errors {
 			if rfcErr, ok := e.(*Error); ok {
-				if rfcErr.ErrCode == int(code) {
+				if rfcErr.ErrCode == code {
 					return code
 				}
 			}

validate/validate.go

@@ -23,7 +23,7 @@ import (
 	"github.com/sirupsen/logrus"
 	"github.com/syndtr/gocapability/capability"
 
-	rerr "github.com/opencontainers/runtime-tools/error"
+	"github.com/opencontainers/runtime-tools/specerror"
 )
 
 const specConfig = "config.json"
@@ -86,7 +86,7 @@ func NewValidatorFromPath(bundlePath string, hostSpecific bool, platform string)
 	configPath := filepath.Join(bundlePath, specConfig)
 	content, err := ioutil.ReadFile(configPath)
 	if err != nil {
-		return Validator{}, rerr.NewError(rerr.ConfigFileExistence, err.Error(), rspec.Version)
+		return Validator{}, specerror.NewError(specerror.ConfigFileExistence, err, rspec.Version)
 	}
 	if !utf8.Valid(content) {
 		return Validator{}, fmt.Errorf("%q is not encoded in UTF-8", configPath)
@@ -120,13 +120,13 @@ func (v *Validator) CheckRoot() (errs error) {
 	if v.platform == "windows" && v.spec.Windows != nil && v.spec.Windows.HyperV != nil {
 		if v.spec.Root != nil {
 			errs = multierror.Append(errs,
-				rerr.NewError(rerr.RootOnHyperV, "for Hyper-V containers, Root must not be set", rspec.Version))
+				specerror.NewError(specerror.RootOnHyperV, fmt.Errorf("for Hyper-V containers, Root must not be set"), rspec.Version))
 			return
 		}
 		return
 	} else if v.spec.Root == nil {
 		errs = multierror.Append(errs,
-			rerr.NewError(rerr.RootOnNonHyperV, "for non-Hyper-V containers, Root must be set", rspec.Version))
+			specerror.NewError(specerror.RootOnNonHyperV, fmt.Errorf("for non-Hyper-V containers, Root must be set"), rspec.Version))
 		return
 	}
 
@@ -138,7 +138,7 @@ func (v *Validator) CheckRoot() (errs error) {
 
 	if filepath.Base(v.spec.Root.Path) != "rootfs" {
 		errs = multierror.Append(errs,
-			rerr.NewError(rerr.PathName, "Path name should be the conventional 'rootfs'", rspec.Version))
+			specerror.NewError(specerror.PathName, fmt.Errorf("path name should be the conventional 'rootfs'"), rspec.Version))
 	}
 
 	var rootfsPath string
@@ -158,22 +158,22 @@ func (v *Validator) CheckRoot() (errs error) {
 
 	if fi, err := os.Stat(rootfsPath); err != nil {
 		errs = multierror.Append(errs,
-			rerr.NewError(rerr.PathExistence, fmt.Sprintf("Cannot find the root path %q", rootfsPath), rspec.Version))
+			specerror.NewError(specerror.PathExistence, fmt.Errorf("cannot find the root path %q", rootfsPath), rspec.Version))
 	} else if !fi.IsDir() {
 		errs = multierror.Append(errs,
-			rerr.NewError(rerr.PathExistence, fmt.Sprintf("The root path %q is not a directory", rootfsPath), rspec.Version))
+			specerror.NewError(specerror.PathExistence, fmt.Errorf("root.path %q is not a directory", rootfsPath), rspec.Version))
 	}
 
 	rootParent := filepath.Dir(absRootPath)
 	if absRootPath == string(filepath.Separator) || rootParent != absBundlePath {
 		errs = multierror.Append(errs,
-			rerr.NewError(rerr.ArtifactsInSingleDir, fmt.Sprintf("root.path is %q, but it MUST be a child of %q", v.spec.Root.Path, absBundlePath), rspec.Version))
+			specerror.NewError(specerror.ArtifactsInSingleDir, fmt.Errorf("root.path is %q, but it MUST be a child of %q", v.spec.Root.Path, absBundlePath), rspec.Version))
 	}
 
 	if v.platform == "windows" {
 		if v.spec.Root.Readonly {
 			errs = multierror.Append(errs,
-				rerr.NewError(rerr.ReadonlyOnWindows, "root.readonly field MUST be omitted or false when target platform is windows", rspec.Version))
+				specerror.NewError(specerror.ReadonlyOnWindows, fmt.Errorf("root.readonly field MUST be omitted or false when target platform is windows"), rspec.Version))
 		}
 	}
 
@@ -188,7 +188,7 @@ func (v *Validator) CheckSemVer() (errs error) {
 	_, err := semver.Parse(version)
 	if err != nil {
 		errs = multierror.Append(errs,
-			rerr.NewError(rerr.SpecVersion, fmt.Sprintf("%q is not valid SemVer: %s", version, err.Error()), rspec.Version))
+			specerror.NewError(specerror.SpecVersion, fmt.Errorf("%q is not valid SemVer: %s", version, err.Error()), rspec.Version))
 	}
 	if version != rspec.Version {
 		errs = multierror.Append(errs, fmt.Errorf("validate currently only handles version %s, but the supplied configuration targets %s", rspec.Version, version))

validate/validate_test.go

@@ -11,7 +11,7 @@ import (
 	rspec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/stretchr/testify/assert"
 
-	rerr "github.com/opencontainers/runtime-tools/error"
+	"github.com/opencontainers/runtime-tools/specerror"
 )
 
 func TestNewValidator(t *testing.T) {
@@ -53,40 +53,40 @@ func TestCheckRoot(t *testing.T) {
 	cases := []struct {
 		val      rspec.Spec
 		platform string
-		expected rerr.SpecErrorCode
+		expected specerror.SpecErrorCode
 	}{
-		{rspec.Spec{Windows: &rspec.Windows{HyperV: &rspec.WindowsHyperV{}}, Root: &rspec.Root{}}, "windows", rerr.RootOnHyperV},
-		{rspec.Spec{Windows: &rspec.Windows{HyperV: &rspec.WindowsHyperV{}}, Root: nil}, "windows", rerr.NonError},
-		{rspec.Spec{Root: nil}, "linux", rerr.RootOnNonHyperV},
-		{rspec.Spec{Root: &rspec.Root{Path: "maverick-rootfs"}}, "linux", rerr.PathName},
-		{rspec.Spec{Root: &rspec.Root{Path: "rootfs"}}, "linux", rerr.NonError},
-		{rspec.Spec{Root: &rspec.Root{Path: filepath.Join(tmpBundle, rootfsNonExists)}}, "linux", rerr.PathExistence},
-		{rspec.Spec{Root: &rspec.Root{Path: filepath.Join(tmpBundle, rootfsNonDir)}}, "linux", rerr.PathExistence},
-		{rspec.Spec{Root: &rspec.Root{Path: filepath.Join(tmpBundle, "rootfs")}}, "linux", rerr.NonError},
-		{rspec.Spec{Root: &rspec.Root{Path: "rootfs/rootfs"}}, "linux", rerr.ArtifactsInSingleDir},
-		{rspec.Spec{Root: &rspec.Root{Readonly: true}}, "windows", rerr.ReadonlyOnWindows},
+		{rspec.Spec{Windows: &rspec.Windows{HyperV: &rspec.WindowsHyperV{}}, Root: &rspec.Root{}}, "windows", specerror.RootOnHyperV},
+		{rspec.Spec{Windows: &rspec.Windows{HyperV: &rspec.WindowsHyperV{}}, Root: nil}, "windows", specerror.NonError},
+		{rspec.Spec{Root: nil}, "linux", specerror.RootOnNonHyperV},
+		{rspec.Spec{Root: &rspec.Root{Path: "maverick-rootfs"}}, "linux", specerror.PathName},
+		{rspec.Spec{Root: &rspec.Root{Path: "rootfs"}}, "linux", specerror.NonError},
+		{rspec.Spec{Root: &rspec.Root{Path: filepath.Join(tmpBundle, rootfsNonExists)}}, "linux", specerror.PathExistence},
+		{rspec.Spec{Root: &rspec.Root{Path: filepath.Join(tmpBundle, rootfsNonDir)}}, "linux", specerror.PathExistence},
+		{rspec.Spec{Root: &rspec.Root{Path: filepath.Join(tmpBundle, "rootfs")}}, "linux", specerror.NonError},
+		{rspec.Spec{Root: &rspec.Root{Path: "rootfs/rootfs"}}, "linux", specerror.ArtifactsInSingleDir},
+		{rspec.Spec{Root: &rspec.Root{Readonly: true}}, "windows", specerror.ReadonlyOnWindows},
 	}
 	for _, c := range cases {
 		v := NewValidator(&c.val, tmpBundle, false, c.platform)
 		err := v.CheckRoot()
-		assert.Equal(t, c.expected, rerr.FindError(err, c.expected), fmt.Sprintf("Fail to check Root: %v %d", err, c.expected))
+		assert.Equal(t, c.expected, specerror.FindError(err, c.expected), fmt.Sprintf("Fail to check Root: %v %d", err, c.expected))
 	}
 }
 
 func TestCheckSemVer(t *testing.T) {
 	cases := []struct {
 		val      string
-		expected rerr.SpecErrorCode
+		expected specerror.SpecErrorCode
 	}{
-		{rspec.Version, rerr.NonError},
+		{rspec.Version, specerror.NonError},
 		//FIXME: validate currently only handles rpsec.Version
-		{"0.0.1", rerr.NonRFCError},
-		{"invalid", rerr.SpecVersion},
+		{"0.0.1", specerror.NonRFCError},
+		{"invalid", specerror.SpecVersion},
 	}
 
 	for _, c := range cases {
 		v := NewValidator(&rspec.Spec{Version: c.val}, "", false, "linux")
 		err := v.CheckSemVer()
-		assert.Equal(t, c.expected, rerr.FindError(err, c.expected), "Fail to check SemVer "+c.val)
+		assert.Equal(t, c.expected, specerror.FindError(err, c.expected), "Fail to check SemVer "+c.val)
 	}
 }

validation/validation_test.go

@@ -12,8 +12,8 @@ import (
 	"github.com/satori/go.uuid"
 	"github.com/stretchr/testify/assert"
 
-	rerr "github.com/opencontainers/runtime-tools/error"
 	"github.com/opencontainers/runtime-tools/generate"
+	"github.com/opencontainers/runtime-tools/specerror"
 )
 
 var (
@@ -114,9 +114,9 @@ func TestValidateCreate(t *testing.T) {
 		errExpected bool
 		err         error
 	}{
-		{"", false, rerr.NewError(rerr.CreateWithID, "'Create' MUST generate an error if the ID is not provided", rspecs.Version)},
-		{containerID, true, rerr.NewError(rerr.CreateNewContainer, "'Create' MUST create a new container", rspecs.Version)},
-		{containerID, false, rerr.NewError(rerr.CreateWithUniqueID, "'Create' MUST generate an error if the ID provided is not unique", rspecs.Version)},
+		{"", false, specerror.NewError(specerror.CreateWithID, "'Create' MUST generate an error if the ID is not provided", rspecs.Version)},
+		{containerID, true, specerror.NewError(specerror.CreateNewContainer, "'Create' MUST create a new container", rspecs.Version)},
+		{containerID, false, specerror.NewError(specerror.CreateWithUniqueID, "'Create' MUST generate an error if the ID provided is not unique", rspecs.Version)},
 	}
 
 	for _, c := range cases {