Topic permissions support #49
Conversation
RabbitMQ >= 3.7.0 added a new API endpoint.
ghost
added
|
@BarnabyShearer Thanks a lot for taking the time to recreate this PR 👍 ! I'll do the review as soon as possible. I've just created an issue on rabbit-hole for this v2 problem. Meanwhile, I think you can use the commit hash of the v2.0.0 (93d9988), I'll fix it when (if) rabbit-hole fixes this problem. Also do you think you could easily add a check for the RMQ version on read/creation method (or all actually) to display a proper error message if someone try to use it on RMQ < 3.7 ? I think you can use |
| _, newPerms := d.GetChange("permissions") | ||
|
|
||
| newPermsList := newPerms.([]interface{}) | ||
| for _, exchange := range newPermsList { |
There was a problem hiding this comment.
Unfortunately update is a bit more complex than that...
To highlight that, you just have to change the testAccTopicPermissionsConfig_update constant in the tests by adding a second permissions in the same resource:
const testAccTopicPermissionsConfig_update = `
resource "rabbitmq_vhost" "test" {
name = "test"
}
resource "rabbitmq_user" "test" {
name = "mctest"
password = "foobar"
tags = ["administrator"]
}
resource "rabbitmq_topic_permissions" "test" {
user = "${rabbitmq_user.test.name}"
vhost = "${rabbitmq_vhost.test.name}"
permissions {
exchange = ".*"
write = ".*"
read = ""
}
permissions {
exchange = "test"
write = ".*"
read = ".*"
}
}`Tests will fail.
The way you did it does not manage:
- Delete the removed permissions block
GetTopicPermissionsIndoes not seem to return permissions in the same order you created them (need to be checked), so there's alway a diff andterraform applywill try to update them endlessly.
If you keep permissions in this format, the first one is not so hard to fix, the second one a bit more complex. I'm not sure how to do that actually 🤔 but maybe you'll have an idea... You'll probably need, in the read function, to sort permissions in the same way that they exist in the state.
Another solution (but maybe less cool for the end user usage), is to create one resource by user/vhost/exchange. So the resource definition will look like (in a simplified view):
"user": String, ForceNew: true
"vhost": String, ForceNew: true
"exchange": String, ForceNew: true
"permissions": List, MaxItems: 1, {
"read": String
"write": Srtring
},
There was a problem hiding this comment.
I will have a 🤔
There was a problem hiding this comment.
Switching to schema.TypeSet for permissions; which as well as being correct; fixes issues where the terraform order and RabbitMQ order differ.
Rabbit-hole currently only supports deleting all permissions for a vhost/user. So to do an update we delete all, then recreate the existing and new ones. This is not ideal as it could lead to removing permissions if you try and add an invalid one.
There was a problem hiding this comment.
Opened a PR upstream for just deleting the removed permissions: michaelklishin/rabbit-hole#147
There was a problem hiding this comment.
Opened a PR upstream for just deleting the removed permissions: michaelklishin/rabbit-hole#147
Thanks a lot 👏 💪 , I was about to do the same.
This is not ideal as it could lead to removing permissions if you try and add an invalid one.
Indeed!
Using the git sha until upstream versioning is resolved.
Switch to `schema.TypeSet` for `permissions`; which as well as being correct; fixes issues where the terraform order and RabbitMQ order differ. Rabbit-hole currently only supports deleting all permissions for a vhost/user. So to do an update we delete all, then recreate the existing and new ones. This is not ideal as it could lead to removing permisions if you try and add an invalid one.
|
Nice that we have this now in master. @cyrilgdn, what do you think when we can expect this in an official release? Can we do anything to support you? |
|
@amir-hadi I'll ask for a new release this week (I already said that last week in other PRs though 😅 but there's one or two other PRs I want to include and it should be good this week) |
|
Lovely, thank you very much @cyrilgdn. |
|
@BarnabyShearer @amir-hadi FYI, v1.3.0 has been released today! |
|
Lovely, I'll use it this week :-)! Thank you so much for your effort. |
@cyrilgdn thank you very much for taking on the maintainership and showing renewed interest in PR #27. The wasn't a
go.modwhen I opened the PR ;-) I am afraid I had removed my fork so AFAIK I can't update the old PR. Instead I have opened this PR based on master.rabbit-holestill needs a bump to gain topic permissions API. I tried bumping to v2.0.0 but either I am not understanding the newgo modrules; or they are not fully complying with them. The v1.6.0-rc.1 seems to be reliable though.