Make AWSConfig its own type

[ararslan]

Rather than defining AWSConfig as a SymbolDict with no restrictions on the contents, we can make it its own type, which gives more control over how things are stored and accessed.

Fixes #53

[ararslan]

bors try

[bors]

try

Build failed

• continuous-integration/travis-ci/push

[ararslan]

Oh no

[omus]

Sorry, my fault I'm moving credentials around: #62

[ararslan]

Nah definitely my fault:

MethodError: no method matching aws_user_arn(::Dict{Symbol,Any})
  Closest candidates are:
    aws_user_arn(!Matched::AWSConfig) at /home/travis/build/JuliaCloud/AWSCore.jl/src/AWSCredentials.jl:123

[iamed2]

Aren't there APIs that don't require a region? I believe boto does not require you to set a region.

We could have it as Union{String, Nothing} and have a region accessor that errors if it's nothing and a hasregion checker. Or not, if @omus thinks it's not worth it.

[ararslan]

bors try

[bors]

try

Build succeeded

• continuous-integration/travis-ci/push

[ararslan]

Are there any APIs where you shouldn't have a region? Unless I'm misunderstanding the code here, I think in most cases currently on master, you'll end up with a region set.

[omus]

Aren't there APIs that don't require a region? I believe boto does not require you to set a region.

I was under the impression that the region was used as the endpoint to send API commands

[iamed2]

There are APIs which don't require the region to determine the endpoint, but since I can't find an example of one where you explicitly don't have to provide the region in any form, I guess we can just always assume a region must be set somewhere.

[ararslan]

Also note that since we allow any ol' String here, region can also be the empty string. So that could be a decent enough proxy to being unset.

[s2maki]

There are other reasons for the type to be open-ended. In certain cases, other AWS* packages allow extra keys in the AWSConfig dictionary to pass along overrides. For example, AWSS3.s3_get will accept :ordered_json_dict, which changes the type of its result when the object being retrieved is JSON.

[s2maki]

IAM is a case in which the service itself is regionless. I'm not sure if there's anything bad that happens if you do have a region set, so an empty string is probably fine for situations like this.

[s2maki]

Please be careful with incompatible changes like these. There is a whole suite of other AWS* modules that may break because of this, not to mention production code.

In particular, take a look at AWSS3.jl. It uses haskey on the AWSConfig struct. It also splats it, which behaves very differently for a dictionary than a struct. Both of those uses look to me like they will break without additional work.

Before merging this, please at least verify that it doesn't break any of the other AWS* packages that depend on this one.

[ararslan]

In certain cases, other AWS* packages allow extra keys in the AWSConfig dictionary to pass along overrides.

That is really weird and should be changed. How it's currently defined, AWSConfig refers to stuff you'd put in ~/.aws/config, which includes specific, documented things.

[s2maki]

There may well be a better way to do it, but the current way, which was introduced here a0ff2b7, is still quite in use in production systems and across various AWS* modules. AWSLambda relies on it as well, and possibly others; I haven't done an exhausive search.

I'm ok with changing it, but please let's do so in a way that's not suddenly going to break stuff, particularly other modules available to the public. :)

[ararslan]

Breaking things isn't my intention of course, so I appreciate you bringing up the other uses. I some ideas for fine-tuning the deprecations that should make it easier for users to adapt.

[s2maki]

Implementing the better approach in AWSS3 and elsewhere should probably be a first step, to eliminate reliance on AWSConfig being a dictionary. s3_get()'s use is pretty shallow; it is simply calling s3() which immediately does the unpacking. An optional arg there ought to do the trick.

I'm concerned though, because I remember there being some deep use of the AWSConfig dictionary data that passes service-specific data across perhaps a dozen functions. I'll have to dig into my code to see if I can find it. In the meantime, @samoconnor tells me he'll be back in a few days. We should definitely get his input on this.

Ultimately to make this change, something will have to break. I think it's a very good idea to tighten up AWSConfig. And I happen to be in a good place for a breaking change to the API as I move from 0.6 to 0.7 to 1.x, which I'm doing over the next couple of weeks. If AWSS3 and the other packages are tweaked first so they continue to work once this PR is merged, and any user code throws a clear error about what to do instead, then I'm pretty cool with whatever the final solution is.

[ararslan]

Ultimately to make this change, something will have to break.

Not necessarily: One of the deprecations I'm working on is implementing the full AbstractDict interface for the AWSConfig type with a "hidden" field in AWSConfig to store extra information. When users interact with AWSConfig as though it was a Dict, they'll receive an informative deprecation. If all goes well, that should ensure that nothing has a hard break, and users of the functions will have the information they need via deprecation warning messages in order to adapt.

[s2maki]

Great! That works for me!

[ararslan]

bors try

[bors]

try

Build succeeded

• continuous-integration/travis-ci/push

[ararslan]

I tried running the AWSS3 tests with this branch of AWSCore and things were going okay, deprecation warnings were popping up as expected, but I got a 403 in the PUT tests, so I'm guessing the tests there require Sam's credentials.

[omus]

so I'm guessing the tests there require Sam's credentials

We should be able to do the same bors trick we did here.

[ararslan]

True, though it might be a little tricker; AWSCore appears only to have needed valid AWS credentials for the existing tests, whereas AWSS3 appears to require privileges of some kind.

[samoconnor]

I tried running the AWSS3 tests with this branch of AWSCore and things were going okay, deprecation warnings were popping up as expected, but I got a 403 in the PUT tests, so I'm guessing the tests there require Sam's credentials.

@ararslan, to run the S3 tests with your own AWS creds you will probably have to:

• Set your local region here: https://github.com/JuliaCloud/AWSS3.jl/blob/master/test/runtests.jl#L36
• Change the bucket prefix "ocaws.jl.test" to something else here: https://github.com/JuliaCloud/AWSS3.jl/blob/master/test/runtests.jl#L48 and here: https://github.com/JuliaCloud/AWSS3.jl/blob/master/test/runtests.jl#L66

Also, you should run the tests both on your local dev box and on an EC2 box to check that automatic EC2 credentials discovery is working.

[samoconnor]

There are APIs which don't require the region to determine the endpoint, but since I can't find an example of one where you explicitly don't have to provide the region in any form, I guess we can just always assume a region must be set somewhere.

I think there are two things that should be modelled here, default_region, and region:

• The local configuration has a default_region (this could come from a config file, or from AWS_DEFAULT_REGION, or be implied by the region a EC2 or Lambda host is in).
• For most services (not IAM), the request endpoint has a region. This is usually the same as the default_region. However, S3 uses a region-less global bucket namespace. S3 will respond with 301 Moved Permanently or with AuthorizationHeaderMalformed and a Region tag specifying the correct endpoint. So, for efficiency, the per-session AWS state needs to cache a bucket-name to region map. See https://github.com/JuliaCloud/AWSS3.jl/blob/master/src/AWSS3.jl#L110-L139. (It has been a while since I was in the detail of this, but I recall that the behaviour is different in the time immediately after creation of a new bucket).
• Sometimes you want to send a request to a specific region that is different from the local configuration, so the top level request function needs to support a region= option.