Added implementation for Google Cloud Storage with REST #1645

amankrx · 2025-03-26T05:24:13Z

Description

This PR implements the Google Cloud Store implementation and closely aligns with the AWS S3 implementation.
This is an update to my previous PR I raised using the gRPC implementation. There were multiple issues with that PR, and the primary issue was the stability. This PR fixes all the underlying issues we were facing with that PR. We are using the google-cloud-storage and google-cloud-auth crates.

Note: To run the gcs_backend.json5 example that I have provided, you should ensure that the path to the credentials file is specified in the GOOGLE_APPLICATION_CREDENTIALS environment variable. Check https://docs.rs/google-cloud-storage/0.24.0/google_cloud_storage/#automatically for more details.

Fixes #659
/claim #659

Type of change

Please delete options that aren't relevant.

New feature (non-breaking change which adds functionality)
This change requires a documentation update

How Has This Been Tested?

I did an E2E testing using the gcs_backend.json5 example that I have provided. Along with that, I have set up a mocking framework for the GCS client and added a few tests for both the GCS store and the GCS client.

Checklist

Updated documentation if needed
Tests added/amended
bazel test //... passes locally
PR is contained in a single commit, using git amend see some docs

This change is

MarcusSorealheis · 2025-03-26T13:14:58Z

The only issue now is your private key.

amankrx · 2025-03-26T13:57:14Z

The only issue now is your private key.

Fixed it!

aaronmondal

I didn't get through all of it, but at the moment I'd say that this would introduce too much maintenance overhead considering that this is all a single store. If this is the amount of logic necessary to support a reliable GCS setup I'd honestly lean towards dropping this entirely.

cc @kubevalet AFAIU we already run on gcs via the filesystem store, right? Would it be viable to just recommend people to use that instead and improve documentation around that? I'm trying to understand the tradeoff that we'd be making here in terms of maintainability vs maintenance overhead. Are there usecases where one "has" to use GCS?

@amankrx Maybe my concerns are unreasonable. Have you explored ways to make this... well let's say "less code"?

Reviewed 19 of 19 files at r1, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained, and all files reviewed, and 12 discussions need to be resolved

-- commits line 2 at r1:
"Add" instead of "Added"

nativelink-store/Cargo.toml line 31 at r1 (raw file):

bytes = { version = "1.10.0", default-features = false }
bytes-utils = { version = "0.1.4", default-features = false }
chrono = "0.4.39"

Add this (and the other creates) with x = { version = "y", default -features = false } by default to prevent pulling in unnecessary deps.

nativelink-store/src/gcs_client/client.rs line 84 at r1 (raw file):

    pub async fn new_with_http_client(
        spec: &GcsSpec,
        client: hyper::Client<C>,

fyi this will need a hyper 1.x implementation after #1639. It might be easier to build on top of that so that we don't need migrate this one as well. The hyper 1.x migration is a bit complicated and we're still testing, but it should make it into main in the next few days.

nativelink-store/src/gcs_client/client.rs line 229 at r1 (raw file):

            return Err(make_err!(
                match status.as_u16() {
                    401 | 403 => Code::PermissionDenied,

nit: Do these have typed names?

nativelink-store/src/gcs_client/client.rs line 315 at r1 (raw file):

        }

        tokio::time::sleep(Duration::from_millis(200)).await; // Small delay to allow GCS to finalize

Let's make this a named const.

nativelink-store/src/gcs_client/client.rs line 642 at r1 (raw file):

        match status.as_u16() {
            200 | 201 | 308 => Ok(()),

nit: as above

nativelink-store/src/gcs_client/operations.rs line 14 at r1 (raw file):

// See the License for the specific language governing permissions and
// limitations under the License.

nit: Seems unnecessary to have this in a dedicated file.

Cargo.lock line 42 at r1 (raw file):

[[package]]
name = "android_system_properties"

Whatever causes these android_* dependencies to be pulled in should probably be disabled. My guess is that it's some default feature in some cratej (chrono maybe?). Try adding the deps as somedep = { version = "x.y.z", default-features = false, features = ["somefeature"] } in Cargo.toml where you only enable the minimal set of features that we need.

nativelink-store/src/gcs_store.rs line 42 at r1 (raw file):

};

struct ConnectionPool {

fyi We'll probably need to rework this as we're moving the entire nativelink-store setup to hyper 1.x (Current WIP branch at #1639). I general I like the explicitness of this approach though and I'll check whether this is something I should add to the aws store as well.

nativelink-store/src/gcs_store.rs line 204 at r1 (raw file):

    NowFn: Fn() -> I + Send + Sync + Unpin + 'static,
{
    async fn has_with_results(

This looks duplicated. We might get away with lifting this out of the aws implementation and reusing it in both places.

nativelink-store/src/gcs_store.rs line 272 at r1 (raw file):

        // For larger files, use resumable upload
        // First, initiate the upload session
        let upload_id = self

note to self: This looks wrong to me. I can't pinpoint it though. Will to evaluate in more detail.

nativelink-store/src/gcs_store.rs line 275 at r1 (raw file):

            .retrier
            .retry(unfold(object.clone(), move |object| async move {
                let conn = match self.connection_pool.acquire().await {

Hmm I'm not entirely sure on this one but I feel like the connection acquisition and retrial of that should be handled via the client side. Otherwise we'd have to "remember" to always acquire the permit in the calls. And it introduces fairly tight coupling between the store implementation and the underlying client.

nativelink-store/src/gcs_store.rs line 436 at r1 (raw file):

            .retrier
            .retry(unfold(
                (object.clone(), offset, end_offset),

I believe all of these xx.clone() calls in the unfolds (here and everywhere else) should be moves. It shouldn't be necessary to clone the objects or at least only do so in the minimal cases necessary.

nativelink-store/src/gcs_client/connector.rs line 83 at r1 (raw file):

        Pin::new(&mut Pin::get_mut(self).connection).poll_shutdown(cx)
    }
}

fyi this will need to be changed after the hyper 1.x migration mentioned earlier.

.gitignore line 6 at r1 (raw file):

.idea/
.zed
.idea/

This is duplicated from two lines above. Should be safe to remove.

nativelink-store/src/gcs_client/auth.rs line 35 at r1 (raw file):

#[derive(Debug, Serialize)]
struct JwtClaims {

Is it actually necessary to roll our own implementation here? Are there reasons we can't use e.g. https://github.com/googleapis/google-cloud-rust/tree/main/src/auth or https://github.com/yoshidan/google-cloud-rust/tree/main/foundation/auth ?

For instance, if we used the official sdk, if we wanted to move the gcs store to non-experimental we could just wait until the official rust sdk reaches GA and then "bump a dependency".

I'm also worried that rolling our own here would require us to maintain perfect coverage and fuzzing as it would be "on us" to maintain the security guarantees of this implementation. This seems like a big lift. cc @kubevalet

amankrx

We can get this done with less code altogether. The primary issue is that we don't have a proper GCS SDK with a custom choice of client to use. So, the majority of the implementation is primarily the client itself. I'm not sure if we would be changing the client that much later on. So, it's up to us if we want a custom client like the one that the one I have provided, or to use third-party ones like https://github.com/abdolence/gcloud-sdk-rs
Also, it was initially less than 1000 lines of code PR, but the primary overhead was implementing traits and associated methods to add the unit tests. Apart from that, if it is absolutely necessary, I can work on reducing the unnecessary addition of extra modularity and concise it a bit.

Reviewable status: 0 of 2 LGTMs obtained, and all files reviewed, and 12 discussions need to be resolved

-- commits line 2 at r1: