ex-aws / ex_aws

A flexible, easy to use set of clients AWS APIs for Elixir
https://hex.pm/packages/ex_aws
MIT License
1.26k stars 521 forks source link

Force refresh of config auth #904

Closed cjbottaro closed 1 year ago

cjbottaro commented 1 year ago

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.

bernardd commented 1 year ago

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.

bernardd commented 1 year ago

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.

sallaumen commented 1 year ago

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

cjbottaro commented 1 year ago

Oh shoot. Ha, I lost track of this also. Yeah, I'll try to write up some docs.

cjbottaro commented 1 year ago

@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!

bernardd commented 1 year ago

Thanks @cjbottaro! Would you be able to give it a quick mix format pass, please? Then I'll get it merged.

aglundahl commented 1 year ago

@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.

bernardd commented 1 year ago

Thanks - yeah, let's close #924 for now. It's not ideal, but I'd rather avoid a breaking change if we can.