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

fix(realtime): allow to use websocket without sticky sessions #5688

Merged
merged 18 commits into from
Nov 29, 2021

Conversation

laurentlp
Copy link
Contributor

@laurentlp laurentlp commented Nov 15, 2021

Description

This PR upgrades the socket.io library to its latest version and Socket V4.

I followed those two migration guides, one after the other:

It also re-implements the socket.io-jwt library that was provided by the auth0 community but has since been deprecated (see: https://community.auth0.com/t/community-repo-deprecations-september-2021-eol/60380).

And finally, it modifies the realtime-service getVisitorIdFromSocketId to allow fetching the rooms of sockets from other nodes (this is required since the channel-web sends socket and HTTP requests).

It is now possible to only use 'websocket' as the only transport mode for socket connection and not configure sticky sessions.

Related to: botpress/studio#203

Fixes botpress/v12#1177
Closes DEV-1179
Also closes this PR #5156

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How has this been tested?

Start multiple instances of Botpress and use Nginx to serve all those instances. Make sure to use Postgres and enable Redis.

  • Open the emulator and converse with a bot
  • Open an embedded webchat and test communication
  • Test the HITLNext module
  • Benchmarking tool (yarn start bench --url http://localhost:3000 --botId=<BOT_ID> --web)
  • Test with the USE_JWT_COOKIES option to test authentication via cookies

Single node, with only websocket (and only longpolling)

  • Open the emulator and converse with a bot

Maybe I am missing some tests?

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (need to update the production checklist)
  • Any dependent changes have been merged and published in downstream modules (chore: bump socket.io to latest version studio#203)

@linear
Copy link

linear bot commented Nov 15, 2021

DEV-1179 [BUG] "Sticky Sessions" should not be required with websockets

Describe the bug
Sticky sessions are required to be able to establish a Websocket connection

To Reproduce
2+ Botpress nodes running behind a 1/1 round robin load balancer (of any kind), with Websockets enabled and no sticky sessions configured.

Try to use the webchat.

Connection does not establish during the "handshake"

Expected behavior
The connection to be established, and be able to chat.

Screenshots
N/A

Environment (please complete the following information):
N/A

Additional context
Socket IO documentation: https://socket.io/docs/v3/using-multiple-nodes/index.html

@@ -95,8 +95,6 @@
"@types/redlock": "^4.0.0",
"@types/seedrandom": "^2.4.28",
"@types/semver": "^7.3.8",
"@types/socket.io": "^2.1.2",
"@types/socket.io-client": "^1.4.36",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those typings are not required anymore as they come with both libraries

path: `${process.ROOT_PATH}/socket.io`,
origins: '*:*',
serveClient: false
cors: { origin: '*' },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we only allow externalURL/localURL as the origin?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, i'm not sure for that point. Maybe we should use the same cors property we have for httpServer in botpress config? We'd need to highlight the change in the release notes since it may be breaking

@allardy
Copy link
Member

allardy commented Nov 16, 2021

Updating this library means we have a lot of scenarios to test.

  1. Single node, with only websocket (and only longpolling)
  2. With 2 servers connected to the same redis instance (ideally with nginx to test that it really works without sticky session)
  3. Test with the USE_JWT_COOKIES option to test authentication via cookies

Copy link
Member

@allardy allardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The webchat no longer loads on IE11, looks like a polyfill is missing related to a change on socketio
image

image

path: `${process.ROOT_PATH}/socket.io`,
origins: '*:*',
serveClient: false
cors: { origin: '*' },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, i'm not sure for that point. Maybe we should use the same cors property we have for httpServer in botpress config? We'd need to highlight the change in the release notes since it may be breaking

packages/bp/src/admin/ui/src/app/EventBus.ts Show resolved Hide resolved
@laurentlp
Copy link
Contributor Author

The webchat no longer loads on IE11, looks like a polyfill is missing related to a change on socketio

Weird, because it should be compatible with IE9+ (see: https://socket.io/docs/v4/client-installation/#browser-support)

Copy link
Member

@EFF EFF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@laurentlp laurentlp merged commit 5181340 into master Nov 29, 2021
@laurentlp laurentlp deleted the llp_upgrade_socketio branch November 29, 2021 15:47
@davidvitora
Copy link
Contributor

@laurentlp does this fix this issue botpress/v12#1541?

@laurentlp
Copy link
Contributor Author

laurentlp commented Nov 29, 2021

@laurentlp does this fix this issue botpress/v12#1541?

@davidvitora No, you still need to use the workaround you suggested. I think this issue will be properly fixed once Messaging will provide the new version of the webchat.

This was referenced Dec 2, 2021
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.

[BUG] "Sticky Sessions" should not be required with websockets
4 participants