Refresh AWS credentials asynchronously

[danielvdao]

Context

Relates to issue #2641

When an AWS client instance is using EKS pods or requires refreshing the AWS token, they refresh the token on the main execution thread. Credential providers have to acquire a mutex and call their respective #refresh implementation, contributing to spikes in runtime latency during expiration time.

The worst case of this is that on the hour, an application running multiple threads will each try to refresh credentials (making a network request), each waiting to acquire the mutex, thereby blocking one another's execution. This can cause periodic latency spikes (see article here for an example Go implementation).

Proposed solution

This PR kicks off a background thread to check whether or not to refresh a token when the credential is about to expire. By doing this async, refreshing doesn't affect the main thread's runtime and doesn't acquire a mutex on the main thread, thus not getting blocked.

Another option was considered where we have a client-side configuration, but that wasn't the best approach as it allows clients to spawn threads unintentionally.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

1. To make sure we include your contribution in the release notes, please make sure to add description entry for your changes in the "unreleased changes" section of the CHANGELOG.md file (at corresponding gem). For the description entry, please make sure it lives in one line and starts with Feature or Issue in the correct format.

2. For generated code changes, please checkout below instructions first:
   https://github.com/aws/aws-sdk-ruby/blob/version-3/CONTRIBUTING.md

Thank you for your contribution!

[danielvdao]

@alextwoods here's the PR discussing the idea. A few questions... for some reason I had trouble getting the tests to run locally 😢 Even when I removed my code and tried to run some tests via bundle exec rspec <path to spec file> a few of them failed.

• Do you have a good idea on how one can test this? There doesn't seem to be a great way to test Thread.new and I'm not sure if it makes sense for us to. Though happy to brainstorm and take suggestions 🙂
• Can this or will this get backported into other AWS versions? E.g. version-1 and version-2, I believe we are on version-1 and I imagine this would be helpful for other client versions as well! I am happy to open separate PRs myself, but not sure what's the release policy here.

[alextwoods]

You should be able to run all of the tests with bundle exec rake test:spec (Thats the target that the CI runs). What test failures are you running into?

[alextwoods]

As a heads up - I'm working on a few other related changes that will touch some of the Refreshing Credential providers so we'll likely hold off on actually merging this until it can be incorporated with those changes.

[alextwoods]

There are a few concerns I want to see if we can think through:

• I want to minimize the chance of breaking any existing users while providing the benefit of non-blocking credential refresh ahead of expiration where possible. In particular, right now if credentials are actually expired, we are relying on the refresh_if_near_expiration to block until they are refreshed, so that client operations are able to get new, non-expired credentials. I don't want to break this case either. I'm wondering if we need to have two time checks: A nearish to expiration (5 minutes? more?) and a near expiration (1 minute? lower?) and a call in the near expiration still blocks.
• Usage in the CredentialProviderChain needs to remain synchronous. When we go through the provider chain to find credentials we check if each provider has credentials by calling each provider's credentials method.
• Any possible race conditions here that we might be introducing? Are there other potential unintended performance impacts or concerns that other use cases might encounter? What testing do we need to feel confident in this change?

