Skip to content

Fix amqp091->amqp10 shovel with complex headers #13798

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

Merged
merged 1 commit into from
Apr 25, 2025

Conversation

mkuratczyk
Copy link
Contributor

Some AMQP 0.9.1 headers, in particular x-death headers, cannot be set as application properties. Before this change, trying to shovel dead-lettered messages from an AMQP 0.9.1 source to AMQP 1.0 destination would fail with:

reason: {badarg,
            [{unicode,characters_to_binary,
                 [[{table,
                       [{<<"count">>,long,1},
                        {<<"reason">>,longstr,<<"maxlen">>},
                        {<<"queue">>,longstr,<<"tmp">>},
                        {<<"time">>,timestamp,1745575728},
                        {<<"exchange">>,longstr,<<>>},
                        {<<"routing-keys">>,array,
                         [{longstr,<<"tmp">>}]}]}]],
                 [{file,"unicode.erl"},
                  {line,1219},
                  {error_info,#{module => erl_stdlib_errors}}]},
             {amqp10_client_types,utf8,1,
                 [{file,"amqp10_client_types.erl"},{line,99}]},
             {amqp10_msg,'-set_application_properties/2-fun-0-',3,
                 [{file,"amqp10_msg.erl"},{line,385}]},
             {maps,fold_1,4,[{file,"maps.erl"},{line,860}]},
             {amqp10_msg,set_application_properties,2,
                 [{file,"amqp10_msg.erl"},{line,384}]},
             {maps,fold_1,4,[{file,"maps.erl"},{line,860}]},
             {rabbit_amqp10_shovel,forward,4,
                 [{file,"rabbit_amqp10_shovel.erl"},{line,337}]},
             {rabbit_shovel_worker,handle_info,2,
                 [{file,"rabbit_shovel_worker.erl"},{line,104}]}]}

Fixes #13797

@mergify mergify bot added the make label Apr 25, 2025
application properties. Before this change, trying to shovel
dead-lettered messages from an AMQP 0.9.1 source to AMQP 1.0 destination
would fail with:
```
reason: {badarg,
            [{unicode,characters_to_binary,
                 [[{table,
                       [{<<"count">>,long,1},
                        {<<"reason">>,longstr,<<"maxlen">>},
                        {<<"queue">>,longstr,<<"tmp">>},
                        {<<"time">>,timestamp,1745575728},
                        {<<"exchange">>,longstr,<<>>},
                        {<<"routing-keys">>,array,
                         [{longstr,<<"tmp">>}]}]}]],
                 [{file,"unicode.erl"},
                  {line,1219},
                  {error_info,#{module => erl_stdlib_errors}}]},
             {amqp10_client_types,utf8,1,
                 [{file,"amqp10_client_types.erl"},{line,99}]},
             {amqp10_msg,'-set_application_properties/2-fun-0-',3,
                 [{file,"amqp10_msg.erl"},{line,385}]},
             {maps,fold_1,4,[{file,"maps.erl"},{line,860}]},
             {amqp10_msg,set_application_properties,2,
                 [{file,"amqp10_msg.erl"},{line,384}]},
             {maps,fold_1,4,[{file,"maps.erl"},{line,860}]},
             {rabbit_amqp10_shovel,forward,4,
                 [{file,"rabbit_amqp10_shovel.erl"},{line,337}]},
             {rabbit_shovel_worker,handle_info,2,
                 [{file,"rabbit_shovel_worker.erl"},{line,104}]}]}
```
@mkuratczyk mkuratczyk force-pushed the amqp091-to-amqp10-shovel-bug branch from c4bf535 to c5271ea Compare April 25, 2025 13:47
@mkuratczyk mkuratczyk marked this pull request as ready for review April 25, 2025 15:11
@michaelklishin
Copy link
Collaborator

@mkuratczyk @ansd should we backport this to patch releases? It seems to be a safe change and a genuine bug fix to me.

@michaelklishin michaelklishin added this to the 4.2.0 milestone Apr 25, 2025
@kjnilsson
Copy link
Contributor

@mkuratczyk @ansd should we backport this to patch releases? It seems to be a safe change and a genuine bug fix to me.

I think it is fine to backport this yes.

@kjnilsson kjnilsson self-requested a review April 25, 2025 15:26
Copy link
Contributor

@kjnilsson kjnilsson left a comment

Choose a reason for hiding this comment

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

This change is fine for now until we can make use of the proper conversions in mc

@michaelklishin michaelklishin merged commit 1806a45 into main Apr 25, 2025
271 checks passed
@michaelklishin michaelklishin deleted the amqp091-to-amqp10-shovel-bug branch April 25, 2025 17:19
michaelklishin added a commit that referenced this pull request Apr 25, 2025
Fix amqp091->amqp10 shovel with complex headers (backport #13798)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shovel: cannot convert dead-lettering metadata from AMQP 0.9.1 to 1.0
4 participants