image-rs: Handle gzip whiteouts correctly #687

squarti · 2024-08-20T15:34:47Z

This PR readds whiteout handling on platforms that support xattr

Fixes: #604

Xynnn007 · 2024-08-21T06:29:19Z

image-rs/src/unpack.rs

@@ -60,6 +130,10 @@ pub async fn unpack<R: AsyncRead + Unpin>(input: R, destination: &Path) -> Resul
            .try_into()
            .context("GID is too large!")?;

+        if !convert_whiteout(&file.path()?, file.header(), destination)? {


I noticed that there are some duplicated logic between convert_whiteout and unpack after line 139. Could we do some refactoring upon this commit to avoid this?

Yes, I will refactor it

@Xynnn007 I pushed some changes. Is that what you have in mind?

bpradipt · 2024-08-21T07:25:26Z

@squarti @Xynnn007 are you aware of any tools that helps to inspect a container image to figure out if it contains gzip whiteouts?
Or any sample image that can be used for testing this PR?

squarti · 2024-08-21T13:24:16Z

@squarti @Xynnn007 are you aware of any tools that helps to inspect a container image to figure out if it contains gzip whiteouts? Or any sample image that can be used for testing this PR?

I am not aware of any tool. I used this image ghcr.io/actions/actions-runner:latest to test it

Xynnn007

Thanks for the refactoring! Two last comments and other parts looks perfect to me

Xynnn007 · 2024-08-22T02:04:51Z

image-rs/src/unpack.rs

+}
+
+#[cfg(any(target_os = "linux", target_os = "freebsd", target_os = "macos"))]
+use anyhow::anyhow;


Let's move this

use anyhow::anyhow;

directly inside convert_whiteout

Xynnn007 · 2024-08-22T02:06:25Z

image-rs/src/unpack.rs

+
+#[cfg(not(any(target_os = "linux", target_os = "freebsd", target_os = "macos")))]
+fn convert_whiteout(path: &Path, uid: u32, gid: u32, mode: Option<u32>, destination: &Path) -> Result<bool> {
+    Ok(true)


Probably we could use a shared convert_whiteout funciton. With cfg-if to firstly judge the platform and return Ok(true) if needed. Thus we do not need two convert_whiteout versions.

mythi · 2024-08-22T05:19:40Z

@squarti @Xynnn007 are you aware of any tools that helps to inspect a container image to figure out if it contains gzip whiteouts? Or any sample image that can be used for testing this PR?

I am not aware of any tool. I used this image ghcr.io/actions/actions-runner:latest to test it

FWIW, I've built images with whiteouts using the distroless tooling in the past. In case it's useful..

Xynnn007

One last thing. Sorry I did not make it clear

Xynnn007 · 2024-08-23T03:14:27Z

image-rs/src/unpack.rs

+    mode: Option<u32>,
+    destination: &Path,
+) -> Result<bool> {
+    if cfg!(target_os = "linux") || cfg!(target_os = "freebsd") || cfg!(target_os = "macos") {


We can try to return ASAP to avoid big {..} block

if !cfg!(target_os = "linux") && !cfg!(target_os = "freebsd") && !cfg!(target_os = "macos") { return Ok(true) } ... other logic

Xynnn007

good. A rebase is needed

Xynnn007 · 2024-08-28T02:32:12Z

image-rs/src/unpack.rs

+            bail!("mknod: {:?} error: {:?}", path, io::Error::last_os_error());
+        }
+
+        set_perms_ownerships(&path, ChownType::LChown, uid, gid, mode).await?;


Do we need also to set_file_times() here?

Good question. Looking into the golang base implementation, set_file_times is not done.

The purpose of the whiteout file is to hide the original file. Even if we update the file times, it is not going to be visible in the file system.

fitzthum

Nice. A few comments. I guess this is mainly being ported from the Go code, but I'd like to understand a little better what's happening.

fitzthum · 2024-08-28T20:47:12Z

image-rs/src/unpack.rs

@@ -77,6 +129,11 @@ pub async fn unpack<R: AsyncRead + Unpin>(input: R, destination: &Path) -> Resul
            .gid()?
            .try_into()
            .context("GID is too large!")?;
+        let mode = file.header().mode().ok();
+
+        if unpack_xattrs && !convert_whiteout(&file.path()?, uid, gid, mode, destination).await? {


I guess you might have inherited some of this code, but the logic here seems a bit odd to me. convert_whiteout doesn't necessarily convert a whiteout. It checks if we are pointing to a whiteout file and then converts it if needed. This results in a slightly clunky Result<bool> being returned.

What about making two functions. One called is_whiteout which returns Result<bool> and one called convert_whiteout which just returns a Result<()>

This seem like a bit of messing around but I think it will make this nicer.

fitzthum · 2024-08-28T20:59:46Z

image-rs/src/unpack.rs

+            original_path.display()
+        ))?;
+
+        let ret = unsafe { nix::libc::mknod(path.as_ptr(), nix::libc::S_IFCHR, 0) };


I'm a little confused by this part. Is this a trick to get rid of a file from a lower layer at the same location as the whiteout? It might be good to put a comment here (and maybe also a higher-level one with some reference to a spec) since this is a somewhat unusual operation. Why not just delete the file at that path or am I totally off here?

fitzthum

This mknod thing is still a bit mysterious to me, but I guess we can live with it.

Fixes: confidential-containers#604 Signed-off-by: Silenio Quarti <[email protected]>

squarti · 2024-08-29T14:32:03Z

mknod creates a character device with 0/0 device number which hides the file in the lower layer. The lower layer cannot be changed since it is read only.

Xynnn007 · 2024-08-29T14:57:09Z

image-rs/src/unpack.rs

+        original_path.display()
+    ))?;
+
+    let ret = unsafe { nix::libc::mknod(path.as_ptr(), nix::libc::S_IFCHR, 0) };


I am not sure we use nix here, s.t. https://docs.rs/nix/0.29.0/nix/sys/stat/fn.mknod.html If we can, we will get code w/o unsafe parts

I am not sure this is what you are after.

Signed-off-by: Silenio Quarti <[email protected]>

fitzthum · 2024-08-29T21:37:05Z

Thanks @squarti

Let's keep an eye on image unpacking when we next bump guest-components just in case.

squarti requested review from sameo, jiangliu, arronwy, lumjjb, jialez0 and Xynnn007 as code owners August 20, 2024 15:34

squarti force-pushed the main branch from a051fef to 573cb05 Compare August 20, 2024 15:49

Xynnn007 reviewed Aug 21, 2024

View reviewed changes

squarti force-pushed the main branch from c1bd004 to 819f183 Compare August 21, 2024 16:16

Xynnn007 reviewed Aug 22, 2024

View reviewed changes

squarti force-pushed the main branch from 6e7cae3 to 6b4b515 Compare August 22, 2024 15:54

Xynnn007 reviewed Aug 23, 2024

View reviewed changes

Xynnn007 approved these changes Aug 23, 2024

View reviewed changes

Xynnn007 mentioned this pull request Aug 26, 2024

image-rs: Whiteout code causing problems with image pull? #604

Closed

squarti force-pushed the main branch 2 times, most recently from 0aa479f to 0c2c5b6 Compare August 27, 2024 14:19

Xynnn007 reviewed Aug 28, 2024

View reviewed changes

squarti force-pushed the main branch 3 times, most recently from 2ad2ced to f65dd6d Compare August 28, 2024 14:47

fitzthum reviewed Aug 28, 2024

View reviewed changes

squarti force-pushed the main branch from f65dd6d to a5dac0f Compare August 29, 2024 03:33

fitzthum approved these changes Aug 29, 2024

View reviewed changes

image-rs: Handle gzip whiteouts correctly

f25a3ca

Fixes: confidential-containers#604 Signed-off-by: Silenio Quarti <[email protected]>

squarti force-pushed the main branch from a5dac0f to f25a3ca Compare August 29, 2024 14:25

Xynnn007 reviewed Aug 29, 2024

View reviewed changes

squarti requested a review from a team as a code owner August 29, 2024 20:36

review: use nix mknod

84c9667

Signed-off-by: Silenio Quarti <[email protected]>

squarti force-pushed the main branch from 5a675bf to 84c9667 Compare August 29, 2024 20:37

fitzthum merged commit 02af65a into confidential-containers:main Aug 29, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

image-rs: Handle gzip whiteouts correctly #687

image-rs: Handle gzip whiteouts correctly #687

squarti commented Aug 20, 2024

Xynnn007 Aug 21, 2024

squarti Aug 21, 2024

squarti Aug 21, 2024

bpradipt commented Aug 21, 2024 •

edited

Loading

squarti commented Aug 21, 2024

Xynnn007 left a comment

Xynnn007 Aug 22, 2024

Xynnn007 Aug 22, 2024

mythi commented Aug 22, 2024

Xynnn007 left a comment

Xynnn007 Aug 23, 2024

Xynnn007 left a comment

Xynnn007 Aug 28, 2024

squarti Aug 28, 2024

fitzthum left a comment

fitzthum Aug 28, 2024

fitzthum Aug 28, 2024

fitzthum left a comment

squarti commented Aug 29, 2024 •

edited

Loading

Xynnn007 Aug 29, 2024

squarti Aug 29, 2024

fitzthum commented Aug 29, 2024

image-rs: Handle gzip whiteouts correctly #687

image-rs: Handle gzip whiteouts correctly #687

Conversation

squarti commented Aug 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpradipt commented Aug 21, 2024 • edited Loading

squarti commented Aug 21, 2024

Xynnn007 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mythi commented Aug 22, 2024

Xynnn007 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xynnn007 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzthum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzthum left a comment

Choose a reason for hiding this comment

squarti commented Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzthum commented Aug 29, 2024

bpradipt commented Aug 21, 2024 •

edited

Loading

squarti commented Aug 29, 2024 •

edited

Loading