From 26a55415abe75e74e22272758a04a2d3000d29eb Mon Sep 17 00:00:00 2001
From: Nir Magnezi <nmagnezi@redhat.com>
Date: Wed, 8 Feb 2017 16:02:44 +0200
Subject: [PATCH] Auto-detect haproxy user_group

As a followup to Id99948aec64656a0532afc68e146f0610bff1378, adding auto
detection to haproxy_amphora.user_group

haproxy is capable[1] handling a list of configuration files.
This patch leverages that capability by simply providing haproxy with an
additional configuration file, which is baked in the amphora image via a
diskimage-builder element.

The above-mentioned element will specify the following values for user group:
Ubuntu: 'nogroup'
RHEL/CentOS/Fedora: 'haproxy'

The amphora-agent will parse and remove any user_group configuration provided
by Octavia controller worker.
This is in order to maintain amphora-agent backward compatibility to old
Octavia workers, who still provide user_group to the amphora-agent.
Octavia Workers that include this patch will no longer provide user_group
configuration to the amphora-agent.

[1] https://cbonte.github.io/haproxy-dconv/1.7/management.html#3

Related-Bug #1548070

Change-Id: Ia8fede9d7da4709a48661d1fc595a16d04fcbfa9
---
 .../install.d/75-run_setup_install            |  2 +-
 .../20-haproxy-user-group-config              | 21 ++++++++++++++
 .../20-haproxy-user-group-config              | 21 ++++++++++++++
 etc/octavia.conf                              |  6 ++--
 .../backends/agent/agent_jinja_cfg.py         |  2 ++
 .../backends/agent/api_server/listener.py     | 29 +++++++++++++++----
 .../api_server/templates/systemd.conf.j2      |  6 ++--
 .../api_server/templates/sysvinit.conf.j2     | 10 ++++---
 .../api_server/templates/upstart.conf.j2      |  6 ++--
 octavia/common/config.py                      |  5 ++--
 octavia/common/constants.py                   |  1 +
 .../backend/agent/api_server/test_server.py   | 17 ++++++-----
 ...t_haproxy_user_group-c220b6a2c8f1d589.yaml |  7 +++++
 13 files changed, 105 insertions(+), 28 deletions(-)
 create mode 100755 elements/haproxy-octavia-ubuntu/post-install.d/20-haproxy-user-group-config
 create mode 100755 elements/haproxy-octavia/post-install.d/20-haproxy-user-group-config
 create mode 100644 releasenotes/notes/auto_detect_haproxy_user_group-c220b6a2c8f1d589.yaml

diff --git a/elements/amphora-agent/install.d/75-run_setup_install b/elements/amphora-agent/install.d/75-run_setup_install
index 3ae520ec37..98216999ff 100755
--- a/elements/amphora-agent/install.d/75-run_setup_install
+++ b/elements/amphora-agent/install.d/75-run_setup_install
@@ -14,5 +14,5 @@ ln -s /bin/amphora-agent /usr/local/bin/amphora-agent || true
 mkdir /etc/octavia
 # we assume certs, etc will come in through the config drive
 mkdir /etc/octavia/certs
-mkdir /var/lib/octavia
+mkdir -p /var/lib/octavia
 
diff --git a/elements/haproxy-octavia-ubuntu/post-install.d/20-haproxy-user-group-config b/elements/haproxy-octavia-ubuntu/post-install.d/20-haproxy-user-group-config
new file mode 100755
index 0000000000..0fb982540a
--- /dev/null
+++ b/elements/haproxy-octavia-ubuntu/post-install.d/20-haproxy-user-group-config
@@ -0,0 +1,21 @@
+#!/bin/bash
+
+set -eu
+set -o pipefail
+
+case $DISTRO_NAME in
+    ubuntu | debian )
+        HAPROXY_USER_GROUP=nogroup
+    ;;
+    fedora | centos* | rhel* )
+        HAPROXY_USER_GROUP=haproxy
+    ;;
+    *)
+        HAPROXY_USER_GROUP=nogroup
+    ;;
+esac
+
+cat >> /var/lib/octavia/haproxy-default-user-group.conf <<EOF
+global
+    group $HAPROXY_USER_GROUP
+EOF
diff --git a/elements/haproxy-octavia/post-install.d/20-haproxy-user-group-config b/elements/haproxy-octavia/post-install.d/20-haproxy-user-group-config
new file mode 100755
index 0000000000..0fb982540a
--- /dev/null
+++ b/elements/haproxy-octavia/post-install.d/20-haproxy-user-group-config
@@ -0,0 +1,21 @@
+#!/bin/bash
+
+set -eu
+set -o pipefail
+
+case $DISTRO_NAME in
+    ubuntu | debian )
+        HAPROXY_USER_GROUP=nogroup
+    ;;
+    fedora | centos* | rhel* )
+        HAPROXY_USER_GROUP=haproxy
+    ;;
+    *)
+        HAPROXY_USER_GROUP=nogroup
+    ;;
+esac
+
+cat >> /var/lib/octavia/haproxy-default-user-group.conf <<EOF
+global
+    group $HAPROXY_USER_GROUP
+EOF
diff --git a/etc/octavia.conf b/etc/octavia.conf
index 9715435e51..5c2990e446 100644
--- a/etc/octavia.conf
+++ b/etc/octavia.conf
@@ -119,7 +119,9 @@
 # build_rate_limit = -1
 # build_active_retries = 300
 # build_retry_interval = 5
-# user_group = nogroup
+
+# This setting is deprecated. It is now automatically discovered.
+# user_group =
 
 # Maximum number of entries that can fit in the stick table.
 # The size supports "k", "m", "g" suffixes.
@@ -140,7 +142,7 @@
 # client_cert = /etc/octavia/certs/client.pem
 # server_ca = /etc/octavia/certs/server_ca.pem
 #
-# This setting is deprecated.  It is now automatically discovered.
+# This setting is deprecated. It is now automatically discovered.
 # use_upstart = True
 #
 # rest_request_conn_timeout = 10
diff --git a/octavia/amphorae/backends/agent/agent_jinja_cfg.py b/octavia/amphorae/backends/agent/agent_jinja_cfg.py
index 7e4a7345f0..5a151bee93 100644
--- a/octavia/amphorae/backends/agent/agent_jinja_cfg.py
+++ b/octavia/amphorae/backends/agent/agent_jinja_cfg.py
@@ -49,6 +49,8 @@ class AgentJinjaTemplater(object):
              'base_path': CONF.haproxy_amphora.base_path,
              'bind_host': CONF.haproxy_amphora.bind_host,
              'bind_port': CONF.haproxy_amphora.bind_port,
+             # TODO(nmagnezi): user_group is Deprecated in Pike,
+             # should be Removed in R cycle.
              'user_group': CONF.haproxy_amphora.user_group,
              'controller_list': CONF.health_manager.controller_ip_port_list,
              'debug': CONF.debug,
diff --git a/octavia/amphorae/backends/agent/api_server/listener.py b/octavia/amphorae/backends/agent/api_server/listener.py
index 99d44e092f..0ca6b7e76a 100644
--- a/octavia/amphorae/backends/agent/api_server/listener.py
+++ b/octavia/amphorae/backends/agent/api_server/listener.py
@@ -13,6 +13,7 @@
 # under the License.
 
 import hashlib
+import io
 import json
 import logging
 import os
@@ -23,6 +24,7 @@ import subprocess
 
 import flask
 import jinja2
+from oslo_config import cfg
 import six
 from werkzeug import exceptions
 
@@ -36,6 +38,8 @@ from octavia.i18n import _LE
 LOG = logging.getLogger(__name__)
 BUFFER = 100
 
+CONF = cfg.CONF
+
 UPSTART_CONF = 'upstart.conf.j2'
 SYSVINIT_CONF = 'sysvinit.conf.j2'
 SYSTEMD_CONF = 'systemd.conf.j2'
@@ -107,15 +111,27 @@ class Listener(object):
         flags = os.O_WRONLY | os.O_CREAT | os.O_TRUNC
         # mode 00600
         mode = stat.S_IRUSR | stat.S_IWUSR
-        with os.fdopen(os.open(name, flags, mode), 'wb') as file:
+        b = stream.read(BUFFER)
+        s_io = io.StringIO()
+        while b:
+            # Write haproxy configuration to StringIO
+            s_io.write(b.decode('utf8'))
             b = stream.read(BUFFER)
