From 75531612fe9870154ad2d1c7272c8b241af5537d 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 PosixProcRlimitsTypeValueError 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]. Also make CheckRlimits a no-op on Windows, because the spec does not define process.rlimits for that OS [5]. [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 [5]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#posix-process Signed-off-by: W. Trevor King --- specerror/config.go | 3 ++ validate/validate.go | 8 ++- validate/validate_test.go | 102 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 111 insertions(+), 2 deletions(-) diff --git a/specerror/config.go b/specerror/config.go index 2ea4d805e..157ead741 100644 --- a/specerror/config.go +++ b/specerror/config.go @@ -46,6 +46,8 @@ const ( PosixProcRlimitsTypeGeneError = "The runtime MUST [generate an error](runtime.md#errors) for any values which cannot be mapped to a relevant kernel interface." // PosixProcRlimitsTypeGet represents "For each entry in `rlimits`, a [`getrlimit(3)`][getrlimit.3] on `type` MUST succeed." PosixProcRlimitsTypeGet = "For each entry in `rlimits`, a [`getrlimit(3)`][getrlimit.3] on `type` MUST succeed." + // PosixProcRlimitsTypeValueError represents "valid values are defined in the ... man page" + PosixProcRlimitsTypeValueError = "valid values are defined in the ... man page" // PosixProcRlimitsSoftMatchCur represents "`rlim.rlim_cur` MUST match the configured value." PosixProcRlimitsSoftMatchCur = "`rlim.rlim_cur` MUST match the configured value." // PosixProcRlimitsHardMatchMax represents "`rlim.rlim_max` MUST match the configured value." @@ -159,6 +161,7 @@ func init() { register(ProcArgsOneEntryRequired, rfc2119.Required, processRef) register(PosixProcRlimitsTypeGeneError, rfc2119.Must, posixProcessRef) register(PosixProcRlimitsTypeGet, rfc2119.Must, posixProcessRef) + register(PosixProcRlimitsTypeValueError, rfc2119.Should, posixProcessRef) register(PosixProcRlimitsSoftMatchCur, rfc2119.Must, posixProcessRef) register(PosixProcRlimitsHardMatchMax, rfc2119.Must, posixProcessRef) register(PosixProcRlimitsErrorOnDup, rfc2119.Must, posixProcessRef) diff --git a/validate/validate.go b/validate/validate.go index a4777a806..8d799b7d8 100644 --- a/validate/validate.go +++ b/validate/validate.go @@ -424,6 +424,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++ { @@ -870,14 +874,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.PosixProcRlimitsTypeValueError, 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.PosixProcRlimitsTypeValueError, 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 82f7b7b6d..52d5ccc87 100644 --- a/validate/validate_test.go +++ b/validate/validate_test.go @@ -188,3 +188,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.PosixProcRlimitsTypeValueError, + }, + { + 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.PosixProcRlimitsTypeValueError, + }, + } + 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)) + } +}