opencontainers / umoci Goto Github PK

just have a busybox image copied with skopeo to OCI format, then:

$ umoci unpack --image busybox bundle
INFO[0000] parsed mappings                               map.gid=[] map.uid=[]
FATA[0000] create runtime bundle: chown rootfs: lchown bundle/rootfs: operation not permitted

$ sudo $GOPATH/bin/umoci unpack --image busybox bundle
INFO[0000] parsed mappings                               map.gid=[] map.uid=[]
INFO[0000] unpack manifest: unpacking layer sha256:56bec22e355981d8ba0878c6c2f23b21f422f30ab0aba188b54f1ffeff59c190  diffid="sha256:e88b3f82283bc59d5e0df427c824e9f95557e661fcb0ea15fb0fb6f97760f9d9"
FATA[0000] create runtime bundle: unpack layer: unpack entry: bin: apply hdr metadata: clear xattr metadata: /home/amurdaca/go/src/github.com/docker/containerd/bundle/rootfs/bin: lclearxattrs: lremovexattr(/home/amurdaca/go/src/github.com/docker/containerd/bundle/rootfs/bin, security.selinux): permission denied

If you use something like time.RFC3339 as the format for time, it won't work when parsing the time. We need to either make (*igen.Generator).{Set,}Created just take a string or figure out why the time parsing isn't working.

If the layout is on a read-only filesystem then cas.Open will fail because we create temporary directories even if we don't use them. We should either:

1. Create the temporary directory immediately as we need it. This means that cas.Open will be opportunistic.

2. Make two different "open" functions.

The former sounds like a better idea.

Currently umoci unpack and umoci repack implement the same functionality in a giant block of code. We should move it to a library so we can create unit tests for it, as well as reduce the duplication.

This is going to be a bit of a layer violation, so I'll have to think about it. But basically we need to have a way to remove all of the temporary directories that have been left behind inside an OCI image.

Let's not end up in the same boat as Docker. We should be using https://github.com/pkg/errors to safely manage wrapping of errors while also allowing for os.IsNotError to work. Let's not build up on the same technical debt that Docker has.

I really would like to license this project under GPLv3+, but currently the image/ code needs to be Apache licensed if we want to have any hope of the code being merged into the OCI. We could do some sort of dual-licensing thing, but I feel like that wouldn't end well.

Currently we have to manually modify the history with umoci config --history. While this sounds like a good idea, tools like skopeo have certain assumptions about the history. Essentially, we will have to have a separate history entry for every change (especially repacking).

We'll have to come up with a different UX.

Because v1.History is a full structure (and v1.Config stores a slice of them), implementing the CLI interface for this might be more than a little painful. We could just require that someone pass the JSON for a v1.History but that's a really gross interface.

We need to output the correct version, and also verify it properly. Currently we're not verifying it at all simply because it breaks with skopeo (we're doing the wrong thing right now). This is bad.

We should also verify that we have a blobs and refs directories in a way that makes sense (no subdirectories or incorrect algorithms).

