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

Feature/mqtt #1717

Closed
wants to merge 5 commits into from
Closed

Feature/mqtt #1717

wants to merge 5 commits into from

Conversation

suneetnangia
Copy link
Contributor

@suneetnangia suneetnangia commented Aug 29, 2023

This initial (and relatively large 🐼) PR adds MQTT support in Spin, following are the key changes for initial review before it moves out of draft:

  1. Added support for MQTT trigger
  2. Added support for MQTT outbound in SDK
  3. Updated examples and templates
  4. Updated WIT definitions to support MQTT functions
  5. Updated tests

Primary reason for adding MQTT support in this instance to allow us to run Spin based Wasm components on the edge where MQTT is more prevalent.

P.S. I'll be revisiting MQTT code and WIT definitions this week to cover any gaps and potential performance bottlenecks. If you do spot anything I missed, please do let me know. Thanks.

suneetnangia and others added 5 commits August 14, 2023 15:41
Signed-off-by: Suneet Nangia <suneetnangia@gmail.com>
Signed-off-by: Suneet Nangia <suneetnangia@gmail.com>
Signed-off-by: Suneet Nangia <suneetnangia@gmail.com>
@itowlson
Copy link
Contributor

@suneetnangia For the MQTT trigger, would the existing messaging trigger meet your needs?

https://developer.fermyon.com/hub/preview/plugin_spin_message_trigger

@suneetnangia
Copy link
Contributor Author

suneetnangia commented Sep 1, 2023

@suneetnangia For the MQTT trigger, would the existing messaging trigger meet your needs?

https://developer.fermyon.com/hub/preview/plugin_spin_message_trigger

@itowlson thanks, preference would be to use native support (i.e. not via plugin model) in this case, similar to existing Redis support we have. Some reasons for this are:

  1. We intend use MQTT in its full fidelity without any additional layer in-between on top of core Spin.
  2. We keep it simpler by not having it generalised for all message brokers (which is useful by all means in some use cases) thus reducing the feature sets or adding deviations from std conventions e.g. not able to use native MQTT topic path patterns etc.
  3. We intend to integrate support for this trigger in wasm-shims as next step (just like we did for Redis trigger) which is simpler (without any side-loading) when we have native support for a trigger IMHO. This will pave the way to run Spin along side OCI containers on edge for us.
  4. We do not need replaceable implementation of pub/sub and layers providing that, MQTT is near defacto std for edge messaging.
  5. In principle, plugins should be small and isolated/sandboxed modules for security reasons as they get side-loaded in the process. Messaging plugin does not aligns well with that approach, customers may not be comfortable with that.

Apart from these, secondary objective here is to have an ongoing commercial support/quality/trust enabled for our customers by having it as a built-in feature (similar to Redis we have today in Spin). Isn't also why we have Redis trigger is part of core triggers? WDYT? happy to get your/others feedback on this and revise the PR with any changes suggested.

@suneetnangia
Copy link
Contributor Author

suneetnangia commented Sep 1, 2023

Re build failure on Windows, I was able to reproduce it locally and fix it, I had to install vcpkg on the host and then install openssl --triplet x64-windows-static-md to allow building openssl-sys v0.9.92 dependency, per this doc. Is there be a better solution to this which I am missing?

@itowlson
Copy link
Contributor

itowlson commented Sep 3, 2023

@suneetnangia Thanks for the feedback. I'm on board with your reasons for wanting a pure MQTT trigger rather than a brokered one. I'd still encourage you to consider a plugin because:

  1. You can have the trigger in a repository you control, and rev it according to your (and other users') needs, without being bound by the Spin release cadence.
  2. Users can file issues directly against the trigger, rather than against Spin, so we don't have to tag you in explicitly.

You mention "secondary objective here is to have an ongoing commercial support/quality/trust enabled for our customers by having it as a built-in feature." This is a tricky one. Certainly core triggers have guarantees of being aligned with the Spin runtime and SDKs, and this is a known problem area for trigger plugins. But I am not sure how support and maintenance would work if this were in the core - would you and your team be on the hook for fixes and updates?

Re wasm-shims: looking at the Spin shim, I believe what you do there is a custom executable that links trigger crates, is that right? If so, it doesn't matter whether your triggers come from the Spin repo or a plugin repo - there's no sidecar or side-loading, you're not spawning a separate executable, you're just statically linking a crate and adding an in-process case and .run() call to https://github.com/deislabs/containerd-wasm-shims/blob/add96b0095ab9cb818b527f16f408810c20757e0/containerd-shim-spin-v1/src/executor.rs#L84). I've linked custom Spin hosts against plugin triggers myself and am happy to share my experience and a sample. Or am I missing something about how the shim works?

All that said, we are really keen to have messaging triggers - we have wanted them since day one. And given the proposal for MQTT APIs, and that APIs can't yet be externalised, there's a symmetry to having the MQTT trigger side built in too.

But we don't yet have a clear vision of how to manage a wider range of triggers going forward. E.g. What are the criteria for being in core? (Stability, popularity, maintenance confidence, etc.) What is the process for choosing one thing to go in core vs another? (Should we also put the SQS and message broker triggers in core?) There is definitely a longer-term strategy issue (governance as well as technical strategy) that we need to discuss with the community.

But that doesn't help us reach a decision in the short term. I guess this is something we will need more people to weigh in on. The good news is that even if we do decline to accept this into core, converting it into a plugin should be super easy (and, as I say, would not require architectural changes / sidecars in the shim). So please don't take any of this as discouragement - it would be really great to have this! - more a heads up that the exact packaging is an area of uncertainty.

@suneetnangia
Copy link
Contributor Author

Appreciate the comments and insights @itowlson.
I/team intend to make significant use of Spin on the edge with various customers, along side our other upcoming edge services and preview cloud services (Event Grid MQTT). One of the reasons for selecting Spin was that it's a commercial product which customers can get support for if needed and have more confidence in.
The idea of the PR here was that I/community can do the majority of work to get MQTT trigger/sdk off the ground for our use cases and subsequently it's maintained/officially supported by Spin product group/org. Of course, I will continue to support/hotfix it, as it works in our favour, and given it's importance I can commit personally to that.

The potential points you mentioned re criteria for core triggers i.e. stability, popularity, maintenance confidence, etc. are key in our case, hence I'd be very keen to have this in core ☺️.

Re wasm-shim: you are absolutely correct we statically link the built-in triggers in the shim, I suppose we can do the same for untrusted/3rd party plugins i.e. it gets compiled into a single shim exe. I appreciate this is not run time side-loading, but it's more on the compile time side-loading of potentially untrusted code (3rd party plugins). This will just push the problem to wasm-shim as wasm-shim will need to validate/rubber-stamp it before including it in their implementation. Currently wasm-shim can include Redis and Http triggers with confidence that it's coming from Spin official repo, we want the same confidence for MQTT. On the edge and in IoT, Mqtt is considered as a foundational protocol, hence it warrants that level of support/confidence/quality which Spin core can provide.

One more point if I can convince you/others, in general, once/if we have Mqtt plugin, we/community will use it both in wasm-shim as well as loading it dynamically in Spin, this brings us back to dynamic side-loading angle again.
Thoughts?

@mikkelhegn
Copy link
Member

I too believe a plug-in is the best place to start for this PR. We (Fermyon Spin maintainers) haven't added new triggers to Spin core since it's initial creation, and I don't think we'll do that, now that we have a plug-in model. I think there's a bigger chance we will move the Redis trigger to a plug-in as well. This way we can enable everyone to package Spin with the type of triggers they want to support in their distribution or hosting (e.g., runwasi). E.g. we don't have Redis trigger support in Fermyon Cloud today. You'll probably see us move other things out of the core repo, e.g. SDKs is on the horizon for that, main reason is to have the opportunity to ship on individual cadences.

For the case of support, I believe that we should add relevant plug-ins to this page: https://developer.fermyon.com/spin/api-guides-overview to bring awareness that these plugins have some level of stablity. We use a three-tier definition of Experimental --> Stabilizing --> Stable. That and having then on Spin Up Hub are good entry point for discoverability. Over time, we can include things like download numbers and comments on Spin Up Hub, to help give guidance about adoption, which i think often helps with the concerns around a certain plug-in being legit for a given use-case.

@mikkelhegn
Copy link
Member

FYI @itowlson and a lot of other people are at conferences this week, so there might be a slight delay in commenting on this issue.

@itowlson
Copy link
Contributor

@Mossaka We chatted a bit about the MQTT proposal at WasmCon and you asked me to tag you in - I'd welcome your thoughts, as a shim expert, on the various approaches - thanks!

@suneetnangia
Copy link
Contributor Author

Reading further comments (@mikkelhegn thanks for providing more details/thoughts) on this, which addresses some of my initial concerns, I am updating my thoughts:

  1. A tiered support model covers concern around commercial use/support.
  2. Moving (or as long as we have a plan) Redis to plugin model covers the consistency re trigger implementations.
  3. Eventually, wasm based plugin will help alleviate the security concerns, I think there was a plan to have wasm based plugins.

@suneetnangia
Copy link
Contributor Author

Reading further comments (@mikkelhegn thanks for providing more details/thoughts) on this, which addresses some of my initial concerns, I am updating my thoughts:

  1. A tiered support model covers concern around commercial use/support.
  2. Moving (or as long as we have a plan) Redis to plugin model covers the consistency re trigger implementations.
  3. Eventually, wasm based plugin will help alleviate the security concerns, I think there was a plan to have wasm based plugins.

Clarifying Q/Thoughts: Are we going to leave sdks for Redis/Mqtt in core? There are going to be scenarios where trigger may be http and component would publish message on Redis/Mqtt so the trigger plugin will not be loaded, do we need to reflect this in core? We can possibly implement all messaging protocols/triggers in a single plugin but that does not sound right or reference other plugins (their core types) from each other to publish messages?
Would love to know more on this so what we will build aligns with wider/long term thinking.

@suneetnangia
Copy link
Contributor Author

Reading further comments (@mikkelhegn thanks for providing more details/thoughts) on this, which addresses some of my initial concerns, I am updating my thoughts:

  1. A tiered support model covers concern around commercial use/support.
  2. Moving (or as long as we have a plan) Redis to plugin model covers the consistency re trigger implementations.
  3. Eventually, wasm based plugin will help alleviate the security concerns, I think there was a plan to have wasm based plugins.

Clarifying Q/Thoughts: Are we going to leave sdks for Redis/Mqtt in core? There are going to be scenarios where trigger may be http and component would publish message on Redis/Mqtt so the trigger plugin will not be loaded, do we need to reflect this in core? We can possibly implement all messaging protocols/triggers in a single plugin but that does not sound right or reference other plugins (their core types) from each other to publish messages? Would love to know more on this so what we will build aligns with wider/long term thinking.

@mikkelhegn @itowlson?

@itowlson
Copy link
Contributor

itowlson commented Nov 6, 2023

@suneetnangia Whether the Redis trigger remains in core or moves to a plugin model is currently up in the air. I believe it will remain in core for compatibility reasons during Spin 2.x, but it is not part of the 2.0 "world" (see #1958).

I agree on having protocol-specific triggers.

Outbound is a trickier proposition because we can't currently implement APIs in plugins - this is likely to evolve as the Component Model and WASI Preview 2 pick up speed and APIs can be implemented as components (or via language libraries) rather than in a monolithic host, but that's a bit speculative right now. So there is no debate about moving MQTT publish to a plugin: if we want MQTT publish (and I imagine we do), then it needs to be in core.

SDKs are a slightly separate concern. Although the Rust and Go SDKs are in the Spin repo, partly for historical reasons and partly for convenience, they are not "part of core." However, as part of a MQTT publish API, I would expect that to be supported at minimum in the Rust SDK and ideally in the Go SDK as well. For JavaScript and Python it would depend on timing: I am not sure if we would do MQTT in js2wasm and py2wasm, but the long game is componentize-js and componentize-py which would acquire new APIs directly from the WITs.

My recommendation to "align with long term thinking" is:

  • MQTT trigger in a plugin, with a WIT-based API
  • MQTT publish as a host component in core, with a WIT-based API
  • Where types are shared, this can use the usual WIT package mechanism

(This may turn out to be "medium" term thinking rather than "long" but it's the best I can offer right now!)

Hope this answers your questions - let me know if you need any more info.

@itowlson
Copy link
Contributor

itowlson commented Nov 6, 2023

Re-reading your message you mention "trigger may be http and component would publish message on Redis/Mqtt so the trigger plugin will not be loaded" so I wonder if there is some confusion here.

Outbound APIs (such as publish APIs) are not provided by a trigger, and are completely unrelated to which trigger is "loaded." For example, the Redis trigger does not participate in any way in Redis API operations; the HTTP trigger does not participate in sending HTTP requests.

@squillace
Copy link

OK, I've had a long chat with @suneetnangia to understand what he wants to build for the customer, and it requires both listening (the trigger part) and also publishing (the ability to send). It's the latter that the plugin cannot currently do, and this is why Suneet is requesting to temporarily match the Redis integration but for only so long as the redis integration is there. Suneet commits to help move the mqtt over to whatever approach is best when redis moves out as well.

Doing so enables him to engage the customer directly with a great Spin solution that supports their needs with mqtt and wasi components.

I chatted with @mikkelhegn about this and he suggested he'd chat with you all about it, but also wanted confirmation that this PR follows the redis work exactly, and @suneetnangia assures me that it does (but will test it all and will update anything needed, as I see a few windows/mac failures).

What do you all think? I realize that taking this seems wrong as you're already thinking about how to handle redis going forward, so I do want to commit that Suneet will help move the work when that happens.

@itowlson
Copy link
Contributor

itowlson commented Dec 7, 2023

@squillace Publishing is not controversial. You are correct that this would need to be baked into the Spin framework. We are ready and willing to accept a PR for a MQTT publishing API (host component).

But listening and publishing are orthogonal. You don't need to have the trigger inside Spin to have the publishing code inside Spin. See #1717 (comment) and #1717 (comment).

So what we've been proposing is that the trigger (the listening) be delivered using the plugin model (a la SQS) and the publishing be delivered using the internal model (because, as you rightly say, that's the only way it can be). I'm not sure what the objection to this is. Would an in-person chat help to clarify the issue? I feel like I keep explaining the same thing so I am clearly not explaining it well...!

@squillace
Copy link

probably, ivan, because I'd missed that you were proposing to split the pr into core (publishing) and plugin (triggering). when reviewing, I had misread the following proposal:

MQTT trigger in a plugin, with a WIT-based API
MQTT publish as a host component in core, with a WIT-based API
Where types are shared, this can use the usual WIT package mechanism

which, as you say, does split the pr into those two halves. @suneetnangia, have a think about what needs to be done for that, will you?

@suneetnangia
Copy link
Contributor Author

Thanks for clarification, sounds good to me, there was a point where we were considering having this merged as-is to follow redis model but I'll find some time to split the PR into two as discussed and agreed above.

@itowlson
Copy link
Contributor

@suneetnangia @squillace What is the plan for this? Would it be helpful if I were to pick it up?

@melissaklein24
Copy link
Contributor

@suneetnangia I think this PR can be closed because we know have newer PRs that cover this work. Is that correct?

@itowlson
Copy link
Contributor

All the work here has now been ported to #2287 and https://github.com/spinkube/spin-mqtt-trigger-sdk/ - thank you once again!

@itowlson itowlson closed this Mar 14, 2024
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.

5 participants