Add support for customising AWS SDK client config

[dhickie]

This is a proposed solution to resolve #3109. Instead of simply adding an optional ServiceURL parameter when configuring the AWS connection, I opted for an optional Action in which the client can customise any of the config that's common to all AWS service clients. This seemed like a more flexible option rather than needing to add more and more parameters piecemeal as more use cases come up.

@iancooper I'm open to ideas on how best to test this - as the AWS clients are constructed and then disposed during function calls, there's no easy way to inspect them to ensure they've got the correct configuration. One option would be to make use of LocalStack to try pointing AWS clients to a different service URL, but that seems like a fairly substantial departure from the current testing style.

[CLAassistant]

All committers have signed the CLA.

[iancooper]

This is a proposed solution to resolve #3109. Instead of simply adding an optional ServiceURL parameter when configuring the AWS connection, I opted for an optional Action in which the client can customise any of the config that's common to all AWS service clients. This seemed like a more flexible option rather than needing to add more and more parameters piecemeal as more use cases come up.

Good idea, we do that for Kafka and it's probably a pattern that we can usefully extend elsewhere to allow interception; it helps to future proof us against properties of brokers that we don't have wrapped yet.

[iancooper]

@iancooper I'm open to ideas on how best to test this - as the AWS clients are constructed and then disposed during function calls, there's no easy way to inspect them to ensure they've got the correct configuration. One option would be to make use of LocalStack to try pointing AWS clients to a different service URL, but that seems like a fairly substantial departure from the current testing style.

If a test depends on infrastructure we tend to put it in its own library. We have a programmer tests approach, so touching infrastructure is fine, but we want fast tests so we tend to try to separate out tests for specific transports (we still have slower core tests due to how we test background processes, which we might want to separate out at some point - but a different issue).

So here it could make sense to have tests that run this against LocalStack. A larger refactoring of how we do this would allow you to configure whether you want to run our AWS tests against cloud infra or LocalStack. That could be useful for folks who like LocalStack, though I would always want CI to be against cloud infra. So I think that for a first step we could just create a separate AWS.LocalStack library and put tests in there that deal with features that explicitly support that model. We would also need to add that into our GA CI script.

We probably need to update the docs as well, but I can talk you through that directly as GitBook's lack of support for multiple versions of the docs has meant we have to solve some problems there with tooling.

Just an opinion, other opinions welcome though

[dhickie]

Another option just occurred to me - since this change is about supporting general customisation of AWS clients and not necessarily about using LocalStack specifically, another option would be to implement an HttpClientFactory for the test which returns an HttpClient with a custom DelegatingHandler that logs something about the request. The HttpClientFactory can then be passed to the AWS client configuration, so if something gets logged by the delegating handler then we know it used the custom config.

That feels a bit more straightforward than making this about LocalStack in particular.

[iancooper]

Another option just occurred to me -

That works. Agreed it is not a LocalStack test, but functionality we need for localstack

[iancooper]

Some suggestions, but none of them are significant. So approved, up to you if you want to change based on that feedback

[iancooper]

I wonder if we should follow the pattern elsewhere of encapsulating this in a CreateSnsClient method. I like symmetry but its just an opinion

[iancooper]

Again should we replicate the pattern of making this a method; it's possible if we do that we may be able to extract a helper class that encapsulates this logic. We can just make that a dependency we new up in the constructor, no need to DI it in, but we save on repeated logic amongst these classes.

[iancooper]

See above comment on extracting this

[dhickie]

@iancooper Yep that makes sense, I've pulled out construction of AWS clients to a factory class instead.

I noticed that the AWS tests and some of the Dynamo tests are failing currently for reasons that don't seem related to this PR - is that to be expected?

[iancooper]

This is great. Thank you.

[iancooper]

@iancooper Yep that makes sense, I've pulled out construction of AWS clients to a factory class instead.

I noticed that the AWS tests and some of the Dynamo tests are failing currently for reasons that don't seem related to this PR - is that to be expected?

Hmmm, let me take a look.

[dhickie]

@iancooper Any luck looking at the failing tests?

[iancooper]

Not had a chance. Will try to look before the weekend

[dhickie]

@iancooper This seems like another occurrence of the transient issue with the command processor tests - is it worth just retrying to check?

[iancooper]

@iancooper This seems like another occurrence of the transient issue with the command processor tests - is it worth just retrying to check?

Yeah, let me see if I can persuade it to build; still not sure why that issue pops up

[dhickie]

@iancooper This is now up to date with master, but we have a return of our old friend Value cannot be null. (Parameter 'factory')

[iancooper]

@dhickie merged