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

Add support for websockets event #5824

Merged
merged 11 commits into from Feb 19, 2019

Conversation

@eahefnawy
Copy link
Member

eahefnawy commented Feb 11, 2019

Closes #5823
Closes #5615

Validation

1. Setup the following service

# serverless.yml

service: websockets-test

provider:
  name: aws
  runtime: nodejs8.10

functions:
  connect:
    handler: handler.connect
    memory: 512
    events:
      - websocket: $connect
  disconnect:
    handler: handler.disconnect
    memory: 512
    events:
      - websocket:
          route: $disconnect
  default:
    handler: handler.default
    memory: 512
    events:
      - websocket:
          route: $default
  echo:
    handler: handler.echo
    memory: 512
    events:
      - websocket: echo
  # NOTE: this is a usual REST API
  greeter:
    handler: handler.greeter
    memory: 512
    events:
      - http: GET greet
// handler.js

'use strict';

const AWS = require('aws-sdk')

// the following section injects the new ApiGatewayManagementApi service
// into the Lambda AWS SDK, otherwise you'll have to deploy the entire new version of the SDK

/* START ApiGatewayManagementApi injection */
const { Service, apiLoader } = AWS

apiLoader.services['apigatewaymanagementapi'] = {}

const model = {
  metadata: {
    apiVersion: '2018-11-29',
    endpointPrefix: 'execute-api',
    signingName: 'execute-api',
    serviceFullName: 'AmazonApiGatewayManagementApi',
    serviceId: 'ApiGatewayManagementApi',
    protocol: 'rest-json',
    jsonVersion: '1.1',
    uid: 'apigatewaymanagementapi-2018-11-29',
    signatureVersion: 'v4'
  },
  operations: {
    PostToConnection: {
      http: {
        requestUri: '/@connections/{connectionId}',
        responseCode: 200
      },
      input: {
        type: 'structure',
        members: {
          Data: {
            type: 'blob'
          },
          ConnectionId: {
            location: 'uri',
            locationName: 'connectionId'
          }
        },
        required: ['ConnectionId', 'Data'],
        payload: 'Data'
      }
    }
  },
  paginators: {},
  shapes: {}
}

AWS.ApiGatewayManagementApi = Service.defineService('apigatewaymanagementapi', ['2018-11-29'])
Object.defineProperty(apiLoader.services['apigatewaymanagementapi'], '2018-11-29', {
  // eslint-disable-next-line
  get: function get() {
    return model
  },
  enumerable: true,
  configurable: true
})
/* END ApiGatewayManagementApi injection */

module.exports.connect = (event, context, cb) => {
  cb(null, {
    statusCode: 200,
    body: 'Connected.'
  });
};

module.exports.disconnect = (event, context, cb) => {
  cb(null, {
    statusCode: 200,
    body: 'Disconnected.'
  });
};

module.exports.default = async (event, context, cb) => {

  const client = new AWS.ApiGatewayManagementApi({
    apiVersion: '2018-11-29',
    endpoint: `https://${event.requestContext.domainName}/${event.requestContext.stage}`
  });

  await client
    .postToConnection({
      ConnectionId: event.requestContext.connectionId,
      Data: `default route received: ${event.body}`
    })
    .promise();

  cb(null, {
    statusCode: 200,
    body: 'Sent.'
  });
};

module.exports.echo = async (event, context, cb) => {

  const client = new AWS.ApiGatewayManagementApi({
    apiVersion: '2018-11-29',
    endpoint: `https://${event.requestContext.domainName}/${event.requestContext.stage}`
  });

  await client
    .postToConnection({
      ConnectionId: event.requestContext.connectionId,
      Data: `echo route received: ${event.body}`
    })
    .promise();

  cb(null, {
    statusCode: 200,
    body: 'Sent.'
  });
};

// this function is used for our regular REST API
module.exports.greeter = async (event, context) => {
  return {
    statusCode: 200,
    body: JSON.stringify({
      message: 'Go Serverless v1.0! Your function executed successfully!',
      input: event,
    }),
  };

  // Use this code if you don't use the http event with the LAMBDA-PROXY integration
  // return { message: 'Go Serverless v1.0! Your function executed successfully!', event };
};

2. Deploy

serverless deploy

Also, you can checkout the generated CF template in the .serverless directory

3. Test Routes

# you can find the url in the dashboard, but we should create an output for it
wscatr -c <api-url> 

It should connect at this point. Try to send the following data in the wscat prompt

# this should trigger the default route & function
hello world
# this should trigger the echo route & function
{"action":"echo","data":"hello echo"}

Todos:

  • Support default route
  • Support connect & disconnnect routes
  • Support additional routes
  • Add policy to postToConnection
  • Add outputs
  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@eahefnawy eahefnawy self-assigned this Feb 11, 2019

@eahefnawy eahefnawy added this to In progress in Serverless via automation Feb 11, 2019

@eahefnawy eahefnawy added this to the 1.38.0 milestone Feb 11, 2019

@eahefnawy eahefnawy requested review from pmuens , dschep , horike37 , exoego and shortjared Feb 13, 2019

@pmuens
Copy link
Member

pmuens left a comment

This looks awesome!

Great job @eahefnawy 👍

Just looked through the code and couldn't find anything suspicious...

@jplock

This comment has been minimized.

Copy link

jplock commented Feb 13, 2019

Is the AWS.ApiGatewayManagementApi stuff required if we are using the latest version of the SDK?

@eahefnawy

This comment has been minimized.

Copy link
Member Author

eahefnawy commented Feb 13, 2019

@jplock nope. Just wanted to make validation easier and speed up test deployments

@jplock

This comment has been minimized.

Copy link

jplock commented Feb 13, 2019

@eahefnawy thanks for clarifying!

@horike37
Copy link
Member

horike37 left a comment

Thank you for working on this @eahefnawy 🎉

It works on my laptop 👍

$ wscat -c wss://xxxxxxx.execute-api.us-east-1.amazonaws.com/dev/
connected (press CTRL+C to quit)
> {"action":"echo","data":"hello echo"}
< echo route received: {"action":"echo","data":"hello echo"}
> aa
< default route received: aa
> 

Is there any reason why WebSocket URL and Connection URL don't output after sls deploy?

@pmuens

This comment has been minimized.

Copy link
Member

pmuens commented Feb 14, 2019

Hey @horike37 thanks for checking it out 👌

Yes, the outputs stuff was smth. which was missing. I just added it in a recent commit. info should now pick it up and display it...

Working on the tests now...

Serverless automation moved this from In progress to Reviewer approved Feb 14, 2019

@horike37
Copy link
Member

horike37 left a comment

Thanks @pmuens 👍
I confirmed to see the endpoint output 😄 Nice!

Serverless automation moved this from Reviewer approved to Needs review Feb 14, 2019

@pmuens

This comment has been minimized.

Copy link
Member

pmuens commented Feb 14, 2019

Great! Thanks for confirming @horike37 👍

Tests are now in. Next up: docs... After that it should be GTM...

@viktorlarsson

This comment has been minimized.

Copy link

viktorlarsson commented Feb 15, 2019

Eager to try this out. I'm a quite heavy user of serverless-offline, which probably won't support this. How do you guys do local development?

Serverless automation moved this from Needs review to Reviewer approved Feb 15, 2019

@pmuens
Copy link
Member

pmuens left a comment

Just finalized the PR and tested it thoroughly.

I'd say it's GTM :shipit:

@pmuens

This comment has been minimized.

Copy link
Member

pmuens commented Feb 15, 2019

Eager to try this out. I'm a quite heavy user of serverless-offline, which probably won't support this. How do you guys do local development?

Hey @viktorlarsson thanks for checking this out. Yes, that's a good question 🤔. That would be really nifty and it's definitely smth. which is on our roadmap!

/cc @eahefnawy @dschep

@shortjared

This comment has been minimized.

Copy link
Contributor

shortjared commented Feb 15, 2019

@eahefnawy it looks like resources get created correctly, but the "endpoints" in the output does not properly handle if your service has both websockets AND http endpoints (thus two api gateways) it only shows the wss endpoint for me.

@exoego
Copy link
Contributor

exoego left a comment

LGTM 👍 and works on my end.

@exoego

This comment has been minimized.

Copy link
Contributor

exoego commented Feb 16, 2019

@eahefnawy it looks like resources get created correctly, but the "endpoints" in the output does not properly handle if your service has both websockets AND http endpoints (thus two api gateways) it only shows the wss endpoint for me.

Ah, this may be bug.
If I understand correctly, an API Gateway endpoint can be REAT API or WebSocket API, not both at same time.

I think it requires a validation to raise error if an endpoint has http and websocket event at same time.

@pmuens

This comment has been minimized.

Copy link
Member

pmuens commented Feb 18, 2019

Great! Thanks for beaking it @shortjared and thanks for finding the bug @exoego 👍

Working on a fix right now...

@pmuens pmuens dismissed stale reviews from exoego and themself via bad6a4a Feb 18, 2019

Serverless automation moved this from Reviewer approved to Needs review Feb 18, 2019

@pmuens

This comment has been minimized.

Copy link
Member

pmuens commented Feb 18, 2019

@shortjared @exoego I fixed both issues you raised. Added regression tests and updated the PR description so that it's easier to validate that the bugs have been fixed...

@eahefnawy

This comment has been minimized.

Copy link
Member Author

eahefnawy commented Feb 18, 2019

@pmuens thanks for the updates! Just reviewed your changes and tested it. Works great! 🙌

@pmuens

pmuens approved these changes Feb 18, 2019

Copy link
Member

pmuens left a comment

LGTM :shipit:

Serverless automation moved this from Needs review to Reviewer approved Feb 18, 2019

@dschep

dschep approved these changes Feb 18, 2019

@exoego

exoego approved these changes Feb 18, 2019

Copy link
Contributor

exoego left a comment

LGTM!!

@eahefnawy eahefnawy merged commit 3408a4e into master Feb 19, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

Serverless automation moved this from Reviewer approved to Done Feb 19, 2019

@conflagrator

This comment has been minimized.

Copy link

conflagrator commented Feb 20, 2019

Hi and thanks for the official support in serverless core!
We used the websocket functionality in earlier versions through the plugin. One major change, which breaks the conventions we built so far, is that only alphanumeric names seem to be allowed for routes now, which seems to be more strict than the AWS rules for route naming (with the plugin, routes like "entity/command" worked fine). Is this a necessary restriction for not-obvious reason? If not, we would be very happy if at least some characters for path separation would be allowed.

@dschep dschep deleted the websockets branch Feb 20, 2019

@farbodsalimi

This comment has been minimized.

Copy link

farbodsalimi commented Feb 20, 2019

Great work guys! now we can fetch the API ID by Ref: WebsocketsApi 🎉

@eahefnawy

This comment has been minimized.

Copy link
Member Author

eahefnawy commented Feb 21, 2019

@conflagrator interesting use case! Yeah this is because we create a Route CF resource that uses the route name as the logical ID, and logical IDs has some more restrictions. However, I think we can work around that with normalization.

I consider this a bug, could you open an issue that describes this issue? The solution is simply to replace slashes with something alphanumeric. Also, could you think of other characters that should be supported?

@conflagrator

This comment has been minimized.

Copy link

conflagrator commented Feb 21, 2019

Hi @eahefnawy, just opened it #5858

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.