buildbarn / bb-remote-asset

An implementation of the Remote Asset API
Apache License 2.0
7 stars 13 forks source link

PushBlob broken on blobs that unmarshal as `Directory`s using the Action Cache asset store #33

Closed tomcoldrick-ct closed 1 month ago

tomcoldrick-ct commented 7 months ago

Due to the API design we have, we don't pass through whether the object is supposed to be a directory, so in order to construct an ActionResult with the correct semantics we must download the referenced asset from CAS, and see if it unmarshals as a Directory. If the thing pushed is a blob that parses as a Directory, we treat it as if it were one, and later probably fail due to the Directory we have being corrupted. BuildStream's Source proto is such a problematic message.

Note that what we're currently doing isn't quite in line with the spec anyway, as there's not actually a requirement that PushDirectory references a Directory proto.

harrysarson commented 1 month ago

This issue can be reproduced by the following:

  1. Upload the following Directory to the CAS and get its digest.

use reapi::build::bazel::remote::{
    asset::v1::{FetchBlobRequest, PushBlobRequest, PushDirectoryRequest},
    execution::v2::Digest,
    execution::v2::DirectoryNode,
    execution::v2::FileNode,
};

let dir = Directory {
    files: vec![],
    directories: vec![DirectoryNode {
        name: "this dir is missing".to_string(),
        digest: None,
    }],
    symlinks: vec![],
    node_properties: None,
};

let digest = upload_blob(&mut remote_connection, dir.encode_vec())?;
  1. Push the digest as an asset (either via PushBlob or PushDirectory)

use reapi::build::bazel::remote::{
    asset::v1::{FetchBlobRequest, PushBlobRequest, PushDirectoryRequest},
    execution::v2::Digest,
    execution::v2::DirectoryNode,
    execution::v2::FileNode,
};

PushBlobRequest {
    instance_name: "".to_string(),
    qualifiers: vec![],
    uris: vec!["does-not-really-matter-what-this-is".to_string()],
    expire_at: None,
    blob_digest: Some(digest.clone()),
    references_blobs: vec![],
    references_directories: vec![],
}
  1. The remote-asset server will respond with status: InvalidArgument, message: "Unknown digest function".
EdSchouten commented 1 month ago
digest: None,

That doesn't seem valid.

harrysarson commented 1 month ago

In the repoduction let dir = Directory {...} is only used to generate the bytes we uploade to the CAS using

let digest = upload_blob(&mut remote_connection, dir.encode_vec())?;

Using a Directory protobuf message, we generate bytes that look "a bit like a directory proto" but are not valid. In practice these bytes come from a different thing uploaded to the CAS (e.g. BuildStream's Source proto).

We should be able to upload anything we like to the CAS and the create a asset for it, even if (as above) that thing is a series of bytes that look like an invalid Directory protobuf message.