Skip to content
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

Rusoto for S3 communication (with support for public buckets). #869

Closed
wants to merge 8 commits into from

Conversation

rexhoffman
Copy link
Contributor

Picking up on the work of @Hugal31 in #826 with support for s3 buckets, but now with support for public buckets as well (read only mode).

Prior to this fix, the the https://rusoto.github.io/rusoto/rusoto_credential/struct.DefaultCredentialsProvider.html called the https://rusoto.github.io/rusoto/rusoto_credential/struct.ChainProvider.html and would error out if no credentials were found.

Based on this pr rusoto/rusoto#1566 a it seems anonymous mode is a mechanism that is appearing in Rusoto, but is not yet a supported via DefaultCredentialsProvider, or perhaps ever, so using a different S3Client constructor.

It would also be possible to build our own extension of DefaultCredentialsProvider, but an upstream builder might be a better solution.

@rexhoffman
Copy link
Contributor Author

By the way we are currently using this in libra, https://github.com/libra/libra/blob/master/x.toml#L11-L23, via our build extension, cargo xbuild. Can confirm success with read/write and public read from modern s3 bucket.

@Crunch09
Copy link

Using this branch as well in our build now with a recently created bucket and can confirm that it works great, thank you @rexhoffman ! 💯

Getting this warning which might be an import you no longer need?

image

@rexhoffman
Copy link
Contributor Author

Rebased since the Cargo.lock file was conflicting, also fixed the warn @Crunch09 commented on.

@rexhoffman
Copy link
Contributor Author

@glandium any chance of this being reviewed, or possibly merged in?

@rexhoffman
Copy link
Contributor Author

@froydnj perhaps you've time to look at this pr? Not sure who's engaged w/ this project?

@froydnj
Copy link
Contributor

froydnj commented Dec 8, 2020

@froydnj perhaps you've time to look at this pr? Not sure who's engaged w/ this project?

I can look, but as I do not have write access, I doubt my looking will do much good. 😁 It certainly looks a lot nicer than the old code, though!

@glandium
Copy link
Collaborator

glandium commented Dec 8, 2020

I'm going through all the PRs this week, I'll eventually reach this one.

@glandium
Copy link
Collaborator

I haven't gone through the entire PR yet, but I'm noticing that it's removing support for AWS_IAM_CREDENTIALS_URL and rusoto doesn't seem to have a knob for something equivalent, and we use AWS_IAM_CREDENTIALS_URL at Mozilla: https://searchfox.org/mozilla-central/rev/ffdb6a4d934b3f5294f18cf0e1df618109ccb36b/build/mozconfig.cache#77
So adding some shim to keep that supported would be great.

@Anton-4 Anton-4 mentioned this pull request Dec 21, 2020
Anton-4 added a commit to roc-lang/roc that referenced this pull request Dec 21, 2020
rtfeldman added a commit to roc-lang/roc that referenced this pull request Dec 23, 2020
Anton-4 added a commit to roc-lang/roc that referenced this pull request Dec 23, 2020
rtfeldman added a commit to roc-lang/roc that referenced this pull request Dec 26, 2020
added sccache binary based on mozilla/sccache/pull/869, updated ci.yml with correct syntax
@glandium glandium added this to the 0.3.0 milestone Jan 8, 2021
@rexhoffman
Copy link
Contributor Author

@glandium has rusoto going maintain mode changed anything on your side? I final have some time to dig in to sccache again.

I was about to start drilling in to the shim you'll need for AWS_IAM_CREDENTIALS_URL today.

@Fuuzetsu
Copy link

Fuuzetsu commented Feb 5, 2021

I tried this branch today because due to #632 we were unable to use a private bucket in ap-northeast-1 region. It seems to be working.

Any plan to push this forward?

@tomjohnburton
Copy link

We would love this as well. Getting a 400 error for a private bucket.

@nicholastmosher
Copy link

Ran into this problem today as well, this fix would be greatly appreciated!

@tobz
Copy link

tobz commented May 26, 2021

Bumping this as, without it, using a recently-created private (maybe public, too) S3 bucket is impossible.

The tests pass, it seems to work fine… is there a specific hold up? Does it need reviewer time that could be provided outside of the core maintainers?

@tomelliff
Copy link

Given that Rusoto isn't being actively maintained any more and that AWS have announced an official Rust SDK that is generated from their API I'd suggest that maybe it would be better to attempt to switch to this new official AWS Rust SDK instead as Rusoto is likely a dead end.

@tobz
Copy link

tobz commented May 27, 2021

If someone wants to roll a PR to use it, that seems fine, although I'm not sure choosing an alpha library is demonstrably better than a mature (even if maintenance mode) library, given that this PR fixes problems right now. As evidenced by the number of other issues and PRs linked to this one, the can has already been kicked down the road significantly.

@Xanewok
Copy link
Collaborator

Xanewok commented May 27, 2021

I just want to echo a statement that using a mature, albeit only maintained library seems like a good solution for now until the official SDK is properly released (at least the S3 parts of it, that is).

@benhannel
Copy link

This worked great for us! We're now able to use a private bucket for our cache. Thanks!

@sylvestre
Copy link
Collaborator

would it be possible to rebase this PR? it is full of conflicts, sorry :(

@jschwe
Copy link
Contributor

jschwe commented Feb 13, 2022

So adding some shim to keep that supported would be great.

Is that requirement from mozilla still up to date? The v2 implementation uses Sha1 as a digest for signing and AFAIK sha1 is viewed as insecure for cryptographic purposes.

@mitchhentges
Copy link
Contributor

Is that requirement from mozilla still up to date?

Looks like it's still the case today.

@mitchhentges
Copy link
Contributor

I'm going to mark this as obsolete in favour of #1086, since that uses the official AWS SDK.
Thanks for the work that you put into this patch, and I'm sorry we weren't able to land it sooner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet