Centralize metadata, move mount logic to atomfs pkg, add tests #23
Conversation
The molecule config contains the OCI path and tag, which is important if we want to track what container image is mounted at a particular path, and thus what container might need to stop if a verity error is detected in one of the atoms in the molecule it's using. This commit writes that config to a JSON file in the metadata path. Signed-off-by: Michael McCracken <[email protected]>
mikemccracken
force-pushed
the
2024.10.15/main/metadata-changes-test-etc
branch
from
14bf28e to
f96a02b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
mikemccracken
force-pushed
the
2024.10.15/main/metadata-changes-test-etc
branch
from
2fb645d to
a80d817
Compare
This comment was marked as outdated.
This comment was marked as outdated.
mikemccracken
force-pushed
the
2024.10.15/main/metadata-changes-test-etc
branch
4 times, most recently
from
7ee00d5 to
b55c7dd
Compare
1 - To make it easy to read the config.json for a given mount, move the metadata path to /run/atomfs/meta/$current-mountns/$munged-mountpt-name/ using the current mount NS name in the path means we can track mounting different images on to the same mountpoint in different mount namespaces. 2 - move mount logic including the metadata dir logic from cmd/atomfs/mount.go to atomfs/molecule.go so that users of the package will also get the same metadir / config.json etc behavior that the `atomfs` tool does. As part of this move, we no longer mount an RO overlay in one place and then either remount another overlay or bind mount to the final target mountpoint. Instead we build one overlay mount for the mountpoint and either it has an upperdir/workdir or not. Signed-off-by: Michael McCracken <[email protected]>
pass through to molecule config, and check to be sure we don't guestmount and ignore verity data without explicitly saying we want to Signed-off-by: Michael McCracken <[email protected]>
In the guestmount case, we use squashfuse and there is no verity mount source to check. Treat this as a verify error. Signed-off-by: Michael McCracken <[email protected]>
mikemccracken
force-pushed
the
2024.10.15/main/metadata-changes-test-etc
branch
from
b55c7dd to
61384ff
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23 +/- ##
==========================================
- Coverage 16.15% 15.68% -0.48%
==========================================
Files 6 7 +1
Lines 953 1358 +405
==========================================
+ Hits 154 213 +59
- Misses 778 1117 +339
- Partials 21 28 +7 ☔ View full report in Codecov by Sentry. |
mikemccracken
force-pushed
the
2024.10.15/main/metadata-changes-test-etc
branch
from
61384ff to
cb9390f
Compare
|
Took a bit of a shortcut to use stacker in the unpriv test suite, otherwise I think this is ready for review. |
|
|
||
| // Allow overriding runtime dir for tests so we can assert empty dirs, etc. | ||
| func RuntimeDir() string { | ||
| testOverrideDir := os.Getenv(TestOverrideRuntimeDirKey) |
There was a problem hiding this comment.
Do we need a more official way of overriding this, like a command line argument? Otherwise, if I just create a new user+mntns, but do not chroot, I'd have to use ATOMFS_TEST_RUN_DIR to override /run, which I won't be able to write to otherwise, right?
There was a problem hiding this comment.
Hm, yeah good catch. Using this in the tests means I wasn't testing the case where the run dir was not writeable. :)
I guess a cli arg is the best option here.
It does mean that non-host-root usage will not actually have centralized metadata, so e.g. any scheme that tries to map dmverity faults to mounted dirs will fail to find the mount metadata.
Is there a better place to consolidate the metadata? What do you think about using /tmp/ instead of /run maybe?
There was a problem hiding this comment.
non-host-root cannot use dmverity, right?
There was a problem hiding this comment.
oh. yes, that's right. So the use cases don't overlap. OK. I am planning on adding a flag for this when I get a minute
molecule.go
Outdated
|
|
||
| upperdir := m.config.WriteableOverlayPath | ||
| if upperdir == "" { | ||
| upperdir = filepath.Join(metadir, "persist") |
There was a problem hiding this comment.
Hm, I think this is an existing "bug" in the code you're moving, but note that workdir and upperdir must be on the same filesystem. Therefore, it might be best to have workdir be created adjecent to the persist dir, right?
There was a problem hiding this comment.
ah, thanks. I didn't know that. yeah this was in the original code but we might as well fix it now
|
oh. yes, that's right. So the use cases don't overlap. OK. I am planning on adding a flag for this when I get a minute
Do you want to merge this in the meantime and make that a new pr?
Or just wait to merge?
|
I'm fine either way, maybe let's just wait |
Add a bats tests suite for mounting and for failing to mount when the images are bad. Uses the ATOMFS_TEST_RUN_DIR env var to avoid polluting your host's /run/atomfs/meta dir. copies the guestmount test from the github yaml into bats and expands it a bit. I apologize for the bash quoting situation in the heredoc in the guestmount tests, forgive me Missing cases: - testing `atomfs verify` on bad images: requires manufacturing a verity image that will mount OK but has a bad block that won't get read until later. I have tested verify with mounted bad images that I mounted with a purposely broken atomfs, but there should be a better way for CI. Signed-off-by: Michael McCracken <[email protected]>
the existing test is now covered there, and we build our own test image so we can avoid the zothub dep and skopeo dep Signed-off-by: Michael McCracken <[email protected]>
This redefines the --persist argument to point to a directory where atomfs will create or use both the workdir and the upperdir. So if you run `atomfs mount --persist=foo/` then the persistent writes will end up at foo/persist/. Signed-off-by: Michael McCracken <[email protected]>
In some cases, e.g. when guestmounting in a new userns and mountns, but not chrooted, /run/atomfs may not be writable. In that situation, use the new --metadir flag to all commands to specify a replacement for the default /run/atomfs. This overlaps slightly with the ATOMFS_TEST_RUN_DIR environment variable, which would have the same effect, but should only be used for tests, as it is not discoverable. Signed-off-by: Michael McCracken <[email protected]>
mikemccracken
force-pushed
the
2024.10.15/main/metadata-changes-test-etc
branch
from
cb9390f to
816ef2b
Compare
|
@hallyn this should be ready to review again. I left the two changes as separate commits for ease of reviewing, but I could be convinced that they should just be squashed for the final merge. Thanks for your attention! |
| } | ||
|
|
||
| upperdir := filepath.Join(persistMetaPath, "persist") |
There was a problem hiding this comment.
Hm, this kinda sucks, but this is not quite in keeping with the intent of --persist as used by lxc. Now it probably doesn't matter - users will know where to find $path/persist, and lxc will always just pass the same dirname to --persist.
So what you have here is probably the best we can do.
There was a problem hiding this comment.
I agree it's not great, this seemed like the least bad option. I am open to changing the flag name to not confuse lxc users, I wasn't too sure of the best naming here, as usual
Replaces #22
Makes config and metadata visible in a central location at
/run/atomfs/meta/$mount-ns-id/$mountpointnameThe logic for configuring the metadata is moved to molecule.go in the atomfs pkg, so users of the pkg will get the same behavior and not have to redo it separately.
We also don't mount onto a
rodir then do a second overlay or bind depending on whether or not we asked for--writeable- instead there's just one overlay mount and either it has an upper dir or not.If you want an upperdir that isn't in
/run/you have to use--persistto set that. That's now stored in the molecule config too so pkg users can just pass a path.Adds a bats test suite that uses stacker to build test images and mount/umount them, and includes a test for tampered images.