Currently we have to go through refs exclusively, which means that you can't tag a blob that doesn't already have a tag. This is a bit of a pain (though with umoci init it won't be a blocking issue).

However, a nice way of fixing this would be implementing an indexer for an OCI image (which does the same sort of indexing that umoci gc does but in addition storing the MediaType of the object referenced). Then we could always guarantee that we had blob references for most blobs, and we could then go with a backup option of just using MediaType as gospel.

The only downside to this approach is that layer diff blobs will not be practically usable (since we can't tell between a distributable and non-distributable blob -- though I'm not sure if the spec even says that a particular layer diff blob is in a strict binary between the two types). But we can just check for a gzip header (or just check that it's not JSON) and move on with our lives, and then not allow operations that directly reference those blobs.

We could also add an explicit --mediatype option that allows for overwriting of the detected mediatype (probably dangerous). Then we can just not allow operations on any untyped blob.

Because of how Go's archive/tar.Writer currently works, AccessTime and CreateTime are not correctly written to the output archive stream. This causes several issues which are really disappointing.

I've filed a bug upstream about it golang/go#17876.

When we do an umoci unpack we currently only keep track of the manifest hash that we extracted. We should also save (in umoci.state or something) the name of the reference that we dereferenced in order to get the extracted rootfs. This will allow us to remove the tedious --from argument in umoci repack.

The only real question to ask is whether we should make it possible for someone to forcefully create a rootfs that we cannot be sure is valid (namely specify --from and --mtree manually).

We need to have proper docs so that people understand what the hell is going on. My only concern is that I don't really want to use go-md2man because of the fun issues we've had in the openSUSE community with packages like that. But we'll see how that goes.

This is because the OCI tooling doesn't appear to handle this properly (image.CreateRuntimeLayoutBundle is to blame). It will probably be a good idea to from-scratch implement this feature inside umoci so that there is more than one implementation of this unpacking functionality.

% umoci unpack --image opensuse --ref latest --bundle bundle1
% sleep 30s
% umoci unpack --image opensuse --ref latest --bundle bundle2
% gomtree -f bundle1/*.mtree -p bundle2/rootfs >/dev/null
% echo $?
1

Where all of the gomtree failed keywords are related to time.

This subcommand will be very similar to oci-runtime-tools generate, which allows you to modify a config.json. In contrast, this tool will allow you to modify the image configuration of a manifest.

It would be nice if we could have a --dry-run flag that allows a user to take an input template (from anywhere) and outputs the modified config. If we correctly break out mutateConfig then this should actually be entirely doable by adding a new subcommand (raw-config or something).

It doesn't make sense to have a mixed command like umoci create that will create an empty image in one mode (no --tag) and create a manifest in another (with --tag).

Currently --from and --image are both implemented by every subcommand separately, which is just unwieldy. It would be nice if we could implement them as global flags or something like that. There's also the question of whether the current flag-based interface even makes sense (should it be positional arguments instead?).

In addition, one of the really annoying things with umoci tag is how you have to understand the spec to use the damn command. umoci stat should be far more friendly and we should have a way of creating a copy of a reference (with umoci add only being used for hardcore users).

We need to have some way of providing information about an image (or a tag, or a blob). We could also call this umoci stat (and have a -L option to not dereference a reference). This is not a necessary feature, but would be pretty useful for debugging.

The config.json generated when unpacking is not useful because it is a config for running in the host, which isn't helpful. While this can be corrected by a user (using oci-runtime-tools generate) this quite a bit of a pain.

The best choice IMO would be to create a library that can take a v1.Image and then apply it to a runtime-tools/generate.Generator. We can then use the default config for runC as the base (which we know is actually sane).

The upstream bug for this is opencontainers/image-tools#76 but we should implement this ourselves.

For subcommand-specific mandatory arguments (like --bundle) really should be positional arguments...

Currently all of the tooling only really supports tags that reference manifests (not manifest lists). The main reason for this is that I'm not really sure how to implement manifest list handling. Should we just take the current OS and architecture (and should that be taken from runtime.GOOS which is not entirely accurate, or from some other source?). Or we could just mandate that --os and --arch be specified with every call (this is annoying).

Not to mention how should we handle repacking? Should we not allow repacking over a different manifest than the one that was used to extract the damn thing (which is how the current implementation works)?

Currently all the testing I'm doing is manual, we need to have some actual testing. Namely integration testing (using bats) for the actual umoci tool as well as unit tests for the libraries. Another nice thing would be to have validation testing against the OCI validation tooling.

• Unit tests.
  • Add tests for #27, to make sure we don't break the safety of paths.
  • Add tests for all of the CAS interfaces against a blank repository.
  • Add tests for generation of tar stream layers.
  • Add tests for unpacking of layers.
• Integration tests.
  • Test for #1, where we run gomtree after extracting the same manifest twice at different times.
  • Test where we pull an image using skopeo, unpack it and then repack it. Then unpack it again to compare the changes (with gomtree).
  • Tests where we unpack things that have changed type.
  • Tests with symlinks that have unusual values (please save me from bad symlink handling).
  • Tests for umoci config to make sure that we set things correctly. Use jq for the testing. We also might want to run the runtime validator against it too.
    • empty config set doesn't change it
    • workingdir.
    • cmd and entrypoint result in the right process.args.
    • metadata: architecture os
    • metadata: author created history [not really sure how to test this]
    • env is set to the right $HOME?
    • Something about volume but I get the feeling it isn't great.
    • user:group is actually parsed according to /etc/passwd.
      • $HOME.
      • and AdditionalGids are set.
  • Add tests for umoci gc to make sure it actually cleans things properly.

Currently in order to actually understand the history of an image you need to convert it to a Docker image so you can use docker history. We should really be including our own history-parsing code.

One issue with all of this is that we're creating special-purpose commands. Maybe we should just implement stat with options to narrow down what sections to output.

When we generate the rspec.Spec, we append to the environment variable set. This is a bit dangerous, and the more sane thing to do would be to overwrite variables if necessary. It would be nice if this was implemented upstream though...

When generating the config.json, we can't handle cases where the user specification is something like "cyphar:users" because of the way the interfaces are designed (we do the extraction and generation separately from one another). This can be fixed by using libcontainer/users.

In addition, we'll also be able to fill the AdditionalGids array, which we couldn't before.

We could also add a $HOME env variable.

Currently we aren't properly cleaning paths inside unpackEntry. In particular, if you have an invalid tar archive that contains entries such as ../../ or if it contains a entry that resolves through a symlink, then umoci will start touching parts of the host.

To fix this we need to use this library, which I helped write specifically to solve this problem inside Docker https://github.com/docker/docker/tree/master/pkg/symlink.

Like oci-create-runtime-bundle but it also generates an mtree(8) manifest for the rootfs which allows us to create diff layers without needing to copy the rootfs twice.

umoci tag's interface is just silly. It needs to be switch to work properly (though #39 will change this as well).

Currently, we have to fake hardlinks using symlinks because of the fact that we cannot be sure of the ordering of tar archive entries. In particular we can't be sure that the thing we're linking to will be hit before we hit the link.

The solution to this problem is creating an inode map to create hardlinks after the fact. In principle this should work without any major hitches (we don't need to save the metadata because the original inode determines everything) and we'd just have to delay it until the end.

As far as I'm aware, a whiteout of a directory implies that all of its children are also to be removed. However, currently we are creating a new whiteout for every mtree.InodeDelta element in the diff from gomtree -- which will can include files that are underneath directories.

There's also a valid question about whether we should generate directory entries if we only detected a deletion between two layers. But that's a question for another time (and according to upstream our current method is fine).

Because of how umoci repack is implemented (we create a bunch of new blobs and leave the old ones) we need to include a garbage collector that can clean up the trash after we've regenerated everything. A standard mark-and-sweep garbage collector will do.

A nice way of doing this would be to use reflect and to parse every Descriptor, adding every referenced descriptor to "black" coloured objects (with the default being "white"). Then the "white" coloured objects are removed.

We need to have a way of creating a new image. The only real question is how do we get a user to specify what they want the "default" rootfs to be. In my opinion, the best thing to do would be to have an empty set of layers that the user can then add the first layer to using unpack (as expected). However, I have a feeling that we'll have to mess around with go-mtree's generated .mtree spec file -- because it will analyse the root even though the image doesn't have a proper root.

One obvious option is to go with the Docker route, where they just added a dummy scratch rootfs that they then added on top of. The reason this is not a great idea (and Docker has since moved away from that) should be obvious.

We could also implement something like the ADD a.tar.xz / functionality that Dockerfiles have, but the problem is that it will make the interface for creating the initial image a bit janky. I'll have to ask the KIWI developers what they think about this.

There's a lot of // TODO comments lying around that we've already fixed.

Sort of like oci-create-layer from opencontainers/image-tools#8, except it uses mtree(8) manifests. It's also automatic and generates all of the necessary blobs to represent the new tagged image.

According to the spec, the DiffIDs in the configuration are meant to be the uncompressed layer's digest. So we probably need to also return some more DiffID information from GenerateLayer. Or we'll have to parse things twice which will be a pain.

The current upstream unpacking utilities have some flaws:

• They are completely separate to my CAS implementation, making using them quite ugly.
• They don't expose a "single layer" unpacking method, meaning that we can't implement raw-unpack (#23) and we can't really test it properly.
• They have bugs (like #1) and also don't support things like "user:group"-style specification of users, which is a problem.
• Features like #26 are unlikely to be nicely solved upstream.

All of this can be solved if we just implement it ourselves.

Some lovely setups (such as inside VMs with shared filesystems) have issues if a path has : in its name. So we should just avoid this overall (the version in the design doc just replaces it with a _ which should be fine).

There are several components of umoci that are not very well defined by the image-spec specification. I'm working on improving this upstream, so that we can be sure that umoci is actually doing thing correctly (because we defined what is the correct way of doing them). Here's a currently list of upstream PRs:

• opencontainers/image-spec#492 -- conversion.md
• opencontainers/image-spec#504 -- Forbid Config.Volumes snapshotting.

This is quite important for us.

As part of #26 we need to just implement --uid-map and --gid-map for unpack and repack.

We need to add some tests that ensure that our unpacked bundle is actually a valid OCI runtime bundle. However, currently this is blocked on opencontainers/runtime-tools#268 -- since we're generated v1.0.0-rc2 bundles that have different required fields to the v1.0.0-rc1-dev bundles which is what oci-runtime-tool currently uses.

There is also the fact that we need to have oci-image-validate run after every build.

In essence we should run this after every unpack in every test.

Currently we have --from and --image tags as separate arguments. This is just silly (it doesn't add any useful information and in fact just makes everything more clunky). So we really should switch to path:tag-style. The only downside is that it will make umoci tag need to have a different interface.

We need to provide an interface for someone to re-tag a given tag with another tag. This touches on a core issue of the MediaType model of the image specification -- you can't really tag a random digest because you have to know what type it is. There's been a lot of (tiring) discussion about this upstream in opencontainers/image-spec#411, and currently upstream doesn't really like the idea of peek-inside detection. This makes an interface like:

% umoci reference tag --digest sha256:<digest> sometag

Not really possible to implement at the moment. Here are the sub-subcommands we'd like:

• umoci reference create
• umoci reference remove
• umoci reference list

These never made sense in the spec, and they've been recently removed so we should drop all mention of them from everywhere. opencontainers/image-spec#495

Currently umoci unpack requires root privileges to set the owners of the files, and thus umoci repack sometimes needs root privileges to even read the files to the archive. This makes rootless image manipulation not practical (and makes it effectively impossible to include umoci as part of a build system).

To fix this we should implement some sort of user mapping into the new version of unpack (which is saved in the unpacked bundle) and then on repack we use the same mapping to modify the *.mtree diffs we got.

In layer.GenerateLayer we use CloseWithError to try to propagate layer generation errors to the reader. However, it looks like the errors are dropped at some point (or the pipe is already closed when we error out). This causes us to generate new invalid layers when we should be erroring out.

This is a critical issue and must be solved before 0.0.0.

This is like unpack except it just does a simple extraction of a given layer diff blob (no mtree or config.json). This probably will end up being in a set of raw-* subcommands that are more hardcore versions of the nice interfaces.

Currently there's no example shell sessions in the man pages, we should really add some.