Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Websocket #711

Merged
merged 87 commits into from Jul 4, 2019
Merged

Websocket #711

merged 87 commits into from Jul 4, 2019

Conversation

dnalborczyk
Copy link
Collaborator

@dnalborczyk dnalborczyk commented Jun 18, 2019

includes #674 and some touchups.

To do:

* Fix bug of test stop running when sending a an update to a non existing connection.
* Fix but when querry string support was not equally handled as in AWS.
POST to /@connections/{connectionId} to send message to client at connectionId.
Here's the new way:
  const endpoint=event.requestContext.domainName+'/'+event.requestContext.stage;
  const apiVersion='2018-11-29';
  let API=null;
  if (!process.env.IS_OFFLINE) {
    API = require('aws-sdk');
    require('aws-sdk/clients/apigatewaymanagementapi');
  } else {
    API = require('serverless-offline').AWS;
  }
  return new API.ApiGatewayManagementApi({ apiVersion, endpoint });
1. Action of routes.
2. Listening on WS message sending via REST API.
Doesn't seem to be needed anymore as a workaround in aws-sdk.
Use plain vanilla AWS SDK:
let endpoint=event.apiGatewayUrl;
if (!endpoint) endpoint = event.requestContext.domainName+'/'+event.requestContext.stage;
const apiVersion='2018-11-29';
return new AWS.ApiGatewayManagementApi({ apiVersion, endpoint });
In the form of $request.body.x.y.z
@dnalborczyk
Copy link
Collaborator Author

Anyone knows why proxy for lambda invoke is added by default for each WebSocket action?

Serverless: Action '$connect'
Serverless: POST /{apiVersion}/functions/manual-test-websocket-authorizer-dev-connect/invocations

@computerpunc what do you mean by this? is this new, or was added/changed after your initial PR?

dnalborczyk and others added 2 commits July 2, 2019 13:09
close connection on error in $connect action handler
@dnalborczyk
Copy link
Collaborator Author

@dherault @computerpunc

I was just looking over the this.clients {Map} and thought that we should use a WeakMap instead. Then I thought we could just hang the connectionId (and time) onto the websocket object itself to make it even simpler, removing the [weak]map altogether.

Thinking about it further I'm leaning towards to remove hapi-plugin-websocket completely and go directly with https://github.com/websockets/ws. I think currently there's no advantage over the wrapper plugin -- and as far as I can see only adds complexity (and dependencies). hapi-plugin-websocket itself is wrapping https://github.com/websockets/ws. I believe the changes would be fairly simple.

@computerpunc Do you see any advantages over using https://github.com/websockets/ws directly?

That being said, I don't wanna hold up the PR, we can still change it later down the road.

@dherault I'd say let's pull everything into master and release the kraken! I'd even go v6.0 - not necessarily as a breaking change -- although at least it signifies that something potentially could break --, but a major plugin addition!

@dnalborczyk
Copy link
Collaborator Author

@computerpunc one more thing from above, it might have gotten buried:

I couldn't find any information on apiGatewayUrl. is that something you added to the event -- or is aws providing that as well? if not, we should probably remove it.

@computerpunc
Copy link
Contributor

Anyone knows why proxy for lambda invoke is added by default for each WebSocket action?

Serverless: Action '$connect'
Serverless: POST /{apiVersion}/functions/manual-test-websocket-authorizer-dev-connect/invocations

@computerpunc what do you mean by this? is this new, or was added/changed after your initial PR?

I didn’t add this (at least not intentionally) to the WS flow. I think it got in when splitting index into WS and HTTP.

@computerpunc
Copy link
Contributor

I was just looking over the this.clients {Map} and thought that we should use a WeakMap instead. Then I thought we could just hang the connectionId (and time) onto the websocket object itself to make it even simpler, removing the [weak]map altogether.

In my advanced branch with authorizer support, I already use a regular JS object for mapping with the key bring the connection ID and taken from the WebSocket client handshake key. In any case, we still need a map to go from connection ID to ws when we want to send messages back to clients.

Thinking about it further I'm leaning towards to remove hapi-plugin-websocket completely and go directly with https://github.com/websockets/ws. I think currently there's no advantage over the wrapper plugin -- and as far as I can see only adds complexity (and dependencies). hapi-plugin-websocket itself is wrapping https://github.com/websockets/ws. I believe the changes would be fairly simple.

@computerpunc Do you see any advantages over using https://github.com/websockets/ws directly?

In that branch, I already use a modified version of the plugin to overcome its limitations.
It works really nicely. Let’s look at his issue again after I PR authorizer support.
If the plugin is still an overkill, I wouldn’t write code to use ws from scratch but minimize the plugin. It really a thin layer of one file.

@dnalborczyk
Copy link
Collaborator Author

dnalborczyk commented Jul 3, 2019

Anyone knows why proxy for lambda invoke is added by default for each WebSocket action?

Serverless: Action '$connect'
Serverless: POST /{apiVersion}/functions/manual-test-websocket-authorizer-dev-connect/invocations

@computerpunc what do you mean by this? is this new, or was added/changed after your initial PR?

I didn’t add this (at least not intentionally) to the WS flow. I think it got in when splitting index into WS and HTTP.

I don't think so. I believe this is (now) in master (and the latest release version) as well. Probably one of the recent commits.

@dnalborczyk
Copy link
Collaborator Author

In any case, we still need a map to go from connection ID to ws when we want to send messages back to clients.

true. but having the 'id' as the key in the map would prevent from (in that case needless) traversing.

Let’s look at his issue again after I PR authorizer support.

ok.

If the plugin is still an overkill, I wouldn’t write code to use ws from scratch but minimize the plugin. It really a thin layer of one file.

when you look at the current implementation and visually switch the hapi-plugin with the ws module, there's seems to be almost no difference. what I mean is, there's nothing I can see the plugin provides in addition, besides adding a layer of complexity. instead of using request.webSocket(), you could just use your own websocket instance. I'll have another look later today.

@dnalborczyk
Copy link
Collaborator Author

removed apiGatewayUrl: c0bea4c, still need to fix the docs and manual tests.

@computerpunc
Copy link
Contributor

@frsechet
In #726, you added 2 tests should be able to close connections via REST API & should receive error code when deleting a previously closed client via REST API.
They are failing for me when running against AWS.
Did these test pass for you on AWS?

@frsechet
Copy link
Contributor

frsechet commented Jul 3, 2019

Sorry about that. It was rather in #725, though.

Full disclosure: I could not get any test (neither those nor the existing ones) to pass on AWS so I guessed I had something missing in my environment, but I just copy-pasted the tests right above for the POST behaviour and changed the expected results. I also felt confident that the tests were all passing in Travis...

If anything breaks, I would guess that the sigV4 URL is malformed or something like that (it does not need to be signed locally). It's probably because I forgot to remove the body.

I will give it a go again this weekend (sorry, absolutely no time before that) but I'm really confident that it does work! But if one of you has a working environment and wants to give it a go, you can try this: frsechet@9cdad89

@computerpunc
Copy link
Contributor

Sorry about that. It was rather in #725, though.

Full disclosure: I could not get any test (neither those nor the existing ones) to pass on AWS so I guessed I had something missing in my environment, but I just copy-pasted the tests right above for the POST behaviour and changed the expected results. I also felt confident that the tests were all passing in Travis...

If anything breaks, I would guess that the sigV4 URL is malformed or something like that (it does not need to be signed locally). It's probably because I forgot to remove the body.

I will give it a go again this weekend (sorry, absolutely no time before that) but I'm really confident that it does work! But if one of you has a working environment and wants to give it a go, you can try this: frsechet@9cdad89

Yes, that solves it. Removing body and Content-Type header solves it.

@computerpunc
Copy link
Contributor

There's a small issue: websocketPort is not behaving as expected.
It supposed to be port+1 or set value but in reality it's 3001 or set value.

@dnalborczyk
Copy link
Collaborator Author

There's a small issue: websocketPort is not behaving as expected.
It supposed to be port+1 or set value but in reality it's 3001 or set value.

I changed it to that, because I think it would be confusing (and annoying) behavior. Although I thought @dherault changed it back, not sure.

imagine the following scenario: user uses default ports, 3000 and +1, now, for whatever reason gets a collision on 3000, and changes the settings for that. (which could involve several things: ngrok, nginx reverse proxy, and what not). now, all the sudden the websocket port also changes.

@frsechet
Copy link
Contributor

frsechet commented Jul 3, 2019

Yes, that solves it. Removing body and Content-Type header solves it.

Here you go: #729

fix: don't add body and content-type headers to sigv4
@dherault
Copy link
Owner

dherault commented Jul 4, 2019

Although I thought @dherault changed it back, not sure.

It's now 3001 by default. Can be changed with --websocketPort.

Ok, let's merge shall we?

@dnalborczyk dnalborczyk merged commit fdbde8f into master Jul 4, 2019
@dnalborczyk
Copy link
Collaborator Author

@dherault done!! 😃

[gonna remove the remaining apiGatewayUrl mentions directly in master now]

@dherault
Copy link
Owner

dherault commented Jul 4, 2019

I'll wait for your commit and then publish the release

@dnalborczyk
Copy link
Collaborator Author

dnalborczyk commented Jul 4, 2019

@dherault alrighty, all done! the manual tests probably don't work anymore (as of this moment), but we can fix them with either with what you mentioned env.IS_OFFLINE ? localhost : http://aws .... or env options. it shouldn't hold back the release. it's just if anyone would look at the manual tests, the event property's existence doesn't cause any confusion.

@dherault
Copy link
Owner

dherault commented Jul 4, 2019

v5.6.0 🎉

@dnalborczyk dnalborczyk deleted the websocket-fixes branch July 13, 2019 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants