Skip to content

Support newer versions of RabbitMQ in check_password#56497

Closed
ScoreUnder wants to merge 7 commits intosaltstack:masterfrom
ScoreUnder:rabbitmq-version-detection
Closed

Support newer versions of RabbitMQ in check_password#56497
ScoreUnder wants to merge 7 commits intosaltstack:masterfrom
ScoreUnder:rabbitmq-version-detection

Conversation

@ScoreUnder
Copy link
Copy Markdown
Contributor

@ScoreUnder ScoreUnder commented Apr 2, 2020 •
edited
Loading

What does this PR do?

Adds support for newer versions of RabbitMQ. Not entirely sure as to the exact range of versions it adds support for.

What issues does this PR fix or reference?

#56258
Duplicates #56193 (This was originally written as a quick fix for our network without checking existing PRs, so it is somewhat unsurprising that someone else has the same PR ready)

Previous Behavior

  1. Support versions of rabbitmq which provide an ETS-form output from rabbitmqctl status
  2. Default to trying old approaches when version is unclear

New Behavior

  1. Also support newer versions of rabbitmq with human-readable rabbitmq status output
  2. Default to trying the newer approach when version is unclear

Tests written?

Yes

Commits signed with GPG?

Yes

@ScoreUnder
Fix RabbitMQ version detection for RMQ 3.8.2
d3d8046 
Sample `rabbitmqctl status` output:

```
Status of node rabbit@localhost ...
Runtime

OS PID: 26859
OS: Linux
Uptime (seconds): 291
RabbitMQ version: 3.8.2
Node name: rabbit@localhost
Erlang configuration: Erlang/OTP 22 [erts-10.6.4] [source] [64-bit] [smp:4:4] [ds:4:4:10] [async-threads:64] [hipe]
Erlang processes: 615 used, 1048576 limit
Scheduler run queue: 0
Cluster heartbeat timeout (net_ticktime): 60

[etc.]
```
@ScoreUnder
Pull version checking function out of check_password
03cdaf0 
Plus:
- reuse in list_policies
- default to newer approaches where version is unclear (because as time
  goes on, older approaches will be less likely to be correct)
@ScoreUnder
Add unit tests for rabbitmq module changes
35ef87d 
- Test with different (incompatible) "rabbitmqctl status" outputs, taken
  from real data
- Test that check_password changes method correctly based on detected
  version
@ScoreUnder ScoreUnder requested a review from a team as a code owner April 2, 2020 12:03
@ghost ghost requested a review from Akm0d April 2, 2020 12:03
@ScoreUnder ScoreUnder changed the title Support newer versions of RabbitMQ Support newer versions of RabbitMQ in check_password Apr 2, 2020
@Ch3LL Ch3LL removed the request for review from a team April 15, 2020 14:13
@Akm0d
Copy link
Copy Markdown
Contributor

Akm0d commented May 22, 2020

The failing tests are directly related to this PR

Traceback (most recent call last):
  File "/tmp/kitchen/testing/tests/unit/modules/test_rabbitmq.py", line 1065, in test__get_server_version_old
    self.assertListEqual(rabbitmq._get_server_version(), [3, 5, 7])
AssertionError: First sequence is not a list: None

@ScoreUnder
Copy link
Copy Markdown
Contributor Author

ScoreUnder commented May 23, 2020

Tests were broken from the merge (_get_server_version got mangled by unrelated changes) but now that #56193 has merged I think this PR is no longer needed so I will close it

@ScoreUnder ScoreUnder closed this May 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Akm0d Akm0d Awaiting requested review from Akm0d

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ScoreUnder @Akm0d