(Just to be clear - I don't expect you to answer all of those questions - I just want us to think through all of them).

[danielvdao]

As an aside (I'll take some time to think of the things you mentioned as well above!) -- thanks for helping with the test run. I was following the doc here for running unit tests. But once I switched over to running bundle exec rake:spec everything worked like a charm. I think the tests are resolved -- I inspected other specs and the best way I could come up with / glean with was putting sleep in the main thread.

There are other ways of doing this like mocking Thread.new (lesser familiar w/ that one) but that doesn't seem used in this codebase.

[danielvdao]

Hey Alex, here are my thoughts (of course y'all have more handle on this than I do 🙂) I'll take some more to think and also please hijack the code if you'd like to as well.

I want to minimize the chance of breaking any existing users while providing the benefit of non-blocking credential refresh ahead of expiration where possible. In particular, right now if credentials are actually expired, we are relying on the refresh_if_near_expiration to block until they are refreshed, so that client operations are able to get new, non-expired credentials. I don't want to break this case either. I'm wondering if we need to have two time checks: A nearish to expiration (5 minutes? more?) and a near expiration (1 minute? lower?) and a call in the near expiration still blocks.

This makes sense 👌🏽 My understanding of this credentials API is that there is latency cost based on availability. Given the worst case if the availability zones are down, I imagine that the asynchronous thread can timeout on its call to refresh depending on region.

Another case: if for any reason an exception raises, this would get handled in the thread but not the main thread. In these cases we should also have a stopgap blocking mechanism, like a near-er expiration? (1 minute? not sure).

For example if t1 executing the async credentials fetch raises an exception for any reason -- the main thread doesn't see that. This is fine since the expectation is async, but I agree that we should have a refresh_synchronously call.

What about keeping the current behavior and then having something like:

def credentials
  refresh_if_near_expiration_async
  refresh_if_near_expiration_sync
  @credentials
end

def refresh_if_near_expiration_async
  Thread.new do
    if within_10_minutes_expiration?
      @mutex.synchronize do
        refresh if within_10_minutes_expiration?
      end
    end
  end
end

def refresh_if_near_expiration_sync
  if within_5_minutes_expiration?
    @mutex.synchronize do
      refresh if within_5_minutes_expiration?
    end
  end
end

If the background thread fails to fetch a credential, we'll keep trying and failing for subsequent calls of credentials, but eventually the client will raise an exception on the main thread since we have a synchronous blocking call.

Usage in the CredentialProviderChain needs to remain synchronous. When we go through the provider chain to find credentials we check if each provider has credentials by calling each provider's credentials method.

I think the top solution will address this, right? We kick off a background thread and if it doesn't finish in time, we'll have the synchronous process block and also make the call to fetch credentials for the initialization.

To outline:

1. Client calls credentials in main thread, t1
2. Kicks off async thread, t2
3. t2 acquires mutex but does not finish
4. t1 checks within_5_minutes_expiration? which returns true
5. t1 is now blocking on t2 to finish its critical section
6. t2 finishes and writes to @credentials
7. t1 checks again and bails because t2 wrote to @credentials

Any possible race conditions here that we might be introducing? Are there other potential unintended performance impacts or concerns that other use cases might encounter? What testing do we need to feel confident in this change?

Not 100% sure -- but I think that since we are duplicating the code, we should be safe since the Mutex will guard us. I see one potential case where two threads are waiting to refresh the token asynchronously and the first to acquire the mutex blocks the other, but I don't see it as a concern.

One spot I see a potential performance impact that I feel is hard to measure, is the initial fetch for credentials. I might have to do more thinking here, but there is a penalty to the main thread blocking just a tiny bit for the async thread to finish. That said, it's not too much since we check if_near_expiration? again within the Mutex to subvert the API call.

What testing do we need to feel confident in this change?

I added some unit tests 🤔 but I'll try to do some more thinking as well.

[alextwoods]

Yeah - I was thinking something along those lines - an async refresh if outside some TBD time (10 minutes seems reasonable) and sync within the current 5 minutes (to minimize impact of the change).

Another edge case we need to handle correctly here - if the async process runs into an error and, for whatever reason, cannot refresh the credentials, it needs to NOT update the credentials (ie, the current ones are not expired, so don't clear them). This behavior may differ per credential provider. As an example of where this would be an issue: The InstanceProfileCredentials when they encounter an error (after retires) during a credential refresh will set the credentials to empty: https://github.com/aws/aws-sdk-ruby/blob/version-3/gems/aws-sdk-core/lib/aws-sdk-core/instance_profile_credentials.rb#L175

[danielvdao]

Sounds good, I'll take a stab doing a 10 min async check, then 5 min sync check. Like I said, feel free to steamroll this whenever you want / need Alex -- don't want to block any initiatives that y'all may have. I'll commit to doing this in my spare time since I have my day job a well, but I'm happy (and learning!) along the ride 😁

It looks like some tests are failing so it's going to take me awhile to debug (gotta set up those old ruby environments!), at first glance looks like some of the requests were attempted rather than mocked 🤔

Another edge case we need to handle correctly here - if the async process runs into an error and, for whatever reason, cannot refresh the credentials, it needs to NOT update the credentials

This one sounds interesting, I'll see if there's something we can do like pass a default flag into the refresh, otherwise we're going to be changing the classes' implementation too much inside versus outside.

---

Couple edits later...

1. Ruby 3+ tests are failing for other main builds so I'll just ignore that.
2. Having trouble replicating the test failures locally even w/ PURE_RUBY / KITCHEN_SINK environment variables set 🤔 I've tried for ruby 2.5.x, jruby, the PURE_RUBY flag enabled, and no success locally -- maybe worth it to think about stub / mocking Thread?

I'll re-visit this tomorrow with some fresher thoughts -- took quite a bit to get my mac set up 🤦🏽‍♂️

[danielvdao]

I'm having a little trouble with replicating some of the tests in my local environment that are failing on CI (ruby3 tests seem to be busting on the main branch right now so ignoring those). Additionally if you had any idea on what's a good way to test the Threads 🙈 -- I would appreciate it as well 🙂 , otherwise I'm happy to keep digging myself! Thanks for all the information so far by the way!

(Surprise surprise I managed to get some of them failing intermittently now 😂)

[alextwoods]

I'll re-visit this tomorrow with some fresher thoughts -- took quite a bit to get my mac set up 🤦🏽‍♂️

Any setup bits we should add to the README to help others?

Yeah - Ruby 3+ tests are currently failing due to rspec-mocks 3.10.3, so fair to ignore those. I can take some time today to dig into the issues. I've got a few thoughts on how to address some new issues I've come up with. I think we may want some mechanism for RefreshingCredential implementations to "opt-in" to background refresh behavior - I think there may be some cases (eg ProcessCredentials or custom, user implementations) where we don't want the async refresh, or we don't know enough about the behavior of the refresh method (ie, the behavior of InstanceProfileCredentials to set credentials to empty when it encounters an error needs to be changed for this to work).

[danielvdao]

Sounds good.

Any setup bits we should add to the README to help others?

I can probably spin up a small PR to update CONTRIBUTING.md. The rake target is wrong, and another thing is that to replicate the test locally -- I need specific versions of ruby (though expected), however I didn't know about the KITCHEN_SINK and PURE_RUBY env vars needed.

As for the tests -- looking closer! Last night I knew those were breaking and didn't get to address it because I needed to think through it more. I wasn't 100% sure why extra calls were being made, but my suspicion is that because we have:

1. A thread
2. A synchronous behavior

the spec is exercising and doing both, however in some cases, the thread might not be finished with making its #refresh call. So yeah, I may have to change some tests to be received_at_least or something of that sort.

I think we may want some mechanism for RefreshingCredential implementations to "opt-in" to background refresh behavior - I think there may be some cases (eg ProcessCredentials or custom, user implementations) where we don't want the async refresh

I'll mull this over a bit and see what can be done 🙂

[alextwoods]

I also think we may want to ensure that the initial credential refresh/fetch is synchronous. Long term/in a new major version of the SDK, I'd love to move to a more promise/lazy evaluation of credentials, but the current SDK makes a lot of assumptions around this - I can take a stab at that change as well - and I think that should also fix a lot of the unit test issues.

[danielvdao]

Alrighty, I tried reverting some of the changes and whatnot. Thanks as usual for having to re-run the workflow. Happy to accept feedback as usual and appreciate the time you've been taking to do this 🙂

[alextwoods]

I pushed a small change - I tweaked the order the refreshing was checked in. It starts by first checking if we are within the sync window and then blocking (the old behavior). Otherwise, if async is supported, its inside the async window and the mutex is unlocked, we kick off the thread.

This should preserve as much of the old behavior as possible and avoid creating threads unless we're actually going to refresh.

Note - this also fixes some of the test changes that were required.

[danielvdao]

Awesome - that makes sense 🙇🏽‍♂️ , thanks so much for helping with that last change. And appreciate the newfound knowledge of and_yield 😍 . I'm guessing this won't be merged for awhile?

[alextwoods]

Yeah - I think we're pretty close on the change, but I'm also working on some other, semi-related changes that touch refreshing credentials and want to wait to merge this until that work is complete.

[danielvdao]

@alextwoods @mullermp 🏓 -- is this PR good to merge? I don't have anymore updates, but I do see other credential PRs so just wanted to bump it again!

[alextwoods]

@danielvdao - I think this PR is in a good place, but there are still some other refreshing credential changes in the pipeline (no PR's available for them yet) that should be completed in the next couple of weeks that I want to get in before we merge this on top of them.

[danielvdao]

Hey @alextwoods -- no rush, but want to check back in on this PR! We're running a fork of the gem with my branch on it and there hasn't been any problems for the past two weeks now - so wondering if it's good to merge? If so, I / or maybe y'all can rebase + address any conflicts (though nothing seems too jarring).

I haven't dived deeply into tail-end performance improvements, but my hypothesis is that there must be a few.

[alextwoods]

Hey - sorry for the delay on this - the other credential related project I was working on has been delayed, so I'll look at addressing merge conflicts, doing a final review/test and merge this week.

[danielvdao]

Perfect, thanks so much Alex. Looking forward to getting this through the line!

[alextwoods]

Edit: other credentials work was unblocked and we were able to get the required PR out, see #2673. This changes a few things about the way IMDS credentials are refreshed and managed - I'll spend some time this week updating this PR for that change.

[danielvdao]

Woohoo! Thanks for the update Alex.

[alextwoods]

I've merged the other credential provider changes - I think this is about ready to go, I'm just doing a final review.

[danielvdao]

Hey Alex, was curious on the status of this again. I know you mentioned you were reviewing though, so I'm curious if you caught anything during the review!

[alextwoods]

Final round of testing looks good - I'll merge and release this! Great work on this, its a great improvement.