-            while (b):
-                file.write(b)
-                b = stream.read(BUFFER)
+
+        # Since haproxy user_group is now auto-detected by the amphora agent,
+        # remove it from haproxy configuration in case it was provided
+        # by an older Octavia controller. This is needed in order to prevent
+        # a duplicate entry for 'group' in haproxy configuration, which will
+        # result an error when haproxy starts.
+        new_config = re.sub(r"\s+group\s.+", "", s_io.getvalue())
+
+        with os.fdopen(os.open(name, flags, mode), 'w') as file:
+            file.write(new_config)
 
         # use haproxy to check the config
-        cmd = "haproxy -c -L {peer} -f {config_file}".format(config_file=name,
-                                                             peer=peer_name)
+        cmd = "haproxy -c -L {peer} -f {config_file} -f {haproxy_ug}".format(
+            config_file=name, peer=peer_name,
+            haproxy_ug=consts.HAPROXY_USER_GROUP_CFG)
 
         try:
             subprocess.check_output(cmd.split(), stderr=subprocess.STDOUT)
@@ -172,6 +188,7 @@ class Listener(object):
                     haproxy_pid=util.pid_path(listener_id),
                     haproxy_cmd=util.CONF.haproxy_amphora.haproxy_cmd,
                     haproxy_cfg=util.config_path(listener_id),
+                    haproxy_user_group_cfg=consts.HAPROXY_USER_GROUP_CFG,
                     respawn_count=util.CONF.haproxy_amphora.respawn_count,
                     respawn_interval=(util.CONF.haproxy_amphora.
                                       respawn_interval),
diff --git a/octavia/amphorae/backends/agent/api_server/templates/systemd.conf.j2 b/octavia/amphorae/backends/agent/api_server/templates/systemd.conf.j2
index da3e6e5e7d..bc009d6b8b 100644
--- a/octavia/amphorae/backends/agent/api_server/templates/systemd.conf.j2
+++ b/octavia/amphorae/backends/agent/api_server/templates/systemd.conf.j2
@@ -7,7 +7,7 @@ Wants=syslog.service
 
 [Service]
 EnvironmentFile=-/etc/default/haproxy
-ExecStartPre=/usr/sbin/haproxy -f {{ haproxy_cfg }} -c -q
+ExecStartPre=/usr/sbin/haproxy -f {{ haproxy_cfg }} -f {{ haproxy_user_group_cfg }} -c -q
 # Re-add the namespace
 ExecStartPre=-/sbin/ip netns add {{ amphora_nsname }}
 # Load the system sysctl into the new namespace
@@ -23,8 +23,8 @@ ExecStartPre=-/sbin/ip netns exec {{ amphora_nsname }} ifup -a
 ExecStartPre=-/bin/awk '{system("/sbin/ip netns exec amphora-haproxy ifup " $2)}' /var/lib/octavia/plugged_interfaces
 {%- endif %}
 #
-ExecStart=/sbin/ip netns exec {{ amphora_nsname }} /usr/sbin/haproxy-systemd-wrapper -f {{ haproxy_cfg }} -p {{ haproxy_pid }} -L {{ peer_name }} $EXTRAOPTS
-ExecReload=/usr/sbin/haproxy -c -f {{ haproxy_cfg }}
+ExecStart=/sbin/ip netns exec {{ amphora_nsname }} /usr/sbin/haproxy-systemd-wrapper -f {{ haproxy_cfg }} -f {{ haproxy_user_group_cfg }} -p {{ haproxy_pid }} -L {{ peer_name }} $EXTRAOPTS
+ExecReload=/usr/sbin/haproxy -c -f {{ haproxy_cfg }} -f {{ haproxy_user_group_cfg }}
 ExecReload=/bin/kill -USR2 $MAINPID
 KillMode=mixed
 Restart=always
diff --git a/octavia/amphorae/backends/agent/api_server/templates/sysvinit.conf.j2 b/octavia/amphorae/backends/agent/api_server/templates/sysvinit.conf.j2
index f105ca4d7c..8898a67634 100644
--- a/octavia/amphorae/backends/agent/api_server/templates/sysvinit.conf.j2
+++ b/octavia/amphorae/backends/agent/api_server/templates/sysvinit.conf.j2
@@ -31,11 +31,13 @@ PATH=/sbin:/usr/sbin:/bin:/usr/bin
 PIDFILE={{ haproxy_pid }}
 CONFIG={{ haproxy_cfg }}
 HAPROXY={{ haproxy_cmd }}
+USER_GROUP_CONF_PATH={{ haproxy_user_group_cfg }}
+
 EXTRAOPTS=
 ENABLED=1
 
 test -x $HAPROXY || exit 0
-test -f "$CONFIG" || exit 0
+test -f "$CONFIG" -f "$USER_GROUP_CONF_PATH" || exit 0
 
 if [ -e /etc/default/haproxy ]; then
     . /etc/default/haproxy
@@ -66,7 +68,7 @@ haproxy_start()
     {%- endif %}
 
     start-stop-daemon --start --pidfile "$PIDFILE" \
-        --exec $HAPROXY -- -f "$CONFIG" -D -p "$PIDFILE" \
+        --exec $HAPROXY -- -f "$CONFIG" -f "$USER_GROUP_CONF_PATH" -D -p "$PIDFILE" \
         $EXTRAOPTS || return 2
     return 0
 }
@@ -86,7 +88,7 @@ haproxy_stop()
 
 haproxy_reload()
 {
-    $HAPROXY -f "$CONFIG" -p $PIDFILE -D $EXTRAOPTS -sf $(cat $PIDFILE) \
+    $HAPROXY -f "$CONFIG" -f "$USER_GROUP_CONF_PATH" -p $PIDFILE -D $EXTRAOPTS -sf $(cat $PIDFILE) \
         || return 2
     return 0
 }
