Add content length to GCP multipart complete #7301
Conversation
|
It's very possible that I'm just holding this wrong, because I'm surprised that no one else has complained yet, however I'm seeing errors when using this library with GCP for multipart uploads of the format: |
jkosh44
force-pushed
the
gcp-multipart-complete
branch
from
8c907b7 to
3c3cf1d
Compare
|
I haven't validated this fix yet, if there's some way to test this via some integration test that would be helpful to know. Otherwise I can try running this against a real GCP bucket, but it will probably take some time to get set up. |
This is what we use for GCP integration arrow-rs/.github/workflows/object_store.yml Lines 129 to 138 in 57942c4 And then run with Then |
|
Thank you very much for this report and proposed fix @jkosh44 -- it looks nice to me In terms of testing, I think it would be ok if you:
I don't think it is required to get this wired into the integration test harness as of now |
I actually found the GCP integration test, but this comment made me think that we can't actually test the multipart uploads (because they're not implemented in the emulator): arrow-rs/object_store/src/gcp/mod.rs Lines 296 to 298 in 57942c4 I'll work on manually validating the fix, but bizarrely I stopped receiving 411s from GCP and started getting 200s. I'll try to create a new bucket and see if I can get the 411 again. |
Weird! Thank you. |
|
So I created a brand new bucket and ran the following test locally about 1000 times on use std::fs::File;
use std::io::Read;
use object_store::gcp::GoogleCloudStorageBuilder;
use object_store::multipart::MultipartStore;
#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
let store = GoogleCloudStorageBuilder::new().with_bucket_name("multipart-complete-header-test").with_application_credentials("path/to/my/credentials.json").build().unwrap();
let file_path = "path/to/some/local/file";
let file_contents = {
let mut buf = Vec::new();
File::open(file_path)
.unwrap()
.read_to_end(&mut buf)
.unwrap();
buf
};
let blob_path = "multi-test/hello.txt".into();
let id = store.create_multipart(&blob_path).await.unwrap();
let mut part_ids = Vec::new();
let part_id = store
.put_part(&blob_path, &id, 0, file_contents.into())
.await
.unwrap();
part_ids.push(part_id);
store
.complete_multipart(&blob_path, &id, part_ids)
.await
.unwrap();
// Sleep to avoid rate limiting.
tokio::time::sleep(tokio::time::Duration::from_millis(1000)).await;
Ok(())
}It does seem very odd that the docs clearly state that content-length is required, but we are not setting the content-length. My best guess as to why I was receiving a 411 is that GCP had a partial roll-out of new API servers that were validating content-length ... but that seems a bit far-fetched. I did try the same test with this branch, and it also works fine. So I'll leave it up to you if you think it's worth merging. In terms of unit tests, is there somewhere where we are already unit testing multipart uploads? It seems difficult to test without refactoring s.t. we generate and execute the request in separate methods. |
I think s3 has multipart upload tests -- but we would have to dig around to be sure I'll plan to merge this PR in over the next few days unless anyone else has an objection (BTW @jkosh44 I wonder if you need to make large multi-part requests -- like actually try to upload 10 parts or something) |
Thanks, I'll do some digging tomorrow.
Good thinking, unfortunately I tried 50 parts each of size 10 MB and it still worked fine. |
|
FWIW in the past the choice of SSL library has had an impact on this behaviour, I seem to remember a bug like this only being reproducible when using openssl instead of rustls |
|
FWIW I found an extremely similar issue in a similar repo: abdolence/gcloud-sdk-rs#121 |
I'm not entirely sure how to validate what SSL library I'm using, but I made the following changes and re-ran my test but it still worked (the |
|
Hm, this is another interesting and potentially related issue from this repo: 50cf8bd8. Though, this happens when receiving a response. I wonder if there's some logic somewhere that optionally compresses the request and somehow causes issues with the content-length header? |
|
I think I am going to assume this is basically "we have no visibility about what is going on the GCP side" and thus I am not going to worry why the error happens intermittently and non-reproducably. Given that this change seems minimally risky and could plasubly fix an issue, I am just going to merge it in Thank you for your diligence @jkosh44 |
I agree with you there. I am still planning to track down that S3 test so we can add a similar one, but I got side-tracked with trying to repro the issue. |
|
I have a repro! It turns out that I was looking at the wrong method this whole time. The error happens when the multipart upload is empty. Here's a minal repro which works on #[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
let store = GoogleCloudStorageBuilder::new().with_bucket_name("multipart-complete-header-test").with_application_credentials("/path/to/credentials.json").build().unwrap();
let blob_path = "multi-test/hello.txt".into();
let id = store.create_multipart(&blob_path).await.unwrap();
let part_ids = Vec::new();
store
.complete_multipart(&blob_path, &id, part_ids)
.await
.unwrap();
Ok(())
}Which results in the following error: The method in question is here: arrow-rs/object_store/src/gcp/client.rs Lines 516 to 522 in 660a3ac What's confusing is that is a PUT, but the error message said POST requests require a <code>Content-length</code> header which threw me off.
Adding |
Super Sleuthing! 🕵️ |
|
@tustvold it looks like you actually added the empty upload handling, https://github.com/apache/arrow-rs/pull/5590/files. Any thoughts on why the PUT is needed? From some local testing, the following also seems to work (i.e. remove the PUT): if completed_parts.is_empty() {
// GCS doesn't allow empty multipart uploads
self.multipart_cleanup(path, multipart_id).await?;
return Ok(PutResult{ e_tag: None, version: None });
}but I'm not really familiar enough with this API to know if that break's something. |
|
That changes the behaviour from uploading an empty object to uploading nothing. The empty part is necessary because otherwise it gets rejected - see the linked issue apache/arrow-rs-object-store#91 |
This commit fixes the GCP mulipart complete implementation by adding the Content-Length header to XML requests. According to the docs, https://cloud.google.com/storage/docs/xml-api/post-object-complete, this header is required in the complete POST request, but wasn't being set previously. It seems like GCP doesn't actually validate this header, but it's better to set it in case they validate in the future. Additionally, GCP is strict about setting the Content-Length header on requests with empty bodies, so we also update an empty PUT request, https://cloud.google.com/storage/docs/xml-api/put-object-multipart, with the header.
jkosh44
force-pushed
the
gcp-multipart-complete
branch
from
3c3cf1d to
ab06852
Compare
My understanding is that the POST for complete (https://cloud.google.com/storage/docs/xml-api/post-object-complete) will get rejected if there's 0 parts, but the DELETE (https://cloud.google.com/storage/docs/xml-api/delete-multipart) will get accepted. Still the empty upload isn't hurting anything so it's probably safer to just keep it for now. I've updated it to include the Content-Length header. I've also kept the content length header in the POST, even though it seems clear to me now that it's never validated. The docs say it's required, so we might as well set it. I'm happy to revert though if people disagree. EDIT: I just realized, apache/arrow-rs-object-store#91 is for AWS not GCP. So is what you're saying that we'd like to keep the behavior consistent across cloud providers? It's already slightly different in that AWS completes empty uploads while GCP aborts them. |
|
The code is performing a regular PUT, i.e. uploading a single empty object not an upload part, and then aborting the multipart upload. |
object_store/src/gcp/client.rs
Outdated
| @@ -540,6 +541,7 @@ impl GoogleCloudStorageClient { | |||
| let response = self | |||
| .client | |||
| .request(Method::POST, &url) | |||
| .header(&CONTENT_LENGTH, data.len()) | |||
There was a problem hiding this comment.
I think this should be unnecessary, and I believe is potentially incorrect if the client decides to use a some sort of transport encoding
| @@ -517,6 +517,7 @@ impl GoogleCloudStorageClient { | |||
| // GCS doesn't allow empty multipart uploads | |||
There was a problem hiding this comment.
I wonder if we could perhaps make this more clear by simply calling self.put directly
There was a problem hiding this comment.
It's a good idea, but self.put also doesn't set Content-length so we'd have the same issue. I could update self.put to always set the Content-Length if the payload is empty. That would have a larger blast radius though. Thoughts?
|
Thank you for this PR. We are in the process of moving the object_store code to its own repository. Would it be possible for you to create a PR in that repository instead?
(we will handle moving all existing issues to the new repository) |
Oh I see now, so the end result is the file exists in GCP but is empty. That makes sense. Thanks for clarifying that for me. |
Here is the new PR: apache/arrow-rs-object-store#257 |
Which issue does this PR close?
Rationale for this change
This commit fixes the GCP mulipart complete implementation by adding the Content-Length header to the XML request. According to the docs, https://cloud.google.com/storage/docs/xml-api/post-object-complete, this header is required but wasn't being set previously.
What changes are included in this PR?
Adding Content-Length header to GCP multipart complete XML request.
Are there any user-facing changes?
No