bytebeamio
feat: Support both v4 & v5 on same port
This PR adds the ability to specify a single port and have that port support both v.3.1.1 and v5. It also allows a simpler configuration for tcp connection. The implementation works by adding a new "protocol" which reads the connect packet and determines which protocol the client is using. For the next packets and the continuation of the connect packet they are forwarded to the determined protocol (If it is supported as per the configuration).
As the comment dynamic.rs indicates. This does add a small bit of overhead in reading the fixed header twice, will try to figure out a smarter way to peek at the packet.
I decided to keep the previous way of configuring to not break existing configurations. It could be possible and possibly cleaner to remove the old way (v4.1 & v5.1) in favor of the dynamic tcp configuration.
Open to feedback on the solution and this might not be something of interest which is fine.
This solves #634 and the discussion in #633.
Type of change
New feature (non-breaking change which adds functionality)
Checklist:
- [x] Formatted with
cargo fmt - [x] Make an entry to
CHANGELOG.mdif its relevant of user of the library. If its not relevant mention why.
I just glanced over the changes, this is really clever way to so it haha, glad that you figured it out! :smile:
The idea of adding the ability to specify we want to use single port for both v4/5 is noicee! :rocket:
But, DynamicProtocol might be confusing for other users. I'm curious what are the issues if we try to replace the Network<P>?
I will go over the PR in depth later, awesome work :100:
Hi! Though about the problem when going to bed last night and had to get up to try it. 😄
Agree that DynamicProtocol is not a optimal name. Might have to think a bit about that.
From my understanding, Network<P> should be non the wiser since Dynamic implements P: Protocol in this case. If in the future another protocol is added it can either be added to Dynamic to have it support that (If possible?) or just have the other Protocol run on a different port. Since I am guessing both v4 & v5 still should still be supported in that case, any other methods added to Protocol trait should be able to be supported by the Dynamic protocol.
Hi! Though about the problem when going to bed last night and had to get up to try it.
Oh btw, i remember you mentioning about taking look at how other brokers implement it, would love to know if you got any insights
Exactly like that! 😆
Ended up just trying to connect to different brokers and changing my protocol version. Most of them just worked. Tried to go though some HiveMQ code but brain was hurting from trying to find anything... But it seems that most brokers just have one large protocol that just checks what protocol level it is (most by doing something like my solution with determining by connect) and then takes different code paths depending on the level.
In my opinion my solution feels very optimal, apart from having a lot of code duplication (fixed header etc which is shared between v4 & v5 if im not mistaken).
I did went through HiveMQ & emqx code ( not that i know java / erlang ) and figured out that they store protocol level with client and pass it down when serializing/deserializing, as you mentioned. ( PS: I used GitHub android app just to increase my pain :rofl: )
But in our case, we don't store the protocol level at client, instead out Network has something that implements Protocol, so your proposed solution seems to be great choice. Though the implementation has scope for improvements, including what to name the protocol haha.
btw, will the following be better than sub_protocol field?
-
tcp.1-> dynamically figure out protocol -
tcp.v4.1/tcp.v5.1-> specify protocol ( same can be done forws)
what do you think @tekjar ?
Agree that there is room for improvement.
As for your example there, does that really make it simpler? In my opinion that feels a bit more difficult? Maybe my toml knowledge is just lacking 😀
sub_protocol thing is something inspired by how our HiveMQ config at work looks like. But in that case it's a list of protocols. So might need a rename.
so i discussed this with team and we think having the protocol version on top is more intuitive, for example:
[v4]
[v4.tcp.1]
name = "v4-tcp-1"
...
[v4.ws.1]
name = "v4-ws-1"
...
[v5]
[v5.tcp.1]
name = "v5-tcp-1"
...
[v5.ws.1]
name = "v5-ws-1"
...
[dynamic]
[dynamic.tcp.1]
name = "dynamic-tcp-1"
...
[dynamic.ws.1]
name = "dynamic-ws-1"
...
Also I think we can remove the SubProtocol logic, because we use dynamic only when we want to support multiple protocol at the same time.
It would make things more consistent related to ws and would make it trivial to add MQTTv5 for ws in #633
wdyt @Nickztar @swanandx ?
Not bad at all. This would be a breaking change right?
Will look into implementing it later today! Any idea about how the type for this would look?
Actually because that would be a breaking change, lets merge this like this only.
We can take that up in a different PR where we can experiment more on this.
I'll change it to SupportedProtocol instead of SubProtocol to keep it clean until we actually change it. 👍🏼
is this working for you @Nickztar?
trying to connect using mosquitto is failing for me with error:
2023-06-17T07:47:47.737667Z ERROR rumqttd::server::broker: Remote link error, error: Network(Protocol(BoundaryCrossed(257)))
in rumqttd::server::broker::remote_link with tenant_id: None
and its originating from call to read_mqtt_string
@henil It is working great for me. That error looks like when you try to connect with TLS without having server configured to TLS. Which is exactly what mosquitto does when you run on port 8883, which is a terrible example port chosen by me. If you change the port in rumqtt.toml to something else, like 8884, and then try with mosquitto it should work.
@henil @swanandx Would you like me to merge main and add support for configuring websockets in the same way you can for tcp here?