support for storage for rootless containers

[giuseppe]

continuation of: #871

[giuseppe]

tests are probably failing for opencontainers/runc#1816

[giuseppe]

@rhatdan the patch for runc got merged (b222ea4469dd5e304f3f6cf7a2482860285639a2), could we get an updated build?

[rh-atomic-bot]

☔ The latest upstream changes (presumably 4b4de5d) made this pull request unmergeable. Please resolve the merge conflicts.

[rh-atomic-bot]

☔ The latest upstream changes (presumably 16ea659) made this pull request unmergeable. Please resolve the merge conflicts.

[giuseppe]

This is currently ready for review. The updated runc is used only on Travis right now (and tests are happy there). Everywhere else, we need a newer runc

[rhatdan]

LGTM
@mheon @nalind @mtrmac @baude @umohnani8 @wking PTAL

[mheon]

Should this happen before LockOSThread()? Seems like it doesn't need to be guaranteed to run in a single thread, and this should be the typical case, so we can save some time by not calling LockOSThread() in this case

[giuseppe]

changed

[wking]

The updated runc is used only on Travis right now (and tests are happy there). Everywhere else, we need a newer runc

Having different runc versions in different places makes me jumpy. Is there something that's blocking us bumping to a single new version of runc in all of our locations (Dockerfile, vendor.conf, other places?)?

The global user namespace approach you have here is just mapping a single user and group to 0 (e.g. you're not using subuid(5) to map multiple UIDs). Is that really sufficient for unpacking images? I'd expect at least some images to require more than one user or group ID. A more robust fix might be possible if we handled this with shiftfs, or if containers/storage grew an API for specifying ID translations (like GNU tar's --owner-map, etc.), or if it could use something like umoci's unpriv package. And regardless of how we handle ID mapping, there's probably no way an unprivileged user will be able to usefully unpack an image that contains device nodes, etc.

[giuseppe]

@wking, there are multiple users mapped when the user has additional uids/gids defined in /etc/subuid and /etc/subgid, this is done from tryMappingTool.
The Go Runtime has support for launching a process in a new userNS but it supports only one mapping as it writes directly to /proc/PID/[gu]id_map, if the mapping is not specified the process is left unconfigured. That is why I needed the C part to create a new process and configure the multiple mappings with newuidmap/newgidmap.

I think we are not vendoring runc but using the version from the host, that is why we are dealing with different versions on different systems :-(

[giuseppe]

I've kept the implementation in a new pkg so that it can be more easily re-used. @nalind could this be useful for Buildah?

[wking]

The spec has:

If $XDG_DATA_HOME is either not set or empty, a default equal to $HOME/.local/share should be used.

A simpler implementation of that would be:

dataDir := os.Getenv("XDG_DATA_HOME")
if dataDir == "" {
  home := os.Getenv("HOME")
  if home == "" {
    return opts, fmt.Errorf("neither XDG_DATA_HOME nor HOME was set non-empty")
  }
  dataDir = filepath.Join(home, ".local", "share")
}
// then append to dataDir to construct GraphRoot

This sort of generic XDG support is something that you can get from XDG libraries as well. For example, here.

Once you have dataDir populated, I think we should be using a single containers-storage instead of two directories. Again from the spec:

Other specifications may reference this specification by specifying the location of a data file as $XDG_DATA_DIRS/subdir/filename. This implies that:

• Such file should be installed to $datadir/subdir/filename with $datadir defaulting to /usr/share.

• A user specific version of the data file may be created in $XDG_DATA_HOME/subdir/filename, taking into account the default value for $XDG_DATA_HOME if $XDG_DATA_HOME is not set.

• Lookups of the data file should search for ./subdir/filename relative to all base directories specified by $XDG_DATA_HOME and $XDG_DATA_DIRS. If an environment variable is either not set or empty, its default value as defined by this specification should be used instead.

containers-storage would be the subdir specific to the containers/storage library. I don't see a need to group all of the container data under a containers/ directory, because it makes managing that shared directory more complicated. dataDir will still be a shared directory potentially consumed by several packages, so sharing directories is obviously something that can work, but I think containers/storage is unnecessarily deep.

Finally, the chained-lookup point at the end of the XDG quote above is something that we'd have to approach by patching containers/storage itself. If we end up patching containers/storage to support the XDG spec (which seems useful to me), we probably won't need any code here. We can just leave GraphRoot empty, and let containers/storage pull it in from the XDG variables and its internal subdir/filename, etc. defaults.

[rhatdan]

Lets open a PR for containers/storage and see what @nalind thinks, but for now, we can handle this here, and just get this PR merged, when it is ready.

[giuseppe]

I am in favour of keeping the two levels containers/storage as we also have containers/atomic

[wking]

... but for now, we can handle this here, and just get this PR merged, when it is ready.

The dataDir unification in my earlier comment (which DRYs up the current two lines with "containers", "storage") can happen in this PR without blocking on upstream containers/storage work.

[rhatdan]

Yes lets keep them separate directories.

[giuseppe]

I addressed all the comments above, is there anything more blocking it?

[mheon]

Now that #1001 is in, I have no more objections. LGTM.

[rhatdan]

@rh-atomic-bot r+

[rh-atomic-bot]

📌 Commit 2bc4eb0 has been approved by rhatdan

[rh-atomic-bot]

⌛ Testing commit 2bc4eb0 with merge 19f5a50...

[rh-atomic-bot]

☀️ Test successful - status-papr
Approved by: rhatdan
Pushing 19f5a50 to master...

[lukasheinrich]

thanks a lot @giuseppe !