common: create stdio symlinks if not present #97

iaguis · 2015-10-20T10:25:52Z

Docker images expect /dev/stdin, /dev/stdout, /dev/stderr and /dev/fd
(https://github.com/opencontainers/runc/blob/master/libcontainer/SPEC.md#filesystem)
but appc/spec doesn't
(https://github.com/appc/spec/blob/master/spec/OS-SPEC.md#devices-and-file-systems).

This adds the mentioned symlinks to the converted ACIs so apps can find them as expected.

Fixes #96

alban · 2015-10-22T10:46:53Z

lib/common/common.go

+	}
+
+	for name, target := range stdioSymlinks {
+		if _, ok := filemap[name]; !ok {


To avoid one level of indentation, you could do

if _, exists := filemap[name]; exists { continue }

alban · 2015-10-22T11:06:39Z

Did you try to run the image https://github.com/deis/example-dockerfile-http mentioned in rkt/rkt#1617?

iaguis · 2015-10-22T11:18:11Z

Did you try to run the image https://github.com/deis/example-dockerfile-http mentioned in rkt/rkt#1617?

> docker build  .
Sending build context to Docker daemon   129 kB
Step 0 : FROM alpine:3.1
 ---> 8dd8107abd2e
Step 1 : RUN apk add -U     bash    nginx   && rm -rf /var/cache/apk*
 ---> Using cache
 ---> 0066806327d8
Step 2 : RUN ln -sf /dev/stdout /var/log/nginx/access.log
 ---> Using cache
 ---> 0e090875dd75
Step 3 : RUN ln -sf /dev/stderr /var/log/nginx/error.log
 ---> Using cache
 ---> 81af5c429795
Step 4 : ENV POWERED_BY Deis
 ---> Using cache
 ---> a1612c4b24e7
Step 5 : COPY rootfs /
 ---> Using cache
 ---> 287913728fd9
Step 6 : CMD /bin/boot
 ---> Using cache
 ---> 012c6131329b
Step 7 : EXPOSE 80
 ---> Using cache
 ---> 63800ba13f46
Successfully built 63800ba13f46
> docker tag 63800ba13f46 nginx-deis
> docker save -o nginx-deis.docker nginx-deis
> docker2aci nginx-deis.docker
Extracting 8dd8107abd2e
Extracting 0066806327d8
Extracting 0e090875dd75
Extracting 81af5c429795
Extracting a1612c4b24e7
Extracting 287913728fd9
Extracting 012c6131329b
Extracting 63800ba13f46

Converted ports:
    name: "80-tcp", protocol: "tcp", port: 80, count: 1, socketActivated: false

Generated ACI(s):
nginx-deis-latest.aci

Now I start the pod and I connect to nginx from a browser:

> sudo rkt run --net=host --insecure-skip-verify nginx-deis-latest.acirkt: using image from file /home/iaguis/work/coreos/go/src/github.com/coreos/rkt/build-rkt/bin/stage1-coreos.aci
rkt: using image from file /home/iaguis/temp/rkt/nginx-deis/example-dockerfile-http/nginx-deis-latest.aci
::1 - - [22/Oct/2015:11:16:46 +0000] "GET / HTTP/1.1" 200 16 "-" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.71 Safari/537.36"
2015/10/22 11:16:46 [error] 6#0: *1 open() "/usr/share/nginx/html/favicon.ico" failed (2: No such file or directory), client: ::1, server: , request: "GET /favicon.ico HTTP/1.1", host: "localhost", referrer: "http://localhost/"
::1 - - [22/Oct/2015:11:16:46 +0000] "GET /favicon.ico HTTP/1.1" 404 570 "http://localhost/" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.71 Safari/537.36"

alban · 2015-10-22T13:36:18Z

Does it work if the original Docker image does not have the /dev directory? Would the /dev directory be added in the tarball? docker://quay.io/coreos/etcd is such an image.

What if the original Docker image has a file /dev which is not a directory but a symlink or a regular file?

iaguis · 2015-10-22T15:04:57Z

Does it work if the original Docker image does not have the /dev directory? Would the /dev directory be added in the tarball? docker://quay.io/coreos/etcd is such an image.

Yes. Yes.

What if the original Docker image has a file /dev which is not a directory but a symlink or a regular file?

I guess it would either fail or follow the symlink depending on the order. We could error out if we find a /dev/ that's not a directory in the original image with a meaningful message.

jonboulle · 2015-11-16T23:08:21Z

ping?

alban · 2015-11-17T08:18:38Z

LGTM

(this will conflict with #100 but I can rebase #100 after this gets merged)

iaguis · 2015-11-17T09:46:18Z

I guess it would either fail or follow the symlink depending on the order. We could error out if we find a /dev/ that's not a directory in the original image with a meaningful message.

This wasn't implemented in the PR. It is now, we return an error if /dev in any layer is not a directory.

krnowak · 2015-11-17T10:35:43Z

lib/common/common.go

@@ -329,6 +329,7 @@ func writeACI(layer io.ReadSeeker, manifest schema.ImageManifest, curPwl []strin
 		return nil, fmt.Errorf("error writing rootfs entry: %v", err)
 	}

+	filemap := make(map[string]struct{})


fileMap? :P

krnowak · 2015-11-17T10:39:37Z

LFAD. You can probably ignore the filemap nitpicks, because we already have something like PathWhitelist instead of PathWhiteList.

Docker images expect /dev/stdin, /dev/stdout, /dev/stderr and /dev/fd (https://github.com/opencontainers/runc/blob/master/libcontainer/SPEC.md#filesystem) but appc/spec doesn't (https://github.com/appc/spec/blob/master/spec/OS-SPEC.md#devices-and-file-systems). This adds the mentioned symlinks to the converted ACIs so apps can find them as expected.

Since docker2aci is creating stdio symlinks in /dev, a layer with a /dev that's not a directory can cause general badness. Return an error in that case.

krnowak · 2015-11-17T10:43:06Z

LFAD.

iaguis · 2015-11-17T10:43:19Z

Whitelist is often a word so PathWhitelist is fine, but filemap is not a word.

common: create stdio symlinks if not present

iaguis force-pushed the add-stdio-symlinks branch from d15c1c2 to 4811117 Compare October 20, 2015 10:26

alban reviewed Oct 22, 2015
View reviewed changes

alban added the reviewed/needs rework label Oct 22, 2015

iaguis added kind/friction priority/P1 labels Oct 22, 2015

iaguis force-pushed the add-stdio-symlinks branch from 4811117 to 681282c Compare October 22, 2015 11:18

iaguis added needs review and removed reviewed/needs rework labels Oct 22, 2015

alban added reviewed/lgtm and removed needs review labels Oct 22, 2015

alban added needs review and removed reviewed/lgtm labels Oct 22, 2015

alban assigned iaguis Nov 17, 2015

krnowak reviewed Nov 17, 2015
View reviewed changes

iaguis and others added 2 commits November 17, 2015 11:41

common: error if /dev is not a directory in any layer

4cad36e

Since docker2aci is creating stdio symlinks in /dev, a layer with a /dev that's not a directory can cause general badness. Return an error in that case.

iaguis force-pushed the add-stdio-symlinks branch from 0c34fb8 to 4cad36e Compare November 17, 2015 10:42

iaguis added a commit that referenced this pull request Nov 17, 2015

Merge pull request #97 from iaguis/add-stdio-symlinks

ec4e5d7

common: create stdio symlinks if not present

iaguis merged commit ec4e5d7 into appc:master Nov 17, 2015

alban mentioned this pull request Nov 24, 2015

rkt doesn't seem to provide /dev/stdout, /dev/stderr etc. rkt/rkt#1617

Closed

alban mentioned this pull request Mar 22, 2016

rkt can't run k8s guestbook frontend rkt/rkt#2300

Closed

iaguis mentioned this pull request May 20, 2016

Cant open pod logs (probably problem with ids) rkt/rkt#2640

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

common: create stdio symlinks if not present #97

common: create stdio symlinks if not present #97

iaguis commented Oct 20, 2015

alban Oct 22, 2015

alban commented Oct 22, 2015

iaguis commented Oct 22, 2015

alban commented Oct 22, 2015

iaguis commented Oct 22, 2015

jonboulle commented Nov 16, 2015

alban commented Nov 17, 2015

iaguis commented Nov 17, 2015

krnowak Nov 17, 2015

iaguis Nov 17, 2015

krnowak commented Nov 17, 2015

krnowak commented Nov 17, 2015

iaguis commented Nov 17, 2015

common: create stdio symlinks if not present #97

common: create stdio symlinks if not present #97

Conversation

iaguis commented Oct 20, 2015

alban Oct 22, 2015

Choose a reason for hiding this comment

alban commented Oct 22, 2015

iaguis commented Oct 22, 2015

alban commented Oct 22, 2015

iaguis commented Oct 22, 2015

jonboulle commented Nov 16, 2015

alban commented Nov 17, 2015

iaguis commented Nov 17, 2015

krnowak Nov 17, 2015

Choose a reason for hiding this comment

iaguis Nov 17, 2015

Choose a reason for hiding this comment

krnowak commented Nov 17, 2015

krnowak commented Nov 17, 2015

iaguis commented Nov 17, 2015