Merge lp:~rvb/maas/packaging.set-rabbitmq-creds into lp:~maas-maintainers/maas/packaging

[Raphaël Badin]

Add method in debian/maas.postinst to create the RabbitMQ user used by workers and update BROKER_URL in /etc/maas/maas_local_settings.py.

Add method in debian/maas.postinst to create the RabbitMQ user used by workers and update BROKER_URL in /etc/maas/maas_local_settings.py.

[lp:~rvb/maas/packaging.set-rabbitmq-creds]

90. By Raphaël Badin on 2012-09-20

Update changelog.

91. By Raphaël Badin on 2012-09-20

Merge trunk, fix conflicts.

[Gavin Panella (allenap)]

[1]

+ if [ -x /usr/sbin/rabbitmqctl ]; then

Just so you know, there's a genuine tab character in there.

Also, Scott has recommended to not hard-code paths. You can check for
the presence of a command (in bash) with:

    if command -v rabbitmqctl >/dev/null; then

or:

    if type -p rabbitmqctl >/dev/null; then

Yay for TIMTOWTDI.

[2]

+ if ! rabbitmqctl list_users | grep -qs "$workers_user"; then

Unless $workers_user is a regex, use fgrep here.

[3]

+ rabbitmqctl add_user "$workers_user" "$workers_pass" || true

Can you add a comment about why errors are being suppressed?

[4]

+ if grep -qs "^BROKER_URL\ \= '[a-zA-Z0-9]\{0,\}'$" /etc/maas/maas_local_celerysettings.py; then

You /may/ find that egrep has a more pleasant and familiar regex syntax.

[5]

+ if grep -qs "^BROKER_URL\ \= '[a-zA-Z0-9]\{0,\}'$" /etc/maas/maas_local_celerysettings.py; then
+ local broker_url="amqp://$workers_user:$workers_pass@$amqp_host:$amqp_port/$workers_vhost"
+ sed -i "s/^BROKER_URL\ \= '[a-zA-Z0-9]\{0,\}'$/BROKER_URL = '"$broker_url"'/" \
+                       /etc/maas/maas_local_celerysettings.py
+ fi

Why is it checking for such a detailed match? Why not just set it?

Also, that sed command is going to break because broker_url contains
forward-slashes. How about:

    local broker_url="amqp://$workers_user:$workers_pass@$amqp_host:$amqp_port/$workers_vhost"
    python - "/etc/maas/maas_local_celerysettings.py" "${broker_url}" <<'EOF'
from os import fdopen, rename
from os.path import dirname
from re import MULTILINE, sub
from sys import argv
from tempfile import mkstemp
conffile, url = argv[1:]
with open(conffile, "rb") as stream:
     output = sub(
         r'^(BROKER_URL)\b.*$', r'\1 = %r' % url,
         stream.read(), flags=MULTILINE)
fd, filename = mkstemp(dir=dirname(conffile))
with fdopen(fd, "wb") as stream:
     stream.write(output)
rename(filename, conffile)
EOF

It's long, but it makes sense, gets quoting right, and is
failure-safe.

Or, how about bash and awk:

    local conffile="/etc/maas/maas_local_celerysettings.py"
    local conftemp=$(mktemp /etc/maas/.tmp.XXXXXX)
    trap 'rm -f "${conftemp}"' EXIT

    local broker_url="amqp://$workers_user:$workers_pass@$amqp_host:$amqp_port/$workers_vhost"
    local broker_url_escaped=$(python -c 'import sys; sys.stdout.write(repr(sys.argv[1]))' "$broker_url")
    awk --assign "url=${broker_url_escaped}" \
        '/^BROKER_URL\y/ { print "BROKER_URL = " url; next } { print }' \
        "${conffile}" > "${conftemp}"
    mv "${conftemp}" "${conffile}"

The Python snippet gets quoting right, but it's line-noise otherwise.

Do you know what kicks both their arses?

    url="amqp://$workers_user:$workers_pass@$amqp_host:$amqp_port/$workers_vhost" \
        perl -pe 's/^(BROKER_URL)\b.*/\1 = "$ENV{'url'}"/' \
            -i /etc/maas/maas_local_celerysettings.py

It doesn't guarantee correct quoting though, because it doesn't know
Python rules, and I'm not sure if -i is atomic.

In all, I think the Perl solution fits the hand best, but the Python
solution is more correct, and, if you actually switch to Python as a
replacement for sh/bash, it'll fit the hand well too. sh/bash is
just...  arc...

review: Needs Fixing

[Julian Edwards (julian-edwards)]

On Thursday 20 September 2012 16:44:18 you wrote:
> Sorry, I'm preaching to the choir. I have written many many long
> complicated bash scripts in the past - forgive me for I have sinned -
> but now I'd like to see it eliminated. I don't expect you to do that
> here though.

I'm happy to be corrected by one of the packaging guys, but I'm not sure using
Python or Perl in postinst is a good idea. While it's probably installed due
to our dependencies, it can't be guaranteed to be available.

[Gavin Panella (allenap)]

On 21 Sep 2012 02:22, "Julian Edwards" <email address hidden> wrote:
>
> On Thursday 20 September 2012 16:44:18 you wrote:
> > Sorry, I'm preaching to the choir. I have written many many long
> > complicated bash scripts in the past - forgive me for I have sinned -
> > but now I'd like to see it eliminated. I don't expect you to do that
> > here though.
>
> I'm happy to be corrected by one of the packaging guys, but I'm not sure
using
> Python or Perl in postinst is a good idea. While it's probably installed
due
> to our dependencies, it can't be guaranteed to be available.

That's a good point, and, if it's the case, is another reason why we should
aim to get more of this stuff into the maas runtime instead of its
packages.

[Raphaël Badin (rvb)]

Thanks for the review Gavin. I think you've found a real issue because broker_url definitely contains forward-slashes… but please be aware that the code that creates the rabbitmq user and populates the config file is based (as in copied from) on existing code that is in that very same file (to set up the rabbitmq user used by longpoll). This does not make it right but as a non-packaging expert, I've tried to blend in with how the packaging scripts have been written so far.

[Gavin Panella (allenap)]

On 21 Sep 2012 08:11, "Raphaël Badin" <email address hidden> wrote:
>
> Thanks for the review Gavin. I think you've found a real issue because
broker_url definitely contains forward-slashes… but please be aware that
the code that creates the rabbitmq user and populates the config file is
based (as in copied from) on existing code that is in that very same file
(to set up the rabbitmq user used by longpoll). This does not make it
right but as a non-packaging expert, I've tried to blend in with how the
packaging scripts have been written so far.

Oh yeah, I accept that, and never expected you to rewrite everything. I,
er, got a bit carried away.

[lp:~rvb/maas/packaging.set-rabbitmq-creds]

92. By Raphaël Badin on 2012-09-21

Use '|' as a separator for sed.

[Raphaël Badin (rvb)]

Ok, I've fixed the flaw you found (by using a different separator in the sed statement) and simplified the regexp a bit.

I'm not sure if it's possible/recommended to use python in that kind of script so I left it like it is. Again, I'm trying to blend in with how the things where *already done* in this file.

[lp:~rvb/maas/packaging.set-rabbitmq-creds]

93. By Raphaël Badin on 2012-09-21

Merge trunk.

94. By Raphaël Badin on 2012-09-21

s/maas_local_celerysettings.py/maas_local_celeryconfig.py/

[Andres Rodriguez (andreserl)]

Looks good to me!

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2012-09-21 13:59:47 +0000
+++ debian/changelog	2012-09-21 16:03:19 +0000
@@ -26,6 +26,12 @@
       Depend on it.
   * debian/rules: Install maas-dhcp-server upstart job.
 
+  [ Raphael Badin ]
+  * Install maas_local_celeryconfig.py in /etc/maas and symlink to
+    /usr/share/maas.
+  * debian/maas.postinst: Create rabbitmq celery user/vhost.
+  * debian/maas.postinst: Update BROKER_URL in maas_local_celeryconfig.py.
+
  -- Andres Rodriguez <[e-mail]>  Wed, 19 Sep 2012 14:47:04 -0400
 
 maas (0.1+bzr971+dfsg-0ubuntu2) quantal; urgency=low

=== modified file 'debian/maas.install'
--- debian/maas.install	2012-09-19 19:00:45 +0000
+++ debian/maas.install	2012-09-21 16:03:19 +0000
@@ -11,6 +11,9 @@
 # Install MAAS local settings
 debian/tmp/etc/maas/maas_local_settings.py
 
+# Install MAAS local celery settings
+debian/tmp/etc/maas/maas_local_celeryconfig.py
+
 # Install WSGI
 debian/tmp/usr/share/maas/wsgi.py
 

=== modified file 'debian/maas.links'
--- debian/maas.links	2012-06-28 20:41:16 +0000
+++ debian/maas.links	2012-09-21 16:03:19 +0000
@@ -1,2 +1,3 @@
 etc/maas/maas_local_settings.py usr/share/maas/maas_local_settings.py
+etc/maas/maas_local_celeryconfig.py usr/share/maas/maas_local_celeryconfig.py
 etc/maas/celeryconfig.py usr/share/maas/celeryconfig.py

=== modified file 'debian/maas.postinst'
--- debian/maas.postinst	2012-09-19 19:00:45 +0000
+++ debian/maas.postinst	2012-09-21 16:03:19 +0000
@@ -77,6 +77,29 @@
 	fi
 }
 
+configure_maas_workers_rabbitmq_user() {
+	local workers_user="maas_workers"
+	local workers_pass="$(pwgen -s 20)"
+	local workers_vhost="/maas_workers"
+	local amqp_host="localhost"
+	local amqp_port="5672"
+	if [ -x /usr/sbin/rabbitmqctl ]; then
+		if ! rabbitmqctl list_users | grep -qs "$workers_user"; then
+			rabbitmqctl add_user "$workers_user" "$workers_pass" || true
+			rabbitmqctl add_vhost "$workers_vhost" || true
+			rabbitmqctl set_permissions -p "$workers_vhost" "$workers_user" ".*" ".*" ".*" || true
+		else
+			rabbitmqctl change_password "$workers_user" "$workers_pass" || true
+		fi
+	fi
+
+	if grep -qs "^BROKER_URL\ \= '.*'$" /etc/maas/maas_local_celeryconfig.py; then
+		local broker_url="amqp://$workers_user:$workers_pass@$amqp_host:$amqp_port/$workers_vhost"
+		sed -i "s|^BROKER_URL\ \= '.*'$|BROKER_URL = '"$broker_url"'|" \
+                       /etc/maas/maas_local_celeryconfig.py
+	fi
+}
+
 configure_maas_database() {
 	local dbc_dbpass="$1"
 	if grep -qs "^\ \{1,\} 'PASSWORD': '[a-zA-Z0-9]\{0,\}',$" /etc/maas/maas_local_settings.py; then
@@ -182,6 +205,13 @@
 	configure_maas_txlongpoll_rabbitmq_user
 
 	#########################################################
+	########## Configure worker rabbitmq config ###########
+	#########################################################
+
+	# Handle celery/rabbitmq publishing
+	configure_maas_workers_rabbitmq_user
+
+	#########################################################
 	######## add maas.conf to tgt conf.d ####################
 	#########################################################
 	configure_maas_tgt