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

[rush-lib] Improve support for S3 storage for the buildCache #2614

Merged
merged 4 commits into from
Apr 17, 2021

Conversation

elliot-nelson
Copy link
Collaborator

@elliot-nelson elliot-nelson commented Apr 15, 2021

Summary

Improve support for S3 storage when enabling the build cache (feature added as part of conversation in #2393).

  • Allow S3 buckets outside the default (us-east-1) region in AWS
  • Support temporary credentials (generated using an IAM Profile or sts:assume-role)

Details

When an S3 bucket is created outside the default region, the URL generated for presigning includes a region suffix (for example, mybucket.s3-us-west-1.amazonaws.com). If this isn't taken into account, the signed request will not be valid and the request will return a 403.

When the user's credentials are temporary credentials (generated by assuming a role or using an assigned IAM Profile), you need three pieces of information and not 2: the key, the secret, and the session token. The user needs a way to give this information and the signing logic needs to include it in the appropriate places.

  • User can now set the RUSH_BUILD_CACHE_WRITE_CREDENTIAL to the value KEY:SECRET:TOKEN, with three fields, instead of just two. I believe this is considered just a string outside this context and doesn't require changes elsewhere.
  • For the signing logic, the x-amz-security-token header needs to be included in 3 places: in the request body used to generate a signature, in the list of headers inside the same request body, and as a header on the actual request (passed to fetch).

How it was tested

"It works on my machine."

(I wouldn't mind adding some new unit tests for this functionality, but I think at minimum they'd require mocking node-fetch, and I don't see a lot of mocking going on in this area of the codebase. Any advice on usual patterns to use here are appreciated.)

@iclanton
Copy link
Member

It'd be useful to have a few credential snapshot tests.

You're welcome to mock fetch. You should just be able to use Jest's built-in mocking.

Copy link
Member

@iclanton iclanton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, but tests would definitely be useful.

common/changes/@microsoft/rush/s3_2021-04-15-21-04.json Outdated Show resolved Hide resolved
@iclanton
Copy link
Member

@elliot-nelson - I just went ahead and added a bunch of tests.

@iclanton iclanton merged commit 58817d7 into microsoft:master Apr 17, 2021
@elliot-nelson
Copy link
Collaborator Author

Wow! Thanks for the test coverage (I'll refer to this later for next time.)

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.

2 participants