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

Scetch a possiblity for custom logic in the broker #47

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

Conversation

flxo
Copy link
Contributor

@flxo flxo commented Apr 29, 2022

Use the authentication stuff as an example how customer code could be plugged into the broker. The authentification is not implemented - it's all just about how to proceed...

The concrete type that impls the authentication has the advantage of static dispatch. There ware probably other traits needed for intercepting publishes and subscriptions e.g

pub trait {
  async fn on_publish(client_id: &str, publish: &PublishPacket) -> PublicationResult;
  ....
}


Do *not* merge this :-)

@flxo
Copy link
Contributor Author

flxo commented Apr 29, 2022

@bschwind a quick attempt how to plug custom logic into the broker - no logic implemented - just to get the discussion started. Thanks for comments!

@flxo flxo force-pushed the auth branch 5 times, most recently from c3df8c0 to 7fcf178 Compare May 2, 2022 07:30
@bschwind
Copy link
Owner

bschwind commented May 8, 2022

Hey @flxo, sorry for the late response! Been on holiday the past week, but I'll try to take a look at this soon.

@flxo
Copy link
Contributor Author

flxo commented May 10, 2022

@bschwind Thanks!

Copy link
Owner

@bschwind bschwind left a comment

Choose a reason for hiding this comment

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

@flxo thanks for the work so far! I saw you originally were using async trait methods (via the async-trait crate), what was your reasoning for switching away from it?

It's not that I'm for or against it, just curious about the reasoning. I think that will be a big decision for us - do we want to allow async operations in the plugins? I can imagine a plugin that would want to read from a database or key-value store before making a decision, so it might be nicer to have the functions be async. I haven't thought through all the implications yet.

I looked less at the logic changes in broker.rs as I think settling on the API design first is important.

I left some more targeted comments in the code itself. Thanks again!

Cargo.lock Outdated Show resolved Hide resolved
fn on_subscribe(&mut self, packet: &SubscribePacket) -> SubscribeAckPacket;

/// Called on publish packets reception for QoS 0. Return true if the packet should be published to the clients.
fn on_publish_received_qos0(&mut self, packet: &PublishPacket) -> bool;
Copy link
Owner

Choose a reason for hiding this comment

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

What's the reasoning for splitting out publishes based on the qos? PublishPacket already has the qos field on it, but maybe there's some other reasoning for it that I'm missing.

Is this for handling the individual steps in the QoS sending handshake, or just a wholesale "We received a qos2 packet, do you want to send it?"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a single "callback" for publication but struggled with the return type. There's no return for QOS0, a Publish Ack for QOS1 and PublishReceivedAck for QOS2.
I'm fine with merging those if you want and but would require to define a struct for that the includes basically the same as the PublishAckPacket and PublishReceivedPacket in an Option.

mqtt-v5-broker/src/broker.rs Outdated Show resolved Hide resolved
mqtt-v5-broker/src/broker.rs Outdated Show resolved Hide resolved
mqtt-v5-broker/src/broker.rs Outdated Show resolved Hide resolved
@flxo
Copy link
Contributor Author

flxo commented May 23, 2022

@flxo thanks for the work so far! I saw you originally were using async trait methods (via the async-trait crate), what was your reasoning for switching away from it?

It's not that I'm for or against it, just curious about the reasoning. I think that will be a big decision for us - do we want to allow async operations in the plugins? I can imagine a plugin that would want to read from a database or key-value store before making a decision, so it might be nicer to have the functions be async. I haven't thought through all the implications yet.

I looked less at the logic changes in broker.rs as I think settling on the API design first is important.

I left some more targeted comments in the code itself. Thanks again!

The reason why I used the async-trait thing is because I need exactly what you described: query an eternal entity where I have a client for that also uses tokio and provides async fn. I haven't look into cool ways to call async code from sync without channels in between.

I will try to answer your findings inline.

@bschwind
Copy link
Owner

bschwind commented Jun 2, 2022

@flxo still keeping an eye on this PR! Sorry though that I don't have the bandwidth for quick reviews. I'll be happy to give it a look when you think it's ready for another review.

@flxo
Copy link
Contributor Author

flxo commented Jun 2, 2022

@flxo still keeping an eye on this PR! Sorry though that I don't have the bandwidth for quick reviews. I'll be happy to give it a look when you think it's ready for another review.

Thanks. I'll try to solve some of the todos I left in the PR and will ping you.

flxo added 3 commits June 2, 2022 11:21
Collect unauthorized connections seperated from the session until the
auth is complete. Introduce a unique identifier for connections.
Replace String connection id with cheaper u64. The connection id is a
atomically inremented counter in the client handler.
@flxo
Copy link
Contributor Author

flxo commented Jun 2, 2022

@bschwind I refactored the connection handling a bit and separated the unauthenticated connections from the session. Introduced a ConnectionId which is a unique identifier for a connection.

I'd love to hear your feedback.

@flxo flxo marked this pull request as ready for review June 2, 2022 09:50
@flxo flxo requested a review from bschwind July 5, 2022 14:25
@bschwind
Copy link
Owner

Hi @flxo

Sorry for the long time radio silence. I still think this is good work, but I don't have enough bandwidth to review this in any sort of timely fashion.

I plan to get back into this project around the time when I update my ESP32 projects to use Rust, but until then you probably won't see too much activity from me on it. I would recommend working with either your fork of this repo, or rumqtt (which I would guess is farther along and has more contributors)

@bschwind bschwind changed the base branch from master to main May 31, 2023 13:33
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.

2 participants