Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

treefile: Add ignore-devices #5114

Merged
merged 1 commit into from
Oct 14, 2024
Merged

Conversation

cgwalters
Copy link
Member

We hit another case where people are pulling a container image with devices in /dev in the tar stream; they're then trying to commit this to an ostree.

There's much better ways to fix this:

  • Change the image to stop including devices as there's no reason to do so
  • Switch to logically bound images instead of physically bound
  • Use the composefs backend for c/storage

Eventually I may look at "quoting" generally in ostree, but it's fairly invasive: ostreedev/ostree#2568

In practice today, simply ignoring the files will happen to work for "podman run" of such images; podman will just use overlayfs to stitch together the diff directories, and doesn't try to do any validation of their contents today.
(Queue the composefs integration, which would do that but would
also fix this anwyays)

Copy link

openshift-ci bot commented Oct 1, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@achilleas-k
Copy link

achilleas-k commented Oct 7, 2024

  • Switch to logically bound images instead of physically bound

For osbuild/image-builder this is certainly possible for day-1 image creation from an ostree commit, but it would need work. For ISOs, it would need Anaconda support, which I think is coming (or already done?).
Lifetime support would depend on the rpm-ostree update mechanisms though, which afaik is not planned, right? The plan is to move these kinds of features to bootc.

We hit another case where people are pulling a container image with devices in /dev in the tar stream; they're then trying to commit this to an ostree.
...
In practice today, simply ignoring the files will happen to work for "podman run" of such images; podman will just use overlayfs to stitch together the diff directories, and doesn't try to do any validation of their contents today.

Isn't this making assumptions about the container image and the application it's meant to run? I suppose all the cases we've seen so far have included devices accidentally/carelessly, but regardless, with this change, we're modifying the container image. The fact that podman doesn't validate sounds more like a bug we're leaning into than a feature we can rely on.

@cgwalters
Copy link
Member Author

Isn't this making assumptions about the container image and the application it's meant to run? I suppose all the cases we've seen so far have included devices accidentally/carelessly,

I cannot think of a use case for embedding device nodes in a container image - they represent dynamic state.

but regardless, with this change, we're modifying the container image. The fact that podman doesn't validate sounds more like a bug we're leaning into than a feature we can rely on.

Yeah, definitely. Fortunately the fix for reliable incremental online verification (composefs) on the c/storage (podman) side will move the device nodes into the EROFS metadata file, avoiding this problem.

That said I only lightly tested this case - I verified that podman run silently ignores the missing (underlying!) device nodes.

I should also xref here bitnami/minideb#171 - hopefully they fix that.

@cgwalters cgwalters marked this pull request as ready for review October 14, 2024 16:01
@cgwalters
Copy link
Member Author

I changed this to be enabled by default, because doing otherwise would require a complex version-detection ratchet ("enable only if we detect it's supported") I believe on the osbuild side and if we're just going to enable it by default, then let's just...enable it by default here.

@cgwalters cgwalters enabled auto-merge October 14, 2024 16:02
Copy link
Member

@jmarrero jmarrero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@cgwalters
Copy link
Member Author

/retest

@jmarrero
Copy link
Member

looks like we are still failing:

FAIL: installroot
   installroot: ostree --repo=/var/tmp/test.00tJfk/repo ls fedora/x86_64/coreos/testing-devel /usr/share/.ostree-wh.foowhiteout >out.txt
   installroot: grep -Ee '\''^-00000'\'' out.txt
   installroot: 
   installroot: # And the devzero should have been ignored
   installroot: if ostree --repo=/var/tmp/test.00tJfk/repo ls fedora/x86_64/coreos/testing-devel /usr/share/devzero; then
   installroot:   echo found' 'devzero 1>&2; exit 1
   installroot: fi
   installroot: 
   installroot: ostree --repo=/var/tmp/test.00tJfk/repo cat fedora/x86_64/coreos/testing-devel /usr/share/rpm-ostree-composetest-split.txt >out.txt
   installroot: grep "Mon Oct 14 16:38:04 UTC 2024" out.txt
   installroot: ostree --repo=/var/tmp/test.00tJfk/repo cat fedora/x86_64/coreos/testing-devel /usr/lib/tmpfiles.d/rpm-ostree-0-integration.conf
   installroot: '
   installroot: + mknod -m 000 cache/instroot/rootfs-directcommit/usr/share/foowhiteout c 0 0
   installroot: + mknod -m 000 cache/instroot/rootfs-directcommit/usr/share/devzero c 1 5
   installroot: + echo 'Mon Oct 14 16:38:04 UTC 2024'
   installroot: + test -f cache/instroot/rootfs-directcommit/usr/lib/tmpfiles.d/rpm-ostree-0-integration.conf
   installroot: + rpm-ostree compose commit --repo=/var/tmp/test.00tJfk/repo /var/tmp/test.00tJfk/config/manifest.json cache/instroot/rootfs-directcommit
   installroot: warning: boot-location: "new" is deprecated, use boot-location: modules
   installroot: Preserved workdir: /var/tmp/rpm-ostree.tQVcnS
   installroot: error: Postprocessing and committing: Finalizing rootfs: Postprocessing devices: Unsupported device file: ./usr/share/devzero

@cgwalters
Copy link
Member Author

looks like we are still failing:

Yeah I only changed the docs, not the actual code for the default. Done now and CI passes.

@jmarrero jmarrero self-requested a review October 14, 2024 20:38
We hit another case where people are pulling a container image
with devices in `/dev` in the tar stream; they're then trying
to commit this to an ostree.

There's much better ways to fix this:

- Change the image to stop including devices as there's no reason
  to do so
- Switch to logically bound images instead of physically bound
- Use the composefs backend for c/storage

Eventually I may look at "quoting" generally in ostree, but
it's fairly invasive: ostreedev/ostree#2568

In practice today, simply ignoring the files will happen to work
for "podman run" of such images; podman will just use overlayfs
to stitch together the `diff` directories, and doesn't try to do
any validation of their contents today.
(Queue the composefs integration, which *would* do that but would
 also fix this anwyays)

Signed-off-by: Colin Walters <[email protected]>
@cgwalters cgwalters merged commit 64d01a8 into coreos:main Oct 14, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants