Cache AWS CredentialProvider

[L3tum]

Hi,

I've used this library a tad bit on AWS and noticed that it requests the credential provider every time it makes a request.

        if ($connection->hasParam('aws_secret_access_key')) {
            return CredentialProvider::fromCredentials(new Credentials(
                $connection->getParam('aws_access_key_id'),
                $connection->getParam('aws_secret_access_key'),
                $connection->hasParam('aws_session_token')
                    ? $connection->getParam('aws_session_token')
                    : null
            ));
        }

        return CredentialProvider::defaultProvider();

One way to improve this would be by caching the credentialprovider, like this

$doctrineCacheAdapter = new DoctrineCacheAdapter(new ApcuCache());
$cachedCredentialsProvider = CredentialProvider::cache(CredentialProvider::defaultProvider(), $doctrineCacheAdapter);

Would you be willing to do something like this?

As background, we ran into some bottlenecks while load testing on AWS and noticed that the connection often times out and the only difference between our local servers and AWS is the request signing portion.

[ruflin]

Would this introduce any new dependency?

[L3tum]

I think it can work without extra dependencies and cache providers by using the aws-sdk memoize function here(scroll down to the end of the page). Could even be done with an extra Transport like CachedAwsAuthV4 to make it opt-in.

Optionally there could be another Transport that accepts some kind of PSR/Cache or so object to cache the provider with, but that'd be some neat extra that isn't really necessary IMO.

[ruflin]

Overall the plan SGTM. One thing I'm generally concerned about all the AWS magic: We can't test it the way we test all the other things. So if we change things, I don't know if it will work :-) I only know it works know, because users are using it :-D

[L3tum]

Yeah agreed, testing AWS stuff in general can be a PITA.

I would propose a separate Transport like CachedAwsAuthV4 so it'd be opt-in and mark it as experimental.

We sadly don't have a testing environment for this right now but I'd be willing to test it as soon as we got it set up.

[ruflin]

Just throwing an idea out there: Would it be somehow possible that the AWS code could exist outside the Elastica repo so we could mark the full repo as experimental? And then with composer or similar the dependency could be pulled in? It's always tricky to have experimental code as nobody will read the code part that it is experimental, even if it is opt-in I guess.

[L3tum]

I'm not too knowledgable on the way php checks classes, but at least in AbstractTransport a path is implied, though maybe that's just the namespace and would work if we'd use the same.

Aside from that it's a good idea and I'd be +1 on it

[ruflin]

Now it would be nice if we would find someone to tackle this. Based on git blame it seems big parts of the AWS code was done by @jeskew about 4 years ago. Perhaps he can chime in.

[L3tum]

If he's not available or doesn't want to I caaaan take a look at it since it's theoretically something my company is interested in.

Putting it into a separate library would mean a separate repo as well, right?

[ruflin]

Would be great if you could take a stab at it. Yes, it would mean separate repo.

[L3tum]

Hi, sorry it took a bit longer. I've set up a basic repo here.

I'm not sure if it's already working like that since I don't have any possibility to check right now, though I'm most likely able to check in a week or two. Until then I'd like to add a generic AWSAuthV4 class accepting any CacheProvider as well.

It's not on packagist yet, as I'm not sure how you want to handle this. I'd be happy to transfer ownership of the repo over to you, if you want.

[jeskew]

Sorry to chime in so late, but I think it would be easier to solve this by adding support for an aws_credential_provider parameter on the connection. I can try to put up a pull request later tonight

[L3tum]

Hi, thanks for the idea/PR. My primary use case with this is using it as part of the FOSElasticaBundle in a Symfony application, so I'm not too sure if they'd ever support this, and it's not just a plug-and-play in that case (e.g. with a separate transport, you just use that name in the configuration and everything still works even without an updated FOSElasticaBundle).

[jeskew]

Wouldn't you then need to hardcode any caching configuration into the class name? The default AwsAuthV4 transport is already performing in-memory caching (via a call to memoize); beyond that you'd need to make transport classes with names like Elastica\Transport\CachingViaLocalhostRedisAwsAuthV4.

[L3tum]

Oh cool, I didn't know it already called memoize.

Then essentially that's my idea, to make a base class that accepts any cache provider and then specialized classes for some providers that might come up to then use the cache function instead of "only" in-memory cache. I guess it's better to have that in a separate library after all then

[jeskew]

Credential providers are meant to be composable, so with the proposed configuration change the config could look like:

$config = [
    'persistent' => true,
    'ssl' => true,
    'aws_credential_provider' => CredentialProvider::cache(CredentialProvider::defaultProvider(), $cache),
];

Honestly, I don't remember the rationale for not supporting a credential provider configuration parameter in the first place. I wrote a plugin for the official Elasticsearch PHP library at roughly the same time and opted to only support credential customization via provider function injection

[L3tum]

That's really cool, but I don't think that this functionality will be integrated (any time soon) into FOSElasticaBundle, particularly because it's not really usable with yaml configuration files. I'm +1 for your changes, obviously, since I use Elastica itself for some other projects. But my company likes the "first class support" that the bundle has with Symfony, so I need something that I can use in conjunction with that.

[L3tum]

@jeskew Ah sorry, I think I've derped out there. It's very much possible with what you did and I think it's perfectly inline with what they've done until now. So I'll take it up with the guys from there to implement it in some way.

Thanks again, took quite some work off my shoulders^^