-
Notifications
You must be signed in to change notification settings - Fork 541
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
kernel keyring control #950
Comments
On Wed, Feb 14, 2018 at 01:45:24PM +0000, Justin Cormack wrote:
This is kind of weird - I think this should be part of the OCI spec
as it is just a specification of resource allocation, like having a
new namespace.
I agree. Tying this behavior to an OCI spec would allow managers like
CRI-O to drive this behavior in a runtime-agnostic way. Currently
CRI-O exposes it via the runc-specific flag
(cri-o/cri-o#1302), which makes swapping in other
runtimes difficult (cri-o/cri-o#1260).
… obviously though this will be a breaking change so want to know
what anyone else thinks.
I don't think this is a breaking change. runtime-spec currently says
nothing about keyrings, so their behavior is unspecified (in the C99
sense [1,2]). Runtime callers relying on runtime-spec should
therefore be making no assumptions about container keyring handling.
The new feature would need a minor bump, but not a major bump [3].
For existing runc users, runc can continue to treat 1.0.x configs as
it already does, and error out if invoked with --no-new-keyring and a
1.1 or later configuration. That's a breaking change for runc
*callers*, but the config versioning is for the configuration API [3],
not the command line API. Dropping --no-new-keyring would be a
breaking change to a command line API (like [4]), but there is no
runtime command line API spec that defines --no-new-keyring.
[1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.1/spec.md#notational-conventions
[2]: http://www.open-std.org/jtc1/sc22/wg14/www/C99RationaleV5.10.pdf#page=18
[3]: https://github.com/opencontainers/runtime-spec/blob/v1.0.1/config.md#specification-version
[4]: https://github.com/opencontainers/runtime-tools/blob/v0.5.0/docs/command-line-interface.md#create
|
I think the most general interface would be for creating a new session keyring to match the config for namespaces
joining an existing keyring/creating a new named on as session keyring:
Initially "session" is probably the only supported value, but it is extensible in future. In theory setting the process keyring might make sense too, but almost all use cases use session.
If this looks sane I will do a spec PR. |
Would you ever need multiple keyrings of the same type? I can't think of a reason, if it's possible at all. If it's not possible, I recommend using an object with ring-type keys: "keyrings": {
"session": {
"name": "mykeyring"
}
} |
I followed what we do for namespaces, where you also can't have more than one of a type. The problem is if |
On Thu, Feb 15, 2018 at 04:56:30PM +0000, Justin Cormack wrote:
I followed what we do for namespaces…
I think the namespace structure would have been easier to use if it
had been an object (#598). I'm not clear on why the runtime-spec
maintainers preferred to stick with an array there. If it was because
of existing implementations for a namespaces array, that reasoning
wouldn't apply to the new keyrings property. If that reasoning was
because they really prefer arrays with an additional duplicate
restriction (vs. an object with our generic key restriction [1]), then
yeah, they'll probably want an array here (and I'd be interested in
hearing what that reason is).
The problem is if `name` is empty, which is the normal null case for
create a new keyring, then the session object is empty as far as Go
is concerned. Its a bit of a Go-kernel mismatch where we are trying
to distinguish between not set and null.
I don't think there's a problem [2].
[1]: https://github.com/opencontainers/runtime-spec/blame/master/glossary.md#L23
[2]: https://play.golang.org/p/7RY4BA5uIxv
|
Ah yes, you can distinguish between empty but nil map value and non existent one, if you try. So default as per current behaviour could be
|
Currently, with `runc` we have a special cmdline flag `--no-new-keyring` for `runc run` that enables/disables the creation of a new kernel keyring. The main reason we have the option is that older kernels had issues with allocating a lot of keyrings (so in order to run containers on old kernels you need to disable the creation of a new keyring). This patch adds keyring support into part of the OCI spec which allows managers to drive this behavior in a runtime-agnostic way and helps make swapping in other runtimes easier. Fixes opencontainers#754 Fixes opencontainers#950 Signed-off-by: Kailun Qin <kailun.qin@intel.com>
currently
runc run
has a command line option--no-new-keyring
which disables the creation of an isolated kernel keyring for the process. This is kind of weird - I think this should be part of the OCI spec as it is just a specification of resource allocation, like having a new namespace. I can write up a proposal for this; obviously though this will be a breaking change so want to know what anyone else thinks.The text was updated successfully, but these errors were encountered: