Merge lp:~pjdc/charms/trusty/rabbitmq-server/no-more-guest-user into lp:charms/trusty/rabbitmq-server

[Paul Collins]

rabbitmq creates a guest account by default, and in versions < 3.3.0, the guest account is usable over the network. This branch adds a config item to control the guest account, defaulting to off, i.e. "delete it".

Note that although check_rabbitmq.py defaults to the guest account, the charm creates and configures custom accounts and vhosts for monitoring purposes.

Resubmit:
 - don't try to manage the account until packages are installed
 - try to avoid collisions in the clustered case

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #12216 rabbitmq-server for pjdc mp276614
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/12216/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #7718 rabbitmq-server for pjdc mp276614
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
make: *** [functional_test] Error 1
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/13107588/
Build: http://10.245.162.77:8080/job/charm_amulet_test/7718/

[Ryan Beisner (1chb1n)]

Here is the bit from the rmq/0 juju unit log where the guest account config failed. Also including the juju stat output for that deploy:

http://paste.ubuntu.com/13112600/

review: Needs Fixing

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #13162 rabbitmq-server for pjdc mp276831
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/13162/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #12314 rabbitmq-server for pjdc mp276831
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/12314/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #7768 rabbitmq-server for pjdc mp276831
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/7768/

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-aws/1407/

review: Needs Fixing (automated testing)

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #8228 rabbitmq-server for pjdc mp276831
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
Timeout occurred (2700s), printing juju status...environment: osci-sv08
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/13871636/
Build: http://10.245.162.77:8080/job/charm_amulet_test/8228/

[Ryan Beisner (1chb1n)]

FYI, test #8228 failed due to unrelated undercloud issue, safe to disregard that result. Re-triggering that test...

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #8230 rabbitmq-server for pjdc mp276831
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
make: *** [functional_test] Error 1
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/13877848/
Build: http://10.245.162.77:8080/job/charm_amulet_test/8230/

[Ryan Beisner (1chb1n)]

FYI, test #8230 failed due to unrelated undercloud metadata issue, safe to disregard that result.

Undercloud metadata issue seems to be resolved, retriggering test again.

[Ryan Beisner (1chb1n)]

FYI:

Given:
A recent change in Juju unit numbering behavior,
And: https://github.com/juju/amulet/issues/111,
And: Juju charm ci recycles a single bootstrap enviro across all test iterations in that charm:

The rmq amulet test will not pass in charm ci, but should be expected to pass in uosci since each test there gets a fresh bootstrap, and units will always start at zero.

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #8243 rabbitmq-server for pjdc mp276831
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
make: *** [functional_test] Error 1
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/13903899/
Build: http://10.245.162.77:8080/job/charm_amulet_test/8243/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #8246 rabbitmq-server for pjdc mp276831
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
make: *** [functional_test] Error 1
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/13946009/
Build: http://10.245.162.77:8080/job/charm_amulet_test/8246/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #8298 rabbitmq-server for pjdc mp276831
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
make: *** [functional_test] Error 124
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/14025794/
Build: http://10.245.162.77:8080/job/charm_amulet_test/8298/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #8303 rabbitmq-server for pjdc mp276831
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/8303/

[Ryan Beisner (1chb1n)]

Thanks again for your work on this, and thank you for your patience while we encountered system issued during testing. With the infrastructure issues resolved, the amulet tests for this proposal are now passing.

I've discussed the proposed changes with others on the OpenStack Engineering team, and, while this is a change in default behavior, it is viewed as an valuable improvement with no impact on Ubuntu OpenStack deployments.

This is a +1 from the OpenStack perspective. I am requesting a co-review from a peer in the Juju Ecosystem team because there is a possibility that non-OpenStack use cases are out there which relied on the previous default behavior (guest user enabled), regardless of whether or not that is good practice.

As a reminder, the review-queue (vapour) tests are failing, unrelated to the changes proposed by pjfc. https://github.com/juju/amulet/issues/111

review: Approve

[Ryan Beisner (1chb1n)]

It occurs to me that we should really have test coverage for the added feature. Unit and Amulet tests should accompany the proposed changes.

review: Needs Information

[Ryan Beisner (1chb1n)]

* Need to add unit + amulet test coverage for the new/changed features.

* This proposed change should be targeted to the "next" (development) branch of the rabbitmq-server charm [1], per the development norms [2]

* Upon further review, and even though these changes work well with the current Juju version (1.25.0), this will likely break deployments on older versions Juju in that:

--It is not safe to assume non-clustered configuration when leadership election support is absent. The charm may indeed be clustered, even though the LE feature may not be present in the older version of Juju.

[1] https://code.launchpad.net/~openstack-charmers/charms/trusty/rabbitmq-server/next

[2] https://wiki.ubuntu.com/ServerTeam/OpenStackCharms

review: Resubmit

[Matt Bruzek (mbruzek)]

According to Ryan's comments this merge needs to be re-created at a different target. I am going to set this charm to rejected. Please resubmit the charm at the correct target.

review: Disapprove

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-lxc/1770/

review: Needs Fixing (automated testing)

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-aws/1748/

review: Needs Fixing (automated testing)

Unmerged revisions

112. By Paul Collins on 2015-11-05

enable-legacy-guest-account: handle clustered configurations

111. By Paul Collins on 2015-11-05

process enable-legacy-guest-account *after* rabbit is installed

110. By Paul Collins on 2015-11-04

add config item "enable-legacy-guest-account", and obey it

=== modified file 'config.yaml'
--- config.yaml	2015-10-22 13:24:30 +0000
+++ config.yaml	2015-11-06 00:09:39 +0000
@@ -209,3 +209,11 @@
         - ['/', 'queue1', 10, 20]
         - ['/', 'queue2', 200, 300]
         Wildcards '*' are accepted to monitor all vhosts and/or queues
+  enable-legacy-guest-account:
+      type: boolean
+      default: False
+      description: |
+        rabbitmq creates a "guest" account by default,
+        which this charm will delete.
+        .
+        Set this config item to "true" to restore the account.

=== modified file 'hooks/rabbit_utils.py'
--- hooks/rabbit_utils.py	2015-10-22 13:24:30 +0000
+++ hooks/rabbit_utils.py	2015-11-06 00:09:39 +0000
@@ -185,6 +185,18 @@
         rabbitmqctl('set_user_tags', user)
 
 
+def delete_user(user):
+    exists, is_admin = user_exists(user)
+
+    if exists:
+        log('Deleting user (%s).' % user)
+        rabbitmqctl('delete_user', user)
+        return True
+    else:
+        log('Not deleting non-existent user (%s).' % user)
+        return False
+
+
 def grant_permissions(user, vhost):
     log("Granting permissions", level='DEBUG')
     rabbitmqctl('set_permissions', '-p',

=== modified file 'hooks/rabbitmq_server_relations.py'
--- hooks/rabbitmq_server_relations.py	2015-10-29 12:10:35 +0000
+++ hooks/rabbitmq_server_relations.py	2015-11-06 00:09:39 +0000
@@ -649,6 +649,25 @@
     status_set('maintenance', 'Installing/upgrading RabbitMQ packages')
     apt_install(rabbit.PACKAGES, fatal=True)
 
+    manage_guest = None
+    try:
+        manage_guest = is_leader()
+        if manage_guest:
+            log('managing guest account')
+        else:
+            log('not managing guest account')
+    except NotImplementedError:
+        manage_guest = True
+        log('managing guest account: is_leader() not implemented: assuming non-clustered configuration')
+
+    if manage_guest:
+        status_set('maintenance', 'Managing RabbitMQ guest account')
+        if config('enable-legacy-guest-account'):
+            rabbit.create_user('guest', 'guest', admin=True)
+        else:
+            rabbit.delete_user('guest')
+
+    status_set('maintenance', 'Configuring RabbitMQ')
     open_port(5672)
 
     chown(RABBIT_DIR, rabbit.RABBIT_USER, rabbit.RABBIT_USER)