feat: Ensure `websocket` parity with API Gateway #1301

cnuss · 2021-11-23T02:57:27Z

Description

This PR gives serverless-offline handling of Web Sockets parity with how AWS API Gateway V2 handles Web Sockets

Discrepancies

Status Code Handling

When a client connects, the Lambda Handler (or Authorizer) can set a statusCode to control whether the connection is allowed or not.

AWS API Gateway:

If a non-2xx Status Code is returned, the handshake is rejected
A wscat command produces the following error: error: Unexpected server response: {statusCode}

Serverless Offline:

Websocket is connected, statusCode is ignored

`routeResponseSelectionExpression`

From the AWS Docs, a routeResponseSelectionExpression (of value $default) can be placed on the $connect handler to enable 2-way communication for websockets.

AWS API Gateway:

After enabling a route response, API Gateway will return whatever is in body to the client.
If disabled, no response is propagated back the the client

Serverless Offline:

Does no such handling, no checking of the routeResponseSelectionExpression option in serverless.yml

Motivation and Context

Most notably, the current implementation of Web Socket handling is a TODO, and this should remedy the TODO to make behavior similar to AWS.

serverless-offline/src/events/websocket/WebSocketClients.js

Line 147 in 2ac804b

// TODO what to do with "result"?

This issue has been raised before in two PRs, and this PR likely supersedes them:

Fixing connections

Using WebSocketServer.verifyClient, initial connections now invoke the lambda $connect handler, and if the status code is non-2xx, it will properly abort the handshake with the statusCode from the lambda response.

This results in the same abort handling as AWS API Gateway, when running offline:

Local:

$ wscat -c "ws://localhost:3001/cool?statusCode=200"
Connected (press CTRL+C to quit)
$ wscat -c "ws://localhost:3001/cool?statusCode=301"
error: Unexpected server response: 301

Remote:

wscat -c "wss://ws-sly.scaffoldly-demo.scaffoldly.dev/cool?statusCode=200"
Connected (press CTRL+C to quit)
$ wscat -c "wss://ws-sly.scaffoldly-demo.scaffoldly.dev/cool?statusCode=301"
error: Unexpected server response: 301

Two-way responses

If the optional routeResponseSelectionExpression has been specified the content of body will be sent back to the websocket.

How Has This Been Tested?

I have the following service running in Lambda:
https://github.com/scaffoldly-demo/cool-sls-rest-api

The $connect handler takes the statusCode query parameter and returns it as a number to simulate response code handling. 2xx's allow the connection to go through.

serverless.yml has also been outfitted with routeResponseSelectionExpression and responses are returned when the setting is enabled

cnuss · 2021-11-23T03:05:55Z

cc: @coulson84 as you were commenting on the other PRs, comments/questions/issues?

cc: @VenomAV @kevbot-git thoughts on this to replace your existing PRs?

coulson84

Unless the aws websockets API gateway setup has changed since the first quarter I am fairly certain that there's no response sent on the $connect or $disconnect routes - it looks like this PR only doesn't send request body on the $disconnect route. Not sure why aws
implemented that way but I am reasonably sure that this is still the case.

cnuss · 2021-11-23T22:40:56Z

with my recent evaluations of AWS, you are correct, there is no response on $connect or $disconnect

$connect does, however, support the return of { statusCode: number }. If statusCode is in the 2xx range, it allows the connection to proceed, any other statusCode (such as returning a 401) does the following after a wscat. API Gateway V2 is poorly documented in this regard, but this is how Lambda Authorizers reject the handshake of a Websocket connection.

cnuss · 2021-11-23T22:57:45Z

Also for clarity, I leveraged verifyClient on the WebSocketServer so handshakes on the response code are properly checked using the $connect handler: https://github.com/dherault/serverless-offline/pull/1301/files#diff-973d50eef2198cd8d3d918e3f31ccb71d61fc715568fdfd08afab940ac86014fR38-R51

What I noticed before was Serverless Offline would blindly let the socket handshake and connect which is not the same as API Gateway V2.

Here's the relevant parts from WebSocketServer in ws:
https://github.com/websockets/ws/blob/master/lib/websocket-server.js#L51
https://github.com/websockets/ws/blob/master/lib/websocket-server.js#L295-L298

cnuss · 2021-11-26T22:28:41Z

@coulson84 hey Joe! any feedback on my responses to your review comment? thanks!

coulson84 · 2021-11-26T23:26:37Z

Hi, just this I think: where there is a check just before sending the lambda response body back down to the client, it checks if the route is $disconnect and then does not send the body if that is the case. I think that check should also include the $connect route. As per your and my comments previously there is no response sent in $connect routes. If that's handled elsewhere I apologise. I'm looking through things on my phone which is never ideal.

cnuss · 2021-11-27T00:01:55Z

hey @coulson84 thanks for the feedback!

`$disconnect` events

added a fix:
ceda78b

this will prevent body from being emitted to the client on $disconnect

`$connect` events

the handler invocation doesn't use WebSocketClients._processEvent(...) anymore, and $connect events are handled specially here:
https://github.com/dherault/serverless-offline/pull/1301/files#diff-031b5ae0f57e9ca68d08fe60c7b90905adafc38bde846c53a814cacbad393015R120

so any checking for $connect events in _processEvent(...), in my opinion, are unnecessary, do you agree?

coulson84 · 2021-11-27T06:51:48Z

Yes, sounds right to me. I thought I must've missed where you had implemented it as you said it was correct to not send down the connect response but I didn't see where. 👍

cnuss · 2021-11-27T15:59:12Z

@coulson84 awesome thanks! whats the process to get this merged and released?

cnuss · 2021-12-03T12:27:30Z

@coulson84 hey there! whats next in getting this merged? build status is currently "awaiting approval" for first time contributors

coulson84 · 2021-12-03T21:45:05Z

Alas, I hold no power here.

@dherault can you comment?

cnuss · 2021-12-09T02:49:43Z

@coulson84 if you could click the Approve button on the PR that'd help things along!

pinging: @Bilal-S @daniel-cottone @leonardoalifraco @mikestaub as project maintainers, whats the process to get this merged

cnuss · 2021-12-14T15:09:21Z

@mikestaub thanks for the approval! can we get the workflow unblocked and get this merged?

thanks

!

DocLM · 2022-01-15T11:30:20Z

Actually developing against @cnuss branch. This is a must have for who use API Gateway with Websockets

cnuss · 2022-02-10T19:12:54Z

@pgrzesik hey there! do you have the ability to approve workflows

pgrzesik · 2022-02-10T19:50:52Z

Hey @cnuss - I do but for some reason no workflows have been triggered here - could you please create a new PR or try to push to this one again? I'm not sure what's going on

cnuss · 2022-02-16T18:57:24Z

@pgrzesik thanks! just sync'd with dherault/serverless-offline@master, and it's still blocked... who are the maintainers on this project?

according to GH docs, they should see a "Approve and Run" button if they are a privileged maintainer on this repo

https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks#approving-workflow-runs-on-a-pull-request-from-a-public-fork

pgrzesik · 2022-02-16T18:58:55Z

I just clicked the approve, it was not available previously @cnuss

cnuss · 2022-02-16T19:08:40Z

@pgrzesik thanks for the run approval! looks like there's green checks across the board, can we get this merged? and whens the next release planned?

pgrzesik · 2022-02-16T20:34:17Z

I have an additional question @cnuss - do you think it would be possible to cover the functionality you're adding with some automated tests? I'm unfortunatelly not familiar with the websockets part of this codebase and I'm not confident in accepting the changes.

cnuss · 2022-02-17T17:16:44Z

@pgrzesik i can throw something together, of course!

cnuss · 2022-02-17T22:00:53Z

@pgrzesik added tests, could you approve the workflow again?

cnuss · 2022-02-17T22:33:50Z

@pgrzesik looks like we're green across the board, went into the logs to ensure that my tests did in fact run and pass!

FYI: it doesn't appear the tests are run against a real AWS account so i took the liberty to run them against my own AWS account:

two-way

➜  websocket-twoway git:(websockets) serverless deploy
Serverless: Running "serverless" installed locally (in service node_modules)
Serverless: Packaging service...
Serverless: Excluding development dependencies...
Serverless: Creating Stack...
Serverless: Checking Stack create progress...
........
Serverless: Stack create finished...
Serverless: Uploading CloudFormation file to S3...
Serverless: Uploading artifacts...
Serverless: Uploading service twoway-websocket-tests.zip file to S3 (1.4 kB)...
Serverless: Validating template...
Serverless: Updating Stack...
Serverless: Checking Stack update progress...
......................................................
Serverless: Stack update finished...
Service Information
service: twoway-websocket-tests
stage: dev
region: us-east-1
stack: twoway-websocket-tests-dev
resources: 19
api keys:
  None
endpoints:
  GET - https://u265pd00t4.execute-api.us-east-1.amazonaws.com/dev/echo
  wss://79voowg6td.execute-api.us-east-1.amazonaws.com/dev
functions:
  handler: twoway-websocket-tests-dev-handler
layers:
  None

➜  serverless-offline git:(websockets) AWS_ENDPOINT="wss://79voowg6td.execute-api.us-east-1.amazonaws.com" npm run test:log -- websocket-twoway

> serverless-offline@8.4.0 test:log /Users/christian/scaffoldly/serverless-offline
> npm run build && jest --verbose "websocket-twoway"


> serverless-offline@8.4.0 build /Users/christian/scaffoldly/serverless-offline
> rimraf dist && babel src --ignore "**/__tests__/**/*" --out-dir dist && copyfiles -u 1 "src/**/*.{vm,py,rb}" dist

Successfully compiled 106 files with Babel (1787ms).
 PASS  tests/integration/websocket-twoway/websocket-twoway.test.js
  two way websocket tests
    ✓ websocket echos sent message (886 ms)
    ✓ websocket connection emits status code 401 (376 ms)
    ✓ websocket connection emits status code 500 (359 ms)
    ✓ websocket connection emits status code 501 (377 ms)
    ✓ websocket connection emits status code 502 (371 ms)
    ✓ websocket emits 502 on connection error (385 ms)
    ✓ execution error emits Internal Server Error (477 ms)

Test Suites: 1 passed, 1 total
Tests:       7 passed, 7 total
Snapshots:   0 total
Time:        5.465 s
Ran all test suites matching /websocket-twoway/i.

one-way

➜  websocket-oneway git:(websockets) ✗ serverless deploy
Serverless: Running "serverless" installed locally (in service node_modules)
Serverless: Packaging service...
Serverless: Excluding development dependencies...
Serverless: Creating Stack...
Serverless: Checking Stack create progress...
........
Serverless: Stack create finished...
Serverless: Uploading CloudFormation file to S3...
Serverless: Uploading artifacts...
Serverless: Uploading service oneway-websocket-tests.zip file to S3 (1.08 kB)...
Serverless: Validating template...
Serverless: Updating Stack...
Serverless: Checking Stack update progress...
...................................................
Serverless: Stack update finished...
Service Information
service: oneway-websocket-tests
stage: dev
region: us-east-1
stack: oneway-websocket-tests-dev
resources: 18
api keys:
  None
endpoints:
  GET - https://8o7bzxx9yg.execute-api.us-east-1.amazonaws.com/dev/echo
  wss://i9eegrlotb.execute-api.us-east-1.amazonaws.com/dev
functions:
  handler: oneway-websocket-tests-dev-handler
layers:
  None

➜  serverless-offline git:(websockets) ✗ AWS_ENDPOINT="wss://i9eegrlotb.execute-api.us-east-1.amazonaws.com" npm run test:log -- websocket-oneway

> serverless-offline@8.4.0 test:log /Users/christian/scaffoldly/serverless-offline
> npm run build && jest --verbose "websocket-oneway"


> serverless-offline@8.4.0 build /Users/christian/scaffoldly/serverless-offline
> rimraf dist && babel src --ignore "**/__tests__/**/*" --out-dir dist && copyfiles -u 1 "src/**/*.{vm,py,rb}" dist

Successfully compiled 106 files with Babel (1827ms).
 PASS  tests/integration/websocket-oneway/websocket-oneway.test.js (6.511 s)
  one way websocket tests
    ✓ websocket echos nothing (5029 ms)
    ✓ execution error emits Internal Server Error (486 ms)

Test Suites: 1 passed, 1 total
Tests:       2 passed, 2 total
Snapshots:   0 total
Time:        7.446 s, estimated 8 s

pgrzesik

Thanks a lot @cnuss - it looks great 👍

cnuss added 2 commits November 22, 2021 16:41

feat: websocket parity with api gateway

b08f37e

save options

33752ad

coulson84 reviewed Nov 23, 2021

View reviewed changes

fix: prevent client responses on $disconnect

ceda78b

coulson84 approved these changes Dec 9, 2021

View reviewed changes

mikestaub approved these changes Dec 9, 2021

View reviewed changes

Merge branch 'dherault:master' into websockets

3530963

add websocket tests

d21b83e

pgrzesik approved these changes Feb 18, 2022

View reviewed changes

pgrzesik added the feature label Feb 18, 2022

pgrzesik changed the title ~~feat: websocket parity with api gateway~~ feat: Ensure websocket parity with API Gateway Feb 18, 2022

pgrzesik merged commit 8f02226 into dherault:master Feb 18, 2022

cnuss mentioned this pull request Mar 20, 2022

Web socket authorizer #1222

Closed

This was referenced Nov 3, 2022

Replicate AWS behavior of lambda exposed through API Gateway WebSockets #1188

Closed

fix: enable responses from result of lambda function #1064

Closed

Allow WebSocket responses to be sent by handler responses #1008

Closed

Itchibon777 mentioned this pull request Apr 25, 2024

[Snyk] Security upgrade serverless-offline from 6.0.0 to 8.6.0 Itchibon777/serverless-offline#60

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Ensure `websocket` parity with API Gateway #1301

feat: Ensure `websocket` parity with API Gateway #1301

cnuss commented Nov 23, 2021

cnuss commented Nov 23, 2021

coulson84 left a comment •

edited

cnuss commented Nov 23, 2021

cnuss commented Nov 23, 2021 •

edited

cnuss commented Nov 26, 2021

coulson84 commented Nov 26, 2021

cnuss commented Nov 27, 2021

coulson84 commented Nov 27, 2021

cnuss commented Nov 27, 2021 •

edited

cnuss commented Dec 3, 2021

coulson84 commented Dec 3, 2021

cnuss commented Dec 9, 2021

cnuss commented Dec 14, 2021 •

edited

DocLM commented Jan 15, 2022

cnuss commented Feb 10, 2022

pgrzesik commented Feb 10, 2022

cnuss commented Feb 16, 2022

pgrzesik commented Feb 16, 2022

cnuss commented Feb 16, 2022

pgrzesik commented Feb 16, 2022

cnuss commented Feb 17, 2022

cnuss commented Feb 17, 2022

cnuss commented Feb 17, 2022

pgrzesik left a comment

feat: Ensure websocket parity with API Gateway #1301

feat: Ensure websocket parity with API Gateway #1301

Conversation

cnuss commented Nov 23, 2021

Description

Discrepancies

Status Code Handling

routeResponseSelectionExpression

Motivation and Context

Fixing connections

Two-way responses

How Has This Been Tested?

cnuss commented Nov 23, 2021

coulson84 left a comment • edited

Choose a reason for hiding this comment

cnuss commented Nov 23, 2021

cnuss commented Nov 23, 2021 • edited

cnuss commented Nov 26, 2021

coulson84 commented Nov 26, 2021

cnuss commented Nov 27, 2021

$disconnect events

$connect events

coulson84 commented Nov 27, 2021

cnuss commented Nov 27, 2021 • edited

cnuss commented Dec 3, 2021

coulson84 commented Dec 3, 2021

cnuss commented Dec 9, 2021

cnuss commented Dec 14, 2021 • edited

DocLM commented Jan 15, 2022

cnuss commented Feb 10, 2022

pgrzesik commented Feb 10, 2022

cnuss commented Feb 16, 2022

pgrzesik commented Feb 16, 2022

cnuss commented Feb 16, 2022

pgrzesik commented Feb 16, 2022

cnuss commented Feb 17, 2022

cnuss commented Feb 17, 2022

cnuss commented Feb 17, 2022

two-way

one-way

pgrzesik left a comment

Choose a reason for hiding this comment

feat: Ensure `websocket` parity with API Gateway #1301

feat: Ensure `websocket` parity with API Gateway #1301

`routeResponseSelectionExpression`

coulson84 left a comment •

edited

cnuss commented Nov 23, 2021 •

edited

`$disconnect` events

`$connect` events

cnuss commented Nov 27, 2021 •

edited

cnuss commented Dec 14, 2021 •

edited