Fix binding arguments parsing and uniqueness in rabbitmq_binding provider

[J-Mx]

Pull Request (PR) description

See #1053 for a clear description of the issue. This PR fix arguments parsing and uniqueness while managing rabbitmq binding with puppet

This Pull Request (PR) fixes the following issues

Fixes #1053

[wyardley]

Thanks for this! Looks good overall. Have you tested this locally? Any corner cases with escaping or quoting or security risks? In the case where the hash changes, will that trigger any changes for existing users of the module that have existing bindings?

As with a lot of stuff in this module, the real fix would probably be to deprecate older versions, get some of the other needed changes across the line, and switch to either using JSON output from the CLI, or using the API directly, but this makes sense, I think.

Just thinking aloud, but I wonder if it would be feasible to JSON parse the entire arguments string vs. trying to use regex to parse it?

[bastelfreak]

Just thinking aloud, but I wonder if it would be feasible to JSON parse the entire arguments string vs. trying to use regex to parse it?

I think proper JSON parsing would make this so so much more readable.

[J-Mx]

Thank you for your review @wyardley. I have updated my branch to address your feedbacks.

Thanks for this! Looks good overall. Have you tested this locally? Any corner cases with escaping or quoting or security risks? In the case where the hash changes, will that trigger any changes for existing users of the module that have existing bindings?

There are no particular edge cases or potential security risks with these changes. Including 'arguments' in the binding hash name calculation prevents perpetual binding modifications. Previously, bindings with identical source, destination, vhost and routing_key but different arguments would generate the same hash, causing Puppet to continuously try to update them thinking they were the same binding. This change ensures that bindings with different arguments are correctly identified as distinct resources.

As with a lot of stuff in this module, the real fix would probably be to deprecate older versions, get some of the other needed changes across the line, and switch to either using JSON output from the CLI, or using the API directly, but this makes sense, I think.

As for parsing the arguments as JSON instead of using regex: it’s definitely possible, but it would require much more time and a significant refactor of the codebase compared to the fix proposed in this pull request. That said, it would likely help avoid these kinds of regex issues in the future.

Just thinking aloud, but I wonder if it would be feasible to JSON parse the entire arguments string vs. trying to use regex to parse it?

The format returned by rabbitmqctl is not parsable JSON code, but rather follows Erlang's tuple list syntax (e.g., [{"key","value"}]). This is why substitutions with regular expressions are present in the code to transform this Erlang format into a JSON-parsable format.

[wyardley]

Thanks for the contribution. Will try to cut a release soon.