@@ -95,7 +97,7 @@ haproxy_checkconf()
 {
     rcode=0
 
-    $HAPROXY -c -f "$CONFIG"
+    $HAPROXY -c -f "$CONFIG" -f "$USER_GROUP_CONF_PATH"
     if [ $? -ne 0 ]; then
         rcode=1
     fi
diff --git a/octavia/amphorae/backends/agent/api_server/templates/upstart.conf.j2 b/octavia/amphorae/backends/agent/api_server/templates/upstart.conf.j2
index da88dc8a53..d571b194cb 100644
--- a/octavia/amphorae/backends/agent/api_server/templates/upstart.conf.j2
+++ b/octavia/amphorae/backends/agent/api_server/templates/upstart.conf.j2
@@ -24,6 +24,7 @@ stop on runlevel [!2345]
 env PID_PATH={{ haproxy_pid }}
 env BIN_PATH={{ haproxy_cmd }}
 env CONF_PATH={{ haproxy_cfg }}
+env USER_GROUP_CONF_PATH={{ haproxy_user_group_cfg }}
 env PEER_NAME={{ peer_name }}
 
 respawn
@@ -53,9 +54,10 @@ script
 exec /bin/bash <<EOF
 echo \$(date) Starting HAProxy
 # The -L trick fixes the HAProxy limitation to have long peer names
-ip netns exec {{ amphora_nsname }} $BIN_PATH -f $CONF_PATH -L $PEER_NAME -D -p $PID_PATH
 
-trap "ip netns exec {{ amphora_nsname }} $BIN_PATH -f $CONF_PATH -L $PEER_NAME -p $PID_PATH -sf \\\$(cat $PID_PATH)" SIGHUP
+ip netns exec {{ amphora_nsname }} $BIN_PATH -f $CONF_PATH -f $USER_GROUP_CONF_PATH -L $PEER_NAME -D -p $PID_PATH
+
+trap "ip netns exec {{ amphora_nsname }} $BIN_PATH -f $CONF_PATH -f $USER_GROUP_CONF_PATH -L $PEER_NAME -p $PID_PATH -sf \\\$(cat $PID_PATH)" SIGHUP
 trap "kill -TERM \\\$(cat $PID_PATH) && rm $PID_PATH;echo \\\$(date) Exiting HAProxy; exit 0" SIGTERM SIGINT
 
 while true; do # Iterate to keep job running.
diff --git a/octavia/common/config.py b/octavia/common/config.py
index ffb63f7291..fdfac10311 100644
--- a/octavia/common/config.py
+++ b/octavia/common/config.py
@@ -172,8 +172,9 @@ haproxy_amphora_opts = [
                default=5,
                help=_('Retry timeout between build attempts in '
                       'seconds.')),
-    cfg.StrOpt('user_group',
-               default='nogroup',
+    cfg.StrOpt('user_group', deprecated_for_removal=True,
+               deprecated_reason='This is now automatically discovered '
+                                 ' and configured.',
                help=_('The user group for haproxy to run under inside the '
                       'amphora.')),
     cfg.StrOpt('haproxy_stick_size', default='10k',
diff --git a/octavia/common/constants.py b/octavia/common/constants.py
index 82307c4097..6d4c16bdbe 100644
--- a/octavia/common/constants.py
+++ b/octavia/common/constants.py
@@ -339,6 +339,7 @@ KEEPALIVED_JINJA2_SYSVINIT = 'keepalived.sysvinit.j2'
 CHECK_SCRIPT_CONF = 'keepalived_check_script.conf.j2'
 
 PLUGGED_INTERFACES = '/var/lib/octavia/plugged_interfaces'
+HAPROXY_USER_GROUP_CFG = '/var/lib/octavia/haproxy-default-user-group.conf'
 AMPHORA_NAMESPACE = 'amphora-haproxy'
 
 # List of HTTP headers which are supported for insertion
diff --git a/octavia/tests/functional/amphorae/backend/agent/api_server/test_server.py b/octavia/tests/functional/amphorae/backend/agent/api_server/test_server.py
index deabf72a67..e066fba2aa 100644
--- a/octavia/tests/functional/amphorae/backend/agent/api_server/test_server.py
+++ b/octavia/tests/functional/amphorae/backend/agent/api_server/test_server.py
@@ -112,13 +112,13 @@ class TestServerTestCase(base.TestCase):
                                          data='test')
             mode = stat.S_IRUSR | stat.S_IWUSR
             mock_open.assert_called_with(file_name, flags, mode)
-            mock_fdopen.assert_called_with(123, 'wb')
+            mock_fdopen.assert_called_with(123, 'w')
             self.assertEqual(202, rv.status_code)
-            handle = m()
-            handle.write.assert_called_once_with(six.b('test'))
+            m().write.assert_called_once_with('test')
             mock_subprocess.assert_any_call(
-                "haproxy -c -L {peer} -f {config_file}".format(
+                "haproxy -c -L {peer} -f {config_file} -f {haproxy_ug}".format(
                     config_file=file_name,
+                    haproxy_ug=consts.HAPROXY_USER_GROUP_CFG,
                     peer=(octavia_utils.
                           base64_sha1_string('amp_123').rstrip('='))).split(),
                 stderr=-2)
@@ -187,7 +187,7 @@ class TestServerTestCase(base.TestCase):
             mock_open.assert_called_with(init_path, flags, mode)
             mock_fdopen.assert_called_with(123, 'w')
             handle = mock_fdopen()
-            handle.write.assert_any_call(six.b('test'))
+            handle.write.assert_any_call('test')
             # skip the template stuff
             mock_makedirs.assert_called_with('/var/lib/octavia/123')
 
@@ -212,12 +212,13 @@ class TestServerTestCase(base.TestCase):
                 json.loads(rv.data.decode('utf-8')))
             mode = stat.S_IRUSR | stat.S_IWUSR
             mock_open.assert_called_with(file_name, flags, mode)
-            mock_fdopen.assert_called_with(123, 'wb')
+            mock_fdopen.assert_called_with(123, 'w')
             handle = mock_fdopen()
-            handle.write.assert_called_with(six.b('test'))
+            handle.write.assert_called_with('test')
             mock_subprocess.assert_called_with(
-                "haproxy -c -L {peer} -f {config_file}".format(
+                "haproxy -c -L {peer} -f {config_file} -f {haproxy_ug}".format(
                     config_file=file_name,
+                    haproxy_ug=consts.HAPROXY_USER_GROUP_CFG,
                     peer=(octavia_utils.
                           base64_sha1_string('amp_123').rstrip('='))).split(),
                 stderr=-2)
diff --git a/releasenotes/notes/auto_detect_haproxy_user_group-c220b6a2c8f1d589.yaml b/releasenotes/notes/auto_detect_haproxy_user_group-c220b6a2c8f1d589.yaml
new file mode 100644
index 0000000000..b5e25694df
--- /dev/null
+++ b/releasenotes/notes/auto_detect_haproxy_user_group-c220b6a2c8f1d589.yaml
@@ -0,0 +1,7 @@
+---
+features:
+  - The amphora haproxy user_group setting is now automatically detected for
+    Ubuntu, CentOS, Fedora, or RHEL based amphora.
+deprecations:
+  - haproxy user_group is no longer being used. it is now auto-detected for
+    Ubuntu, CentOS, Fedora and RHEL based amphora images.