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

Support long-running streams with task role authentication #924

Closed aglundahl closed 1 year ago

aglundahl commented 1 year ago

Context

ExAws.stream! calls ExAws.Config.new with the provided config_overrides, before calling the relevant ExAws.Operation implementation. This resolved config is subsequently provided as config_overrides to all calls the ExAws.Operation implementation will make to ExAws.request, leaving no possibility for task role authentication when the config is resolved again in those calls.

This means that credentials will only be fetched once for a stream. And since credentials are only valid for up to six hours, long-running streams are likely or guaranteed to fail. This has also been reported in #824.

Implementation

This removes the call to ExAws.Config.new in ExAws.stream!, letting the ExAws.Operation implementation take the actual overrides and resolve the config later. We have tested this successfully with the ExAws.S3.Download operation, but I'm not sure about other operations. Are there implementations of ExAws.Operation.stream! that expects a resolved config?

If there are, we could also resolve everything but the credentials in ExAws.stream!. Or resolve the config for all operations but ExAws.S3.Download and other operations we know are safe. But that's of course not as nice.

904 also fixes this problem. But if possible, I think it would be better to fix the problem without configuration updates, since this is the expected behavior (I think).

bernardd commented 1 year ago

Hey @aglundahl - thanks for the contribution.

I suspect you're probably right that in all cases the service-specific implementations of stream!/2 would happily take a set of config_overrides rather than a fully-resolved config struct. Indeed, just checking the ones I have to hand, all of them only pass the config through to a further call to request (like S3.Download does) which re-resolves it. My concern is that I'm not sure about it, and I don't want to merge this only to find it breaks some other service implementation, since what we'd be doing here is really changing a cross-library function contract. There's even a non-zero (albeit I'd guess pretty low) chance that someone has their own internal ExAws service implementation that relies on that behaviour, so just going through all the services in the ex-aws project may not in itself be sufficient.

That all said, it's nothing that couldn't be resolved with a minor version bump and a well documented changelog explaining the incompatibility. And I can totally see why it's a sensible thing to do.

Let me think about it for a bit - if we were to change it, I'd like to couple it with adding some typespecs to the function signatures to make it completely clear what's expected (and so that dialyzer has a chance of picking up errors where they do exist).

aglundahl commented 1 year ago

Thanks for the update! I agree; it's a breaking change, so let's tread carefully. Let me know what you decide. I'd be happy to add typespecs and proper documentation to this.

bernardd commented 1 year ago

Closing for now (see #904).