-
Notifications
You must be signed in to change notification settings - Fork 213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support XML API (#331) #1164
base: main
Are you sure you want to change the base?
Support XML API (#331) #1164
Conversation
@@ -596,23 +592,6 @@ func TestServerClientSignedUploadBucketCNAME(t *testing.T) { | |||
if resp.StatusCode != http.StatusOK { | |||
t.Errorf("wrong status returned\nwant %d\ngot %d", http.StatusOK, resp.StatusCode) | |||
} | |||
data, err := io.ReadAll(resp.Body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was incorrect, the XML APIs should not return a body and certainly shouldn't return JSON
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Oh are signed uploads powered by the XML API? Interesting!
@@ -296,12 +299,6 @@ func (s *Server) buildMuxer() { | |||
handler.Host(s.publicHost).Path("/{bucketName}").MatcherFunc(matchFormData).Methods(http.MethodPost, http.MethodPut).HandlerFunc(xmlToHTTPHandler(s.insertFormObject)) | |||
handler.Host(bucketHost).MatcherFunc(matchFormData).Methods(http.MethodPost, http.MethodPut).HandlerFunc(xmlToHTTPHandler(s.insertFormObject)) | |||
|
|||
// Signed URLs (upload and download) | |||
handler.MatcherFunc(s.publicHostMatcher).Path("/{bucketName}/{objectName:.+}").Methods(http.MethodPost, http.MethodPut).HandlerFunc(jsonToHTTPHandler(s.insertObject)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can't have ever worked as insertObject expects query parameters encoding the object name, etc...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
insertObject
does not necessarily read the object name from query parameters:
fake-gcs-server/fakestorage/upload.go
Lines 85 to 95 in d7832ec
default: | |
// Support Signed URL Uploads | |
if r.URL.Query().Get("X-Goog-Algorithm") != "" { | |
switch r.Method { | |
case http.MethodPost: | |
return s.resumableUpload(bucketName, r) | |
case http.MethodPut: | |
return s.signedUpload(bucketName, r) | |
} | |
} | |
return jsonResponse{errorMessage: "invalid uploadType", status: http.StatusBadRequest} |
I need to take a deeper look at this, but curious if you're basing on comment on the docs? There are many undocumented things in the GCS API that we run into when someone tries to integrate with some different SDK, and I've ironically done a poor job in documenting those in fake-gcs-server 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, insertObject has a peculiar special case that uses X-Goog-Algorithm
to detect a signed URL and then falls back to implementing something that looks like the XML API, because that is what signed URLs are. With this PR that can probably be removed 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, absolutely. insertObject definitely looks like a little monster doing too many things. Thank you very much! I'll go over the PR in details later today.
Hi, bumping this one - any plans to fix and merge it soon? Thanks |
@ramir-savvy someone needs to take a look at the test failures and fix them. I can do it, but not in the near future. Maybe in a couple of weeks. |
Just wondering what the status of this is, the lack of support for the XML APIs is causing some upstream confusion - apache/arrow-rs#5263 |
=( ohh, if could be a great functionality which allow us test GCS over S3 in Altinity/clickhouse-backup#695 |
…t when XML-API will implement on fsouza/fake-gcs-server#1164, fsouza/fake-gcs-server#1330
@tustvold Is this something still on your radar? |
We've been using this branch to run integration tests for the last year. I'd be happy to see this incorporated but that's out of my hands 😅 |
Yeah I think the code overall looks good, but I'd need conflicts resolved and CI to be green before merging. I can give it a shot myself, but I don't expect to have any time to work on this in the next 2-3 weeks. |
@fsouza if i merge conclicts and create new PR, will it merge? |
it would be great to get this functionality included! |
I tried using this with a mix of Rust's import os
os.environ['STORAGE_EMULATOR_HOST'] = 'http://localhost:4443'
os.environ['GOOGLE_SERVICE_ACCOUNT_KEY'] = '{"gcs_base_url": "http://localhost:4443", "disable_oauth": true, "client_email": "", "private_key": "", "private_key_id": ""}'
import pyarrow
from deltalake import DeltaTable
from google.cloud import storage
client = storage.Client()
bucket = client.bucket('test-bucket') # type: ignore
print(list(bucket.list_blobs()))
bucket.blob('test.txt').upload_from_string(b'Hello, world!')
print(list(bucket.list_blobs()))
blob = bucket.blob('test.txt')
print(blob.download_as_string())
DeltaTable.create('gs://test-bucket', schema=pyarrow.schema([('id', pyarrow.int64())]), mode='overwrite') Running Switching to running I'm guessing this has to do with virtual bucket name parsing? |
Closes #331
It has been years since I last wrote any Golang so please let me know if I've made some silly mistakes. Most of the necessary functionality already existed, the only necessary change was to add support for the XML put API