Merge ~mthaddon/charm-k8s-openldap/+git/charm-k8s-openldap:password-action into charm-k8s-openldap:master

[Tom Haddon]

Remove admin_password as a config option, add an action to retrieve it

Remove admin_password as a config option, add an action to retrieve it

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Stuart Bishop (stub)]

Looks fine, comments inline. If you decide to drop RichLeaderData, the changes will be minor and unlikely to need a re-review.

review: Approve

[Stuart Bishop (stub)]

Still looks good, ta.

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 82adb10deb3df836125a4b4638ca7a7b54ff99e3

diff --git a/README.md b/README.md
index bcde053..5fd477d 100644
--- a/README.md
+++ b/README.md
@@ -11,10 +11,15 @@ cloud, attached to a controller using `juju add-k8s`.
 
 To deploy this charm with both OpenLDAP and PostgreSQL inside a k8s model, run:
 
-    juju deploy cs:~openldap-charmers/openldap --admin_password=admin
+    juju deploy cs:~openldap-charmers/openldap
     juju deploy cs:~postgresql-charmers/postgresql-k8s postgresql
     juju add-relation openldap:db postgresql:db
 
+To retrieve the auto-generated LDAP admin password, run, assuming you're using
+Juju 2.x:
+
+    juju run-action openldap/0 --wait get-admin-password
+
 ### Developing
 
 Notes for deploying a test setup locally using microk8s:
diff --git a/actions.yaml b/actions.yaml
 new file mode 100644
index 0000000..04086e6
--- /dev/null
+++ b/actions.yaml
@@ -0,0 +1,3 @@
+get-admin-password:
+  description: >
+    Retrieve the auto-generated LDAP admin password.
diff --git a/config.yaml b/config.yaml
index 5db4484..c442062 100644
--- a/config.yaml
+++ b/config.yaml
@@ -18,8 +18,3 @@ options:
         description: |
           The password associated with image_username for accessing the registry specified in image_path.
         default: ''
-    admin_password:
-        type: string
-        description: |
-          The password for the OpenLDAP admin user.
-        default: ''
diff --git a/image-scripts/build-openldap.sh b/image-scripts/build-openldap.sh
index e696847..283466a 100755
--- a/image-scripts/build-openldap.sh
+++ b/image-scripts/build-openldap.sh
@@ -6,6 +6,8 @@ apt-get update
 apt-get -y dist-upgrade
 apt-get --purge autoremove -y
 apt-get install -y wget unixodbc make gcc unixodbc-dev groff-base ldap-utils odbc-postgresql gettext postgresql-client
+# Needed for juju actions to work.
+apt-get install -y python3-yaml
 
 echo "Making build dir"
 mkdir -p /srv/build
diff --git a/requirements.txt b/requirements.txt
index fd6adcd..12aa474 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1,2 +1,3 @@
+charmhelpers
 ops
 ops-lib-pgsql
diff --git a/src/charm.py b/src/charm.py
index 069d3dd..f8d5489 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -4,6 +4,7 @@
 
 import logging
 
+from charmhelpers.core import host
 import ops.lib
 from ops.charm import (
     CharmBase,
@@ -17,10 +18,10 @@ from ops.framework import (
 )
 from ops.model import (
     ActiveStatus,
-    BlockedStatus,
     MaintenanceStatus,
     WaitingStatus,
 )
+from leadership import LeadershipSettings
 
 
 pgsql = ops.lib.use("pgsql", 1, "[e-mail]")
@@ -28,7 +29,6 @@ pgsql = ops.lib.use("pgsql", 1, "[e-mail]")
 logger = logging.getLogger(__name__)
 
 
-REQUIRED_SETTINGS = ['admin_password']
 DATABASE_NAME = 'openldap'
 
 
@@ -50,10 +50,13 @@ class OpenLDAPK8sCharm(CharmBase):
     def __init__(self, *args):
         super().__init__(*args)
 
+        self.leader_data = LeadershipSettings()
+
         self.framework.observe(self.on.start, self._configure_pod)
         self.framework.observe(self.on.config_changed, self._configure_pod)
         self.framework.observe(self.on.leader_elected, self._configure_pod)
         self.framework.observe(self.on.upgrade_charm, self._configure_pod)
+        self.framework.observe(self.on.get_admin_password_action, self._on_get_admin_password_action)
 
         # database
         self._state.set_default(postgres=None)
@@ -93,30 +96,6 @@ class OpenLDAPK8sCharm(CharmBase):
 
         self.on.db_master_available.emit()
 
-    def _check_for_config_problems(self):
-        """Check for some simple configuration problems and return a
-        string describing them, otherwise return an empty string."""
-        problems = []
-
-        missing = self._missing_charm_settings()
-        if missing:
-            problems.append('required setting(s) empty: {}'.format(', '.join(sorted(missing))))
-
-        return '; '.join(filter(None, problems))
-
-    def _missing_charm_settings(self):
-        """Check configuration setting dependencies and return a list of
-        missing settings; otherwise return an empty list."""
-        config = self.model.config
-
-        # Options in config.yaml are always present as at least ""
-        # so config[setting] will not fail with a KeyError if unset via juju.
-        # We define missing in terms of being required by the charm and explicitly
-        # set rather than default from config.yaml.
-        missing = {setting for setting in REQUIRED_SETTINGS if not config[setting]}
-
-        return sorted(missing)
-
     def _make_pod_spec(self):
         """Return a pod spec with some core configuration."""
         config = self.model.config
@@ -142,9 +121,28 @@ class OpenLDAPK8sCharm(CharmBase):
             ],
         }
 
+    def _on_get_admin_password_action(self, event):
+        """Handle on get-admin-password action."""
+        admin_password = self.get_admin_password()
+        if admin_password:
+            event.set_results({"admin-password": self.get_admin_password()})
+        else:
+            event.fail("LDAP admin password has not yet been set, please retry later.")
+
+    def get_admin_password(self):
+        """Get the LDAP admin password.
+
+        If a password hasn't been set yet, create one if we're the leader,
+        or return an empty string if we're not."""
+        admin_password = self.leader_data["admin_password"]
+        if not admin_password:
+            if self.unit.is_leader:
+                admin_password = host.pwgen(40)
+                self.leader_data["admin_password"] = admin_password
+        return admin_password
+
     def _make_pod_config(self):
         """Return an envConfig with some core configuration."""
-        config = self.model.config
         pod_config = {
             'POSTGRES_NAME': self._state.postgres['dbname'],
             'POSTGRES_USER': self._state.postgres['user'],
@@ -153,8 +151,7 @@ class OpenLDAPK8sCharm(CharmBase):
             'POSTGRES_PORT': self._state.postgres['port'],
         }
 
-        if 'admin_password' in config:
-            pod_config['LDAP_ADMIN_PASSWORD'] = config['admin_password']
+        pod_config['LDAP_ADMIN_PASSWORD'] = self.get_admin_password()
 
         return pod_config
 
@@ -169,11 +166,6 @@ class OpenLDAPK8sCharm(CharmBase):
             self.unit.status = ActiveStatus()
             return
 
-        problems = self._check_for_config_problems()
-        if problems:
-            self.unit.status = BlockedStatus(problems)
-            return
-
         self.unit.status = MaintenanceStatus('Assembling pod spec')
         pod_spec = self._make_pod_spec()
 
diff --git a/src/leadership.py b/src/leadership.py
 new file mode 100644
