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

Convert a serialization config JSON to a OCI runtime configuration. #87

Closed
s-urbaniak opened this issue May 25, 2016 · 24 comments
Closed

Comments

@s-urbaniak
Copy link
Collaborator

As per #82 (comment): Currently, the oci-image-tool supports a simple unpacking mechanism to inspect deflated images and validation.

More concretely the config/image JSON https://github.com/opencontainers/image-spec/blob/master/serialization.md#image-json-description has to be converted to a https://github.com/opencontainers/runtime-spec/blob/master/config.md#container-configuration-file.

@wking
Copy link
Contributor

wking commented May 25, 2016

On Tue, May 24, 2016 at 06:44:43PM -0700, Sergiusz Urbaniak wrote:

More concretely the config/image JSON
https://github.com/opencontainers/image-spec/blob/master/serialization.md#image-json-description
has to be converted to a
https://github.com/opencontainers/runtime-spec/blob/master/config.md#container-configuration-file.

I don't think this is possible. For example, the image-spec schema
currently has ‘ExposedPorts’, but networking is out-of-scope for the
runtime config 1. You'd have to select a particular network manager
(that isn't covered by an OCI spec) and setup hooks to drive that
network manager.

Stepping back, I see no reason for the image-spec repo to care about
what is in the runtime config (see also @stevvooe's “decouple”
wording in 2). I think imaging tools need to know “this is a
directory that I'm (de)serializing”. Then the image spec can add on
distribution-oriented layers like “distribute that directory using
layered tarballs” or “sign name ↔ directory associations”. But baking
knowledge of the filesystem directory (e.g. there's a ‘config.json’
specified by runtime-spec, there may be a ‘rootfs’ directory
referenced from ‘root.path’ in that config.json, …) into the image
spec seems like it's breaking a perfectly good abstraction boundary
for no purpose.

If the “we don't care what's inside the directory” approach from my
last paragraph gains traction, there will still be a need for some
conversion logic for backwards compatibility with existing images.
But I think we want the image spec to get out of that business (and
let the runtimes handle it).

 Subject: Do we have any plan for network and storage specs?
 Date: Fri, 14 Aug 2015 02:36:15 -0700 (PDT)
 Message-Id: <[email protected]>

@philips
Copy link
Contributor

philips commented May 25, 2016

@wking exposed ports is useful metadata from the image. I think it needs to exist to retain useful UX from builder to runner.

I am really really unclear on what you want to see here.

Furthermore, retaining the UX of container runtimes is a desirable part of the spec and explicitly called out in the README and image format spec project proposal:

docker run example.com/org/app:v1.0.0
rkt run example.com/org/app,version=v1.0.0

@wking
Copy link
Contributor

wking commented May 25, 2016

On Wed, May 25, 2016 at 08:22:16AM -0700, Brandon Philips wrote:

@wking exposed ports is useful metadata from the image. I think it
needs to exist to retain useful UX from builder to runner.

Then I think you want to file a PR against runtime-spec (or a
networking OCI spec?) to get it supported. I don't think image-spec
is a good place to specify container networking.

I am really really unclear on what you want to see here.

As I understand it, #82 handles unpacking a local directory from layer
tarballs. I want to see image-spec drop
application/vnd.oci.image.serialization.config.v1+json, and have
runtime-spec claim a MIME type for config.json. Then
application/vnd.oci.image.manifest.v1+json is just a list of layers
(and optional annotations). Then runtime-spec can wander around
however it likes and image-spec would still be able to distribute the
new configs without changes. And folks could add alternative runtime
configs, and image-spec could distribute them without changes. And I
could package up a non-container package (e.g. a Gentoo stage3) and
use image-spec tooling to distribute that too (although I obviously
wouldn't want to point runC at the config.json-less unpacked
directory).

Teaching image-spec about what's inside the distributed directory
pokes holes in the runtime's abstraction, and defining a local config
that has to be translated to a runtime-spec config pokes big holes in
the abstraction. Sometimes that sort of abstraction-breaking is
useful, but in this case I don't see an upside, and I see flexibility
and maintenance downsides.

@philips
Copy link
Contributor

philips commented May 25, 2016

@wking the runtime can IGNORE this information and it can be used by a higher level system. I will file an issue about how to convert it but I don't think dropping it makes sense.

@philips
Copy link
Contributor

philips commented May 25, 2016

@wking I disagree with your suggestion of the split. The image format needs a way of expressing runtime defaults that aren't host specific which the runtime spec has not tackled and made difficult by recombining everything into a single config.json.

@wking
Copy link
Contributor

wking commented May 25, 2016

On Wed, May 25, 2016 at 09:11:49AM -0700, Brandon Philips wrote:

the runtime can IGNORE this information and it can be used by a
higher level system.

And I'm fine with that, although I think we want to be careful about
how external tools like network managers are included [1]. I just
don't think the image-spec should be in the business of orchestrating
this. You can have:

  • Orchestration layer
    • Call image-spec tooling to fetch a generic directory by name
    • Call network-setup tooling to convert a config file's ExposedPorts
      entries to libnetwork hooks (or some such).
    • Call ocitools to translate fromContainer entries in the config.
    • … Call other tools to localize the config …
    • Call runtime-spec tooling (e.g. runC) to launch the adapted config.

I think that such a narrowly-scoped image-spec would be easier to
maintain and use than a broadly-scoped image-spec like:

  • Orchestration layer
    • image-spec
      • Call image-spec tooling to fetch a generic directory by name
      • Call network-setup tooling to convert a config file's
        ExposedPorts entries to libnetwork hooks (or some such).
      • Call ocitools to translate fromContainer entries in the config.
      • … Call other tools to localize the config …
    • Call runtime-spec tooling (e.g. runC) to launch the adapted config.

With the narrowly-scoped image-spec, you can add as many localization
steps to your orchestration layer as you like, and don't have to file
image-spec PRs for “please have ocitools translate fromContainer”,
etc.

On Wed, May 25, 2016 at 09:13:45AM -0700, Brandon Philips wrote:

The image format needs a way of expressing runtime defaults that
aren't host specific which the runtime spec has not tackled and made
difficult by recombining everything into a single config.json.

The orchestration layer needs to do that sort of thing, but I don't
see how it benefits from being tied into image-spec (which is about
efficiently and securely shipping directories between hosts).

s-urbaniak pushed a commit to s-urbaniak/image-spec that referenced this issue Jun 2, 2016
s-urbaniak pushed a commit to s-urbaniak/image-spec that referenced this issue Jun 2, 2016
s-urbaniak pushed a commit to s-urbaniak/image-spec that referenced this issue Jun 3, 2016
s-urbaniak pushed a commit to s-urbaniak/image-spec that referenced this issue Jun 6, 2016
s-urbaniak pushed a commit to s-urbaniak/image-spec that referenced this issue Jun 6, 2016
@philips
Copy link
Contributor

philips commented Jun 7, 2016

I think what we should do is just have an annotation on the runtime spec config that lists the exposed ports. Something like:

{
    "ociVersion": "0.2.0",
...
    "annotations": {
        "opencontainers.org/image-spec/exposedPorts": "8080,53/udp"
    }
}

Then that can be consumed by whatever tool is setting up the networking.

@wking
Copy link
Contributor

wking commented Jun 7, 2016

On Mon, Jun 06, 2016 at 10:50:41PM -0700, Brandon Philips wrote:

I think what we should do is just have an annotation on the runtime
spec config…

This approach doesn't work as well for more complex values like the
‘history’ structure 1. I'd rather drop the hashmap restriction on
‘annotations’ 2 and embed the whole serialization config there, or
copy the full serialization config to separate file in the unpacked
bundle. I'm not entirely clear on what the ‘history’ field is for (UI
hints?), but having it and a record of DiffIDs in the unpacked bundle
may make it easier for tools to build a new image that recycles a
parent's images layer tarballs. You'll probably want a copy of the
manifest too, so you can use the same digests as the parent image 3
without having to maintain an external DiffID → digest lookup service.

@philips
Copy link
Contributor

philips commented Jun 7, 2016

To what end do we add stuff to the runtime config.json though? We could simply say that the original manifest sits inside the bundle instead.

The reason this annotation is needed in the container runtime is because presumably an OCI network plugin would need to read this to setup forwarding firewalls or something.

cc @opencontainers/runtime-spec-maintainers to discuss. @mrunalp lets put this on the agenda tomorrow on how these things interoperate.

@wking
Copy link
Contributor

wking commented Jun 7, 2016

On Tue, Jun 07, 2016 at 04:40:25PM -0700, Brandon Philips wrote:

To what end do we add stuff to the runtime config.json though? We
could simply say that the original manifest sits inside the bundle
instead.

“copy the full serialization config to separate file” was one of my
options 1.

The reason this annotation is needed in the container runtime is
because presumably an OCI network plugin would need to read this to
setup forwarding firewalls or something.

And they can access that information in the bundle directory without
us needing to put it in the annotations field 2.

@philips
Copy link
Contributor

philips commented Jun 7, 2016

@wking fair point. I think the runtime spec maintainers should chime in on this point of external files vs serialized metadata into the config.json.

@wking
Copy link
Contributor

wking commented Jun 8, 2016

On Tue, Jun 07, 2016 at 04:50:56PM -0700, Brandon Philips wrote:

I think the runtime spec maintainers should chime in on this point
of external files vs serialized metadata into the config.json.

I've filed opencontainers/runtime-spec#492 to see if they have any
generic opinions on this sort of thing.

@wking
Copy link
Contributor

wking commented Jun 14, 2016

On Wed, May 25, 2016 at 01:21:39PM -0700, W. Trevor King wrote 1:

I just don't think the image-spec should be in the business of
orchestrating this.

Trying another angle at this…

If the mapping from RunConfig 2 to runtime-spec's config.json 3 is
supposed to be complete 4 and deterministic, what do we gain by
making the translation a post-fetch action instead of making it a
pre-publish action? If image-spec transmits an opaque directory, you
get complete decoupling between image-spec and the payload. That
means image-spec is:

  1. Independent of runtime-spec evolution.
  2. Capable of transmitting other image formats besides runtime-spec
    versions.
  3. Capable of transmitting runtime-spec seed configurations (e.g. ones
    that use fromContainer settings 1).

The only possible benefit I see to funneling through RunConfig is that
you restrict the ability of publishers to push malicious bundles. But
you can get the same protection by requiring a runtime-spec
config.json and inspecting it for fields that the current RunConfig →
config.json mapping doesn't set.

@philips
Copy link
Contributor

philips commented Jun 14, 2016

@wking the main reason for keeping the config format as-is: 7.g "The first version of the OCI Specification should strive to be backwards compatible with the initial container image format..."

We might add a new media-type to the image spec to allow folks to provide a runtime spec directly but in its current form this spec only requires users to change the media-types in the manifest with a %s%docker%oci% and leave all of the objects like this config, and the filesystem serializations untouched.

@wking
Copy link
Contributor

wking commented Jun 14, 2016

On Tue, Jun 14, 2016 at 10:17:46AM -0700, Brandon Philips wrote:

@wking the main reason for keeping the config format as-is: 7.g "The
first version of the OCI Specification should strive to be backwards
compatible with the initial container image format..."

I'm not clear on spec entries for backwards compat vs. tooling support
for backwards compat (I prefer restricting it to the tooling 1 and
migration notes 2). But if the goal is just backwards compat with
existing Docker versions, I can take a stab at an oci-image-tool
update that will make us both happy ;). I'll try to get at least a
sketch up by tomorrow's meeting.

@philips
Copy link
Contributor

philips commented Jun 15, 2016

@wking There is a cost to throwing out backwards compatibility at the format level: large numbers of objects in a registry CAS need to be re-interpreted. Because only the manifest and manifest list change with the %s%docker%oci%g of the media types a registry can start serving OCI manifests without changing any stored objects.

@wking
Copy link
Contributor

wking commented Jun 15, 2016

On Wed, Jun 15, 2016 at 07:59:45AM -0700, Brandon Philips wrote:

@wking There is a cost to throwing out backwards compatibility at
the format level: large numbers of objects in a registry CAS need to
be re-interpreted.

That's only true if you want the registry to translate versions on the
fly, and I don't see a need for that. For example, I have v1.1
through v1.7 PDFs on my hard-drive, but evince 3.16.1 based on poppler
can 0.42 can read them all without trouble (even though I don't have
tooling to translate v1.1 to v1.7 or vice versa). An older
evince/poppler pair would not be capable of reading the v1.7 PDFs, but
that's not a big deal, because it never could. I don't think we need
on-the-fly translation to serve new-version images to old-version-only
clients. Publishers interested in supporting old-version-only clients
can use application/vnd.docker.distribution.manifest.list.v2+json
which includes application/vnd.docker.distribution.manifest.v2+json,
but that's not a registry concern.

wking referenced this issue Sep 7, 2016
The validation/unpacking code doesn't really care what the reference
and CAS implemenations are.  And the new generic interfaces in
image/refs and image/cas will scale better as we add new backends than
the walker interface.

The old tar/directory distinction between image and imageLayout is
gone.  The new CAS/refs engines don't support directory backends yet
(I plan on adding them once the engine framework lands), but the new
framework will handle tar/directory/... detection inside
layout.NewEngine (and possibly inside a new (cas|refs).NewEngine when
we grow engine types that aren't based on image-layout).

I'd prefer casLayout and refsLayout for the imported packages, but
Stephen doesn't want camelCase for package names [1].

[1]: #159 (comment)

Signed-off-by: W. Trevor King <[email protected]>
@philips
Copy link
Contributor

philips commented Sep 21, 2016

This issue was moved to opencontainers/image-tools#25

@philips philips closed this as completed Sep 21, 2016
@wking
Copy link
Contributor

wking commented Sep 21, 2016

On Tue, Sep 20, 2016 at 07:49:16PM -0700, Brandon Philips wrote:

This issue was moved to opencontainers/image-tools#25

I think we want to re-open this as:

Can we drop the ‘config’ property from
application/vnd.oci.image.config.v1+json and either inline the
runtime-spec config entirely or distribute it as part of the layered
directory?

That's a spec decision, not a tooling decision. And I still see a
reduction in duplicated effort with no downside (users that don't want
to migrate can continue using config media types that use the old
‘config’ property). My most recent attempt to explain this is in 1.

wking added a commit to wking/ocitools-v2 that referenced this issue Sep 22, 2016
Based on image-spec, which currently exposes [1]:

* User (user and group by ID or name)
  Represented in runtime-spec by process.user.*, although I'm clearing
  additionalGids for now because the old formats gave no way to
  represent that.
* Memory (limit)
  Represented in runtime-spec by linux.resources.memory.limit and
  solaris.cappedMemory.physical.
* MemorySwap (memory + swap limit)
  Represented in runtime-spec by linux.resources.memory.swap and
  solaris.cappedMemory.physical + solaris.cappedMemory.swap.
* CpuShares (relative weight vs. other containers)
  Represented in runtime-spec by linux.resources.cpu.shares.  Solaris
  has an ncpus property, but no shares analog.
* ExposedPorts (a set of port/protocol entries)
  This is not covered by runtime-spec, but image-spec was planning on
  stuffing it into annotations [2].
* Env (array of environment variables)
  Represented in runtime-spec by process.env.
* Entrypoint (array of positional arguments)
  Represented in runtime-spec by process.cmd.
* Cmd (array of positional arguments)
  Represented in runtime-spec by process.cmd.
* Volumes (set of directories which should have data volumes mounted
  on them)
  Represented in runtime-spec by mounts, although Volumes doesn't
  include source locations.
* WorkingDir (initial working directory of container process)
  Represented in runtime-spec by process.cwd.

Entrypoint and Cmd are slightly different and have a few different
forms each [3,4].  Both are represented in the runtime-spec config by
process.args.

This commit sets all other config settings to their default values,
and it clears mounts[].source to mimic the Volumes information.
image-spec currently sets up source-less bind-mounts when translating
Volumes [5].

With the sanitization command, *any* runtime configuration can be
sanitized to be just as safe and limited as the current image-spec
config.

[1]: https://github.com/opencontainers/image-spec/blob/v0.5.0/serialization.md#container-runconfig-field-descriptions
[2]: opencontainers/image-spec#87 (comment)
     Subject: Convert a serialization config JSON to a OCI runtime
       configuration.
[3]: https://docs.docker.com/engine/reference/builder/#entrypoint
[4]: https://docs.docker.com/engine/reference/builder/#cmd
[5]: https://github.com/opencontainers/image-spec/blob/v0.5.0/image/config.go#L127-L136

Signed-off-by: W. Trevor King <[email protected]>
@jonboulle
Copy link
Contributor

Re-opening this as the topic is still coming up in various contexts and it's nagging at me. Even if we don't define a process here, we should have some nod towards what users should do (e.g. given an image-spec compatible image how do I even start to think about ending up with a runtime-spec compatible bundle?)

@jonboulle jonboulle reopened this Nov 22, 2016
@cyphar
Copy link
Member

cyphar commented Nov 22, 2016

For reference, I've implemented this in umoci so maybe that would be a good source of inspiration? I feel like there's a few questions that you can't really give a nice answer to without knowing more about the context that the image is being used (like what ExposedPorts or Volumes should expand to).

@wking
Copy link
Contributor

wking commented Nov 22, 2016

Even if we don't define a process here, we should have some nod towards what users should do...

#454 has a very small step in this direction, although to get something reliable / certifiable you'd probably need to specify how more runtime-config field are populated.

#451 avoids translation entirely, letting the image-spec tooling treat the config as an opaque blob.

@cyphar
Copy link
Member

cyphar commented Jun 16, 2017

This is effectively done with #492. Though there's still some in-flight PRs that clean up some of the language.

@stevvooe
Copy link
Contributor

I would consider this closed. If there are further questions, please open more actionable issues with specific questions.

@cyphar Thanks for the great work on #492!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants