config: adding labels #371

vbatts · 2016-10-06T08:22:22Z

the "Labels" field is not specified in the docker image spec, but is
present and used.
https://github.com/docker/docker/blob/master/image/spec/v1.2.md

This carries over that functionality

Signed-off-by: Vincent Batts [email protected]

jonboulle · 2016-10-06T13:02:45Z

config.md

        },
-        "WorkingDir": "/home/alice"
+        "WorkingDir": "/home/alice",
+	"Labels": {


indenting is off

jonboulle · 2016-10-06T13:02:58Z

Where is it specified in Docker? Only in code?

vbatts · 2016-10-06T13:47:42Z

yep

On Thu, Oct 6, 2016 at 9:02 AM, Jonathan Boulle [email protected]
wrote:

Where is it specified in Docker? Only in code?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#371 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEF6W5igbF5VdZywlSIK9Wr3tCEKn6rks5qxPGDgaJpZM4KPq7j
.

the "Labels" field is not specified in the docker image spec, but is present and used. https://github.com/docker/docker/blob/master/image/spec/v1.2.md This carries over that functionality Signed-off-by: Vincent Batts <[email protected]>

philips · 2016-10-07T13:02:36Z

config.md

     Sets the current working directory of the entrypoint process in the container.
     This value acts as a default and is replaced by a working directory specified when creating a container.

+   - **Labels ** *object*, OPTIONAL


Extra space

philips · 2016-10-07T13:02:43Z

LGTM

Signed-off-by: Jonathan Boulle <[email protected]>

jonboulle · 2016-10-07T13:30:07Z

@vbatts how can we be confident we aren't missing other fields that are present, used, but only defined in code?

coolljt0725 · 2016-10-08T08:28:58Z

@vbatts @jonboulle @philips
I think there are several other fields that are present and used in docker images, but not defined in the spec. The entire config of a docker image is https://github.com/docker/docker/blob/master/api/types/container/config.go#L36 , but Memory MemorySwap CpuShares are actually not used in docker images any more . Here is a config of an image pull from docker hub use skopeo.


  "config": {
    "Labels": {},
    "OnBuild": null,
    "OpenStdin": false,
    "Tty": false,
    "AttachStderr": false,
    "AttachStdout": false,
    "AttachStdin": false,
    "User": "",
    "Domainname": "",
    "Hostname": "b18b9cafe0e0",
    "StdinOnce": false,
    "Env": [
      "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
    ],
    "Cmd": [
      "/bin/bash"
    ],
    "ArgsEscaped": true,
    "Image": "sha256:16ca893df3275f0d891c5d004788609363adce7b6e6c36d67a8d5bf89c12d708",
    "Volumes": null,
    "WorkingDir": "",
    "Entrypoint": null
  },

From what I know, there are concepts of config(https://github.com/docker/docker/blob/master/api/types/container/config.go#L36) and hostConfig(https://github.com/docker/docker/blob/master/api/types/container/host_config.go#L278) in docker. config are supposed to be portable and will be commit to images, hostConfig are supposed to be non-portable and are not to be commit to images.
Memory MemorySwap CpuShares does exist in config of image before docker 1.7.0, but after moby/moby#10298, resource config will not commit to image any more.

Not related to this, I'm always curious about why we only have Memory MemorySwap CpuShares defined in the spec since there are many other cgroup resource config for container, after viewing the git history of config.go, I think it's because docker only have these three filed defined at the very beginning(https://github.com/docker/docker/pull/4085/files#diff-2660508eef7a8eeadb6d27b8bbf6d718R15), so the spec (https://github.com/docker/docker/pull/9560/files?short_path=f10cb3a#diff-f10cb3a0965ab8eb7051f8e367802e69) at that moment only define these three filed. and this spec doesn't updat after adding more fields and removing some fields.

So I'm just wondering should we add are all the missing but used and present filed of docker image to the config of spec and also removing Memory MemorySwap CpuShares so we can keep consistent with docker images?
My thoughts is that the config should only contains portable container runtime config like Env Cmd... and we define a annotations just like #372 does, so user can add implementation specified filed.

wking · 2016-10-10T18:44:48Z

On Sat, Oct 08, 2016 at 01:28:59AM -0700, Lei Jitang wrote:

So I'm just wondering should we add are all the missing but used and
present filed of docker image to the config of spec and also
removing Memory MemorySwap CpuShares so we can keep consistent
with docker images? My thoughts is that the config should only
contains portable container runtime config like Env Cmd... and
we define a annotations just like #372 does, so user can add
implementation specified filed.

The problem here is that “portable container runtime config” is a
pretty fuzzy boundary (opencontainers/runtime-spec#284). I think we
want to drop the image-spec runtime configuration, distribute the
runtime-spec runtime configuration (with whatever the image author
wants to put in it), and sanitize the config after unpacking it 1.
Sanitization code in opencontainers/runtime-tools#219.

If that's too big a departure from the current Docker approach to fit
into 1.0, I think we should blindly do whatever Docker is doing now
until we are ready to drop the image-spec runtime configuration ;).
But if we're intending to unpack a runtime-spec config, we'll have to
figure out the application/vnd.docker.container.image.v1+json →
runtime-spec config translation somewhere. I'd rather have that be an
image-build-time problem. And santization/localization would be a
post-unpack, just-before-runtime problem.

vbatts · 2016-10-19T16:29:11Z

@jonboulle we can be reasonably certain that there are other fields that are not defined, but only in code.

vbatts · 2016-10-19T16:30:50Z

@coolljt0725 on that note, I would be fine seeing Memory* and CPU* removed from the OCI image spec. Further, having an annotation that provides a hint for memory and CPU demands of the contained application.

vbatts · 2016-10-28T14:42:40Z

I'm tagging this for the v1.0.0-rc3 milestone, because it is needed for compatibility and it is something that the ecosystem has grown to depend on.

vbatts · 2016-11-01T15:02:34Z

@opencontainers/image-spec-maintainers PTAL

stevvooe · 2016-11-02T21:55:14Z

@vbatts I'm not quite sure what exactly is going on here with the extra fields. Relevant code is in https://github.com/docker/docker/blob/2a2f183627af25fb6f1c93c5fc2c4d04ab7e75bb/daemon/commit.go#L196. The interesting one to me is container_config and config. Further analysis must be done to see if these are applied at runtime.

Regarding labels, we need to be careful about the semantics of label inheritance. It is not well-defined in docker, at all, and there are some tricky situations to address, such as removing labels. It may be sufficient to say to inheritance model isn't specified, but we should at least study the problem.

For the most part, these missing fields are of little consequence, as docker maintains the original byte content, rather than relying on serialization round trips.

vbatts · 2016-11-03T20:55:13Z

@stevvooe i really do not follow you regarding "if these are applied at runtime". What are you expecting? Because it seems to me you are describing something more than just arbitrary metadata, which is the perception labels have to the majority of folks I have seen using them. It is not like an environment variable or something that is exposed into the container.

stevvooe · 2016-11-03T23:09:26Z

@vbatts I would not expect that labels on image be applied to a running container. With the way labels are used, it is a massive security hole. Unfortunately, looks like that cat is out of the bag: https://github.com/docker/docker/blob/2a2f183627af25fb6f1c93c5fc2c4d04ab7e75bb/daemon/commit.go#L67.

vbatts · 2016-11-04T01:32:06Z

I am not seeing this massive security hole. :-\

Is this cat the one regarding inheritance of labels? If so, it is as I
figured. They just stack or clobber.

On Thu, Nov 3, 2016 at 4:09 PM Stephen Day [email protected] wrote:

@vbatts https://github.com/vbatts I would not expect that labels on
image be applied to a running container. With the way labels are used, it
is a massive security hole. Unfortunately, looks like that cat is out of
the bag:
https://github.com/docker/docker/blob/2a2f183627af25fb6f1c93c5fc2c4d04ab7e75bb/daemon/commit.go#L67
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#371 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEF6X-Ux9zbMDLyZYLKgfbsH2z5vAP-ks5q6mmngaJpZM4KPq7j
.

stevvooe · 2016-11-16T23:02:07Z

LGTM

I am not seeing this massive security hole. :-\

Let's say I have a label-based load balancer or ACL system. One can then craft an image such that the label on the image will cause the created container to join the load balancing group or gain privileges that it doesn't already have.

This cat has outgrown the bag, so I'm not sure there is much to do from OCI's perspective.

stevvooe · 2016-11-16T23:19:29Z

@philips PTAL

philips · 2016-11-17T03:48:36Z

LGTM

As I said in opencontainers#371 (comment), I'd like to remove these resource limit from image-spec. On the on hand, if we have this, we should have limit the min and max size of these value. For example, in docker, there are some min and max limit for some resource, see https://github.com/docker/docker/blob/master/daemon/daemon_unix.go#L48 Signed-off-by: Lei Jitang <[email protected]>

As I said in opencontainers/image-spec#371 (comment), I'd like to remove these resource limit from image-spec. On the on hand, if we have this, we should have limit the min and max size of these value. For example, in docker, there are some min and max limit for some resource, see https://github.com/docker/docker/blob/master/daemon/daemon_unix.go#L48 Signed-off-by: Lei Jitang <[email protected]>

vbatts mentioned this pull request Oct 6, 2016

config: add annotations #372

Closed

jonboulle reviewed Oct 6, 2016

View reviewed changes

config: adding labels

021bb35

the "Labels" field is not specified in the docker image spec, but is present and used. https://github.com/docker/docker/blob/master/image/spec/v1.2.md This carries over that functionality Signed-off-by: Vincent Batts <[email protected]>

vbatts force-pushed the config_labels branch from c70c029 to 021bb35 Compare October 6, 2016 13:59

philips reviewed Oct 7, 2016

View reviewed changes

config: fix header formatting

6c271ab

Signed-off-by: Jonathan Boulle <[email protected]>

wking mentioned this pull request Oct 10, 2016

Clarify User and MemorySwap are platform specific #375

Merged

wking mentioned this pull request Oct 19, 2016

Created should be omitted from image json #399

Closed

vbatts added this to the v1.0.0-rc3 milestone Oct 28, 2016

stevvooe merged commit b716c40 into opencontainers:master Nov 17, 2016

coolljt0725 mentioned this pull request Nov 17, 2016

specs-go: add label to config.go #460

Merged

wking mentioned this pull request Nov 18, 2016

lables: use labels instead of annotation in the description of labels #464

Merged

cgonyeo mentioned this pull request Nov 30, 2016

Labels field missing from exported struct #478

Closed

vbatts deleted the config_labels branch December 9, 2016 14:24

coolljt0725 mentioned this pull request Dec 14, 2016

Remove Memory MemorySwap CpuShares from config #495

Merged

jonjohnsonjr mentioned this pull request Mar 29, 2021

Clarrify usage of predefined annotations as labels within image config #833

Open

config: adding labels #371

config: adding labels #371

Uh oh!

Conversation

vbatts commented Oct 6, 2016

Uh oh!

jonboulle Oct 6, 2016

Choose a reason for hiding this comment

Uh oh!

vbatts Oct 6, 2016

Choose a reason for hiding this comment

Uh oh!

jonboulle commented Oct 6, 2016

Uh oh!

vbatts commented Oct 6, 2016

Uh oh!

philips Oct 7, 2016

Choose a reason for hiding this comment

Uh oh!

philips commented Oct 7, 2016 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonboulle commented Oct 7, 2016

Uh oh!

coolljt0725 commented Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wking commented Oct 10, 2016

Uh oh!

vbatts commented Oct 19, 2016

Uh oh!

vbatts commented Oct 19, 2016

Uh oh!

vbatts commented Oct 28, 2016

Uh oh!

vbatts commented Nov 1, 2016

Uh oh!

stevvooe commented Nov 2, 2016

Uh oh!

vbatts commented Nov 3, 2016

Uh oh!

stevvooe commented Nov 3, 2016

Uh oh!

vbatts commented Nov 4, 2016

Uh oh!

stevvooe commented Nov 16, 2016 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevvooe commented Nov 16, 2016

Uh oh!

philips commented Nov 17, 2016 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

philips commented Oct 7, 2016 •

edited by caniszczyk

Loading

coolljt0725 commented Oct 8, 2016 •

edited

Loading

stevvooe commented Nov 16, 2016 •

edited by caniszczyk

Loading

philips commented Nov 17, 2016 •

edited by caniszczyk

Loading