[bug] Nolocal flag for bridges is not respected

[oldrich-s]

System Information

• Aedes: 0.46.3
• NodeJS: 16.14.2 LTS
• Windows Server
• Arch: x86_64

Describe the bug

If I am not mistaken (and it could easily be that I am mistaken) then the nolocal flag is not respected by Aedes:

https://github.com/mqtt/mqtt.org/wiki/bridge_protocol

the subscriptions are nolocal (in the JMS sense), to reduce the chances of infinite message loops.

Basically what happens is that when I connect to Aedes broker as a broker and publish a message to the Aedes broker while also subscribing on '#', I receive the message that I have just published.

Could you please point me to the piece of Aedes code where you potentially do something like:

if (subscription.nl === true && targetClient.id === fromClient.id) {
  dontSendThePacket()
}

As much as I read the Aedes code, I could not find any nl === true or similar handling.

[robertsLando]

Aedes doens't fully support brigde yet. Is it this what you are looking for?

[oldrich-s]

Thanks for your answer.

No, not really. If I understand the code you sent me correctly, then if the client makes a new subscription and it already exists (with the same topic) and is different from the old one then the code unsubscribes the old subscription and subscribes the new one.

What I need is that Aedes does something like the following logic:

• Aedes receives a publish packet from e.g client1
• Aedes finds all the respective subscriptions where to forward the packet to (e.g. subscription1, subscription2, subscription3)
• Aedes checks if the subscription (e.g. subscription1) is nl === true
• If subscription1 is nl === true then
  • Aedes checks if subscription1 is part of client1 and if so then
    • Aedes does not forward the message to the client1 because the packet originates from client1 and the subscription1 was set to be nolocal

[robertsLando]

@oldrich-s Would you like to send a PR for this?

IMO you could handle this in client deliver0/deliveQos method but you should give it a try. Just add a case in the toForward var that makes the checks you need

[oldrich-s]

I can give it a shot but I think it will be more complicated than that. I need to know the origin clientId and the target clientId.

All the information about the origin client is being lost in the following function:

function emitPacket (packet, done) {
  this.broker.mq.emit(packet, done)
}

where only the packet is sent further and all other information is lost.

Therefore I need to put clientId inside of the packet so that the receiver of the emit event can know the original clientId.

function emitPacket (packet, done) {
  packet.clientId = this.client.id
  this.broker.mq.emit(packet, done)
}

This is currently not possible as Aedes strips the unrecognized clientId property.

Do you know how to pass the origin clientId to the mqemitter listeners?

[oldrich-s]

I can also see that it is not possible to get access to subscriptions nl flag through mqemitter as it only sends packets and nothing else, so it would be necessary to add nl property to aedes-packet (together with clientId).

An another issue is that persistent storages pt. only save topic and qos on subscriptions but it is also necessary to save nl and other information if it should work properly.

Also I am afraid that the following:

aedes/lib/handlers/subscribe.js

Line 145 in 81080a7

if (!rap) {

does not work either as the rap information is lost in the persistent storage.

[oldrich-s]

I have just made some changes that I believe would fix the issue. Could you please take a look at them?

Also all the persistent storages such as

https://github.com/moscajs/aedes-persistence/blob/main/persistence.js

need to save and restore nl and rap to make it work

[robertsLando]

I think the first thing to do is to update aedes-packet in order to parse those fields

[oldrich-s]

Also if it would be nice to update aedes-packet before I write additional unit tests to aedes so that they do not fail. Or what process do you propose?

Will you update aedes-packet version?

[robertsLando]

Will you update aedes-packet version?

Yep I will but I would like to know @gnought and/or @mcollina opinions on this approach

[robertsLando]

@oldrich-s Aedes packet v3 is out now: https://github.com/moscajs/aedes-packet

[oldrich-s]

Thanks, I will write the new unit tests next week. 👍

[robertsLando]

Thanks @oldrich-s !

[oldrich-s]

To make the tests not fail, aedes-persistence needs to persist rh, nl and rap and to make aedes-persistence to work, the qlobber also needs to understand the flags.

[davedoesdev]

@oldrich-s thanks for your PR, it's published as qlobber 7.0.0. Let me know whether it works for you.

[robertsLando]

@oldrich-s 0.47.0 is out now with this feat 🚀 Having an example of this in action with mosquitto would be awesome :)

[oldrich-s]

Nice, thanks 👍

As for the example - I am not sure I understand what you would like me to do 😂. We have the unit tests that should test the scenario.

[robertsLando]

You said that in another thread (cannot find it now), if we were interested in an example using aedes as bridge. Not a test, it's just a use case to help users interested in it.

[oldrich-s]

Ok, that's actually something else 😊. This PR was about when somebody else (e.g. mosquitto) connects to Aedes as a bridge then Aedes should respond properly (with nl = true). The other thing is if Aedes shall make bridge connection to an another broker. This functionality is missing in Aedes and that is what I have proof of concept of.

I am also considering if it was not better to include the new functionality directly into Aedes or at least as aedes-bridge plugin so it is something to rely on (with unit tests etc.).

I will start by making a gist file to see what is inside and then we can talk more about it 👍.

[oldrich-s]

Just to give you an idea here is the gist:

https://gist.github.com/oldrich-s/27930cabc1669b6be39c372984e89eca

I copy pasted code from our code base and stripped typescript and all non-related code. The gist is therefore probably not runnable but can give you an idea of what it is.

As you can see it is probably too much for an example - I see it more as a Aedes plugin or core part of Aedes with unit tests, documentation etc.

What do you think?

[robertsLando]

This PR was about when somebody else (e.g. mosquitto) connects to Aedes as a bridge then Aedes should respond properly (with nl = true).

Yep I know that, ATm we only support incoming connection, you added the piece that was missing to fully support them :)

I think you was using it with mosquitto that also supports outgoing bridge.

I am also considering if it was not better to include the new functionality directly into Aedes or at least as aedes-bridge plugin so it is something to rely on (with unit tests etc.).

Giving that bridge is not a commonly used feature but it's kind of an advanced usage it would be awesome to have it as plugin!

If you want I could create the repo for you if you are interested in developing it :)

PS: We are looking for contributors/maintainers like you here as actually I'm the only one that is active, if you are interested in join our org let me know

[oldrich-s]

Thanks, a repo would be nice 👍

With regards to joining the org - sure why not. We are going to use Aedes at work so it kind of make sense to help Aedes. 😊

[robertsLando]

Invitation to team sent and repo created: https://github.com/moscajs/aedes-bridge

If you need any help reach me here or also via Github team discussions 😄

Happy to have you in our team 🚀

[oldrich-s]

Hi, just a brief status - I am currently busy with other things but should be able to look at the bridge during summer 😉

[robertsLando]

@oldrich-s No problem, thanks for this 🙏🏼