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

conversion: add document about image -> runtime configuration #492

Merged
merged 1 commit into from May 31, 2017
Merged

conversion: add document about image -> runtime configuration #492

merged 1 commit into from May 31, 2017

Conversation

cyphar
Copy link
Member

@cyphar cyphar commented Dec 12, 2016

Add documentation about how to convert from an OCI image configuration
to an OCI runtime configuration. In particular, describe precisely what
fields need to be filled given a particular configuration.

The fields have been grouped into several categories, because some of
the image fields are not as well-thought-out as others (such as the
resource limitation fields, which should really be decided by the user
not by the image creator). In addition, some fields (such as Volumes)
cannot be understood by a generic configuration converter.

Fixes: #479
Signed-off-by: Aleksa Sarai asarai@suse.de

@cyphar cyphar changed the title conversion: add documentation about image config -> runtime config conversion: add document about image -> runtime configuration Dec 12, 2016
Copy link
Contributor

@wking wking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to specify defaults for runtime-spec fields not set in the image? E.g. in a converted Linux config.json, should linux.namespaces be set?

Also, do we have version requirements for the generated config.json? Or is the converter free to target any tagged runtime-spec release?

conversion.md Outdated
@@ -0,0 +1,89 @@
# Conversion to OCI Runtime Configuration

When extracting an OCI Image into an [OCI Runtime bundle][oci-runtime-bundle], two components of the orthogonal components of the extraction are relevant:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“components of the orthogonal components of the extraction” → “orthogonal components”?

conversion.md Outdated
| `Config.Cmd` | `process.args` | 2 |
| `Config.Labels` | `annotations` | 3 |

1. The converter MAY *prepend* additional environment variables to `process.env`, but it MUST NOT *append* values to the environment.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From POSIX:

There is no meaning associated with the order of strings in the environment. If more than one string in an environment of a process has the same name, the consequences are undefined.

So I think we should replace the append/prepend rule with one about not duplicating names.

conversion.md Outdated

| Image Field | Runtime Field | Notes |
| ------------------- | --------------- | ----- |
| `Config.User` | `process.user.*` | |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless we have notes entries, I'd rather drop the column. You have notes on this conversion below, so maybe add [notes](#config-user) or similar to the notes column here?

conversion.md Outdated
### `Config.User`

When converting the value of `Config.User` a converter SHOULD resolve the user information using a method appropriate for the container's context.
For Linux based systems this would involve parsing `/etc/passwd` from the extracted container's root filesystem to determine the values of `process.user.uid` and `process.user.gid`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The config docs allow both numeric and named user/group values, and only mention /etc/passwd in the context of default groups. So I think we need to weaken this line to say “MAY involve parsing /etc/passwd and /etc/group” to allow for:

  • Configs using numberic uid/gids (where there is no need for a lookup)
  • Agents using other NSS backends (e.g. LDAP), since the config spec doesn't require a particular NSS backend.

conversion.md Outdated
When converting the value of `Config.User` a converter SHOULD resolve the user information using a method appropriate for the container's context.
For Linux based systems this would involve parsing `/etc/passwd` from the extracted container's root filesystem to determine the values of `process.user.uid` and `process.user.gid`.

In addition, a converter SHOULD set the value of `process.user.additionalGids` to a value corresponding to the user described by `Config.User`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably “corresponding to the user” → “corresponding to the group”.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because on Linux additionalGids is defined by group association of a user. Maybe "corresponding to the value in Config.User" if you want to make it more generic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. Maybe there is wording that makes “this is an NSS lookup in the container context” more obvious, so folks don't trip into the same hole I just did.

conversion.md Outdated

## Optional Fields

Certain image configuration fields are not considered to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence is incomplete.

conversion.md Outdated

| Image Field | Runtime Field | Notes |
| --------------------- | ------------------ | ----- |
| `Config.Memory` | platform dependant | 1 |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“dependant” → “dependent”

conversion.md Outdated
| `Config.ExposedPorts` | none | 2 |
| `Config.Volumes` | `mounts` | 3 |

1. The meaning of these fields is platform dependant, and runtimes MAY ignore these fields.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“runtimes” → “converters”

conversion.md Outdated
1. The meaning of these fields is platform dependant, and runtimes MAY ignore these fields.
On Linux these image fields correspond to the runtime fields `linux.resources.memory.limit`, `linux.resources.memory.swap`, and `linux.resources.cpu.shares` respectively.
2. The runtime configuration does not have a corresponding field for this image field.
Converters SHOULD use this as a hint to allow a container to provide a network service from the corresponding container-side port.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to SHOULD an annotations key for this information?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, but since we haven't defined such an annotation in the org.opencontainers. namespace I'm a bit iffy about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

… but since we haven't defined such an annotation in the org.opencontainers. namespace…

Can't we define one here? With something like:

Converters SHOULD set org.opencontainers.imageSpec.exposedPorts to [stringified value].

As @philips floated in #87.

conversion.md Outdated
2. The runtime configuration does not have a corresponding field for this image field.
Converters SHOULD use this as a hint to allow a container to provide a network service from the corresponding container-side port.
3. The converter MUST set the `destination` of the mountpoint to the value specified in `Config.Volumes`.
The other fields are platform and context dependant, though the converter SHOULD NOT allow volumes to be include in newly created [layers](layer.md).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“include” → “included”.

And “SHOULD NOT allow volumes to be included in the newly created layers” sounds like it may belong here, and not in the conversion spec.

conversion.md Outdated
3. `annotations` in the [manifest list](manifest-list.md) of an image.

A converter SHOULD extract all three of these sets of annotations to `annotations` in the runtime configuration.
However, since there may be conflicts between different sets of annotations (same key but different value), the order of precedence SHOULD be in the following (ascending) order:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this ordering?

To tell you the truth, this is why have been against adding annotations everywhere, especially when there isn't easy ways to distinguish runtime annotations/labels from those inherited from the source image.

To tell you the truth, this should probably be the opposite order. Manifests and Manifest-List tend not to survive to the runtime portion and are more oriented towards distribution.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ordering was based on what I thought made sense (the different blob layers are like onions, with each descriptor dereference going down a layer). But I wasn't sure why there were multi-level annotations in the first place (I assumed it was historical).

The other reason is that it means that if you share the same config between different manifests it's possible to override the config's annotations without needing to modify it -- if the ordering was in the reverse order then you wouldn't be able to do anything similar (you'd have to store two copies of the config).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To tell you the truth, this should probably be the opposite order. Manifests and Manifest-List tend not to survive to the runtime portion and are more oriented towards distribution.

These are in order of ascending precedence, and while that's a somewhat unusual way to list precedence, it does mean that Config.Labels has the highest precedence. I think that's what you're asking for too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other reason is that it means that if you share the same config between different manifests it's possible to override the config's annotations without needing to modify it…

Heh, so I read this completely wrong. Whatever we settle on, I suggest a clarifying example to avoid future confusion like mine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually meant descending order when writing this. But we should figure out what order makes sense first. I'd also understand if we don't include the annotations from manifests at all (which would be an even better idea because it would mean that the conversion is entirely done with v1.Image and doesn't require knowing what the manifest was).

That would make the distinction more clear between distribution and extraction/runtime.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also understand if we don't include the annotations from manifests at all (which would be an even better idea because it would mean that the conversion is entirely done with v1.Image and doesn't require knowing what the manifest was).

This sounds reasonable to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevvooe @philips What do you think? Should we not require that runtimes include Manifest and ManifestList annotations in the runtime bundle?

@cyphar
Copy link
Member Author

cyphar commented Dec 14, 2016

@stevvooe Can you take another look? I've switched around the wording and changed my annotation conversion description so that only Config.Labels MUST be extracted (and it takes precedence over any of the implicit conversions).

@stevvooe
Copy link
Contributor

@cyphar Could update this to reflect the removal of Memory, et. al.?

@cyphar
Copy link
Member Author

cyphar commented Dec 16, 2016

Yup, it's on my list of things to do. If I get the chance I'll do it this weekend or on Monday if I'm too busy.

@cyphar
Copy link
Member Author

cyphar commented Dec 18, 2016

@stevvooe Updated.

conversion.md Outdated
The former component of extraction is defined [elsewhere](layers.md) and is orthogonal to configuration of a runtime bundle.

[oci-runtime-bundle]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc2/bundle.md
[oci-runtime-config]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc2/config.md
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runtime-spec has cut an rc3.

conversion.md Outdated

| Image Field | Runtime Field | Notes |
| --------------------- | ------------------ | ----- |
| `Config.ExposedPorts` | none | 1 |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the verbatim section, you use annotations as the runtime field in cases like this.

conversion.md Outdated
| `Config.Volumes` | `mounts` | 2 |

1. The runtime configuration does not have a corresponding field for this image field.
However, converters SHOULD set the [`org.opencontainers.imageSpec.exposedPorts` annotation](#config.exposedports).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "... annotation to the JSON serialization of the ExpopsedPorts value? And copy footnote 1 about precedence from the verbatim table. Then we could drop the ExposedPorrs section below without losing specificity.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scratch this, because the JSON for ExposedPorts is wonky. Your current handling is fine, although coping down your precedence footnote is probably still a good idea for consistency.

conversion.md Outdated
If there is a conflict (same key but different value) between an implicit annotation and an explicitly specified annotation in `Config.Labels`, the value specified in `Config.Labels` MUST take precedence.

A converter MAY add annotations which have keys not specified in the image.
A converter MUST NOT modify the values of annotations specified in an image.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"an image" -> "the image" to match the preceding sentence and be more accurate.

Copy link
Contributor

@jonboulle jonboulle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at a first pass this looks good!

conversion.md Outdated

## Optional Fields

Certain image configuration fields are not applicable to all conversion usecases, and thus are optional for configuration converter to implement.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

converters

@cyphar
Copy link
Member Author

cyphar commented Dec 28, 2016

Can you all PTAL? Is there any outlying issues I need to fix up?

/ping @opencontainers/image-spec-maintainers

Copy link
Contributor

@wking wking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this still needs wording around the runtime-config properties which aren't related to the image config. Are they forbidden? Unspecified? Implementation-defined? For any fields not specified here, I prefer implementation-defined.

conversion.md Outdated
| `os` | `platform.os` | |
| `author` | `annotations` | 1,2 |
| `created` | `annotations` | 1,3 |
| `os` | `platform.os` | |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate entry for os.

conversion.md Outdated
### `Config.ExposedPorts`

The OCI runtime configuration does not provide a way of expressing the concept of "container exposed ports".
However, converters SHOULD provide a hint to the runtime what the set of container exposed ports are by using the **org.opencontainers.imageSpec.exposedPorts** annotation, unless doing so will [cause a conflict](#annotations).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"provide a hint to the runtime what the set of container exposed ports are by using" -> "set"?

conversion.md Outdated

If the values of [`user` or `group`](config.md#properties) in `Config.User` are numeric (`uid` or `gid`) then the values MUST be copied verbatim to `process.user.uid` and `process.user.gid` respectively.
If the values of [`user` or `group`](config.md#properties) in `Config.User` are not numeric (`user` or `group`) then a converter SHOULD resolve the user information using a method appropriate for the container's context.
For Linux and Solaris based systems, this MAY involve resolution through NSS or parsing `/etc/passwd` from the extracted container's root filesystem to determine the values of `process.user.uid` and `process.user.gid`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just disallow NSS? Is this happening anywhere today?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a compelling reason to disallow NSS. The point of this section is to say that the mapping of Config.User -> {uid, gid} is implementation defined, with some hints about how it should be done.

conversion.md Outdated

## Optional Fields

Certain image configuration fields are not applicable to all conversion usecases, and thus are optional for configuration converters to implement.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usecases -> use cases

conversion.md Outdated

This section defines how to convert an `application/vnd.oci.image.config.v1+json` blob to an [OCI runtime configuration blob][oci-runtime-config] (the latter component of extraction).
The former component of extraction is defined [elsewhere](layers.md) and is orthogonal to configuration of a runtime bundle.
The values of runtime configuration properties not specified by this document are implementation defined.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implementation defined -> implementation-defined

conversion.md Outdated
If the values of [`user` or `group`](config.md#properties) in `Config.User` are not numeric (`user` or `group`) then a converter SHOULD resolve the user information using a method appropriate for the container's context.
For Linux and Solaris based systems, this MAY involve resolution through NSS or parsing `/etc/passwd` from the extracted container's root filesystem to determine the values of `process.user.uid` and `process.user.gid`.

In addition, a converter SHOULD set the value of `process.user.additionalGids` to a value corresponding to the user described by `Config.User`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't sound right. Config.User should have a colon separated user and primary group. If there is no colon, the primary group is the user id. I don't think there is any support for supplementary (somehow, these where NIH'd to "additional") in the image format.

I am not sure why runtime-spec diverged from this. Having a separate field doesn't really provide much and makes it clear what is primary version. For example, in swarmkit, we have User and Groups, where user can be user or user:group and Groups is always supplementary.

I am not sure about the utility of supplementary groups in the image format.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is any support for supplementary (somehow, these where NIH'd to "additional") in the image format.

The config doesn't support it directly, which is why the following line suggests a way to extract this from the root filesystem or other source (“…MAY involve resolution through NSS or parsing /etc/group…”). Resolving this via the root filesystem is the same type of thing that this conversion doc suggest for non-numeric users and groups, although @jonboulle recently questioned the usefulness of generic NSS vs. requiring /etc/passwd / /etc/group.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevvooe To clarify, "corresponding to the user" refers to the concept of a user in the image as opposed to the user string. The utility is so that groups like video, dialout and audio all still work inside containers.

I actually completely agree with what you said in your first paragraph. The point is that the resolution of supplementary groups (either through NSS or /etc/group) is recommended.

conversion.md Outdated
| `Config.Labels` | `annotations` | 6 |

1. If a user has explicitly specified this annotation with `Config.Labels`, then the value specified in this field takes lower [precedence](#annotations) and the converter MUST instead use the value from `Config.Labels`.
2. This value of this field MUST be set as the value of `org.opencontainers.authors` in `annotations`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's separate the active fields from the annotation fields.

Also, we need to be careful about the impact of picking up image labels into runtime annotations. I know I am having trouble convincing everyone this isn't a good idea, but it is a security hole waiting to happen.

conversion.md Outdated
For Linux and Solaris based systems, this MAY involve resolution through NSS or parsing `/etc/passwd` from the extracted container's root filesystem to determine the values of `process.user.uid` and `process.user.gid`.

In addition, a converter SHOULD set the value of `process.user.additionalGids` to a value corresponding to the user described by `Config.User`.
For Linux and Solaris based systems, this MAY involve resolution through NSS or parsing `/etc/group` and determining the group memberships of the user specified in `process.user.uid`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unices?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change it to Unix-like

conversion.md Outdated

1. The runtime configuration does not have a corresponding field for this image field.
However, converters SHOULD set the [`org.opencontainers.imageSpec.exposedPorts` annotation](#config.exposedports).
2. If a converter implements conversion for this field, it MUST set the `destination` of the mountpoint to the value specified in `Config.Volumes`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Config.Volumes field doesn't necessarily require a mount. That is really an implementation detail.

Config.Volumes really just marks a section of the file system as not part of the primary container filesystem.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, but is there a way of phrasing it in terms of the runtime specification to guarantee this? Maybe there is another way of pasting together a filesystem, but I'm having trouble thinking of one at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a good suggestion here. The volume really acts as almost a "mask" of the filesystem. The implementation of the mask can vary.

conversion.md Outdated
2. This value of this field MUST be set as the value of `org.opencontainers.authors` in `annotations`.
3. This value of this field MUST be set as the value of `org.opencontainers.created` in `annotations`.
4. The converter MAY add additional entries to `process.env` but it MUST NOT add entries that have variable names present in `Config.Env`.
5. If both `Config.Entrypoint` and `Config.Cmd` are specified, the converter MUST append the value of `Config.Cmd` to the value of `Config.Entrypoint` and set `process.args` to that combined value.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MUST is too strong here when taking user input for entrypoint and args.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main issue with entrypoint and cmd is that it's not really clear anywhere (outside of the Docker documentation) what their individual purposes are. The semantics also are quite hazy if you read config.md (and I think there's some hard-to-parse sentences there too).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cyphar Entrypoint and Cmd are indeed a bit of a mess. We've shored this up slightly in the docker documentation.

If you can find a way to word it such that implementations may substitute either Entrypoint or Cmd with user input, that would shore up the problem with this statement.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can find a way to word it such that implementations may substitute either Entrypoint or Cmd with user input, that would shore up the problem with this statement.

Without an API spec to pin down “user input”, I think this is going to be hard to do. Can the user-input additions be a follow-up step? So:

image config →{this translation}→ runtime config →{user input}→ runtime config

Then whether the translator handles this translation correctly can be validated without worrying about user input, and whether the user-input injection happens correctly is up to whoever specified the user-input API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wking Not really.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you need to know where the break between the entrypoint and command is, you can turn it around and have:

image config →{user input}→ image config →{this translation}→ runtime config

Then @cyphar's current append wording matches what is listed here for the case where both are arrays of strings. Or is image spec supposed to support the shell form versions too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wking

Or is image spec supposed to support the shell form versions too?

Absolutely not. IMO those were a bad decision which Docker has had to stick with because of backwards compatibility issues.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevvooe I'll give it a shot in terms of wording.

conversion.md Outdated

**org.opencontainers.imageSpec.exposedPorts** is a list of comma-separated values that correspond to the [keys defined for `Config.ExposedPorts`](config.md).

## Annotations
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as pointed out in #541 - can we have an optional, but still org.opencontainers.* prefixed, annotation which specifies the signal the image author wants the container to be kill'ed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have already been here. Is it missing from the config?

https://github.com/docker/docker/blob/master/api/types/container/config.go#L59

@wking wking mentioned this pull request Jan 30, 2017
@cyphar
Copy link
Member Author

cyphar commented Feb 14, 2017

I would prefer if we can get this into one of the next milestones, @opencontainers/image-spec-maintainers. WDYT?

@cyphar
Copy link
Member Author

cyphar commented Feb 27, 2017

/ping @stevvooe You said you'd take another look at this?

Copy link
Contributor

@jonboulle jonboulle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm overall, minor things but also happy to fix this post-merge (although I guess this needs a rebase anyway now)

conversion.md Outdated

1. `Config.Labels` in the [configuration](config.md) of the image.
2. `annotations` in the [manifest](manifest.md) of the image.
3. `annotations` in the [manifest list](manifest-list.md) of the image.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

manifest list -> image index

conversion.md Outdated
The former component of extraction is defined [elsewhere](layers.md) and is orthogonal to configuration of a runtime bundle.
The values of runtime configuration properties not specified by this document are implementation-defined.

[oci-runtime-bundle]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc3/bundle.md
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably bump to latest rc

conversion.md Outdated
3. `annotations` in the [manifest list](manifest-list.md) of the image.

In addition, there are also implicit annotations that are defined by this section which are determined from the values of the image configuration.
A converter SHOULD NOT attempt to extract annotations from [manifests](manifest.md) or [manifest lists](manifest-list.md).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

manifest list -> image index

conversion.md Outdated

1. The converter MAY add additional entries to `process.env` but it MUST NOT add entries that have variable names present in `Config.Env`.
2. If both `Config.Entrypoint` and `Config.Cmd` are specified, the converter MUST append the value of `Config.Cmd` to the value of `Config.Entrypoint` and set `process.args` to that combined value.
However, if a user has specified an alternate value for `Config.Entrypoint` or `Config.Cmd`, the user provided value SHOULD supercede the configuration-defined value in the above computation of `process.args`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer "supersede"

@vbatts
Copy link
Member

vbatts commented May 19, 2017

@cyphar bump. please ❤️

@cyphar
Copy link
Member Author

cyphar commented May 20, 2017

@vbatts I was having trouble tracking what issues have not already been addressed. Here is my understanding of the current issues:

  • Config.User (@stevvooe) appears to have been addressed. The concern was the difficulty in requiring converters to implement getpwnam(3) but my response was that implementing user:group support is only a recommendation (and the method of resolving them is also only a recommendation) and only numeric uid:gid forms MUST be implemented verbatim.

  • The worries about Config.Env and other user-changable options (@crosbymichael + @stevvooe) should be resolved by the new wording which was sent after the last comments on those issues. There hasn't been much discussion in this thread recently so I don't know if anyone actually objects to @philips's proposal (and my extensions to it).

The one final point which we discussed in the call and also mentioned by @crosbymichael is

[...] I'm wondering how you will identify what a converter is and what an end user is?

The distinction in this document is not between end users and converters. It's between implementation-defined defaults and externally provided inputs. In the basic case there is no distinction, but for the spec the difference is quite important.

An implementation-defined default is something that we can test by providing a bunch of images that contain various bits of metadata and our testing will only check the outputs that were actually specified in the image (the image is the source of truth). Any extra fields or whatever don't matter. But if the converter changes fields that we specified in the image, that's where non-compliance kicks in.

When it comes to externally defined input, the way I'm thinking of it is that when an externally provided input is specified it is applied to the source image as though the source image always had the value specified. For a more concrete example, imagine that umoci had some flags to modify the configuration. The following two examples would be identical.

% # unpack the image, with an externally provided value for Config.User
% umoci unpack --image theimage:latest --override=config.user="someone:else" bundle
% # modify the image and change Config.User
% umoci config --config.user="someone:else" --image theimage:latest
% umoci unpack --image theimage:latest bundle

But the important point is that a converter wouldn't be able to change Config.User without being told to by the user.

And please note that this entire document is describing the "default generated runtime configuration". The point is that if a converter decides to start overriding things it's okay as long as they specify that they aren't outputting the default generated runtime configuration (and maybe we should require that implementations provide a way to use the "default generated runtime configuration" -- I'm not sure).

However, converters SHOULD set the [`org.opencontainers.image.exposedPorts` annotation](#config.exposedports).
2. If a converter implements conversion for this field using mountpoints, it SHOULD set the `destination` of the mountpoint to the value specified in `Config.Volumes`.
The other `mounts` fields are platform and context dependent, and thus are implementation-defined.
Note that the implementation of `Config.Volumes` need not use mountpoints, as it is effectively a mask of the filesystem.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cyphar could you help me fully understand point 2 for Config.Volumes; the first line reads:

If a converter implements conversion for this field using mountpoints, it SHOULD set the destination of the mountpoint to the value specified in Config.Volumes.

what does that mean? are there any other way to implement conversion for this field other than mountpoints? if yes, which ones?
How's the expected flow to implement the field with mountpoints for runtimes? create a host dir for that volume and bind mount it in the container?

Note that the implementation of Config.Volumes need not use mountpoints, as it is effectively a mask of the filesystem.

as a non-native speaker this seems in contrast with the first sentence. It now reads "don't use mountpoints!"

Copy link
Member Author

@cyphar cyphar May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there any other way to implement conversion for this field other than mountpoints?

Yes, you could just copy the contents of the volume into the container. Hopefully (see #496) the purpose of volumes is to restrict how implementations should handle diff layers (allow external data that is not snapshotted).

In the case of umoci I could envisage this all being done through manifests and black-holing certain directories so umoci will simply ignore them.

How's the expected flow to implement the field with mountpoints for runtimes?

I only mention the destination. In particular what this is meant to mean is that the value of Config.Volumes should be a mountpoint (though there are reasons to not do that). The source of the mountpoint, the type of filesystem and so on can be whatever you want (it could be a tmpfs for example or NFS).

as a non-native speaker this seems in contrast with the first sentence. It now reads "don't use mountpoints!"

The first sentence says you SHOULD, but the second sentence says you don't have to. To be fair, maybe I shouldn't repeat that point but I felt worried people would make the same assumption you did.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you could just copy the contents of the volume into the container.

alright, I guess what's still not clear to me is the definition of a volume which this doc is missing. What's a volume then? some directory/mountpoint on the host or somewhere else?
from that sentence, as you said, it can be anything.

However, converters SHOULD set the [`org.opencontainers.image.exposedPorts` annotation](#config.exposedports).
2. If a converter implements conversion for this field using mountpoints, it SHOULD set the `destination` of the mountpoint to the value specified in `Config.Volumes`.
The other `mounts` fields are platform and context dependent, and thus are implementation-defined.
Note that the implementation of `Config.Volumes` need not use mountpoints, as it is effectively a mask of the filesystem.
Copy link
Contributor