index 0000000..8e88779
--- /dev/null
+++ b/src/leadership.py
@@ -0,0 +1,218 @@
+# This file is part of the PostgreSQL k8s Charm for Juju.
+# Copyright 2020 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3, as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# TODO: Most of all of this module should move into the Operator Framework core
+
+import collections.abc
+import subprocess
+from typing import Any, Iterable, Dict, MutableMapping, Protocol
+
+import ops
+import yaml
+
+
+class _Codec(Protocol):
+    def encode(self, value: Any) -> str:
+        raise NotImplementedError("encode")
+
+    def decode(self, key: str, value: str) -> Any:
+        raise NotImplementedError("decode")
+
+
+class _ObjectABCMeta(type(ops.framework.Object), type(collections.abc.MutableMapping)):
+    """This metaclass can go once the Operator Framework drops Python 3.5 support.
+
+    Per ops.framework._Metaclass docstring.
+    """
+
+    pass
+
+
+class _PeerData(ops.framework.Object, collections.abc.MutableMapping, metaclass=_ObjectABCMeta):
+    """A bag of data shared between peer units.
+
+    Only the leader can set data. All peer units can read.
+    """
+
+    def __init__(self, parent: ops.framework.Object, key: str, _store: MutableMapping, _codec: _Codec):
+        super().__init__(parent, key)
+        self._store = _store
+        self._codec = _codec
+        self._prefix = self.handle.path
+
+    def _prefixed_key(self, key: str) -> str:
+        return self._prefix + "/" + key
+
+    def __getitem__(self, key: str) -> Any:
+        if not isinstance(key, str):
+            raise TypeError(f"key must be a string, got {repr(key)} {type(key)}")
+        raw = self._store[self._prefixed_key(key)]
+        return self._codec.decode(key, raw)
+
+    def __setitem__(self, key: str, value: Any) -> None:
+        if not isinstance(key, str):
+            raise TypeError(f"key must be a string, got {repr(key)} {type(key)}")
+        if not self.model.unit.is_leader():
+            raise RuntimeError("non-leader attempting to set peer data")
+        self._store[self._prefixed_key(key)] = self._codec.encode(value)
+
+    def __delitem__(self, key: str) -> None:
+        if not isinstance(key, str):
+            raise TypeError(f"key must be a string, got {repr(key)} {type(key)}")
+        if not self.model.unit.is_leader():
+            raise RuntimeError("non-leader attempting to set peer data")
+        del self._store[self._prefixed_key(key)]
+
+    def __iter__(self) -> Iterable[str]:
+        return iter(self._store)
+
+    def __len__(self) -> int:
+        return len(self._store)
+
+
+class LegacyLeaderData(_PeerData):
+    """Raw Juju Leadership settings, a bag of data shared between peers.
+
+    Only the leader can set data. All peer units can read.
+
+    Behavior matches the Juju leader-get and leader-set tools; keys and
+    values must be strings, Setting an value to the empty string is the
+    same as deleting the entry, and accessing a missing entry will
+    return an empty string.
+
+    This class provides access to legacy Juju Leadership data, and namespace
+    collisions may occur if multiple components attempt to use the same key.
+    """
+
+    def __init__(self, parent, key=""):
+        super().__init__(parent, key, LeadershipSettings(), _RawCodec())
+
+    def _prefixed_key(self, key: str) -> str:
+        return key
+
+
+class RawLeaderData(_PeerData):
+    """Raw Juju Leadership settings, a bag of data shared between peers.
+
+    Only the leader can set data. All peer units can read.
+
+    Behavior matches the Juju leader-get and leader-set tools; keys and
+    values must be strings, Setting an value to the empty string is the
+    same as deleting the entry, and accessing a missing entry will
+    return an empty string.
+
+    Keys are automatically prefixed to avoid namespace collisions in the
+    Juju Leadership settings.
+    """
+
+    def __init__(self, parent, key=""):
+        super().__init__(parent, key, LeadershipSettings(), _RawCodec())
+
+
+class RichLeaderData(_PeerData):
+    """Encoded Juju Leadership settings, a bag of data shared between peers.
+
+    Only the leader can set data. All peer units can read.
+
+    Operates as a standard Python MutableMapping. Keys must be strings.
+    Values may be anything that the yaml library can marshal.
+
+    Keys are automatically prefixed to avoid namespace collisions in the
+    Juju Leadership settings.
+    """
+
+    def __init__(self, parent, key=""):
+        super().__init__(parent, key, LeadershipSettings(), _YAMLCodec())
+
+
+class _YAMLCodec(object):
+    def encode(self, value: Any) -> str:
+        return yaml.safe_dump(value)
+
+    def decode(self, key: str, value: str) -> Any:
+        if not value:
+            # Key never existed or was deleted. If set to
+            # empty string or none, value will contain
+            # the YAML representation.
+            raise KeyError(key)
+        return yaml.safe_load(value)
+
+
+class _RawCodec(object):
+    def encode(self, value: Any) -> str:
+        if not isinstance(value, str):
+            raise TypeError(f"{self.__class__.__name__} only supports str values, got {type(value)}")
+        return value
+
+    def decode(self, value: str) -> Any:
+        return value
+
+
+class LeadershipSettings(collections.abc.MutableMapping):
+    """Juju Leadership Settings data.
+
+    This class provides direct access to the Juju Leadership Settings,
+    a bag of data shared between peer units. Only the leader can set
+    items. Keys all share the same namespace, so beware of collisions.
+
+    This MutableMapping implements Juju behavior. Only strings are
+    supported as keys and values. Deleting an entry is the same as
+    setting it to the empty string. Attempting to read a missing
+    key will return the empty string (this class will never raise
+    a KeyError).
+    """
+
+    __cls_cache = None
+
+    @property
+    def _cache_loaded(self) -> bool:
+        return self.__class__.__cls_cache is not None
+
+    @property
+    def _cache(self) -> Dict[str, str]:
+        # There might be multiple instances of LeadershipSettings, but
+        # the backend is shared, so the cache needs to be a class
+        # attribute.
+        cls = self.__class__
+        if cls.__cls_cache is None:
+            cmd = ["leader-get", "--format=yaml"]
+            cls.__cls_cache = yaml.safe_load(subprocess.check_output(cmd).decode("UTF-8")) or {}
+        return cls.__cls_cache
+
+    def __getitem__(self, key: str) -> str:
+        return self._cache.get(key, "")
+
+    def __setitem__(self, key: str, value: str):
+        if "=" in key:
+            # Leave other validation to the leader-set tool
+            raise RuntimeError(f"LeadershipSettings keys may not contain '=', got {key}")
+        if value is None:
+            value = ""
+        cmd = ["leader-set", f"{key}={value}"]
+        subprocess.check_call(cmd)
+        if self._cache_loaded:
+            if value == "":
+                del self._cache[key]
+            else:
+                self._cache[key] = value
+
+    def __delitem__(self, key: str):
+        self[key] = ""
+
+    def __iter__(self) -> Iterable[str]:
+        return iter(self._cache)
+
+    def __len__(self) -> int:
+        return len(self._cache)
diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
index 717c937..00aba40 100644
--- a/tests/unit/test_charm.py
+++ b/tests/unit/test_charm.py
@@ -2,13 +2,13 @@
 # See LICENSE file for licensing details.
 
 import unittest
+from unittest.mock import patch
 
 from charm import OpenLDAPK8sCharm
 from collections import namedtuple
 from ops import testing
 from ops.model import (
     ActiveStatus,
-    BlockedStatus,
     WaitingStatus,
 )
 
@@ -18,34 +18,24 @@ CONFIG_ALL = {
     'image_path': 'example.com/openldap:latest',
     'image_username': 'image_user',
     'image_password': 'image_pass',
-    'admin_password': 'badmin_password',
 }
 
 CONFIG_IMAGE_NO_CREDS = {
     'image_path': 'example.com/openldap:latest',
     'image_username': '',
     'image_password': '',
-    'admin_password': 'badmin_password',
 }
 
 CONFIG_IMAGE_NO_IMAGE = {
     'image_path': '',
     'image_username': '',
     'image_password': '',
-    'admin_password': 'badmin_password',
 }
 
 CONFIG_IMAGE_NO_PASSWORD = {
     'image_path': 'example.com/openldap:latest',
     'image_username': 'production',
     'image_password': '',
-    'admin_password': 'badmin_password',
-}
-
-CONFIG_NO_ADMIN_PASSWORD = {
-    'image_path': 'example.com/openldap:latest',
-    'image_username': 'production',
-    'image_password': '',
 }
 
 DB_URI = {
@@ -63,12 +53,6 @@ class TestOpenLDAPK8sCharmHooksDisabled(unittest.TestCase):
         self.harness.begin()
         self.harness.disable_hooks()
 
-    def test_check_for_config_problems(self):
-        """Config problems as a string."""
-        self.harness.update_config(CONFIG_NO_ADMIN_PASSWORD)
-        expected = 'required setting(s) empty: admin_password'
-        self.assertEqual(self.harness.charm._check_for_config_problems(), expected)
-
     def test_make_pod_config(self):
         """Make basic, correct pod config."""
         self.harness.update_config(CONFIG_IMAGE_NO_CREDS)
@@ -81,66 +65,58 @@ class TestOpenLDAPK8sCharmHooksDisabled(unittest.TestCase):
             'POSTGRES_PORT': '5432',
             'LDAP_ADMIN_PASSWORD': 'badmin_password',
         }
-        self.assertEqual(self.harness.charm._make_pod_config(), expected)
-
-    def test_make_pod_config_no_password(self):
-        """Missing admin password in config shouldn't explode at least."""
-        self.harness.update_config(CONFIG_NO_ADMIN_PASSWORD)
-        self.harness.charm._state.postgres = DB_URI
-        expected = {
-            'LDAP_ADMIN_PASSWORD': '',
-            'POSTGRES_NAME': 'openldap',
-            'POSTGRES_USER': 'ldap_user',
-            'POSTGRES_PASSWORD': 'ldap_password',
-            'POSTGRES_HOST': '1.1.1.1',
-            'POSTGRES_PORT': '5432',
-        }
-        self.assertEqual(self.harness.charm._make_pod_config(), expected)
+        with patch.object(self.harness.charm, "get_admin_password") as get_admin_password:
+            get_admin_password.return_value = 'badmin_password'
+            self.assertEqual(self.harness.charm._make_pod_config(), expected)
 
     def test_make_pod_spec(self):
         """Basic, correct pod spec."""
         self.harness.update_config(CONFIG_ALL)
         self.harness.charm._state.postgres = DB_URI
-        expected = {
-            'version': 3,
-            'containers': [
-                {
-                    'name': 'openldap',
-                    'imageDetails': {
-                        'imagePath': 'example.com/openldap:latest',
-                        'username': 'image_user',
-                        'password': 'image_pass',
-                    },
-                    'ports': [{'containerPort': 389, 'protocol': 'TCP'}],
-                    'envConfig': self.harness.charm._make_pod_config(),
-                    'kubernetes': {
-                        'readinessProbe': {'tcpSocket': {'port': 389}},
-                    },
-                }
-            ],
-        }
-        self.assertEqual(self.harness.charm._make_pod_spec(), expected)
+        with patch.object(self.harness.charm, "get_admin_password") as get_admin_password:
+            get_admin_password.return_value = 'badmin_password'
+            expected = {
+                'version': 3,
+                'containers': [
+                    {
+                        'name': 'openldap',
+                        'imageDetails': {
+                            'imagePath': 'example.com/openldap:latest',
+                            'username': 'image_user',
+                            'password': 'image_pass',
+                        },
+                        'ports': [{'containerPort': 389, 'protocol': 'TCP'}],
+                        'envConfig': self.harness.charm._make_pod_config(),
+                        'kubernetes': {
+                            'readinessProbe': {'tcpSocket': {'port': 389}},
+                        },
+                    }
+                ],
+            }
+            self.assertEqual(self.harness.charm._make_pod_spec(), expected)
 
     def test_make_pod_spec_no_image_creds(self):
         self.harness.update_config(CONFIG_IMAGE_NO_CREDS)
         self.harness.charm._state.postgres = DB_URI
-        expected = {
-            'version': 3,
-            'containers': [
-                {
-                    'name': 'openldap',
-                    'imageDetails': {
-                        'imagePath': 'example.com/openldap:latest',
-                    },
-                    'ports': [{'containerPort': 389, 'protocol': 'TCP'}],
-                    'envConfig': self.harness.charm._make_pod_config(),
-                    'kubernetes': {
-                        'readinessProbe': {'tcpSocket': {'port': 389}},
-                    },
-                }
-            ],
-        }
-        self.assertEqual(self.harness.charm._make_pod_spec(), expected)
+        with patch.object(self.harness.charm, "get_admin_password") as get_admin_password:
+            get_admin_password.return_value = 'badmin_password'
+            expected = {
+                'version': 3,
+                'containers': [
+                    {
+                        'name': 'openldap',
+                        'imageDetails': {
+                            'imagePath': 'example.com/openldap:latest',
+                        },
+                        'ports': [{'containerPort': 389, 'protocol': 'TCP'}],
+                        'envConfig': self.harness.charm._make_pod_config(),
+                        'kubernetes': {
+                            'readinessProbe': {'tcpSocket': {'port': 389}},
+                        },
+                    }
+                ],
+            }
+            self.assertEqual(self.harness.charm._make_pod_spec(), expected)
 
     def test_configure_pod_no_postgres_relation(self):
         """Check that we block correctly without a Postgres relation."""
@@ -161,17 +137,6 @@ class TestOpenLDAPK8sCharmHooksDisabled(unittest.TestCase):
         self.harness.charm._configure_pod(mock_event)
         self.assertEqual(self.harness.charm.unit.status, expected)
 
-    def test_configure_pod_config_problems(self):
-        """Test pod config with missing juju config options."""
-        mock_event = MagicMock()
-
-        self.harness.update_config(CONFIG_NO_ADMIN_PASSWORD)
-        self.harness.charm._state.postgres = DB_URI
-        self.harness.set_leader(True)
-        expected = BlockedStatus('required setting(s) empty: admin_password')
-        self.harness.charm._configure_pod(mock_event)
-        self.assertEqual(self.harness.charm.unit.status, expected)
-
     def test_configure_pod(self):
         """Test pod configuration with everything working appropriately."""
         mock_event = MagicMock()
@@ -180,8 +145,10 @@ class TestOpenLDAPK8sCharmHooksDisabled(unittest.TestCase):
         self.harness.charm._state.postgres = DB_URI
         self.harness.set_leader(True)
         expected = ActiveStatus()
-        self.harness.charm._configure_pod(mock_event)
-        self.assertEqual(self.harness.charm.unit.status, expected)
+        with patch.object(self.harness.charm, "get_admin_password") as get_admin_password:
+            get_admin_password.return_value = 'badmin_password'
+            self.harness.charm._configure_pod(mock_event)
+            self.assertEqual(self.harness.charm.unit.status, expected)
 
     def test_on_database_relation_joined(self):
         mock_event = MagicMock()
@@ -206,5 +173,21 @@ class TestOpenLDAPK8sCharmHooksDisabled(unittest.TestCase):
         mock_event.master = master
         mock_event.database = "openldap"
 
-        self.harness.charm._on_master_changed(mock_event)
-        self.assertEqual(self.harness.charm._state.postgres['dbname'], "openldap")
+        with patch.object(self.harness.charm, "get_admin_password") as get_admin_password:
+            get_admin_password.return_value = 'badmin_password'
+            self.harness.charm._on_master_changed(mock_event)
+            self.assertEqual(self.harness.charm._state.postgres['dbname'], "openldap")
+
+    def test_on_get_admin_password_action(self):
+        mock_event = MagicMock()
+
+        self.harness.update_config(CONFIG_ALL)
+        with patch.object(self.harness.charm, "get_admin_password") as get_admin_password:
+            get_admin_password.return_value = 'badmin_password'
+            self.harness.charm._on_get_admin_password_action(mock_event)
+            mock_event.set_results.assert_called_with({"admin-password": "badmin_password"})
+            # And now return an empty result.
+            get_admin_password.return_value = ""
+            mock_event.reset_mock()
+            self.harness.charm._on_get_admin_password_action(mock_event)
+            mock_event.fail.assert_called_with("LDAP admin password has not yet been set, please retry later.")