Support topic authorization #73

abompard · 2021-05-21T14:07:38Z

SUMMARY

Add support for topic authorizations in the user module

Topic authorization has been added in RabbitMQ 3.7.0.

Only the full topic permission list is supported for now, similar to the existing vhost permission list.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

rabbitmq_user

ADDITIONAL INFORMATION

I think there may be places for code refactoring in this change, but not as much as one would think since the format is not identical, and we still want different error messages between vhost authorization and topic authorization.

Andersson007 · 2021-07-16T13:04:49Z

I close-reopened to trigger CI

Andersson007 · 2021-07-16T13:38:52Z

could you please rebase the PR https://docs.ansible.com/ansible/latest/dev_guide/developing_rebasing.html#rebasing-your-branch

Andersson007 · 2021-07-21T08:39:16Z

@abompard ^

Andersson007

Thanks! Also please add a changelog fragment https://docs.ansible.com/ansible/latest/community/development_process.html#creating-a-changelog-fragment

Andersson007 · 2021-07-21T09:46:54Z

plugins/modules/rabbitmq_user.py

+# Add user to server and assign some topic permissions on / vhost.
+# The user doesn't have topic permission rules for other vhosts
+- community.rabbitmq.rabbitmq_user:


Suggested change

# Add user to server and assign some topic permissions on / vhost.

# The user doesn't have topic permission rules for other vhosts

- community.rabbitmq.rabbitmq_user:

- name: Add user to server and assign some topic permissions on / vhost, the user doesn't have topic permission rules for other vhosts

community.rabbitmq.rabbitmq_user:

Or something. - name: statement is required in EXAMPLES according to the development doc

Also there's the sanity error:

00:52 >>> Standard Output 00:52 plugins/modules/rabbitmq_user.py:143:-1: traceback: SyntaxError: invalid escape sequence \.

Andersson007 · 2021-07-21T09:47:43Z

plugins/modules/rabbitmq_user.py

+    description:
+      - A list of dicts, each dict contains vhost, exchange, read_priv and write_priv,
+        and represents a topic permission rule for that vhost.
+      - By default vhost is / and exchange is amq.topic.


Suggested change

- By default vhost is / and exchange is amq.topic.

- By default vhost is C(/) and exchange is C(amq.topic).

- Supported since RabbitMQ 3.7.0.

I'd also put a note here that the feature has been added in RabbitMQ 3.7.0.

or "supported since ..."

and that the option is ignored otherwise / the module will fail. Though I didn't look at the code so far, so I have no idea how it is implemented.

@abompard ^

plugins/modules/rabbitmq_user.py

cognifloyd · 2021-09-09T05:14:47Z

Tests are working on the main branch now. @abompard Could you rebase or merge in master?
I'd love to merge this soonish. I don't like leaving PRs dangling... 😃

[Topic authorization](https://www.rabbitmq.com/access-control.html#topic-authorisation) has been added in RabbitMQ 3.7.0. Only the full topic permission list is supported for now, similar to the existing vhost permission list. Signed-off-by: Aurélien Bompard <aurelien@bompard.org>

The default vhost is now `/` and the default exchange is `amq.topic`. Signed-off-by: Aurélien Bompard <aurelien@bompard.org>

abompard · 2021-09-09T07:41:39Z

Done, thanks!

abompard · 2021-09-09T07:42:09Z

Oh wait there were review suggestions, let me implement them first

Signed-off-by: Aurélien Bompard <aurelien@bompard.org>

abompard · 2021-09-09T08:00:26Z

@Andersson007 I've implemented your suggestions in a separate commit for ease of review, but I can squash it in the main commit if you prefer.

Signed-off-by: Aurélien Bompard <aurelien@bompard.org>

Andersson007 · 2021-09-09T08:18:05Z

@Andersson007 I've implemented your suggestions in a separate commit for ease of review, but I can squash it in the main commit if you prefer.

@abompard thanks for fixing! No need to squash, we'll do it when merging.
There are sanity issues:

01:28 ERROR: Found 2 pylint issue(s) which need to be resolved:
01:28 ERROR: plugins/modules/rabbitmq_user.py:484:11: use-a-generator: Use a generator instead 'any(vhost_count > 1 for vhost_count in count(vhosts).values())' (50%)
01:28 ERROR: plugins/modules/rabbitmq_user.py:503:11: use-a-generator: Use a generator instead 'any(ve_count > 1 for ve_count in count(vhost_exchanges).values())' (100%)

Please fix it a bit. I think you can try to remove the generator ([]) from any() statement

Andersson007 · 2021-09-09T08:23:15Z

also version_added should be 1.2.0 as we already released 1.1.0

Signed-off-by: Aurélien Bompard <aurelien@bompard.org>

Andersson007 · 2021-09-09T08:45:06Z

please ping us again when CI is green (or if you need help with it) and the PR is ready for review

abompard · 2021-09-09T08:51:24Z

It's green @Andersson007 ! :-)

changelogs/fragments/73-topic-authorization.yml

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

plugins/modules/rabbitmq_user.py

Andersson007

LGTM (general things like formatting, etc. as I'm not a specialist in the area)

Andersson007 · 2021-09-09T09:04:05Z

@cognifloyd if the PR is fine for you, feel free to merge of course.
@abompard , cognifloyd is a new maintainer of this collection!

Andersson007 · 2021-09-09T09:09:58Z

@abompard I created an issue for doc examples #95 and mention you there. Doc is a face of the collection where examples are especially important cause people usually scroll down right to them:)

Andersson007 · 2021-09-13T09:03:01Z

@abompard thanks for the contribution! @cognifloyd thanks for reviewing and merging!
(@abompard fyi @duttaANI is gonna fix the examples across the collection, thanks!)

Andersson007 closed this Jul 16, 2021

Andersson007 reopened this Jul 16, 2021

abompard force-pushed the topic-authz branch from 8f5c55d to 3e898ae Compare July 21, 2021 09:27

Andersson007 reviewed Jul 21, 2021

View reviewed changes

Andersson007 reviewed Jul 27, 2021

View reviewed changes

plugins/modules/rabbitmq_user.py Show resolved Hide resolved

abompard added 2 commits September 9, 2021 09:40

Set sane defaults for the topic_authorization dict

0d17953

The default vhost is now `/` and the default exchange is `amq.topic`. Signed-off-by: Aurélien Bompard <aurelien@bompard.org>

abompard force-pushed the topic-authz branch from 3e898ae to 0d17953 Compare September 9, 2021 07:41

Implement review suggestions from Andersson007

5a4f5a4

Signed-off-by: Aurélien Bompard <aurelien@bompard.org>

Fix indentation

d7da82a

Signed-off-by: Aurélien Bompard <aurelien@bompard.org>

Fix generators

a30aad4

Signed-off-by: Aurélien Bompard <aurelien@bompard.org>

Andersson007 reviewed Sep 9, 2021

View reviewed changes

changelogs/fragments/73-topic-authorization.yml Outdated Show resolved Hide resolved

Add the PR link in the fragment

3406c12

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

Andersson007 reviewed Sep 9, 2021

View reviewed changes

plugins/modules/rabbitmq_user.py Show resolved Hide resolved

Andersson007 approved these changes Sep 9, 2021

View reviewed changes

Andersson007 mentioned this pull request Sep 9, 2021

Improve examples formatting #95

Open

cognifloyd approved these changes Sep 11, 2021

View reviewed changes

cognifloyd merged commit e748054 into ansible-collections:main Sep 11, 2021

abompard deleted the topic-authz branch September 14, 2021 08:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support topic authorization #73

Support topic authorization #73

abompard commented May 21, 2021

Andersson007 commented Jul 16, 2021

Andersson007 commented Jul 16, 2021

Andersson007 commented Jul 21, 2021

Andersson007 left a comment

Andersson007 Jul 21, 2021

Andersson007 Jul 21, 2021 •

edited

Loading

Andersson007 Jul 21, 2021

Andersson007 Jul 21, 2021

Andersson007 Jul 21, 2021

Andersson007 Jul 27, 2021

cognifloyd commented Sep 9, 2021

abompard commented Sep 9, 2021

abompard commented Sep 9, 2021

abompard commented Sep 9, 2021

Andersson007 commented Sep 9, 2021

Andersson007 commented Sep 9, 2021

Andersson007 commented Sep 9, 2021

abompard commented Sep 9, 2021

Andersson007 left a comment •

edited

Loading

Andersson007 commented Sep 9, 2021

Andersson007 commented Sep 9, 2021

Andersson007 commented Sep 13, 2021

	- By default vhost is / and exchange is amq.topic.
	- By default vhost is C(/) and exchange is C(amq.topic).
	- Supported since RabbitMQ 3.7.0.

Support topic authorization #73

Support topic authorization #73

Conversation

abompard commented May 21, 2021

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

Andersson007 commented Jul 16, 2021

Andersson007 commented Jul 16, 2021

Andersson007 commented Jul 21, 2021

Andersson007 left a comment

Choose a reason for hiding this comment

Andersson007 Jul 21, 2021

Choose a reason for hiding this comment

Andersson007 Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

Andersson007 Jul 21, 2021

Choose a reason for hiding this comment

Andersson007 Jul 21, 2021

Choose a reason for hiding this comment

Andersson007 Jul 21, 2021

Choose a reason for hiding this comment

Andersson007 Jul 27, 2021

Choose a reason for hiding this comment

cognifloyd commented Sep 9, 2021

abompard commented Sep 9, 2021

abompard commented Sep 9, 2021

abompard commented Sep 9, 2021

Andersson007 commented Sep 9, 2021

Andersson007 commented Sep 9, 2021

Andersson007 commented Sep 9, 2021

abompard commented Sep 9, 2021

Andersson007 left a comment • edited Loading

Choose a reason for hiding this comment

Andersson007 commented Sep 9, 2021

Andersson007 commented Sep 9, 2021

Andersson007 commented Sep 13, 2021

Andersson007 Jul 21, 2021 •

edited

Loading

Andersson007 left a comment •

edited

Loading