From 89d5f47695531936c95f75029e2d9892f0dc57e7 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Thu, 21 Sep 2017 11:14:28 -0700 Subject: [PATCH] validate: Soften unrecognized rlimit types to SHOULD violations The spec isn't particuarly clear on this, saying [1]: * Linux: valid values are defined in the [`getrlimit(2)`][getrlimit.2] man page, such as `RLIMIT_MSGQUEUE`. * Solaris: valid values are defined in the [`getrlimit(3)`][getrlimit.3] man page, such as `RLIMIT_CORE`. and [2]: For each entry in `rlimits`, a [`getrlimit(3)`][getrlimit.3] on `type` MUST succeed. It doesn't say: Linux: The value MUST be listed in the getrlimit(2) man page... and it doesn't require the runtime to support the values listed in the man page [3,4]. So there are three sets: * Values listed in the man page * Values supported by the host kernel * Values supported by the runtime And as the spec stands, these sets are only weakly coupled, and any of them could be a sub- or superset of any other. In practice, I expect the sets to all coincide, with the kernel occasionally adding or removing values, and the man page and runtimes trailing along behind. To address that, this commit weakens the previous hard error to a SHOULD-level error. The PosixProcRlimitsValueError constant is new to this commit, because the spec contains neither a MUST nor a SHOULD for this condition, although I expect a SHOULD-level suggestion was implied by [1]. The posixProcRef constant is cherry-picked from 27503c50 (complete spec errors of config.md, 2017-09-05, #458). [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0/config.md#L168-L169 [2]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0/config.md#L168-L169 [3]: https://github.com/opencontainers/runtime-spec/issues/813 [4]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0/config.md#L463 Signed-off-by: W. Trevor King --- specerror/error.go | 8 +++ validate/validate.go | 10 ++-- validate/validate_test.go | 102 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 117 insertions(+), 3 deletions(-) diff --git a/specerror/error.go b/specerror/error.go index c75bb6b14..eec6785c0 100644 --- a/specerror/error.go +++ b/specerror/error.go @@ -44,6 +44,9 @@ const ( // ReadonlyOnWindows represents the error code of readonly setting test on Windows ReadonlyOnWindows + // PosixProcRlimitsValueError represents "valid values are defined in the ... man page" + PosixProcRlimitsValueError + // DefaultFilesystems represents the error code of default filesystems test DefaultFilesystems @@ -79,6 +82,9 @@ var ( rootRef = func(version string) (reference string, err error) { return fmt.Sprintf(referenceTemplate, version, "config.md#root"), nil } + posixProcRef = func(version string) (reference string, err error) { + return fmt.Sprintf(referenceTemplate, version, "config.md#posix-process"), nil + } defaultFSRef = func(version string) (reference string, err error) { return fmt.Sprintf(referenceTemplate, version, "config-linux.md#default-filesystems"), nil } @@ -106,6 +112,8 @@ var ociErrors = map[Code]errorTemplate{ ReadonlyFilesystem: {Level: rfc2119.Must, Reference: rootRef}, ReadonlyOnWindows: {Level: rfc2119.Must, Reference: rootRef}, + PosixProcRlimitsValueError: {Level: rfc2119.Should, Reference: posixProcRef}, + // Config-Linux.md // Default Filesystems DefaultFilesystems: {Level: rfc2119.Should, Reference: defaultFSRef}, diff --git a/validate/validate.go b/validate/validate.go index 496f40c12..e570242c1 100644 --- a/validate/validate.go +++ b/validate/validate.go @@ -51,7 +51,7 @@ var ( "RLIMIT_RTPRIO", "RLIMIT_RTTIME", "RLIMIT_SIGPENDING", - }) + }...) configSchemaTemplate = "https://raw.githubusercontent.com/opencontainers/runtime-spec/v%s/schema/config-schema.json" ) @@ -423,6 +423,10 @@ func (v *Validator) CheckCapabilities() (errs error) { // CheckRlimits checks v.spec.Process.Rlimits func (v *Validator) CheckRlimits() (errs error) { + if v.platform == "windows" { + return + } + process := v.spec.Process for index, rlimit := range process.Rlimits { for i := index + 1; i < len(process.Rlimits); i++ { @@ -878,14 +882,14 @@ func (v *Validator) rlimitValid(rlimit rspec.POSIXRlimit) (errs error) { return } } - errs = multierror.Append(errs, fmt.Errorf("rlimit type %q is invalid", rlimit.Type)) + errs = multierror.Append(errs, specerror.NewError(specerror.PosixProcRlimitsValueError, fmt.Errorf("rlimit type %q may not be valid", rlimit.Type), v.spec.Version)) } else if v.platform == "solaris" { for _, val := range posixRlimits { if val == rlimit.Type { return } } - errs = multierror.Append(errs, fmt.Errorf("rlimit type %q is invalid", rlimit.Type)) + errs = multierror.Append(errs, specerror.NewError(specerror.PosixProcRlimitsValueError, fmt.Errorf("rlimit type %q may not be valid", rlimit.Type), v.spec.Version)) } else { logrus.Warnf("process.rlimits validation not yet implemented for platform %q", v.platform) } diff --git a/validate/validate_test.go b/validate/validate_test.go index ba50e6eba..8e403d268 100644 --- a/validate/validate_test.go +++ b/validate/validate_test.go @@ -154,3 +154,105 @@ func TestCheckSemVer(t *testing.T) { assert.Equal(t, c.expected, specerror.FindError(err, c.expected), "Fail to check SemVer "+c.val) } } + +func TestCheckProcess(t *testing.T) { + cases := []struct { + val rspec.Spec + platform string + expected specerror.Code + }{ + { + val: rspec.Spec{ + Version: "1.0.0", + Process: &rspec.Process{ + Args: []string{"sh"}, + Cwd: "/", + }, + }, + platform: "linux", + expected: specerror.NonError, + }, + { + val: rspec.Spec{ + Version: "1.0.0", + Process: &rspec.Process{ + Args: []string{"sh"}, + Cwd: "/", + Rlimits: []rspec.POSIXRlimit{ + { + Type: "RLIMIT_NOFILE", + Hard: 1024, + Soft: 1024, + }, + { + Type: "RLIMIT_NPROC", + Hard: 512, + Soft: 512, + }, + }, + }, + }, + platform: "linux", + expected: specerror.NonError, + }, + { + val: rspec.Spec{ + Version: "1.0.0", + Process: &rspec.Process{ + Args: []string{"sh"}, + Cwd: "/", + Rlimits: []rspec.POSIXRlimit{ + { + Type: "RLIMIT_NOFILE", + Hard: 1024, + Soft: 1024, + }, + }, + }, + }, + platform: "solaris", + expected: specerror.NonError, + }, + { + val: rspec.Spec{ + Version: "1.0.0", + Process: &rspec.Process{ + Args: []string{"sh"}, + Cwd: "/", + Rlimits: []rspec.POSIXRlimit{ + { + Type: "RLIMIT_DOES_NOT_EXIST", + Hard: 512, + Soft: 512, + }, + }, + }, + }, + platform: "linux", + expected: specerror.PosixProcRlimitsValueError, + }, + { + val: rspec.Spec{ + Version: "1.0.0", + Process: &rspec.Process{ + Args: []string{"sh"}, + Cwd: "/", + Rlimits: []rspec.POSIXRlimit{ + { + Type: "RLIMIT_NPROC", + Hard: 512, + Soft: 512, + }, + }, + }, + }, + platform: "solaris", + expected: specerror.PosixProcRlimitsValueError, + }, + } + for _, c := range cases { + v := NewValidator(&c.val, ".", false, c.platform) + err := v.CheckProcess() + assert.Equal(t, c.expected, specerror.FindError(err, c.expected), fmt.Sprintf("failed CheckProcess: %v %d", err, c.expected)) + } +}