From d268fb212d79a3411470edb76ba60c5097c72c02 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Wed, 30 Aug 2017 13:33:28 -0700 Subject: [PATCH 1/2] 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, #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 25202126 (add stretchr/testify/assert pkgs; use rfc code in bundle validation, 2017-08-29, #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]: https://github.com/opencontainers/runtime-tools/pull/451#discussion_r135844806 [2]: https://github.com/opencontainers/runtime-tools/pull/451#discussion_r135953382 [3]: https://github.com/opencontainers/runtime-tools/pull/451#issuecomment-325874457 Signed-off-by: W. Trevor King --- cmd/runtimetest/main.go | 17 ++-- error/{rfc2199.go => error.go} | 1 - error/runtime_spec.go => specerror/error.go | 89 +++++++++++++-------- validate/validate.go | 20 ++--- validate/validate_test.go | 36 ++++----- validation/validation_test.go | 9 ++- 6 files changed, 96 insertions(+), 76 deletions(-) rename error/{rfc2199.go => error.go} (99%) rename error/runtime_spec.go => specerror/error.go (61%) diff --git a/cmd/runtimetest/main.go b/cmd/runtimetest/main.go index 858f216bd..5b4819644 100644 --- a/cmd/runtimetest/main.go +++ b/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,9 +780,9 @@ 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 @@ -789,7 +790,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) @@ -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) diff --git a/error/rfc2199.go b/error/error.go similarity index 99% rename from error/rfc2199.go rename to error/error.go index 2b4ab6267..c2345c1d8 100644 --- a/error/rfc2199.go +++ b/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. diff --git a/error/runtime_spec.go b/specerror/error.go similarity index 61% rename from error/runtime_spec.go rename to specerror/error.go index 031a3ccf0..c75bb6b14 100644 --- a/error/runtime_spec.go +++ b/specerror/error.go @@ -1,20 +1,23 @@ -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. -type SpecErrorCode int +// Code represents the spec violation, enumerating both +// configuration violations and runtime violations. +type Code int const ( // NonError represents that an input is not an error - NonError SpecErrorCode = iota + NonError Code = iota // NonRFCError represents that an error is not a rfc2119 error NonRFCError @@ -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 + + // Code is a matchable holds a Code + Code Code +} + var ( containerFormatRef = func(version string) (reference string, err error) { return fmt.Sprintf(referenceTemplate, version, "bundle.md#container-format"), nil @@ -75,62 +87,69 @@ var ( } ) -var ociErrors = map[SpecErrorCode]errorTemplate{ +var ociErrors = map[Code]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 Code, 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, + }, + Code: 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 { +func FindError(err error, code Code) Code { if err == nil { return NonError } @@ -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.Code == code { return code } } diff --git a/validate/validate.go b/validate/validate.go index bbad95a8b..0d62a53ef 100644 --- a/validate/validate.go +++ b/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)) diff --git a/validate/validate_test.go b/validate/validate_test.go index be89a008e..45e351eb7 100644 --- a/validate/validate_test.go +++ b/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.Code }{ - {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.Code }{ - {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) } } diff --git a/validation/validation_test.go b/validation/validation_test.go index 163613e8a..c85bbec2f 100644 --- a/validation/validation_test.go +++ b/validation/validation_test.go @@ -1,6 +1,7 @@ package validation import ( + "fmt" "io/ioutil" "os" "os/exec" @@ -12,8 +13,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 +115,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, fmt.Errorf("create MUST generate an error if the ID is not provided"), rspecs.Version)}, + {containerID, true, specerror.NewError(specerror.CreateNewContainer, fmt.Errorf("create MUST create a new container"), rspecs.Version)}, + {containerID, false, specerror.NewError(specerror.CreateWithUniqueID, fmt.Errorf("create MUST generate an error if the ID provided is not unique"), rspecs.Version)}, } for _, c := range cases { From f9152f19b48bf81174a1e5aa0c163ef49d1473eb Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Wed, 30 Aug 2017 13:46:09 -0700 Subject: [PATCH 2/2] error/error.go: Improve godocs (no OCI references, etc.) Remove OCI references from the comments, because this is a generic RFC 2119 package and has nothing OCI-specific. Also document the Error properties. Signed-off-by: W. Trevor King --- error/error.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/error/error.go b/error/error.go index c2345c1d8..f5a90800e 100644 --- a/error/error.go +++ b/error/error.go @@ -7,7 +7,7 @@ import ( "strings" ) -// Level represents the OCI compliance levels +// Level represents the RFC 2119 compliance levels type Level int const ( @@ -43,14 +43,19 @@ const ( Required ) -// Error represents an error with compliance level and OCI reference. +// Error represents an error with compliance level and specification reference. type Error struct { - Level Level + // Level represents the RFC 2119 compliance level. + Level Level + + // Reference is a URL for the violated specification requirement. Reference string - Err error + + // Err holds additional details about the violation. + Err error } -// ParseLevel takes a string level and returns the OCI compliance level constant. +// ParseLevel takes a string level and returns the RFC 2119 compliance level constant. func ParseLevel(level string) (Level, error) { switch strings.ToUpper(level) { case "MAY": @@ -81,7 +86,7 @@ func ParseLevel(level string) (Level, error) { return l, fmt.Errorf("%q is not a valid compliance level", level) } -// Error returns the error message with OCI reference +// Error returns the error message with specification reference. func (err *Error) Error() string { return fmt.Sprintf("%s\nRefer to: %s", err.Err.Error(), err.Reference) }