Azure / azure-sdk-for-rust

This repository is for active development of the *unofficial* Azure SDK for Rust. This repository is *not* supported by the Azure SDK team.
MIT License
702 stars 243 forks source link

need AMQP 1.0 crate for Service Bus #643

Closed cataggar closed 1 year ago

cataggar commented 2 years ago

There does not appear to be an AMQP 1.0 spec compliant crate. Crates like https://github.com/amqp-rs/lapin implement 0.9.1, but Service Bus requires 1.0. https://docs.microsoft.com/en-us/azure/service-bus-messaging/service-bus-amqp-overview

andrei-ionescu commented 2 years ago

What about Dove?

cataggar commented 2 years ago

There is also https://github.com/ntex-rs/ntex-amqp. Looks like there are contributors who work at Microsoft https://github.com/ntex-rs/ntex-amqp/issues/15.

minghuaw commented 2 years ago

I have been working on a tokio and serde based AMQP 1.0 implementation called fe2o3-amqp https://github.com/minghuaw/fe2o3-amqp , and I am planning to start working on the qpid integration test soon. It might be worth a try, and any feedback would be really appreciated.

minghuaw commented 2 years ago

How much of the AMQP 1.0 spec is needed for Azure SDK? I have currently implemented most of the core spec (both client and server) as well as the web socket binding (only the client), and I am currently debating which extension spec to implement next.

minghuaw commented 2 years ago

I have made two working examples showing how to use fe2o3-amqp with Azure Service Bus, one over TLS and the other one over WebSocket. Both have been tested with my Azure free trial.

  1. https://github.com/minghuaw/fe2o3-amqp/tree/main/examples/service_bus
  2. https://github.com/minghuaw/fe2o3-amqp/tree/main/examples/service_bus_over_websocket

I might start working on implementing the claim based security next. Would love to have some feedback :)

minghuaw commented 2 years ago

Here is another quick update. I have added some working examples of sending to and receiving from Azure Event Hubs.

The receiver example, however, seems like it's not moving the offset with an Accepted disposition.

minghuaw commented 2 years ago

I think I figured out the offset issue with the filter example https://github.com/minghuaw/fe2o3-amqp/blob/main/examples/event_hubs/src/bin/receiver_with_filter.rs

minghuaw commented 2 years ago

I am planning to work on an amqp client for servicebus and event hub. Is there any recommendations? Would mimicking the dotnet sdk be a good starting point?

cataggar commented 2 years ago

Sounds great @minghuaw! Yes, mimicking https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/servicebus/Azure.Messaging.ServiceBus and not the older https://github.com/Azure/azure-sdk-for-net/tree/main/sdk/servicebus/Microsoft.Azure.ServiceBus is the way to go.

minghuaw commented 2 years ago

A quick question on getter/setter convention.

Is there any preference on the getter/setter naming convention? The rust official naming convention guide (https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter) suggests xxx() and xxx_mut() (possibly set_xxx()) instead of get_xxx() and set_xxx().

rylev commented 2 years ago

Yes, I believe we want to follow the conventions laid out in that guide.

minghuaw commented 2 years ago

Is there any public documentation on how these servicebus-specific message annotations (https://docs.microsoft.com/en-us/azure/service-bus-messaging/service-bus-amqp-protocol-guide#message-annotations) are encoded in AMQP?

minghuaw commented 1 year ago

@cataggar Is there a public spec on how Service Bus session ID should be encoded? It seems like the current dotnet SDK (Azure.Messaging.ServiceBus) impl doesn't really comply with the AMQP 1.0 core spec.

The way it is implemented in Azure.Messaging.ServiceBus is like following (Line 681)

// even if supplied sessionId is null, we need to add the Session filter if it is a session receiver
if (isSessionReceiver)
{
    filters.Add(AmqpClientConstants.SessionFilterName, sessionId);
}

var linkSettings = new AmqpLinkSettings
{
    Source = new Source { Address = endpoint.AbsolutePath, FilterSet = filters }, // Line 690
    // ... other setting
};

where sessionId is just a string. The sessionId is added as one entry to the filter-set field of the receiver link's Source

However, regarding the filter-set type, the AMQP 1.0 core spec states that (http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-messaging-v1.0-os.html#type-filter-set)

A set of named filters. Every key in the map MUST be of type symbol, every value MUST be either null or of a described type which provides the archetype filter.

and string is not a described type. The entry may still be serialized as a described type, but I was unable to find a spec or code that does so.

For the rust implementation, this could be worked around by kinda hacking the upstream type definition (relax the value type from Described<Value> to just Value, https://github.com/minghuaw/fe2o3-amqp/blob/40c78b8e435edc9e69a5734508a55e8a6bc919a6/fe2o3-amqp-types/src/messaging/mod.rs#L61). I am debating whether to make that change in the upstream though.

minghuaw commented 1 year ago

The discussion about the session ID in Azure/azure-amqp can be found in this issue: Azure/azure-amqp#231

minghuaw commented 1 year ago

The discussion about the session ID in Azure/azure-amqp can be found in this issue: Azure/azure-amqp#231

The answer regarding session filter is copied from that issue just for reference:

<type name="com.microsoft:session-filter" class="restricted" source="string" provides="filter">
    <descriptor name="com.microsoft:session-filter" code="0x00000137:000000C"/>
</type>
minghuaw commented 1 year ago

@cataggar When is the planned next release date? I just want to see whether I would be able to have something before the next release.

I almost have a minimal viable impl for all APIs except for the ones related to processors. I haven't got the time to write enough tests either.

bmc-msft commented 1 year ago

We work towards a monthly release cadence.

minghuaw commented 1 year ago

I have a working partial AMQP 1.0 implementation that currently includes:

  1. ServiceBusClient
  2. ServiceBusSender
  3. ServiceBusReceiver
  4. ServiceBusSessionReceiver

What remains to be implemented include (in no particular order):

  1. Transaction
  2. ServiceBusRuleManager
  3. ServiceBusProcessor
  4. ServiceBusSessionProcessor
  5. Azure.Messaging.ServiceBus.Administration (this might need a separate crate or gated behind a feature).
  6. Diagnostics and metrics

These implemented APIs cover roughly the same "surface area" provided by the golang SDK (azservicebus). And the implementation should look very similar to that of the dotnet SDK.

I have only run some quick live tests with my own service bus namespace, so I definitely need to add more testing. However, this also raises a problem, suppose we need to run the live tests in GitHub Actions, is there a service bus namespace that can be used publicly? Or is there a way to set up the Actions to not leak the testing service bus namespace?

Another question is that whether this would be a good point (after adding more tests) to submit a PR and then start working on the remaining of the planned components? Or is it better to implement everything before submitting a PR?

Thanks in advance :)

mario-guerra commented 1 year ago

Hi @minghuaw, thanks for your work on this! I think I may be able to set up a service bus namespace you can use for testing, although it would be temporary. Would that suffice or are you looking for something permanent?

minghuaw commented 1 year ago

Thank you @mario-guerra . That would be great. A temporary one would suffice.

mario-guerra commented 1 year ago

Hi @minghuaw, can you confirm the email address listed in your profile is correct? If so, I will send you a connection string for an instance of Service Bus you can use for testing.

minghuaw commented 1 year ago

@mario-guerra Thanks. That one is correct (michael.wu1107@gmail.com)

mario-guerra commented 1 year ago

@minghuaw thanks for confirming, email with details has been sent.

mario-guerra commented 1 year ago

Hi @minghuaw, how is the testing going? Is there anything else you need?

minghuaw commented 1 year ago

Hi @mario-guerra, thanks for checking out. Sorry I was sick for the past few days.

The testing with the basic plan queue went fine, but one problem with the basic plan is that I couldn't test session-enabled queues, which I am still using my own service bus namespace for the testing.

I am currently working on the documentation and examples and will soon be ready to submit a PR.

mario-guerra commented 1 year ago

@minghuaw that's great! Thanks for the update, we look forward to seeing your PR.

minghuaw commented 1 year ago

Another quick update, the service still carries a legacy SessionFilter value in the responded Attach frame (Azure/azure-amqp#232). This would require supporting the legacy Filter in fe2o3-amqp-types, which is already implemented in a dev branch. I would just want to see whether the service plans to fix this or not, and this may introduce a few more days of delay for the PR.

mario-guerra commented 1 year ago

I've tagged the maintainer in the repo for comment, thanks for the heads up.

minghuaw commented 1 year ago

While waiting for updates on Azure/azure-amqp#232, I will just provide some updates to the current implementation. And I think there are a few design decisions that could be discussed here.

BTW, the current impl should be ready for PR (other than a few intra-doc-links that need to be fixed) whether or not they decide to fix that issue, but their decision is needed so that I can decide whether to publish a breaking update to the AMQP 1.0 dependencies

Summary of current implementation status

What key public APIs have been implemented:

  1. ServiceBusClient
  2. ServiceBusSender
  3. ServiceBusMessageBatch
  4. ServiceBusReceiver
  5. ServiceBusSessionReceiver
  6. ServiceBusRuleManager

What remains to be implemented:

  1. Transaction
  2. ServiceBusProcessor
  3. ServiceBusSessionProcessor
  4. ServiceBusAdministrationClient
  5. In general, reducing amount of .clone()

Some design decisions that could be discussed right now

  1. Use of generics in ServiceBusClient, ServiceBusSender, ServiceBusReceiver, ServiceBusSessionReceiver, ServiceBusRuleManager These types all have a inner field that is a generic type that implements a trait (TransportClient, TransportSender, TransportReceiver, TransportSessionReceiver, TransportRuleManager). This was originally just to follow what the dotnet sdk does. However, there is only one implementation of each of these traits, making the generic look kind of unnecessary. So the question is essentially
    • Should we change the generic to the concrete type that implements the corresponding trait?
  2. Whether AzureNamedkeyCredential and AzureSasCredential should be moved to azure_core crate?
  3. Current implementation implements a ServiceBusRetryPolicy trait that mimics what dotnet sdk does, and it is not exactly the same as azure_core::RetryPolicy. Plus, the current retry policy is a generic with trait bound on AmqpClient, AmqpSender, AmqpReceiver, AmqpSessionReceiver, and AmqpRuleManager. So the questions are:
    • Should we switch to azure_core::RetryPolicy?
    • Should we change retry policy to a trait object instead of a generic?
  4. The current method I use to test connection recovery is by manually turning off my network and then turn it back on, and the loss of network is verified with a ping on a separate terminal. Is there a better or automated way to test connection recovery?
mario-guerra commented 1 year ago

Hi @minghuaw, for the sake of moving this PR forward I believe it's safe for you to assume the service team will not change the legacy behavior you identified any time soon. Given that it would be a breaking change, the odds of it happening are low.

minghuaw commented 1 year ago

Thank you @mario-guerra . That sounds reasonable. I will try to get the PR ready later today or tomorrow then.

mario-guerra commented 1 year ago

@minghuaw that's great! I look forward to reviewing it.

minghuaw commented 1 year ago

I have submitted the PR #1185. I will be happy to discuss and provide any explanation or help for your reviews :)

mario-guerra commented 1 year ago

Great! I see Ryan and Cameron are already tagged as reviewers. I believe they are both out of office for the remainder of the year, so we will review the PR after the new year. Thanks again for your work on this!

minghuaw commented 1 year ago

This is kind of a general question regarding CI (more or less in the long run).

It looks like the current CI setup doesn't run integration test for wasm32-unknown-unknown target. I guess setting tests for wasm32-unknown-unknown itself is already tricky (especially because tokio "net" and "time" doesn't work in browser), but given that there seems to be a general goal of supporting wasm, would it be better to have some kind of strategy to run maybe a selected set of unit/integration tests?

mario-guerra commented 1 year ago

Hi @minghuaw, great question. I do think it makes sense to have some amount of testing for wasm but I'll defer to @cataggar on this one.

minghuaw commented 1 year ago

I feel like the current wasm ecosystem is quite fragmented given the various wasm targets (web, node, wasi, etc.). Is there a general scope of what wasm targets need to be supported?

cataggar commented 1 year ago

See @minghuaw's https://crates.io/crates/azservicebus More info in #1185