-
Notifications
You must be signed in to change notification settings - Fork 143
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
Changes from all commits
4029999
6b1d25d
1259e3e
6316a4e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -30,6 +32,13 @@ const PrGetNoNewPrivs = 39 | |
const specConfig = "config.json" | ||
|
||
var ( | ||
defaultFS = map[string]string{ | ||
"/proc": "proc", | ||
"/sys": "sysfs", | ||
"/dev/pts": "devpts", | ||
"/dev/shm": "tmpfs", | ||
} | ||
|
||
defaultSymlinks = map[string]string{ | ||
"/dev/fd": "/proc/self/fd", | ||
"/dev/stdin": "/proc/self/fd/0", | ||
|
@@ -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) | ||
|
@@ -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", | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ;). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} | ||
} | ||
|
@@ -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) | ||
} | ||
} | ||
|
@@ -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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
package validate | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this making a new There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proc -> procfs
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/opencontainers/runtime-spec/blame/master/config-linux.md#L15
In the
spec
is defined.There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine.