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

feat(traefik) allow websocket traffic #275

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

DrummyFloyd
Copy link

@DrummyFloyd DrummyFloyd commented Apr 13, 2024

as @tomaszduda23 mentionned, i reoppend a PR
to make some reviews

atm

  • docker test => ✅
  • swarm (need to fix on my own swarm ) => ❌
  • Kube => bad request (seems to be a race conditions, because runing test mannualy seems to work 302 error on first try) ❌

but tested manually

with docker

i can reach ws://localhost:8080/echo
container is ready , i can talk with the whoami WS even, it's not shtudown , once i stop , after session-duration hte container i stopped

edit:
can i split a bit the code like

  • types.go
  • wsHandler.go
  • main.go
    to have a better reading ? or i let you do this on the v2 @acouvreur

Closed: #21 #42 #152

@tomaszduda23
Copy link
Contributor

  • Kube => bad request (seems to be a race conditions, because runing test mannualy seems to work 302 error on first try)

This is by design you need to follow redirection. Explained in https://github.com/acouvreur/sablier/pull/241

plugins/traefik/main.go Show resolved Hide resolved
plugins/traefik/main.go Show resolved Hide resolved
plugins/traefik/main.go Outdated Show resolved Hide resolved
if isWebsocketRequest(req) {
// FIXME dynamic make no sense for websocket since client return error
fmt.Println("=== websocket request")
go monitorWebSocketActivity(sablierRequest, sm)
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about this approach also. You create go routine per connection. I wondered if this would scale up. I didn't check details though.

Copy link
Author

@DrummyFloyd DrummyFloyd Apr 14, 2024

Choose a reason for hiding this comment

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

it should scale correctly , maybe in next iteration , we should add Mutex for security , but not really conformtable with it

Copy link

Choose a reason for hiding this comment

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

I wrote a proxy for Sablier to handle TCP only services and had a similar problem: How to prevent the service from shutting down, if there are only long running connections.

I don't check if there is actual activity on the connection, but I have a single task/process that will, as long as there is at least 1 active connection, ping the Sablier endpoint in regular intervals. Maybe something like this would work here, too:

https://github.com/vbrandl/sablier-proxy/blob/main/src/main.rs#L154

This way, you don't have to spawn another goroutine per connection, but there will be exactly 1 monitoring task/routine, which might scale better if there are many connections.

@DrummyFloyd
Copy link
Author

DrummyFloyd commented Apr 14, 2024

  • Kube => bad request (seems to be a race conditions, because runing test mannualy seems to work 302 error on first try)

This is by design you need to follow redirection. Explained in #241

will check that! thank you

EDIT: tbh , i don't fully understand what happnd , can you please have a look

@DrummyFloyd
Copy link
Author

  • Kube => bad request (seems to be a race conditions, because runing test mannualy seems to work 302 error on first try)

This is by design you need to follow redirection. Explained in #241

will check that! thank you

EDIT: tbh , i don't fully understand what happnd , can you please have a look @tomaszduda23

@tomaszduda23
Copy link
Contributor

@acouvreur could you please enable github action on this PR. I would like to see the test results.

@tomaszduda23
Copy link
Contributor

@DrummyFloyd
Copy link
Author

It looks that you have to handle retry on client side https://github.com/trazfr/tcp-over-websocket/blob/fcb1b85e9fd0e282eee88815efeca8d729bd762b/httpClient.go#L86C14-L86C36

Will have a look a it asap

Thank you :)

@tomaszduda23
Copy link
Contributor

any progress on that?

@DrummyFloyd
Copy link
Author

DrummyFloyd commented Jun 29, 2024

Did not have time sorry to debug this
Will try to give it a try next week

EDIT: been a while, since i touched Sablier's code,

@tomaszduda23 can you please elaborate a bit what you means , by retry on client side ?
because if i remembell well , this retry mechanism was already made your PR #241

Copy link

sonarcloud bot commented Jul 5, 2024

@tomaszduda23
Copy link
Contributor

@tomaszduda23 can you please elaborate a bit what you means , by retry on client side ? because if i remembell well , this retry mechanism was already made your PR #241

It seems that go client won't handle redirect by itself. You can reference this
https://github.com/trazfr/tcp-over-websocket/blob/fcb1b85e9fd0e282eee88815efeca8d729bd762b/httpClient.go#L86C14-L86C36. It works similar way to curl without -L option. Most clients will handle this by itself though. Redirect from http to https is pretty common.

@DrummyFloyd
Copy link
Author

@tomaszduda23 can you please elaborate a bit what you means , by retry on client side ? because if i remembell well , this retry mechanism was already made your PR #241

It seems that go client won't handle redirect by itself. You can reference this https://github.com/trazfr/tcp-over-websocket/blob/fcb1b85e9fd0e282eee88815efeca8d729bd762b/httpClient.go#L86C14-L86C36. It works similar way to curl without -L option. Most clients will handle this by itself though. Redirect from http to https is pretty common.

i tried some stuff, but everytime i brooke something else, dunno if i'll be able to finish that quickly

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.

Websocket traffic is not detected as activity
3 participants