awslabs / soci-snapshotter

A containerd snapshotter plugin which enables standard OCI images to be lazily loaded without requiring a build-time conversion step.
Apache License 2.0
534 stars 55 forks source link

Add ID-mapping capabilities #1392

Closed sondavidb closed 2 weeks ago

sondavidb commented 1 month ago

Issue #, if available:

Description of changes: Working off the POC at Kern--/idmap-v2, added id-mapping capabilities to the snapshotter.

The PR additionally has a commit to add a separate ctr binary nerdctl binary (EDIT: later iterations changed this to a nerdctl binary) to the container. This is because containerd doesn't support id-mapping in ctr nerdctl in an official release yet, so we need a specific binary to pass the proper labels. This is done by adding a new Makefile target and copying it in the Dockerfile, but I'm open to different approaches here.

Testing performed: make test and make integration

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

sondavidb commented 1 month ago

New changes address logging concerns as well (EDIT: missed some) as moved the FUSE server setup to a new function. ~I think this also made the tests pass properly, possibly because I previously didn't setup the fuse logging properly before?~ NVM, still not good, seems the FUSE servers aren't being set up properly. Might have to do with the layer bit Austin mentioned...There's also parts w/ overlay fallback which means that the layer isn't being setup properly, but the new code shouldn't even be called unless id-mapping is turned on? So I'm a bit confused...

Also added a new internal patch which will make the diff look even bigger but it's just so we can store the patch locally.

sondavidb commented 1 month ago

FWIW, I tested these changes on the ubuntu runners and they all seemed to pass?? Not sure what it might be indicative of.

https://github.com/sondavidb/soci-snapshotter/pull/20

sondavidb commented 3 weeks ago

Looks like I undid a ton of changes and messed things up while rebasing 😓 Manually re-added everything I think but should double check.

Will double-check multi-uid/gid mapping shenanigans tomorrow as well as using nerdctl instead of ctr but I'll have to see.

sondavidb commented 3 weeks ago

Tons of changes. Notably:

TODO:

sondavidb commented 3 weeks ago

Added a check for containerd version to skip the new tests which means we should wait for #1401 to be merged before merging this if we are happy with the changes

sondavidb commented 3 weeks ago

Added commits from #1401 preemptively just to get CI running and make sure we're all good, will rebase before merging but CI should be green now.

sondavidb commented 3 weeks ago

Ended up just using -a --no-preserve=links and it seems to work

sondavidb commented 3 weeks ago

Rebased with main

sondavidb commented 3 weeks ago

Newest changes should address most recent concerns with variable naming and also adds testing for files inside the container as well

sondavidb commented 3 weeks ago

After this is merged I think documentation on how to actually use this feature would be nice. I can take it as a followup.