search

crossbario / crossbar

Crossbar.io - WAMP application router
https://crossbar.io/
Other
2.05k stars 274 forks source link

Fix proxy routes when multiple roles configured #1972

Closed om26er closed 2 years ago

om26er commented 2 years ago

Fixes https://github.com/crossbario/crossbar/issues/1971

Separately, I think the round robin logic there is completely broken.

om26er commented 2 years ago

Separately, I think the round robin logic there is completely broken.

What I mean by this is that the existing roundrobin logic assumes that the number of routes is always static. We need to think an alternate approach here because the routes can potentially be dynamic hence it might make sense to have a roundrobin index per realm per role. I can work on that in a follow up branch, if that approach makes sense

oberstet commented 2 years ago

I think the round robin logic there is completely broken.

do you have a unit test that demonstrates that it is broken? without a unit test, broken is undefined, as it works for me: clients can do RPC and PubSub, so what's wrong?

om26er commented 2 years ago

Just a clarification, I am talking about the round robin logic inside the ProxyController which decides what proxy route to use for a connection request. This is going to be a difficult task to create an automated test for, do you have any pointers on writing automated tests for this stuff of features in Crossbar ?

And regarding the PR, there is a crossbar config in issue https://github.com/crossbario/crossbar/issues/1971, which makes the relevant bug 100% reproducible.

oberstet commented 2 years ago

do you have any pointers on writing automated tests for this stuff of features in Crossbar

yes, in this repo in the test folder, eg https://github.com/crossbario/crossbar/blob/master/test/functests/cfctests/test_cfc_rlink_mesh.py programmatically creates a multi-node setup (via the plain management API exposed by individual nodes), and then runs a few WAMP application routing tests

this approach only tests the setup proxy routes and rlinks without also testing the automatic orchestration of the setup of these resources (which the master node also does)

I think this approach of splitting up tests into A. base infra (proxy routes/connections and rlinks) and B. orchestration (web/routerclusters etc) makes sense. I also think before adding more tests we should reactivate the existing ones, part of which were deactivated due to issues. mentioned that before couple of times. also that I think it would not be a good idea to (at this point) start randomly patching code without having tests. in general.