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.

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

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

Merged
merged 4 commits into from
Jul 28, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 53 additions & 1 deletion cmd/runtimetest/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@ import (
"github.com/hashicorp/go-multierror"
"github.com/mndrix/tap-go"
rspec "github.com/opencontainers/runtime-spec/specs-go"
"github.com/opencontainers/runtime-tools/cmd/runtimetest/mount"
"github.com/syndtr/gocapability/capability"
"github.com/urfave/cli"

"github.com/opencontainers/runtime-tools/cmd/runtimetest/mount"
ociErr "github.com/opencontainers/runtime-tools/validate"
)

// PrGetNoNewPrivs isn't exposed in Golang so we define it ourselves copying the value from
Expand All @@ -30,6 +32,13 @@ const PrGetNoNewPrivs = 39
const specConfig = "config.json"

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

Choose a reason for hiding this comment

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

proc -> procfs

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

fine.

"/sys": "sysfs",
"/dev/pts": "devpts",
"/dev/shm": "tmpfs",
}

defaultSymlinks = map[string]string{
"/dev/fd": "/proc/self/fd",
"/dev/stdin": "/proc/self/fd/0",
Expand Down Expand Up @@ -308,6 +317,28 @@ func validateRootFS(spec *rspec.Spec) error {
return nil
}

func validateDefaultFS(spec *rspec.Spec) error {
logrus.Debugf("validating linux default filesystem")

mountInfos, err := mount.GetMounts()
if err != nil {
return ociErr.NewError(ociErr.DefaultFilesystems, err.Error())
}

mountsMap := make(map[string]string)
for _, mountInfo := range mountInfos {
mountsMap[mountInfo.Mountpoint] = mountInfo.Fstype
}

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

return nil
}

func validateLinuxDevices(spec *rspec.Spec) error {
for _, device := range spec.Linux.Devices {
fi, err := os.Stat(device.Path)
Expand Down Expand Up @@ -615,6 +646,10 @@ func validate(context *cli.Context) error {
test: validateDefaultSymlinks,
description: "default symlinks",
},
{
test: validateDefaultFS,
description: "default file system",
},
{
test: validateDefaultDevices,
description: "default devices",
Expand Down Expand Up @@ -660,11 +695,20 @@ func validate(context *cli.Context) error {
t := tap.New()
t.Header(0)

complianceLevelString := context.String("compliance-level")
complianceLevel, err := ociErr.ParseLevel(complianceLevelString)
if err != nil {
complianceLevel = ociErr.ComplianceMust
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.(*ociErr.Error); ok && e.Level < complianceLevel {
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

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 ;).

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened #362 to keep track of this.

}
validationErrors = multierror.Append(validationErrors, err)
}
}
Expand All @@ -674,6 +718,9 @@ func validate(context *cli.Context) error {
err := v.test(spec)
t.Ok(err == nil, v.description)
if err != nil {
if e, ok := err.(*ociErr.Error); ok && e.Level < complianceLevel {
continue
}
validationErrors = multierror.Append(validationErrors, err)
}
}
Expand All @@ -700,6 +747,11 @@ func main() {
Value: ".",
Usage: "Path to the configuration",
},
cli.StringFlag{
Name: "compliance-level",
Value: "must",
Usage: "Compliance level (may, should or must)",
},
}

app.Action = validate
Expand Down
16 changes: 16 additions & 0 deletions completions/bash/oci-runtime-tool
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,14 @@ __oci-runtime-tool_complete_log_level() {
" -- "$cur" ) )
}

__oci-runtime-tool_complete_compliance_level() {
COMPREPLY=( $( compgen -W "
may
should
must
" -- "$cur" ) )
}

__oci-runtime-tool_complete_propagations() {
COMPREPLY=( $( compgen -W "
private
Expand Down Expand Up @@ -218,6 +226,10 @@ _oci-runtime-tool_oci-runtime-tool() {
--log-level
"

local options_with_args="
--compliance-level
"

local boolean_options="
--help -h
--host-specific
Expand All @@ -231,6 +243,10 @@ _oci-runtime-tool_oci-runtime-tool() {
__oci-runtime-tool_complete_log_level
return
;;
--compliance-level)
__oci-runtime-tool_complete_compliance_level
return
;;
esac

case "$cur" in
Expand Down
3 changes: 3 additions & 0 deletions man/oci-runtime-tool.1.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ oci-runtime-tool is a collection of tools for working with the [OCI runtime spec
**--log-level**=LEVEL
Log level (panic, fatal, error, warn, info, or debug) (default: "error").

**--compliance-level**=LEVEL
Compliance level (may, should or must) (default: "must").

**-v**, **--version**
Print version information.

Expand Down
110 changes: 110 additions & 0 deletions validate/error.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
package validate
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


import (
"errors"
"fmt"
"strings"

rspec "github.com/opencontainers/runtime-spec/specs-go"
)

// ComplianceLevel represents the OCI compliance levels
type ComplianceLevel int

const (
// MAY-level

// ComplianceMay represents 'MAY' in RFC2119
ComplianceMay ComplianceLevel = iota
// ComplianceOptional represents 'OPTIONAL' in RFC2119
ComplianceOptional

// SHOULD-level

// ComplianceShould represents 'SHOULD' in RFC2119
ComplianceShould
// ComplianceShouldNot represents 'SHOULD NOT' in RFC2119
ComplianceShouldNot
// ComplianceRecommended represents 'RECOMMENDED' in RFC2119
ComplianceRecommended
// ComplianceNotRecommended represents 'NOT RECOMMENDED' in RFC2119
ComplianceNotRecommended

// MUST-level

// ComplianceMust represents 'MUST' in RFC2119
ComplianceMust
// ComplianceMustNot represents 'MUST NOT' in RFC2119
ComplianceMustNot
// ComplianceShall represents 'SHALL' in RFC2119
ComplianceShall
// ComplianceShallNot represents 'SHALL NOT' in RFC2119
ComplianceShallNot
// ComplianceRequired represents 'REQUIRED' in RFC2119
ComplianceRequired
)

// ErrorCode represents the compliance content
type ErrorCode int

const (
// DefaultFilesystems represents the error code of default filesystems test
DefaultFilesystems ErrorCode = iota
)

// Error represents an error with compliance level and OCI reference
type Error struct {
Level ComplianceLevel
Reference string
Err error
}

const referencePrefix = "https://github.com/opencontainers/runtime-spec/blob"

var ociErrors = map[ErrorCode]Error{
DefaultFilesystems: Error{Level: ComplianceShould, Reference: "config-linux.md#default-filesystems"},
}

// ParseLevel takes a string level and returns the OCI compliance level constant
func ParseLevel(level string) (ComplianceLevel, error) {
switch strings.ToUpper(level) {
case "MAY":
fallthrough
case "OPTIONAL":
return ComplianceMay, nil
case "SHOULD":
fallthrough
case "SHOULDNOT":
fallthrough
case "RECOMMENDED":
fallthrough
case "NOTRECOMMENDED":
return ComplianceShould, nil
case "MUST":
fallthrough
case "MUSTNOT":
fallthrough
case "SHALL":
fallthrough
case "SHALLNOT":
fallthrough
case "REQUIRED":
return ComplianceMust, nil
}

var l ComplianceLevel
return l, fmt.Errorf("%q is not a valid compliance level", level)
}

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

Choose a reason for hiding this comment

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

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 ;).

Copy link
Member Author

Choose a reason for hiding this comment

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

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


return &err
}

// Error returns the error message with OCI reference
func (oci *Error) Error() string {
return fmt.Sprintf("%s\nRefer to: %s/v%s/%s", oci.Err.Error(), referencePrefix, rspec.Version, oci.Reference)
}