disruptek / atoz

Amazon Web Services (AWS) APIs in Nim
MIT License
48 stars 5 forks source link

Problems with the DynamoDB API #1

Closed epiphone closed 4 years ago

epiphone commented 4 years ago

Hi and thanks for a great library!

I tried the SNS/SQS examples from the README file and they work fine. However, when interacting with the DynamoDB API I ran into some issues. First, I tried the following code:

import asyncdispatch, httpclient, httpcore, json
import atoz/dynamodb_20120810

let response = waitfor describeTable.call(%* {"TableName": "my-table"}).issueRequest()
echo waitfor response.body

This results in the following error: Error: unhandled exception: /home/aleksi/.nimble/pkgs/atoz-2602.0.0/atoz/dynamodb_20120810.nim(1904, 10) header != nil' header argument is necessary due to required 'X-Amz-Target' field [AssertionError]. So I went ahead and added the header:

let
  headers: JsonNode = %* {"X-Amz-Target": "DynamoDB_20120810.DescribeTable"}
  response = waitfor describeTable.call(nil, nil, headers, nil, %* {"TableName": "my-table"}).issueRequest()
echo waitfor response.body

Now the request goes through but results in a HTTP 400 response: {"__type":"com.amazon.coral.service#SerializationException"}.

Would you have any idea on what's going on here? Do we need to manually include headers or something?

I'm running atoz version 2602.0.0.

disruptek commented 4 years ago

I guess I don't have Nim code that runs against DynamoDB yet, so that's why I didn't run into this.

I should fix atoz to add the header automatically.

As far as the serialization error, I think I've seen that before in other implementations I've done (Python). I don't remember what it means, though. Let me do some digging and I'll update the issue.

Thanks for checking it out!

disruptek commented 4 years ago

So there are a few issues:

  1. You can pass header = newJObject() and the X-Amz-Target will get added automatically. This should be fixed so it's fully automatic if we can generate all the headers from default values such as this. I guess this is a buglet in OpenAPI
  2. The body needs to get stringified in the atozHook. This is an atoz bug fixed in 2604
  3. We may need to normalizeUrl without discarding the anchor. This would be a sigv4 bug

Basically, even when I'm transmitting the body and the headers and (even) the anchor, I'm still getting a 404. Can you see if the above tweaks solve your problem? If so, I suppose I've just got my access controls misconfigured.

epiphone commented 4 years ago

v2604 gave me the same result as in your case: tweaks 1 and 2 seem to work great but AWS responds with a 404.

What worked was adding the Content-Type: application/x-amz-json-1.0 header. However, adding it in the function call itself didn't work:

let headers: JsonNode = %* {"Content-Type": "application/x-amz-json-1.0"}
  response = waitfor describeTable.call(nil, nil, headers, nil, %* {"TableName": "my-table"}).issueRequest()

Instead I had to edit atoz source by adding section.add "Content-Type", %"application/x-amz-json-1.0" to line 1946.

I guess user-submitted headers are overwritten somewhere?

disruptek commented 4 years ago

Hmm, it's probably a signing issue. I will look at sigv4 after a quick look at atoz. But I don't know why I wouldn't have encountered this sooner in my own apps...

disruptek commented 4 years ago

So, it's a bit of a hack, but this what I ended up with:

We have the information about what content types can be submitted to the server for any given call in the swagger definition, so openapi needs to grow that functionality itself. Then this will be automated for atoz (and thousands of other APIs).

The little rest hack should probably also default the Content-Type when it is instantiated with JSON input, but that won't necessarily solve your problem since x-amz-json-1.0 may be somehow significant.

Anyway, since I had to rebuild atoz, I also added means to bake the AWS_* environment into the app via --define or edit: by reading the environment at compile-time.

This is all in v2605, just released. Please let me know how it looks on your side, and thanks for debugging.

epiphone commented 4 years ago

v2605 works brilliantly, thanks!

let
  body: JsonNode = %* {"TableName": "my-table"}
  header: JsonNode = newJObject()
  request = describeTable.call(nil, nil, header, nil, body)
  response = waitfor request.issueRequest()

One suggestion related to the API: do you think it would make sense to add default nil values to call function's parameters? That'd make it possible to call describeTable.call(header=header, body=body) instead of describeTable.call(nil, nil, header, nil, body).

disruptek commented 4 years ago

It seems like an obvious improvement, right? I did it this way for some reason or another, and I never expected to have to use the JSON versions of these calls; I thought you'd just want to describeTable.call(TableName="my-table"). Seems like we can implement it and see what breaks.

Maybe what we really want is to accrete these with a series of addHeader or query["key"] = value statements?

epiphone commented 4 years ago

I think you're right, after updating openapi to automatically pass header = newJObject() the describeTable.call(TableName="my-table") API should cover most use cases. Hardly any reasons to start messing with headers yourself if Content-Type, X-Amz-Target (and maybe X-Amz-Security-Token #3 :wink:) are covered by atoz.