Switch to use AWS signature v4 #1496
Conversation
|
It looks like the tests need to be updated too. Do you need any help with that? |
|
Sorry this was stuck behind a few other changes that I wanted to get into cargo, hopefully shouldn't take too much to get the tests passing, so I will sort them out. |
Conflicts: Cargo.lock src/bin/render-readmes.rs src/lib.rs src/s3/Cargo.toml src/s3/lib.rs src/uploaders.rs src/util/errors.rs
There was a problem hiding this comment.
@cswindle based on the tests that are failing, I think you're running into some issues with the http-data recording proxy used by the test code. You'll want to merge in master again as I recently landed some changes to the JSON output that will help here.
Once you have the proxy wired in, you'll need to record updated test data. I think that will go something roughly like:
- Delete the files in
src/tests/http-datathat are related to your failing tests. I think the recording only happens if the file doesn't yet exist. Some of these test against the GitHub API, so don't delete more than you need to. Unfortunately, it looks like you have a few tests that sound like they do interact with GitHub:owners::new_crate_owner,team::publish_owned,team::remove_team_as_team_owner. - Set up your environment variables to point to a bucket that the tests can run against as they are recorded.
- Run
RECORD=1 cargo test - You should now have files for ALL the tests. A lot of them will just contain
[]. You can ignore the files that show up as new ingit status, and just pay attention to the ones that show as modified. - If the diff looks good, you can run
cargo testto verify that the tests work with the newly recorded data.
I hope that helps with getting the test green.
| let path = Uploader::crate_path(crate_name, version); | ||
| Some(format!("https://{}/{}", host, path)) | ||
| Some(format!("https://{}/{}/{}", host, bucket, path)) |
There was a problem hiding this comment.
It looks like this would change the location of where crates are stored in production. If this is necessary, it will need to be configurable.
| }; | ||
| let path = Uploader::readme_path(crate_name, version); | ||
| Some(format!("https://{}/{}", host, path)) | ||
| Some(format!("https://{}/{}/{}", host, bucket, path)) |
| @@ -148,7 +183,7 @@ impl Uploader { | |||
| let mut body = Vec::new(); | |||
| LimitErrorReader::new(req.body(), maximums.max_upload_size).read_to_end(&mut body)?; | |||
| verify_tarball(krate, vers, &body, maximums.max_unpack_size)?; | |||
| self.upload(&app.http_client()?, &path, body, "application/x-tar")? | |||
| self.upload(&path, body, "application/x-tar")? | |||
There was a problem hiding this comment.
The removed call to app.http_client() is part of the test infrastructure to record and play back HTTP requests. It looks like upload needs to add the proxy information as part of building the PutObjectRequest.
| let req = DeleteObjectRequest { | ||
| bucket: bucket.to_string(), | ||
| key: path.to_string(), | ||
| ..Default::default() |
There was a problem hiding this comment.
Same thing here, this needs to go through the proxy when testing.
|
Thanks for the pointers @jtgeibel, I have now managed to get one of the tests working by using record, so should be able to start making progress on this again. |
|
@cswindle Are you still interested in working on this? |
|
This is blocked on #1607 |
|
@sgrif, we are currently running with a fork of crates.io to run internally as a private registry at work, so I am keen to gradually get all the changes merged back into crates.io codebase (which this is the first of a few changes), however I have been very busy with work, so not had any opportunity to get this work done. Likely be at least a month before I even think about picking this up again. |
|
@cswindle Thanks for the update. Are you ok with closing this PR until you're more able to work on it? |
|
I'm going to close this PR for now; please feel free to reopen when you have time to work on this again @cswindle! |
The main changes in the PR are to add support for AWS signature v4 (rather than the deprecated v2) to fix #1486. It also includes the following minor changes that I made to make it easier to validate the fix: