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

config: Drop "subset of the valid values" line #927

Closed

Conversation

wking
Copy link
Contributor

@wking wking commented Oct 4, 2017

In #673 we got:

Implementations MUST error out when invalid values are encountered and MUST generate an error message and error out when encountering valid values it chooses to not support.

In #681, I'd moved that out into the current “Valid values” section with the line I'm removing in this commit. However, giving runtimes a blanket clause to ignore valid values makes it harder to use runtimes, because you can't be sure an OCI-compliant runtime supports the spec-defined value you need (#813).

There have been concerns about requiring runtimes to support values which are not supported by the host system. But since #559 we've had runtime.md wording that gives the runtime the ability to compliantly die in those cases. That text had a wording tweak in #700, and is now:

If the runtime cannot apply a property as specified in the configuration, it MUST generate an error and a new container MUST NOT be created.

With this commit, consumers will be able to rely on valid-value support in compliant runtimes. (edit: this commit is not enough for this, see here) Many properties could use clearer runtimes-MUST-support wording for those values, but we can sort those out on a per-property basis in follow-up work.

And runtimes are still allowed to support extention values not defined in the spec (e.g. new capability types, or mount options, or whatever). Like all extentions, it is up to the runtime and runtime-caller to negotiate support in those cases.

@wking wking force-pushed the drop-subset-of-valid-values branch from 2378487 to 386f006 Compare October 4, 2017 21:31
@tianon
Copy link
Member

tianon commented Oct 4, 2017

So, to summarize, the TL;DR here would be that runtimes are free to not support certain values, but then they're also required to error out on configurations which request them. Correct?

@wking
Copy link
Contributor Author

wking commented Oct 4, 2017

So, to summarize, the TL;DR here would be that runtimes are free to not support certain values, but then they're also required to error out on configurations which request them. Correct?

Yes. And that's still broader than I'd like, due to the “runtime cannot apply a property as specified” language. What I really want is to require runtimes to support valid values regardless of whether the host supports them. For example, I'd like to MUST runtime support for CAP_AUDIT_READ. On hosts running Linux <3.16 without CAP_AUDIT_READ patched in, any compliant runtime will die in create with an:

I couldn't set CAP_AUDIT_READ [because the host doesn't support it].

error. That makes sense.

But as it stands both before and after this commit, a runtime on a host running Linux ≥3.16 may either succeed (setting CAP_AUDIT_READ as configured) or die in create with:

I couldn't set CAP_AUDIT_READ.

I'd like to block that, and require the runtime to support CAP_AUDIT_READ on hosts that support it. This PR moves us in that direction, but doesn't get us all the way there. We'd probably want something like #680 (but for values instead of properties) to close off more of the gap. I'm happy to work that up for this PR, or leave it to follow-up work. We'd also need either a versioned link out to capabilities(7) or an internal list of required capabilities to avoid having the runtime compliance requirements drift with the live version.

We probably want to switch to versioned links (or inline lists of valid values) before we completely close off the loophole.

In 718f9f3 (minor narrative cleanup regarding config compatibility,
2017-01-30, opencontainers#673) we got:

  Implementations MUST error out when invalid values are encountered
  and MUST generate an error message and error out when encountering
  valid values it chooses to not support

In c763e64 (config: Move valid-value rules to their own section,
2017-02-07, opencontainers#681), I'd moved that out into the current "Valid values"
section with the line I'm removing in this commit.  However, giving
runtimes a blanket clause to ignore valid values makes it harder to
use runtimes, because you can't be sure an OCI-compliant runtime
supports the spec-defined value you need [1].

There have been concerns about requiring runtimes to support values
which are not supported by the host system [2].  But since 766abd6
(runtime.md: Require 'create' to fail if config.json asks for the
impossible, 2016-09-08, opencontainers#559) we've had runtime.md wording that gives
the runtime the ability to compliantly die in those cases.  That text
had a wording tweak in 72e8062 (runtime: Explicitly make process.*
timing implementation-defined, 2017-02-27, opencontainers#700), and is now:

  If the runtime cannot apply a property as specified in the
  configuration, it MUST generate an error and a new container MUST
  NOT be created.

As it stands both before and after this commit, a runtime can *still*
die in 'create' because it cannot apply values supported by the host.
This commit is just a step towards requiring runtimes to support as
many values as the host supports; it doesn't get us all the way there.

Many properties could use clearer runtimes-MUST-support wording for
those values, but we can sort those out on a per-property basis in
follow-up work.

And runtimes are still allowed to support extention values not defined
in the spec (e.g. new capability types, or mount options, or
whatever).  Like all extentions, it is up to the runtime and
runtime-caller to negotiate support in those cases.

[1]: opencontainers#813 (comment)
[2]: opencontainers#673 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
@vbatts
Copy link
Member

vbatts commented Dec 17, 2019

house keeping: I get the clarity, but dropping this sentence doesn't make things any more clear.
Closing for now.

@vbatts vbatts closed this Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants