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

util/s3: enable more credential providers #4929

Merged

Conversation

algolucky
Copy link
Contributor

@algolucky algolucky commented Dec 21, 2022

this will not be live until update.sh is updated to use the version this updater is released with, e.g.

UPDATER_MIN_VERSION="3.12.2"

Summary

  • updates the s3Helper to allow more credential providers. now AWS_ environment variables, config files, and EC2 metadata are supported credential providers when using tools like updater and goal logging send.
  • removes error if AWS_ environment variables are not set
  • removes the ability to configure credentials with a s3.json file
  • forces use of Anonymous credentials when communicating with algorand-releases
  • if no credentials are found, it uses Anonymous credentials

Test Plan

  • ci
  • internal performance testing

@algolucky algolucky self-assigned this Dec 21, 2022
@algolucky algolucky force-pushed the util/s3-use-more-cred-options branch from e31cf0d to 8d0930f Compare December 21, 2022 16:12
@algolucky algolucky force-pushed the util/s3-use-more-cred-options branch from 8d0930f to c059238 Compare December 21, 2022 16:46
@algolucky algolucky force-pushed the util/s3-use-more-cred-options branch from c059238 to d8620da Compare December 21, 2022 16:46
@algolucky algolucky requested review from a team, excalq, algobarb and algojack December 21, 2022 16:48
@algolucky algolucky changed the title util/s3: enable more credential options util/s3: enable more credential providers Dec 21, 2022
@codecov
Copy link

codecov bot commented Dec 21, 2022

Codecov Report

Merging #4929 (026f0aa) into master (3fbe53c) will decrease coverage by 0.00%.
The diff coverage is 20.00%.

❗ Current head 026f0aa differs from pull request most recent head 3c49eed. Consider uploading reports for the commit 3c49eed to get more accurate results

@@            Coverage Diff             @@
##           master    #4929      +/-   ##
==========================================
- Coverage   53.51%   53.50%   -0.01%     
==========================================
  Files         432      432              
  Lines       53615    53624       +9     
==========================================
  Hits        28691    28691              
- Misses      22695    22702       +7     
- Partials     2229     2231       +2     
Impacted Files Coverage Δ
logging/cyclicWriter.go 46.46% <0.00%> (-3.00%) ⬇️
logging/telemetryhook.go 50.70% <50.00%> (+0.34%) ⬆️
crypto/merkletrie/trie.go 66.42% <0.00%> (-2.19%) ⬇️
crypto/merkletrie/node.go 91.62% <0.00%> (-1.87%) ⬇️
catchup/peerSelector.go 98.95% <0.00%> (-1.05%) ⬇️
catchup/service.go 69.32% <0.00%> (-0.49%) ⬇️
ledger/acctupdates.go 68.99% <0.00%> (-0.25%) ⬇️
network/wsPeer.go 69.45% <0.00%> (+0.47%) ⬆️
ledger/testing/randomAccounts.go 56.88% <0.00%> (+1.22%) ⬆️
ledger/blockqueue.go 88.50% <0.00%> (+2.87%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@algolucky algolucky force-pushed the util/s3-use-more-cred-options branch from 4e97167 to 5b9cb3b Compare December 21, 2022 17:48
@algolucky algolucky marked this pull request as ready for review December 21, 2022 18:16
Copy link
Contributor

@algobarb algobarb left a comment

Choose a reason for hiding this comment

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

It looks much more clean now. What is the benefit of changing it to "only uses Anonymous credentials when communicating with algorand-releases" if bucket == s3DefaultReleaseBucket?

@algolucky
Copy link
Contributor Author

algolucky commented Dec 22, 2022

@algobarb for this specific scenario:

I am in my account, on an EC2 with an IAM role. If that IAM role does not have access to S3, then updater will return a 403.

This is just a shortcut so that whenever updater is dealing with algorand-releases we use anonymous credentials (or no credentials). when there are no credentials passed, GET on algorand-releases should work.

algobarb
algobarb previously approved these changes Dec 22, 2022
Copy link
Contributor

@algobarb algobarb left a comment

Choose a reason for hiding this comment

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

LGTM

@algolucky algolucky force-pushed the util/s3-use-more-cred-options branch from 67129e6 to 3c49eed Compare December 22, 2022 18:43
@algorandskiy algorandskiy merged commit a2856b1 into algorand:master Jan 6, 2023
@algolucky algolucky deleted the util/s3-use-more-cred-options branch January 6, 2023 22:23
winder pushed a commit to winder/go-algorand that referenced this pull request Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants