ex_aws
ex_aws copied to clipboard
Force refresh of config auth
Problem
When ExAws streams an operation, it calculates a config map once, then passes that config as "overrides" for each request during the stream. This means that auth creds are resolved once and used for the entire duration of the stream. If you have a stream that takes longer than 6 hours on an EC2 instance, you'll eventually get "token expired" errors.
Fix
This PR detects refreshable creds in a config map, and uses ExAws.Config.AuthCache
to resolve the creds on every request, even if an exist config map is passed as overrides.
This fix is completely transparent to downstream stream builders.
Fixes #824
Testing
I tested locally by printing to stdout, but I have no idea how to unit test it. I'm currently testing in production to see if it's ultimately effective.
How it works
Currently two types of configuration values (:instance_role
and :awscli
) use ExAws.Config.AuthCache
, and thus have refreshable content.
The config map is augmented with a new field :refreshable
which can be false
or a list of refreshable values. For example:
iex> config = ExAws.Config.new(:s3)
%{
...
refreshable: [:awscli],
...
}
Now on subsequent calls, passing that config in as overrides, new/2
knows to remove certain fields from the overrides which needs to be refreshed.
iex> ExAws.Config.new(:s3, config)
%{
...
refreshable: [:awscli],
access_key_id: ..., # Potentially different
secret_access_key: ..., # Potentially different
security_token: ..., # Potentially different
...
}
Because config
has refreshable: [:awscli]
, we know we can delete things like :access_key_id
, :secret_access_key
, and :security_token
from the overrides and let them be recalculated.
You can opt out of this behavior with...
iex> config = ExAws.Config.new(:s3, refreshable: false)
%{
...
refreshable: false,
...
}
But I don't know why you would want to.
Aw dang, I just notice that this had slipped through the cracks - really sorry I haven't gotten to it sooner. Rest assured it is (now) on my list.
This looks good @cjbottaro. Would you be able to add a note to the docs about setting refreshable: false
, just so that people know they can override it if they need to? Then we'll get it merged. Cheers.
Hey guys, are we planning on continuing this pull request? I've got the same issue over here due to Stream of big S3 files that seems to be helped by this fix
Oh shoot. Ha, I lost track of this also. Yeah, I'll try to write up some docs.
@bernardd I added the docs in 7a23314. Let me know if that looks good, it's been a while since I thought about this; I kinda forgot what's going on and how it works a little... 😂 On the up side, we've been using this branch in prod since Sept of last year!
Thanks @cjbottaro! Would you be able to give it a quick mix format
pass, please? Then I'll get it merged.
@bernardd Do you want me to close #924? At least I won't need it, if we're moving forward with this PR.
Getting rid of the premature config resolution might be nice for other reasons, though, if you think it's worth a potentially breaking change.
Thanks - yeah, let's close #924 for now. It's not ideal, but I'd rather avoid a breaking change if we can.