rabbit_definitions: Import topic permissions after exchanges (backport #14407) #14409
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
[Why] Topic permissions depend on an exchange, in addition to a user and a vhost like other permissions. This fixes a bug where an exchange imported after a topic permission that depends on it caused the following crash when Khepri is used: {case_clause,{error,{khepri,mismatching_node, #{node_name => <<"exchange_name">>, node_props => #{payload_version => 1}, node_path => [rabbitmq,vhosts,<<"/">>,exchanges, <<"exchange_name">>], condition => {if_node_exists,false}, node_is_target => true}}}} The crash comes from the fact that the exchange code expect to either create the tree node in Khepri for that exchange, or there is an existing tree node holding an exchange tree node. Here, there was a tree node created implicitly when the topic permission was stored, but that tree node didn't have an exchange record (because the exchange was not imported yet). [How] We simply swap the import of topic permissions and exchanges. (cherry picked from commit 92c572e) …hepri [Why] When the module wanted to create an exchange in Khepri, it used `rabbit_khepri:create/2` which ensures that the tree node doesn't exist before. If the tree node exists, it expects it to contain an exchange record which is returned to the caller, indicating an exchange with that name already exists. However, there are several other resources stored under an exchange tree node, like bindings and topic permissions. In particular, during a definitions import, topic permissions used to be imported before exchanges. This caused a crash because the write of the topic permission automatically created a parent tree node for the exchange it dpends on, but without an exchange record obviously (see previous commit). [How] As an addition improvement to the previous commit, we change the conditions: instead of matching on the fact the tree node doesn't exist, the module also accepts that the tree node exists but has no payload. Under any of these conditions, the exchange is considered to be new and written to Khepri. (cherry picked from commit 5bcbfad)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Why
Topic permissions depend on an exchange, in addition to a user and a vhost like other permissions.
This fixes a bug where an exchange imported after a topic permission that depends on it caused the following crash when Khepri is used:
{case_clause,{error,{khepri,mismatching_node, #{node_name => <<"exchange_name">>, node_props => #{payload_version => 1}, node_path => [rabbitmq,vhosts,<<"/">>,exchanges, <<"exchange_name">>], condition => {if_node_exists,false}, node_is_target => true}}}}The crash comes from the fact that the exchange code expect to either create the tree node in Khepri for that exchange, or there is an existing tree node holding an exchange tree node. Here, there was a tree node created implicitly when the topic permission was stored, but that tree node didn't have an exchange record (because the exchange was not imported yet).
How
We simply swap the import of topic permissions and exchanges.
In addition to that, we also relax the conditions used to write a new exchange record in Khepri. This is not strictly required, but this makes the code more robust (see the second commit in the branch).
This is an automatic backport of pull request #14407 done by Mergify.