Skip to content
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

Add some validation to peer discovery #11586

Closed
lukebakken opened this issue Jun 28, 2024 · 4 comments
Closed

Add some validation to peer discovery #11586

lukebakken opened this issue Jun 28, 2024 · 4 comments
Assignees
Milestone

Comments

@lukebakken
Copy link
Collaborator

lukebakken commented Jun 28, 2024

Is your feature request related to a problem? Please describe.

In this issue, I thought I had uncovered a bug in inter-node TLS and peer discovery, when in fact I had a simple configuration error in my environment:

https://github.com/lukebakken/docker-rabbitmq-cluster/blob/69791a36b664ed448b970ca0f2ead1742e4822b4/rmq1/rabbitmq.conf#L12-L14

Note that there is a duplicated node, and that node rmq0.local is not in that list. This error was only discovered after @dumbbell carefully looked at debug logs.

Describe the solution you'd like

RabbitMQ should log warnings, or perhaps, even fail to start when an invalid list of nodes is returned by a peer discovery backend:

Ret = Backend:list_nodes(),

In the case of the classic peer discovery backend, @dumbbell suggested that the following are invalid:

  • A list that does not contain the current node.
  • A list with duplicates

@michaelklishin should be able to verify if the above should apply to all backends (k8s, consul, AWS, etc).

@lukebakken lukebakken self-assigned this Jun 28, 2024
@michaelklishin
Copy link
Member

But what is an "invalid list of nodes"? Peer discovery by definition seeks to return a list of nodes that are not yet known to the current node.

@lukebakken
Copy link
Collaborator Author

Right, but, as I saw in #11534, if the list of nodes returned from a backend does not include the current node, it causes issues. Likewise, a list that contains duplicates is suspect as well.

@dcorbacho
Copy link
Contributor

dcorbacho commented Aug 5, 2024

#11898 adds some log warnings for classic config. It warns if the local node is missing and if there are duplicates in the list. We force the inclusion of the local node, so failing to start seems like a breaking change to me. This just makes easier to discover any issue just by inspecting the logs

@lukebakken
Copy link
Collaborator Author

Thanks @dcorbacho - resolved by #11898

@lukebakken lukebakken added this to the 3.13.7 milestone Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants