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

Automatically refresh expired container credentials #754

Merged
merged 5 commits into from
Apr 12, 2022

Conversation

josb
Copy link
Contributor

@josb josb commented Apr 10, 2022

So far, testing by @mk4437 suggests this patch addresses the S3 errors [#728] we are seeing. Happy to tweak this patch further as needed.

@josb josb requested a review from a team April 10, 2022 19:04
@codecov-commenter
Copy link

codecov-commenter commented Apr 10, 2022

Codecov Report

Merging #754 (d014f94) into master (ad8f261) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

❗ Current head d014f94 differs from pull request most recent head e580187. Consider uploading reports for the commit e580187 to get more accurate results

@@            Coverage Diff             @@
##           master     #754      +/-   ##
==========================================
- Coverage   73.26%   73.21%   -0.05%     
==========================================
  Files          49       49              
  Lines       10831    10838       +7     
==========================================
  Hits         7935     7935              
- Misses       2896     2903       +7     
Impacted Files Coverage Δ
crates/symbolicator/src/services/download/s3.rs 40.65% <0.00%> (-0.80%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad8f261...e580187. Read the comment docs.

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

Might be good to clear up that panic/unwrap situation. Otherwise this looks good, apart from CI complaining.

The Changelog check should have actually suggested a code change, but that seems to have broken recently; I will look into that.

let container_provider = rusoto_credential::ContainerProvider::new();
let provider = match rusoto_credential::AutoRefreshingProvider::new(container_provider) {
Ok(provider) => provider,
Err(err) => panic!("Unable to instantiate rusoto_credential::AutoRefreshingProvider: {:?}", err),
Copy link
Member

Choose a reason for hiding this comment

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

As source configs are being fed into symbolicator with each request, we should avoid panic-ing as much as possible. I haven’t looked at the docs to figure out the exact failure conditions for this Result, it might as well be an extreme edge-case condition in which case it might as well be okay to unwrap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the source of rusoto_credential::AutoRefreshingProvider::new, I don't even understand how it can return CredentialsError?

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

As the upstream implementation unconditionally returns an Ok, I’m a bit more inclined to just do an unwrap. Either way, I think the compiler should be smart enough to optimize all that away.

@Swatinem Swatinem merged commit 43258fa into getsentry:master Apr 12, 2022
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.

3 participants