Shovel: convert AMQP 1.0 props and app props to AMQP 0.9.1 props and headers #10022

luos · 2023-12-01T14:00:48Z

Hi,

We've implemented Application Property conversion to AMQP 0.9.1 headers for shovel. We wanted this to work on 3.12 and we were not sure if this is appropriate for these message container changes.

Let us know what you think.

Timestamps are milliseconds in AMQP 1.0, but in AMQP 0.9.1 it is seconds. Fixed by multiplying the timestamp by 1 000.
Shovel crashed if user_id was set in the message because the encoding was as utf8 while it should be a byte array.
Negative integers were encoded as integers - therefore leading to incorrect positive values.
Float values were not supported by the client.
Fixed priority header encoding in AMQP 1.0. It was set as uint but it should be ubyte.
Priority of the message is now in the Headers instead of Application Properties. This is potentially a breaking change.

GitHub issue: #7508.

A v3.12.x version of this PR: #10037.

Proposed Changes

Implement conversion of Application Properties to Headers.

Configurable by using the following key:

shovel.convert_amqp10_props_to_amqp091 = true

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

Checklist

Put an x in the boxes that apply.
You can also fill these out after creating the PR.
If you're unsure about any of them, don't hesitate to ask on the mailing list.
We're here to help!
This is simply a reminder of what we are going to look for before merging your code.

I have read the CONTRIBUTING.md document
I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
I have added tests that prove my fix is effective or that my feature works
All tests pass locally with my changes
If relevant, I have added necessary documentation to https://github.com/rabbitmq/rabbitmq-website
If relevant, I have added this change to the first version(s) in release-notes that I expect to introduce it

michaelklishin · 2023-12-01T14:33:29Z

Message containers were introduced in part to make this kind of conversions easier to pull off.

michaelklishin · 2023-12-01T15:05:57Z

@luos why was the priority value moved to a header in AMQP 1.0? I doubt we can ship this in 3.12.x because of this change.

luos · 2023-12-01T15:10:04Z

Hi,

I think we can definitely remove the priority change, I changed this because there is already a place for it in the AMQP 1.0 Headers and it gets placed into Application Properties. This is not critical for us.

Regarding the MC, I'd be happy to make this more MC compatible, though I am quite unsure how as shovels are a level above that if I understand it correctly, or if somehow we can make this in a way that in the future it can be easily changed to use MC?

michaelklishin · 2023-12-01T15:11:29Z

Ah, so it is a header in AMQP 1.0 (section 3.2.1). Then I guess it should be acceptable.

michaelklishin · 2023-12-01T15:12:34Z

@luos for Shovels specifically this can remain where it is. Larger changes can come in Shovel v2 for which we have a lot of other internal changes.

michaelklishin · 2023-12-01T16:43:18Z

@luos we can drop the configuration key and treat this as a bug fix for Shovels with AMQP 1.0 destinations.

luos · 2023-12-04T14:23:03Z

We prepared the backporting here: #10037

We will work on the MC version at the end of this week.

- Timestamps are milliseconds in AMQP 1.0, but in AMQP 0.9.1 it is seconds. Fixed by multiplying the timestamp by 1 000. - Shovel crashed if user_id was set in the message because the encoding was as utf8 while it should be a byte array. - Negative integers were encoded as integers - therefore leading to incorrect positive values. - Float values were not supported by the client. - Fixed priority header encoding in AMQP 1.0. It was set as uint but it should be ubyte. - Priority of the message is now in the Headers instead of Application Properties. This is potentially a breaking change. Fixes: rabbitmq#7508

luos · 2023-12-12T12:16:40Z

Hi @michaelklishin ,

We've started looking at the conversion with MC. We are planning to remove the Props and Payload from the rabbit_shovel_behaviour:forward function, and replace it with an mc "object".

