Set sender/receiver capabilities to attach frame in the amqp10_client #11337

MarcialRosales · 2024-05-28T06:46:17Z

Proposed Changes

It partially addresses the feature request #10752. This PR lets a developer set the capabilities to the sender and receiver links in the amqp10_client. It will not add this capability to the shovel plugin though.

The system test group ibmmq requires a docker image for IBM MQ server. This docker image is built by a workflow added by this other PR #11419. The official IBM MQ docker image does not come with AMQP built-in capability hence we have to build one. And given the amount of time it takes to build this image it was better to build it from its own CI workflow.

NOTE for reviewer:

To run system test group ibmmq, it pulls a docker image based on amd64 arch. It does not take into account the local arch because the other possible architecture is arm64 and it is not possible to build a docker image for ARM64 with AMQP capabilities. (see https://github.com/rabbitmq/rabbitmq-server/blob/set-amqp10-capabilities/deps/amqp10_client/test/system_SUITE_data/ibmmq_runner#L11 for more context)

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

Bug fix (non-breaking change which fixes issue #NNNN)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause an observable behavior change in existing systems)
Documentation improvements (corrections, new content, etc)
Cosmetic change (whitespace, formatting, etc)
Build system and/or CI

ansd

I suggest that we first test end-to-end that the RabbitMQ <-> IBM MQ integration works before merging this PR.
Right now, this PR adds a lot of boilerplate code that is not used and contains bugs.

ansd · 2024-05-29T18:59:16Z

deps/amqp10_client/src/amqp10_client.erl

         attach_sender_link/3,
         attach_sender_link/4,
         attach_sender_link/5,
+         attach_sender_link/6,
         attach_sender_link_sync/3,
         attach_sender_link_sync/4,
         attach_sender_link_sync/5,
+         attach_sender_link_sync/6,
         attach_receiver_link/3,
         attach_receiver_link/4,
         attach_receiver_link/5,
         attach_receiver_link/6,
         attach_receiver_link/7,
+         attach_receiver_link/8,
         attach_link/2,


After this change the API of this client lib contains 15 different functions to attach a link.
This is poor API design. We should keep the API minimal. Instead of adding 3 new attach functions to the API, I suggest that applications can start to set capabilities by using attach_link/2.

I will look into using attach_link. When I was doing it I thought there could be another way of doing this like setting a structure like Attach_args and passing it . But the API already presented this pattern or design . I just followed it.

ansd · 2024-05-29T19:02:20Z

deps/amqp10_client/src/amqp10_client_session.erl

+translate_terminus_capabilities(none) ->
+    undefined;
+translate_terminus_capabilities(Capabilities) when is_binary(Capabilities) ->
+    {utf8, Capabilities};
+translate_terminus_capabilities(CapabilitiesList) when is_list(CapabilitiesList) ->
+    {list, [filter_capability(V) || V <- CapabilitiesList]}.
+
+filter_capability(V) when is_binary(V) ->
+    {utf8, V};
+filter_capability({T, _} = V) when is_atom(T) ->
+    %% looks like an already tagged type, just pass it through
+    V.
+


Wrong AMQP types are used:

Capabilities are symbols, not strings.

Multiple values of a composite type are represented by an array, not by a list.

It is encoded using array and symbol. See the tests https://github.com/rabbitmq/rabbitmq-server/blob/set-amqp10-capabilities/deps/amqp10_client/test/system_SUITE.erl#L935 where we send an array of symbols.

The API exposes or accepts Erlang types from users where it accepts binaries (strings) and list of binaries but the library is serliazing/deserlizing it as an array of symbols .. It is more or less how we treat Filters ...

It is true that multiple values are encoded using array rather than list.

ansd

I get sometimes the following error

system_SUITE > ibmmq > basic_roundtrip_ibmmq
    #1. {error,
            {shutdown,
                {gen_statem,call,
                    [<0.540.0>,
                     {attach,
                         #{name => <<"banana-sender">>,filter => #{},
                           properties => #{},rcv_settle_mode => first,
                           role =>
                               {sender,
                                   #{address => <<"DEV.QUEUE.3">>,
                                     capabilities => <<"queue">>,
                                     durable => unsettled_state}},
                           snd_settle_mode => settled}},
                     5000]}}}

when running

make -C deps/amqp10_client/ ct-system

locally on Ubuntu.
Might be worth checking this flake?

ansd · 2024-06-20T07:41:19Z

deps/amqp10_client/src/amqp10_client.erl

@@ -19,7 +19,7 @@
         end_session/1,
         attach_sender_link/3,
         attach_sender_link/4,
-         attach_sender_link/5,
+         attach_sender_link/5,        


Please remove all additional white spaces at the end of lines.

ansd · 2024-06-20T07:43:09Z

deps/amqp10_client/src/amqp10_client.erl

@@ -166,6 +166,8 @@ end_session(Pid) ->
 %% for the link before returning.
 attach_sender_link_sync(Session, Name, Target) ->
    attach_sender_link_sync(Session, Name, Target, mixed).
+-spec attach_sender_link_sync(pid(), binary(), binary()) ->
+    {ok, link_ref()} | link_timeout.


Put function spec before the function as done in other places.

ansd · 2024-06-20T08:18:00Z

deps/amqp10_client/src/amqp10_client_session.erl

@@ -109,6 +112,7 @@
 -export_type([snd_settle_mode/0,
              rcv_settle_mode/0,
              terminus_durability/0,
+              terminus_capabilities/0,


This export doesn't seem to be used outside. We should remove it.

ansd · 2024-06-20T08:23:54Z

deps/amqp10_client/src/amqp10_client_session.erl

+    {symbol, V};
+filter_capability({T, _} = V) when is_atom(T) ->
+    %% looks like an already tagged type, just pass it through
+    V.


This code was wrongly copied from filter_value_type/1.
A filter value has more permissive types.
For capabilities, only symbols are allowed.

It's best to only allow binaries in the list of capabilities provided by the client.
We then map it to an array of symbols here.

ansd · 2024-06-20T08:27:13Z

deps/amqp10_client/src/amqp10_client_session.erl

@@ -73,11 +73,14 @@
 -type rcv_settle_mode() :: first | second.

 -type terminus_durability() :: none | configuration | unsettled_state.
+-type terminus_capabilities() :: none | binary() | list().


none doesn't make sense.
Looks like this was wrongly copied from terminus_durability where none maps to 0 (see https://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-messaging-v1.0-os.html#type-terminus-durability).

If the client app doesn't want to set any capabilities, just don't set it in the target / source map.

Let's refine the list() type further to by of type [binary(),...].

nice refinement ! Thanks

ansd · 2024-06-20T09:03:30Z