pkg/archive: support overlayfs in userns (Ubuntu kernel only) by AkihiroSuda · Pull Request #38038 · moby/moby

AkihiroSuda · 2018-10-15T08:18:28Z

Signed-off-by: Akihiro Suda [email protected]

- What I did

Ubuntu kernel supports overlayfs in user namespaces.

However, Docker had previously crafting overlay opaques directly
using mknod(2) and setxattr(2), which are not supported in userns.

This PR allows pkg/archive to craft opaques by creating overlayfs rather than mknod/setxattr.

Tested with LXD, Ubuntu 18.04, kernel 4.15.0-36-generic #39-Ubuntu.

Cherry-picked from Usernetes patch set: https://github.com/rootless-containers/usernetes

- How I did it

mknod c 0 0:

Mkdir lower,upper,merged,work
Create lower/dummy
Mount overlayfs
Unlink merged/dummy
Unmount overlayfs
The 0,0 character node is ready as upper/dummy

mkdir with trusted.overlay.opaque xattr:

Mkdir lower,upper,merged,work
Mkdir lower/dummy
Mount overlayfs
Rmdir merged/dummy
Mkdir merged/dummy
Unmount overlayfs
The dir with trusted.overlay.opaque xattr is ready as upper/dummy

- How to verify it

Set up Ubuntu 18.04 host
Create Docker-in-LXD environment ( https://gist.github.com/AkihiroSuda/772604c6895d5ced5357e5dcc154be22 )
Make sure overlay2 is selected as the storage driver (docker info)
docker pull ubuntu

Without patch, docker pull ubuntu fails as follows:

root@c0:~# docker version
Client:
 Version:           master-dockerproject-2018-10-14
 API version:       1.39
 Go version:        go1.11.1
 Git commit:        d18aad38
 Built:             Sun Oct 14 23:42:14 2018
 OS/Arch:           linux/amd64
 Experimental:      false

Server:
 Engine:
  Version:          master-dockerproject-2018-10-14
  API version:      1.39 (minimum version 1.12)
  Go version:       go1.11.1
  Git commit:       ee6fc90
  Built:            Sun Oct 14 23:51:03 2018
  OS/Arch:          linux/amd64
  Experimental:     false
root@c0:~# docker pull ubuntu
Using default tag: latest
latest: Pulling from library/ubuntu
124c757242f8: Pull complete 
9d866f8bde2a: Pull complete 
fa3f2f277e67: Extracting [==================================================>]     469B/469B
398d32b153e8: Download complete 
afde35469481: Download complete 
failed to register layer: Error processing tar file(exit status 1): operation not permitted

- Description for the changelog

pkg/archive: support overlayfs in userns (Ubuntu kernel only)

- A picture of a cute animal (not mandatory but encouraged)
🐧

AkihiroSuda · 2018-10-15T08:20:04Z

cc @brauner @stgraber @dmcgowan

AkihiroSuda · 2018-10-15T08:23:52Z

also cc @dhiltonp

estesp · 2018-10-15T16:48:58Z

Looks good @AkihiroSuda; windows CI failure looks to be a teardown test flake; unrelated to changes

cpuguy83

I haven't validated this yet, but just a minor comment on the code.

cpuguy83 · 2018-10-15T15:52:52Z

pkg/archive/archive_linux.go

Instead of the if/else here, could we have a wrapper for the userns case that catches the error and handles accordingly?

Could you be more specific?

I was thinking we could just wrap ConverRead with something like if err := c.wrapped.ConvertRead(...); err != nil && c.isInUserNS {...}

But looking back now, I think that may not be all that clean.

thaJeztah · 2018-10-18T17:51:45Z

/cc @dmcgowan @tonistiigi

codecov · 2018-10-19T06:59:48Z

Codecov Report

❗ No coverage uploaded for pull request base (master@6e3113f). Click here to learn what that means.
The diff coverage is 8.82%.

@@            Coverage Diff            @@
##             master   #38038   +/-   ##
=========================================
  Coverage          ?   36.53%           
=========================================
  Files             ?      610           
  Lines             ?    45371           
  Branches          ?        0           
=========================================
  Hits              ?    16575           
  Misses            ?    26467           
  Partials          ?     2329

thaJeztah · 2018-10-30T13:49:57Z

pkg/archive/archive_linux.go

(only glancing over the code); It's not an exported function, so not a big deal; just thinking if it would be cleaner to pass the options struct, instead of adding a boolean

What would be other fields of options?

not sure 😅 . My train of thought was that in case more options appeared in future, we didn't have to change the signature

thaJeztah · 2018-10-30T13:50:23Z

ping @cpuguy83 @dmcgowan @tonistiigi PTAL

AkihiroSuda · 2018-11-21T06:32:39Z

ping @cpuguy83 @dmcgowan @tonistiigi

cpuguy83 · 2018-11-26T21:16:40Z

This seems sane. Is there a good way we can add this to the test suite?

AkihiroSuda · 2018-11-28T03:52:32Z

As this is Ubuntu-specific and planned to be deprecated in favor of containerd overlay snapshotter, I think manual testing is ok for now

cpuguy83 · 2018-11-28T19:19:40Z

IMO, if it's not worth adding tests for it's not worth merging.

AkihiroSuda · 2018-11-29T09:39:07Z

Added TestReexecUserNSOverlayWhiteoutConverter.

The test passed locally on Ubuntu 18.04.1, kernel 4.15.0-39-generic #42-Ubuntu

AkihiroSuda · 2018-11-29T09:40:15Z

The PR now contains commit from #38292 so that the PR can be tested on recent kernels.

cpuguy83 · 2018-12-04T01:10:10Z

pkg/archive/archive_unix_test.go

I'm sure I don't know what this is doing, but why do we mention v3 and set 2 here, and then check 2 later?

2 is just a filename and had nothing to do with the version

cpuguy83

LGTM

Ubuntu kernel supports overlayfs in user namespaces. However, Docker had previously crafting overlay opaques directly using mknod(2) and setxattr(2), which are not supported in userns. Tested with LXD, Ubuntu 18.04, kernel 4.15.0-36-generic moby#39-Ubuntu. Signed-off-by: Akihiro Suda <[email protected]>

`rootlesskit go test ./pkg/archive` now succeeds Signed-off-by: Akihiro Suda <[email protected]>

Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda · 2018-12-10T22:19:27Z

rebased the PR, as #38292 got merged

@dmcgowan PTAL?

AkihiroSuda · 2018-12-26T05:32:43Z

@cpuguy83 @tonistiigi @dmcgowan mergeable?

AkihiroSuda · 2019-01-03T09:04:23Z

ping @cpuguy83 @tonistiigi @dmcgowan

vdemeester

LGTM 🐯

thaJeztah · 2019-01-04T11:55:56Z

@dmcgowan let us know if there's any changes needed after-the-fact 🤗

GordonTheTurtle added the status/0-triage label Oct 15, 2018

AkihiroSuda added the rebuild/windowsRS1 label Oct 15, 2018

GordonTheTurtle removed the rebuild/windowsRS1 label Oct 15, 2018

AkihiroSuda mentioned this pull request Oct 15, 2018

Can't pull docker images containing device nodes when running docker inside user namespace #35245

Open

AkihiroSuda force-pushed the ubuntu-overlayfs branch 2 times, most recently from 4c2473f to 90ea528 Compare October 15, 2018 08:35

AkihiroSuda mentioned this pull request Oct 15, 2018

[UMBRELLA] patch submission status rootless-containers/usernetes#42

Closed

3 tasks

estesp added status/2-code-review and removed status/0-triage labels Oct 15, 2018

cpuguy83 requested changes Oct 15, 2018

View reviewed changes

AkihiroSuda mentioned this pull request Oct 16, 2018

Allow running dockerd as a non-root user (Rootless mode) #38050

Merged

thaJeztah added area/storage/overlay area/security/userns labels Oct 18, 2018

AkihiroSuda force-pushed the ubuntu-overlayfs branch from 90ea528 to d73c7ff Compare October 19, 2018 06:59

GordonTheTurtle assigned MHBauer Oct 30, 2018

thaJeztah reviewed Oct 30, 2018

View reviewed changes

thaJeztah mentioned this pull request Oct 31, 2018

Remove overlay2 native diff #38111

Closed

AkihiroSuda force-pushed the ubuntu-overlayfs branch from d73c7ff to cfb4b0b Compare November 29, 2018 09:38

AkihiroSuda added the rebuild/powerpc label Nov 29, 2018

GordonTheTurtle removed the rebuild/powerpc label Nov 29, 2018

AkihiroSuda added the rebuild/powerpc label Nov 29, 2018

GordonTheTurtle removed the rebuild/powerpc label Nov 29, 2018

cpuguy83 reviewed Dec 4, 2018

View reviewed changes

cpuguy83 approved these changes Dec 10, 2018

View reviewed changes

AkihiroSuda added 3 commits December 11, 2018 07:18

pkg/archive: annotate tests that requires initial userns

037e370

`rootlesskit go test ./pkg/archive` now succeeds Signed-off-by: Akihiro Suda <[email protected]>

pkg/archive: add TestReexecUserNSOverlayWhiteoutConverter

ec153cc

Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda force-pushed the ubuntu-overlayfs branch from cfb4b0b to ec153cc Compare December 10, 2018 22:18

AkihiroSuda added the rebuild/janky label Dec 11, 2018

GordonTheTurtle removed the rebuild/janky label Dec 11, 2018

tonistiigi approved these changes Dec 13, 2018

View reviewed changes

tonistiigi assigned dmcgowan and unassigned MHBauer Dec 13, 2018

vdemeester approved these changes Jan 4, 2019

View reviewed changes

vdemeester merged commit de640c9 into moby:master Jan 4, 2019

Conversation

AkihiroSuda commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Oct 15, 2018

Uh oh!

AkihiroSuda commented Oct 15, 2018

Uh oh!

estesp commented Oct 15, 2018

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 18, 2018

Uh oh!

codecov bot commented Oct 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 30, 2018

Uh oh!

AkihiroSuda commented Nov 21, 2018

Uh oh!

cpuguy83 commented Nov 26, 2018

Uh oh!

AkihiroSuda commented Nov 28, 2018

Uh oh!

cpuguy83 commented Nov 28, 2018

Uh oh!

AkihiroSuda commented Nov 29, 2018

Uh oh!

AkihiroSuda commented Nov 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Dec 10, 2018

Uh oh!

AkihiroSuda commented Dec 26, 2018

Uh oh!

AkihiroSuda commented Jan 3, 2019

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jan 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

AkihiroSuda commented Oct 15, 2018 •

edited

Loading

codecov bot commented Oct 19, 2018 •

edited

Loading