We see that there is a somewhat related PR already which contains some AMQP 1.0 MC conversion functions, we may depend on these changes. What would be your preference, should we base our changes on that PR (#9022) by @ansd ?

https://github.com/rabbitmq/rabbitmq-server/pull/9022/files#diff-fb45468cd5ac6820b904a3e908da68af368f0e70126a7df503efedc940e0d03b

michaelklishin · 2023-12-12T14:05:43Z

@luos #9022 is vast in scope and while you can coordinate your changes with it, it is something for 4.0.

- remove old code converinting message properties to map and any related conversion function - use mc to hold the message - implement some helper functions to convert amqp10 to mc - use some reasonable default for AMQP1.0 exchange and routing keys as they are mandatory for mc to work - the differences in behaviour are the following compared to 3.12.x - default priority is undefined instead of 4 - some long tagged amqp values are now signedint / unsignedint

luos

Hi @michaelklishin , we've updated the changes already merged into 3.12.x with the conversion to mc. Let us know what you think.

luos · 2023-12-14T13:06:32Z

deps/rabbit/src/mc_amqp.erl

@@ -63,6 +63,10 @@
 init(Sections) when is_list(Sections) ->
    Msg = decode(Sections, #msg{}),
    init(Msg);
+init(Amqp10Msg) when element(1, Amqp10Msg) =:= amqp10_msg ->
+    [#'v1_0.transfer'{}| AmqpRecords] = amqp10_msg:to_amqp_records(Amqp10Msg),


This conversions seems a little bit hacky, though we could not come up with a better way.

By adding the v1_0.transfer record match at the beginning of the list, it effectively drops that list item.

This introduce an implicit dependency on the AMQP 1.0 client to RabbitMQ core. We plan on making AMQP 1.0 support a core feature later this year (after #9022 is merged) but we cannot accept a dependency on an AMQP 1.0 client.

Just like at some point rabbit_core had to be introduced to avoid a dependency on amqp_client for AMQP 0-9-1.

luos · 2023-12-14T13:07:39Z

deps/rabbitmq_shovel/src/rabbit_amqp10_shovel.erl

-    case send_msg(Link, Msg) of
+    MsgCont = mc:convert(mc_amqp, InMsg),
+    AmqpMessage = mc:protocol_state(MsgCont),
+    NewMsg = amqp10_msg:from_amqp_records([#'v1_0.transfer'{} | AmqpMessage]),


This is the other side of that conversion, we have to prepend the v1_0.transfer record to have a valid amqp10_msg.

luos · 2023-12-14T13:09:04Z

deps/rabbitmq_shovel/src/rabbit_amqp10_shovel.erl

+        % these are required values, however I don't think they make sense for AMQP 1.0
+        exchange => <<>>,
+        routing_keys => [<<>>]
+    }),


exchange and routing_keys seem to be required by the mc implementation. If it is not set, then some crashes can be expected.

luos · 2023-12-14T13:09:46Z

deps/rabbitmq_shovel/test/amqp10_dynamic_SUITE.erl

@@ -168,6 +170,169 @@ simple_amqp10_src(Config) ->
              ok
      end).

+message_prop_conversion(Config) ->


These are the same tests as on 3.12.x. With minimal changes, they produce the same output.

luos · 2023-12-14T13:27:28Z

deps/rabbitmq_shovel/src/rabbit_amqp091_shovel.erl

-    rabbit_shovel_behaviour:forward(Tag, Props, Payload, State);
+    Anns = #{exchange => Exchange, routing_keys => [RoutingKey]},
+    Content = rabbit_basic:build_content(Props0, [Payload]),
+    Mc = mc:init(mc_amqpl, Content, Anns),


This conversion as well looks a bit hacky. Let us know if you already have some better way to implement these.

michaelklishin · 2023-12-14T23:40:43Z

@kjnilsson @ansd this must be the first use of mc outside of core and outside of the core team. WDYT? Should Shovels simply do what they do in v3.12.x, at least for now?

michaelklishin

We have decided to postpone this until after 3.13 ships and then #9022 is merged.

michaelklishin · 2024-01-18T21:19:40Z

deps/rabbit/src/mc_amqp.erl

@@ -63,6 +63,10 @@
 init(Sections) when is_list(Sections) ->
    Msg = decode(Sections, #msg{}),
    init(Msg);
+init(Amqp10Msg) when element(1, Amqp10Msg) =:= amqp10_msg ->
+    [#'v1_0.transfer'{}| AmqpRecords] = amqp10_msg:to_amqp_records(Amqp10Msg),


This introduce an implicit dependency on the AMQP 1.0 client to RabbitMQ core. We plan on making AMQP 1.0 support a core feature later this year (after #9022 is merged) but we cannot accept a dependency on an AMQP 1.0 client.

Just like at some point rabbit_core had to be introduced to avoid a dependency on amqp_client for AMQP 0-9-1.

mergify bot added the bazel label Dec 1, 2023

michaelklishin added the backport-v3.12.x label Dec 1, 2023

luos mentioned this pull request Dec 4, 2023

Convert AMQP 1.0 props and app props to AMQP 0.9.1 props and headers #10037

Merged

12 tasks

michaelklishin changed the title ~~Convert AMQP 1.0 props and app props to AMQP 0.9.1 props and headers~~ main (3.13): convert AMQP 1.0 props and app props to AMQP 0.9.1 props and headers Dec 5, 2023

michaelklishin mentioned this pull request Dec 5, 2023

Sync AMQP 1.0 Application Properties and AMQP 0.9.1 Properties/Headers in RabbitMQ Shovel #7508

Closed

luos force-pushed the shovel-amqp10-to-amqp091-prop-conversion branch from 2c5b2ff to 2695dd9 Compare December 14, 2023 13:05

luos commented Dec 14, 2023

View reviewed changes

michaelklishin removed the backport-v3.12.x label Jan 18, 2024

michaelklishin changed the title ~~main (3.13): convert AMQP 1.0 props and app props to AMQP 0.9.1 props and headers~~ Shovel: convert AMQP 1.0 props and app props to AMQP 0.9.1 props and headers Jan 18, 2024

michaelklishin reviewed Jan 18, 2024

View reviewed changes

This was referenced Jul 16, 2024

Convert AMQP 1.0 props and app props to AMQP 0.9.1 props and headers (backport #10037) #11728

Merged

Convert AMQP 1.0 props and app props to AMQP 0.9.1 props and headers (backport #10037) #11729

Open

michaelklishin closed this Jul 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shovel: convert AMQP 1.0 props and app props to AMQP 0.9.1 props and headers #10022

Shovel: convert AMQP 1.0 props and app props to AMQP 0.9.1 props and headers #10022

luos commented Dec 1, 2023 •

edited by michaelklishin

Loading

michaelklishin commented Dec 1, 2023

michaelklishin commented Dec 1, 2023

luos commented Dec 1, 2023

michaelklishin commented Dec 1, 2023

michaelklishin commented Dec 1, 2023

michaelklishin commented Dec 1, 2023

luos commented Dec 4, 2023

luos commented Dec 12, 2023

michaelklishin commented Dec 12, 2023

luos left a comment

luos Dec 14, 2023

michaelklishin Jan 18, 2024

luos Dec 14, 2023

luos Dec 14, 2023

luos Dec 14, 2023

luos Dec 14, 2023

michaelklishin commented Dec 14, 2023

michaelklishin left a comment

michaelklishin Jan 18, 2024

Shovel: convert AMQP 1.0 props and app props to AMQP 0.9.1 props and headers #10022

Shovel: convert AMQP 1.0 props and app props to AMQP 0.9.1 props and headers #10022

Conversation

luos commented Dec 1, 2023 • edited by michaelklishin Loading

Proposed Changes

Types of Changes

Checklist

michaelklishin commented Dec 1, 2023

michaelklishin commented Dec 1, 2023

luos commented Dec 1, 2023

michaelklishin commented Dec 1, 2023

michaelklishin commented Dec 1, 2023

michaelklishin commented Dec 1, 2023

luos commented Dec 4, 2023

luos commented Dec 12, 2023

michaelklishin commented Dec 12, 2023

luos left a comment

Choose a reason for hiding this comment

luos Dec 14, 2023

Choose a reason for hiding this comment

michaelklishin Jan 18, 2024

Choose a reason for hiding this comment

luos Dec 14, 2023

Choose a reason for hiding this comment

luos Dec 14, 2023

Choose a reason for hiding this comment

luos Dec 14, 2023

Choose a reason for hiding this comment

luos Dec 14, 2023

Choose a reason for hiding this comment

michaelklishin commented Dec 14, 2023

michaelklishin left a comment

Choose a reason for hiding this comment

michaelklishin Jan 18, 2024

Choose a reason for hiding this comment

luos commented Dec 1, 2023 •

edited by michaelklishin

Loading