Aws appsync websocket transport

[chadfurman]

AWS AppSyncWebsocketTransport inspired by (and largely copied from) the work of @joseph-wortmann

For original discussion thread, go here: #125

At the time of creating this draft PR, these are the next steps:

1. Revisit the _send_query overload in AppSyncWebsocketTransport -- note that I'm not sure about "data" being passed to "get_headers" as I need to revisit what, exactly, is supposed to be getting signed and how, exactly, the non-IAM authorizations are including the request data.
2. ensure nothing critical is missing from the _send_query overload. It's substantially smaller than the method in WebsocketTransport so I'm 99.99999% certain I'm missing a substantial amount of logic there. I simply ran out of time to verify
3. Try to instantiate a connection. This may require revisiting the AWSRequest object. Specifically, there's a "AWSRequestPreparer" but honestly "auth.add_auth()" is likely sufficient I just need to make sure that I build the URL correctly, I think?
4. figure out what was lost by stripping out the new connect() and subscribe() methods and how to replace that. Do we need to add the hook in WebsocketTransport, as was suggested? Or can we take the alternative path and build the URL up-front and let everything else automatically happen?
5. Definitely need some code cleanup. I started with some tests, but the coverage is abysmal and I'm not even trying to pass black/flake8 etc.

Follow the discussion here below for further updates.

[chadfurman]

picking this back up today. Going to start with:

1. Read through the WebsocketTransport base class and list out key elements we need to replicate in _send_query. Brainstorm if it's possible to re-use the base-class's _send_query somehow
2. Interface with / overload connect() and subscribe() as necessary. See AWS Appsync support? #125 (comment)
3. Start code cleanup. Work on black and flake8 and, if time allows, increase test coverage.

[chadfurman]

A couple questions, as I read through this:

1. Why does our "_send_query" have to wait for "ack" but the original websocket implementation does not?
2. Why does our "_send_query" need to have its own receive_data_loop()? Wouldn't the one from the base class's connect() be sufficient?

Note w.r.t. the above two questions, I've removed the wait_for_ack and the new receive_data_loop from the overloaded _send_query(). I can easily add them back, but I wanted to be as compatible with the base _send_query as possible and did not want to leak extra event loops nor duplicate wait events.

Note also that, because we've overloaded the _send_query() method as suggested and because we're manipulating the URL once during init, we've left both the subscribe() and connect() methods out of this implementation.

Next steps:

1. expand test coverage to the auth types as well as the _send_query method
2. clean up flake8 and black errors
3. add better docblocks
4. add instructions on expected usage
5. test connecting to AWS and subscribing to an appsync endpoint. Trigger a mutation and ensure that we receive the modified data. Follow the instructions for connecting and, if any errors occur, update the documentation and code as necessary.
   6 explore non-realtime queries and mutations to appsync -- these likely do not go over wss and so we may need a controller class which uses AIOHttpRequest under the hood with an option to overload this.

Will be pushing code shortly.

[leszekhanusz]

1. Why does our "_send_query" have to wait for "ack" but the original websocket implementation does not?

Because with the aws protocol, an ack is sent by the backend but it is not the case with the apollo protocol

2. Why does our "_send_query" need to have its own receive_data_loop()?  Wouldn't the one from the base class's `connect()` be sufficient?

Indeed, only the one in the connect is necessary

[chadfurman]

Very little time this weekend. Going to start on expanding the test coverage and trying to clean up the code checks.

[chadfurman]

Started working on getting existing appsync tests to pass. Found out that the add_auth call is actually failing due to missing credentials -- I expected the Signer would find them itself. I'll have to grab the credentials somehow and bind them.

Gotta stop here for the night, unfortunately. Didn't get very far. Will keep coming back to thins until I get it, though.

[chadfurman]

Looking into how to get AWS credentials object into add_auth

[chadfurman]

Figured out it was because my profile was wrong. Need to add a test for NoCredentialsError() where I inform the user that they need to specify a profile or set their default profile in their ~/.aws/credentials file (not sure how this works in windows, but I assume it's similar)

[chadfurman]

New error is about the fake request:

self = <botocore.auth.SigV4Auth object at 0x7fa69bba8220>
request = <botocore.awsrequest.AWSRequest object at 0x7fa69ab03b80>

    def credential_scope(self, request):
        scope = []
        scope.append(request.context['timestamp'][0:8])
        scope.append(self._region_name)
        scope.append(self._service_name)
        scope.append('aws4_request')
>       return '/'.join(scope)
E       TypeError: sequence item 1: expected str instance, NoneType found

I'm guessing I need to set a timestamp on the request or find a better way of creating it that will set my timestamp for me. I had really hoped to avoid having to muck with dates.

[chadfurman]

Adding the exception handling for the missing credentials and working on getting the last test to pass before adding more.

[chadfurman]

Added the exception handling for the missing credentials. Also added exception handling for missing region even if credentials are provided. Got all tests to pass as-is. Next will be adding the e2e tests that talk to AWS using my actual credentials, and then also tying it into the existing websocket server tests as well. Then I'll do a smoke test and clean up the docblocks and add documentation.

[codecov-commenter]

Codecov Report

Merging #239 (4439121) into master (09e4b30) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            master      #239    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           23        26     +3     
  Lines         1822      1997   +175     
==========================================
+ Hits          1822      1997   +175     

Impacted Files	Coverage Δ
gql/client.py	100.00% <100.00%> (ø)
gql/transport/aiohttp.py	100.00% <100.00%> (ø)
gql/transport/appsync_auth.py	100.00% <100.00%> (ø)
gql/transport/appsync_websockets.py	100.00% <100.00%> (ø)
gql/transport/websockets.py	100.00% <100.00%> (ø)
gql/transport/websockets_base.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09e4b30...4439121. Read the comment docs.

[leszekhanusz]

For the linting issues, you can run make check to fix most problems (See Contributing.md file)

For the failing tests, you should add an aws element in the all_transport_dependencies in conftest.py and mark your tests with it.

[chadfurman]

Starting.

Goal:

• Add "aws element" as suggested for failing tests
• Run "make check" for linting issues
• review existing websocket tests
• devise how I may tie into the existing websocket tests
• review existing e2e tests
• devise how the aws transport e2e test should be structured
• Commit the beginning of my e2e test

[chadfurman]

I appear to have had some sort of system failure.

[chadfurman]

Reformatted and installed latest fedora, back up and running. Not sure I'll be able to get much done tonight, unfortunately, but I should be able to add the "aws" line and fix the linting once I get pycharm and git set back up.

[chadfurman]

Going to have to stop here. Let's see what happens with CI. I'll pick away a bit more throughout the week now that I'm back up and running. No idea what happened, but my filesystem randomly became "readonly" in the middle of playing with PyCharm and tweaking some battery-related stuff.

[chadfurman]

Not sure why the tests still failed. Will look into that later. Looking into the existing websocket test and going to try to get it to validate my aws extension as well. Not sure if I'll need to copy/paste lots of it (I'd prefer not to). Need to better understand that websocket server thing you got in conftest.py

[leszekhanusz]

Note: I merged the two JWT classes into one as from our point of view it is the same thing.

A̶l̶s̶o̶ ̶I̶ ̶h̶a̶v̶e̶ ̶s̶o̶m̶e̶t̶h̶i̶n̶g̶ ̶s̶t̶r̶a̶n̶g̶e̶,̶ ̶t̶h̶e̶ ̶t̶e̶s̶t̶ ̶̶t̶e̶s̶t̶s̶/̶t̶e̶s̶t̶_̶a̶p̶p̶s̶y̶n̶c̶_̶a̶u̶t̶h̶.̶p̶y̶:̶:̶t̶e̶s̶t̶_̶a̶p̶p̶s̶y̶n̶c̶_̶i̶n̶i̶t̶_̶w̶i̶t̶h̶_̶i̶a̶m̶_̶a̶u̶t̶h̶_̶a̶n̶d̶_̶n̶o̶_̶r̶e̶g̶i̶o̶n̶̶ ̶i̶s̶ ̶s̶o̶m̶e̶h̶o̶w̶ ̶f̶a̶i̶l̶i̶n̶g̶ ̶o̶n̶ ̶m̶y̶ ̶m̶a̶c̶h̶i̶n̶e̶ ̶b̶e̶c̶a̶u̶s̶e̶ ̶i̶t̶ ̶s̶e̶e̶m̶s̶ ̶b̶o̶t̶o̶c̶o̶r̶e̶ ̶f̶i̶n̶d̶s̶ ̶m̶y̶ ̶r̶e̶g̶i̶o̶n̶,̶ ̶e̶v̶e̶n̶ ̶i̶f̶ ̶I̶ ̶r̶e̶n̶a̶m̶e̶ ̶m̶y̶ ̶̶.̶a̶w̶s̶/̶c̶o̶n̶f̶i̶g̶̶ ̶f̶i̶l̶e̶.̶
It was because I still had an AWS_DEFAULT_REGION environment variable.

[leszekhanusz]

For Reference my Permission denied error with IAM was fixed by adding the AWSAppSyncInvokeFullAccess policy to my IAM user.

Next, I add the following error:
{"errorType":"Unauthorized","message":"Not Authorized to access onCreateMessage on type Message"}

This one was solved by creating the amplify/backend/api/<your-api-name>/custom-roles.json file as described here

Now I can execute queries, mutations and subscriptions with IAM.

[leszekhanusz]

Theappsync.pyfile was split in 2 so that we could use the AppSync auth classes either with the websockets transport or with the aiohttp transport without needing the other dependency.

[leszekhanusz]

@joseph-wortmann @chadfurman could you please check that the PR is ok for you as it is now? Do you have remarks?
I did not test the JWT auth. @joseph-wortmann could you please test it as it seems you have some jwts ?
I would like to merge this Friday.

[chadfurman]

I've been following your commits and have no reason to object to anything here. If there's functionality missing / needed / broken it will show during usage more than my review and so will lend itself to iteration. I think this is fine as is. Great work and thank you for taking it over.