-
Notifications
You must be signed in to change notification settings - Fork 660
Fixing Google Cloud Storage (GCS) support #32573
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
base: main
Are you sure you want to change the base?
Conversation
0df9b1c
to
2901ae4
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #32573 +/- ##
==========================================
- Coverage 64.02% 63.97% -0.05%
==========================================
Files 1987 2002 +15
Lines 195630 198415 +2785
Branches 6550 6550
==========================================
+ Hits 125256 126943 +1687
- Misses 60573 61478 +905
- Partials 9801 9994 +193
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2901ae4
to
7873e0d
Compare
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.
LGTM!
Nit: Missing changes/
file, but don't worry, we can add it after merge.
7873e0d
to
7fb32b5
Compare
@lucasmrod I just added a |
@lucasmrod so you have successfully reproduced the issue — that "Forbidden" error is reflective of the HTTP 403 response that GCS returns back to fleet. That said, while this has fixed part of the bug (e.g. HEAD requests to GCS are working now), some requests to GCS are still failing. I am able to reproduce what you are showing and am looking into it further. Looks like it is specifically failing to upload the installer package to GCS:
|
@aaronmaxlevy Thanks for looking into this! |
@lucasmrod No problem! It looks like 41fd840 complicated this further because it switched to using multipart uploads, and there seems to be a compatibility issue with GCS with that approach. I am looking further into it and hoping to have a workaround / solution soon. |
@lucasmrod I have pushed a new commit that fixes multipart upload to GCS also — it's a bit janky, but there isn't really a better option right now AFAICT. Probably the best long term solution would be to support GCS as a separate storage mechanism backed by Google's own SDK, but that would be a larger, breaking change (the GCS SDK uses different authentication), and IMO, this fixes things until something better is in place. FWIW, the Accept-Encoding header issue is specific to GCS, but the multipart upload issue with trailing checksums I can see being an issue potentially with other "S3-compatible" providers that haven't implemented support for that yet. |
e15dba8
to
3ae7d7d
Compare
Hi @aaronmaxlevy! I'm testing your latest changes and one upload worked (7.5 MB installer) but then uploading other larger installers started to fail/hang. Also tried a very small one (< 1MB) and it failed with signature error. Maybe we can try not doing multipart upload when using GCS? |
3ae7d7d
to
6abde9d
Compare
@lucasmrod that is interesting — I have tried it multiple times now with installers of various larger sizes and it is working consistently with larger installers. The largest I tested with was 530 MB. Often times it would hang for a bit at 97%, but then complete successfully — I think that is just reflective of the time it takes to transfer the data, and the challenges with accurately estimating progress percentage. I was able to reproduce the signing error you received with a small installer package, and I have pushed a fix for this. Currently, the S3 Upload Manager code being used will only do multipart upload for installers larger than 5 MB. For smaller installers, it will just do a normal "PutObject", which is the only other S3 API for which the AWS Go SDK v2 enables trailing checksums. In other words, even if we didn't use multipart uploads at all for GCS, this would still be an issue. Anyhow, this should work fully / properly now. Let me know if there are any issues. |
Fixes #32571
Implementing workaround from aws/aws-sdk-go-v2#1816 (comment)