Add get_blob_stream (prefer GetRawBlob) #114

0verflowme · 2025-12-19T18:29:23Z

This adds a higher-level blob streaming API that prefers the newer GetRawBlob method (when supported by the negotiated proxy protocol version) and otherwise falls back to GetBlob.

Motivation: bootc-dev/bootc#1424

Signed-off-by: Priyanshu Kumar <priyanshu.kumar@broadcom.com>

gemini-code-assist

Code Review

This pull request introduces a new get_blob_stream API that intelligently prefers the GetRawBlob method when supported by the proxy protocol, falling back to GetBlob otherwise. A significant addition is the VerifiedBlobReader and associated hashing logic, which enables on-the-fly digest and size verification for streamed blobs, enhancing data integrity. The changes also include necessary dependency updates (hex, sha2), new error types, and protocol version checks. The ProxyTooOld error message has been corrected for clarity. Overall, this is a well-implemented feature that improves the robustness and security of blob fetching.

gemini-code-assist · 2025-12-19T18:31:07Z

src/imageproxy.rs

+                let driver = async move {
+                    err.await.map_err(Error::from)?;
+                    match rx.await {
+                        Ok(r) => verify_blob_bytes_read(&expected, expected_size, r),


In the driver future, if rx.await returns an Err(_) (meaning the oneshot::Sender was dropped without sending a message), the driver currently returns Ok(()). This could potentially mask an issue if the VerifiedBlobReader was dropped unexpectedly (e.g., due to a panic) before its Drop implementation could send VerifiedBlobReadResult::Incomplete. While the Drop impl is designed to send Incomplete, an Err from rx.await indicates an even more unexpected state. It might be safer to propagate this error or at least log it, rather than silently succeeding, to ensure that all potential issues during blob streaming are surfaced.

Suggested change

Ok(r) => verify_blob_bytes_read(&expected, expected_size, r),

Err(e) => Err(Error::Other(format!("Blob stream verification channel error: {e}").into())),

If I understand this correctly, this review is incorrect. rx.await returns Err(_) when the sender is dropped without sending (canceled). A panic will still run Drop, and Drop sends Incomplete, so you’d receive Ok(Incomplete), not Err(_).

0verflowme · 2025-12-19T18:32:37Z

Question on fallback robustness:
Right now get_blob_stream() decides between GetRawBlob and GetBlob purely based on the negotiated protocol version (no string-matching on errors). Can we confirm the protocol contract that Initialize returning >= 0.2.8 guarantees GetRawBlob is implemented?
If that guarantee doesn’t hold, we probably need a machine-readable capability signal so that fallback remains reliable.

cc: @cgwalters

cgwalters

Thanks for working on this! Just as a first pass I think what would help build confidence here is ensuring in CI we're always testing both the old and new code.

One suggestion: A CI run that tests in quay.io/almalinuxorg/almalinux-bootc:10.0 as that uses skopeo version 1.18.1 which predates https://github.com/containers/skopeo/releases/tag/v1.19.0 when this feature appeared.

Run tests in containers with skopeo 1.18 and >=1.19 to exercise both GetBlob fallback and GetRawBlob paths. Signed-off-by: Priyanshu Kumar <priyanshu.kumar@broadcom.com>

0verflowme · 2025-12-22T09:39:02Z

@cgwalters I've updated the PR Can you please check?

cgwalters

Thanks so much for working on this! A second pass review

cgwalters · 2026-01-22T18:41:38Z

src/imageproxy.rs

+            Digest::from_str(&format!("sha256:{}", hex::encode(h.finalize()))).unwrap()
+        }
+
+        fn write_blob(root: &std::path::Path, bytes: &[u8]) -> Result<(Digest, u64)> {


There's an https://docs.rs/ocidir/latest/ocidir/ crate for this

cgwalters · 2026-01-22T18:42:16Z

.github/workflows/ci.yaml

+            -v "$PWD:/src:ro" \
+            -v "$PWD/.ci-cargo-home:/root/.cargo" \
+            -w /src \
+            quay.io/fedora/fedora:41 \


I don't thin fedora:41 has the new skopeo, also it's end of life. Let's use quay.io/centos/centos:stream10` instead

cgwalters · 2026-01-22T18:44:58Z

src/imageproxy.rs

+                debug_assert!(after >= before);
+                let delta = after - before;


How about let delta = after.checked_sub(before).unwrap()

cgwalters · 2026-01-22T19:04:18Z

src/imageproxy.rs

+        digest: &Digest,
+        expected_size: u64,
+    ) -> Result<BlobStream<'a>> {
+        let fallback_to_get_blob = || async move {


Why the closure?

cgwalters · 2026-01-22T19:04:30Z

src/imageproxy.rs

+            return fallback_to_get_blob().await;
+        }
+
+        match self.get_raw_blob(img, digest).await {


We can just ? here

cgwalters · 2026-01-22T19:08:08Z

src/imageproxy.rs

+
+#[derive(Debug)]
+enum VerifiedBlobReadResult {
+    Complete { nbytes: u64, digest_hex: String },


Let's add type safety to digest_hex using https://docs.rs/oci-spec/latest/oci_spec/image/struct.Digest.html

cgwalters · 2026-01-22T19:12:51Z

src/imageproxy.rs

+                let delta = after - before;
+                if delta > 0 {
+                    let chunk = &buf.filled()[before..after];
+                    if let Some(hasher) = self.hasher.as_mut() {


We should always have a hasher right?

cgwalters · 2026-01-22T19:15:39Z

src/imageproxy.rs

+                debug_assert!(after >= before);
+                let delta = after - before;
+                if delta > 0 {
+                    let chunk = &buf.filled()[before..after];


I am a bit confused by this...it seems like we're trying to avoid skipping the start of the buffer, but why do we need to do that? Can't we just consume filled() each time?

Also isn't this missing calling clear()?

(I am not super familiar with implementing AsyncRead this way to be clear)

cgwalters · 2026-01-22T19:16:12Z

src/imageproxy.rs

+    inner: R,
+    nbytes: u64,
+    hasher: Option<Hasher>,
+    completion: Option<oneshot::Sender<VerifiedBlobReadResult>>,


A comment on this would be useful, the expected control flow here is not obvious to me.

If there's a way we can avoid this oneshot channel that'd be good

cgwalters · 2026-01-22T19:17:16Z

src/imageproxy.rs

+        }
+        let before = buf.filled().len();
+        match Pin::new(&mut self.inner).poll_read(cx, buf) {
+            v @ std::task::Poll::Ready(Ok(_)) => {


Let's use () instead of _ to strengthen the match

Add get_blob_stream (prefer GetRawBlob)

98c7973

Signed-off-by: Priyanshu Kumar <priyanshu.kumar@broadcom.com>

gemini-code-assist bot reviewed Dec 19, 2025

View reviewed changes

cgwalters reviewed Dec 19, 2025

View reviewed changes

ci: test get_blob_stream against old/new skopeo

a8886bc

Run tests in containers with skopeo 1.18 and >=1.19 to exercise both GetBlob fallback and GetRawBlob paths. Signed-off-by: Priyanshu Kumar <priyanshu.kumar@broadcom.com>

0verflowme force-pushed the get-raw-blob-stream branch from 824950e to a8886bc Compare December 20, 2025 05:30

0verflowme marked this pull request as ready for review December 22, 2025 09:39

cgwalters mentioned this pull request Jan 21, 2026

skopeo: Get uncompressed layer when pulling from containers-storage containers/composefs-rs#210

Open

cgwalters reviewed Jan 22, 2026

View reviewed changes

cgwalters self-assigned this Jan 22, 2026

	Ok(r) => verify_blob_bytes_read(&expected, expected_size, r),
	Err(e) => Err(Error::Other(format!("Blob stream verification channel error: {e}").into())),

Add get_blob_stream (prefer GetRawBlob) #114

Are you sure you want to change the base?

Add get_blob_stream (prefer GetRawBlob) #114

Uh oh!

Conversation

0verflowme commented Dec 19, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0verflowme commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

0verflowme commented Dec 22, 2025

Uh oh!

cgwalters left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

0verflowme commented Dec 19, 2025 •

edited

Loading

cgwalters left a comment •

edited

Loading