Fix rabbitmq_binding idempotency when arguments are given (#160)

[janpieper]

SUMMARY

Fixes #160

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

community.rabbitmq.rabbitmq_binding

ADDITIONAL INFORMATION

This PR is a replacement for the 2 year old PR #161 as the original author @oneoneonepig did not respond to a question by @csmart for nearly a year and the bug is still not fixed, so I decided to create a new PR so we can get this bug fixed.

Copied from #161:

Tested locally by running the same task twice. The second run shows all test cases [ok]: .... See issue #160 to see what test cases were included.

[janpieper]

@csmart ping :wave:

[janpieper]

Some tests are failing, but I am not familar with python nor ansible-test :confused:

Tried to follow the "Local testing" section from the README, but it is failing because libssl1.1 is missing :man_shrugging:

ASK [setup_rabbitmq : Install RabbitMQ Erlang dependencies] *******************
fatal: [testhost]: FAILED! => {"changed": false, "msg": "No package matching 'libssl1.1' is available"}

[Andersson007]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 3 pipeline(s).

[Andersson007]

@janpieper hi, thanks for the PR!

• which command do you use to run the tests locally? try to run it with --docker ubuntu2204 -vvv. Also check out the quick-start dev guide: we should update README.
• if you click details near the failing checks and go deep, you'll see:

01:32 TASK [rabbitmq_binding : Check that binding succeeds with a change] ************
01:32 task path: /root/ansible_collections/community/rabbitmq/tests/output/.tmp/integration/rabbitmq_binding-xpuu4ikh-ÅÑŚÌβŁÈ/tests/integration/targets/rabbitmq_binding/tasks/tests.yml:26
01:32 fatal: [testhost]: FAILED! => {
01:32     "assertion": "add_binding.changed == true",
01:32     "changed": false,
01:32     "evaluated_to": false,
01:32     "msg": "Assertion failed"
01:32 }

it means the integration tests don't work as it was expected after the change any more. You need to investigate the failing task, the path of the file is tests/integration/targets/rabbitmq_binding/tasks/tests.yml

• If you have any questions, please ask

Thanks

[janpieper]

@Andersson007

Without my changes, the testsuite is green, but when applying my changes, I get the following error:

$ ansible-test integration rabbitmq_binding --docker ubuntu2204 --color --python 3.10 --start-at rabbitmq_binding -vvv

[...]

TASK [rabbitmq_binding : Remove binding] ***************************************
task path: /root/ansible_collections/community/rabbitmq/tests/output/.tmp/integration/rabbitmq_binding-rikys0dg-ÅÑŚÌβŁÈ/tests/integration/targets/rabbitmq_binding/tasks/tests.yml:77
Using module file /root/ansible_collections/community/rabbitmq/plugins/modules/rabbitmq_binding.py
Pipelining is enabled.
<testhost> ESTABLISH LOCAL CONNECTION FOR USER: root
<testhost> EXEC /bin/sh -c '/usr/bin/python3.10 && sleep 0'
fatal: [testhost]: FAILED! => {
    "changed": false,
    "details": "",
    "invocation": {
        "module_args": {
            "arguments": {},
            "ca_cert": null,
            "client_cert": null,
            "client_key": null,
            "destination": "queue-foo",
            "destination_type": "queue",
            "login_host": "localhost",
            "login_password": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER",
            "login_port": "15672",
            "login_protocol": "http",
            "login_user": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER",
            "name": "exchange-foo",
            "routing_key": "#",
            "source": "exchange-foo",
            "state": "absent",
            "type": "queue",
            "vhost": "/"
        }
    },
    "msg": "Unexpected reply from API",
    "status": 405
}

PLAY RECAP *********************************************************************
testhost                   : ok=26   changed=18   unreachable=0    failed=1    skipped=0    rescued=0    ignored=0

NOTICE: To resume at this test target, use the option: --start-at rabbitmq_binding
Cleaning up temporary python directory: /tmp/python-mlembj3c-ansible
FATAL: Command "ansible-playbook rabbitmq_binding-9v6zscid.yml -i inventory -vvv" returned exit status 2.
exit status 1

I am wondering what removing a binding has to do with my changes and why I get 405 Method Not Allowed from the API :thinking:

Edit: I think this is because self.url is changed by my changes :thinking:

[janpieper]

Okay, I just updated my changes to make the tests work again.

• Removed building url for RabbitMqBinding.create() - URL is the same as already in self.url
• Added building url for RabbitMqBinding.remove() - URL needs to contain self.props

I still need to add tests for the changed functionality.

[Andersson007]

@janpieper thanks for the update! Once it's ready for review, please ping csmart and myself explicitly in a comment here

[janpieper]

I tried to add tests for the changed behaviour but deleting the binding is a bit tricky :thinking:

The HTTP endpoint we need to call is: DELETE /api/bindings/<vhost>/e/<exchange>/q/<queue>/<props>

The propertiey key (<props>) is based on the routing key and/or the arguments:

Scenario	Routing Key	Arguments	Properties Key
1	N/A	N/A	~
2	example	N/A	example
3	N/A	{"x-match":"all"}	~3ujGfg
4	example	{"x-match":"all"}	example~3ujGfg

The current implementation only handles the routing key:

https://github.com/ansible-collections/community.rabbitmq/blob/b253557c8282b8971370c4a441d4dd82acae9de1/plugins/modules/rabbitmq_binding.py#L114

Scenarios 3 and 4 are problematic as we don't know the properties key. We could generate it based on the given routing key and/or the arguments.

Here's the implementation how it is done in RabbitMQ:

-spec args_hash(rabbit_framing:amqp_table()) -> binary().
args_hash([]) ->
    <<>>;
args_hash(Args)
  when is_list(Args) ->
    %% Args is already sorted.
    Bin = <<(erlang:phash2(Args, 1 bsl 32)):32>>,
    base64:encode(Bin, #{mode => urlsafe,
                         padding => false}).

(Source: deps/rabbit/src/rabbit_amqp_management.erl#L657-L665 @ rabbitmq/rabbitmq-server)

Example:

1> Args = [{<<"x-match">>, longstr, <<"all">>}].
[{<<"x-match">>,longstr,<<"all">>}]
2> Bin = <<(erlang:phash2(Args, 1 bsl 32)):32>>.
<<"ÞèÆ~">>
3> base64:encode(Bin, #{mode => urlsafe, padding => false}).
<<"3ujGfg">>

Is there any equivalent to erlang:phash2/2 in python?

[janpieper]

As far as I get it, erlang:phash2/2 is an erlang specific hashing function which heavily relies on the erlang data types. So, it sounds like there is no way to also have that function implemented in python :thinking:

The only thing I can currently think of as a workaround would be to read the binding from RabbitMQ, compare the routing key and the arguments to be the same and then use its properties key for deleting the binding.

Pseudo code:

self.api_result = self.request.get(self.url, auth=self.authentication, verify=self.verify, cert=(self.cert, self.key))

assert self.api_result.json()["routing_key"] == self.routing_key
assert self.api_result.json()["arguments"] == self.arguments

url = '{0}/{1}'.format(self.url, self.api_result.json()["properties_key"])
self.api_result = self.request.delete(url, auth=self.authentication, verify=self.verify, cert=(self.cert, self.key))

@Andersson007 @csmart WDYT?

[janpieper]

@Andersson007 @csmart ping :wink:

[janpieper]

No no, my question is not about merging. It won't work like it is at the moment.

My question was about reading the binding from RabbitMQ before deleting, to get the needed properties key.

[Andersson007]

@janpieper ah, thanks for clarifying, then i have no idea:) @csmart , any thoughts? if there's no response within a few days, you could decide yourself. please be available in case related issues appear after next release:) feel free to ping us to merge the PR whenever you feel it's ready

[janpieper]

@Andersson007 @csmart

I've updated the commit to get the properties key (from self.api_result) before removing the binding and added tests for bindings with arguments and/or a routing key.

In the past self.props was used instead of the properties_key but by changing the implementation self.props became unused, so I dropped it.

[csmart]

Thanks @janpieper ! Let me take a look tomorrow.