wordpress-charmers team mailing list archive

[Tom Haddon <tom.haddon@xxxxxxxxxxxxx>]

Tom Haddon has proposed merging ~mthaddon/charm-k8s-wordpress/+git/charm-k8s-wordpress:admin-password-action into charm-k8s-wordpress:master.

Commit message:
Add an action to retrieve initial password

Requested reviews:
  Canonical IS Reviewers (canonical-is-reviewers)
  Wordpress Charmers (wordpress-charmers)
Related bugs:
  Bug #1907063 in charm-k8s-wordpress: "Add a juju action to retrieve admin password"
  https://bugs.launchpad.net/charm-k8s-wordpress/+bug/1907063

For more details, see:
https://code.launchpad.net/~mthaddon/charm-k8s-wordpress/+git/charm-k8s-wordpress/+merge/395982

Add an action to retrieve initial password
-- 
Your team Wordpress Charmers is requested to review the proposed merge of ~mthaddon/charm-k8s-wordpress/+git/charm-k8s-wordpress:admin-password-action into charm-k8s-wordpress:master.

diff --git a/README.md b/README.md
index 886c92a..bd66985 100644
--- a/README.md
+++ b/README.md
@@ -73,9 +73,9 @@ and perform the initial setup for you. Look for this line in the output of
 This is due to [issue #166](https://github.com/canonical/operator/issues/166) and will be fixed once Juju supports a Kubernetes
 pod ready hook.
 
-To retrieve the random admin password, run the following (until [LP#1907063](https://bugs.launchpad.net/charm-k8s-wordpress/+bug/1907063) is addressed):
+To retrieve the auto-generated admin password, run the following:
 
-    microk8s.kubectl exec -ti -n wordpress wordpress-operator-0 -- cat /root/initial.passwd
+    juju run-action --wait wordpress/0 get-initial-password
 
 You should now be able to browse to https://myblog.example.com/wp-admin.
 
diff --git a/actions.yaml b/actions.yaml
new file mode 100644
index 0000000..796aa4e
--- /dev/null
+++ b/actions.yaml
@@ -0,0 +1,2 @@
+get-initial-password:
+  description: Retrieve the auto-generated initial password.
diff --git a/src/charm.py b/src/charm.py
index edade47..1c2f1de 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -12,6 +12,7 @@ from ops.charm import CharmBase, CharmEvents
 from ops.framework import EventBase, EventSource, StoredState
 from ops.main import main
 from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus, WaitingStatus
+from leadership import LeadershipSettings
 
 logger = logging.getLogger()
 
@@ -110,11 +111,16 @@ class WordpressCharm(CharmBase):
     def __init__(self, *args):
         super().__init__(*args)
 
+        self.leader_data = LeadershipSettings()
+
         self.framework.observe(self.on.start, self.on_config_changed)
         self.framework.observe(self.on.config_changed, self.on_config_changed)
         self.framework.observe(self.on.update_status, self.on_config_changed)
         self.framework.observe(self.on.wordpress_initialise, self.on_wordpress_initialise)
 
+        # Actions.
+        self.framework.observe(self.on.get_initial_password_action, self._on_get_initial_password_action)
+
         self.state.set_default(
             initialised=False, valid=False,
         )
@@ -146,7 +152,8 @@ class WordpressCharm(CharmBase):
             msg = "Wordpress needs configuration"
             logger.info(msg)
             self.model.unit.status = MaintenanceStatus(msg)
-            installed = self.wordpress.first_install(self.get_service_ip())
+            initial_password = self._get_initial_password()
+            installed = self.wordpress.first_install(self.get_service_ip(), initial_password)
             if not installed:
                 msg = "Failed to configure wordpress"
                 logger.info(msg)
@@ -287,6 +294,26 @@ class WordpressCharm(CharmBase):
             return self.wordpress.is_vhost_ready(service_ip)
         return False
 
+    def _get_initial_password(self):
+        """Get the initial 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."""
+        initial_password = self.leader_data["initial_password"]
+        if not initial_password:
+            if self.unit.is_leader:
+                initial_password = password_generator()
+                self.leader_data["initial_password"] = initial_password
+        return initial_password
+
+    def _on_get_initial_password_action(self, event):
+        """Handle the get-initial-password action."""
+        initial_password = self._get_initial_password()
+        if initial_password:
+            event.set_results({"password": initial_password})
+        else:
+            event.fail("Initial password has not been set yet.")
+
 
 if __name__ == "__main__":
     main(WordpressCharm)
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/src/wordpress.py b/src/wordpress.py
index a3f586e..9ac5336 100644
--- a/src/wordpress.py
+++ b/src/wordpress.py
@@ -44,15 +44,10 @@ class Wordpress:
     def __init__(self, model_config):
         self.model_config = model_config
 
-    def _write_initial_password(self, password, filepath):
-        with open(filepath, "w") as f:
-            f.write(password)
-
-    def first_install(self, service_ip):
+    def first_install(self, service_ip, admin_password):
         """Perform initial configuration of wordpress if needed."""
         config = self.model_config
         logger.info("Starting wordpress initial configuration")
-        admin_password = password_generator()
         payload = {
             "admin_password": admin_password,
             "blog_public": "checked",
@@ -61,12 +56,6 @@ class Wordpress:
         payload.update(safe_load(config["initial_settings"]))
         payload["admin_password2"] = payload["admin_password"]
 
-        # Ideally we would store this in state however juju run-action does not
-        # currently support being run inside the operator pod which means the
-        # StorageState will be split between workload and operator.
-        # https://bugs.launchpad.net/juju/+bug/1870487
-        self._write_initial_password(payload["admin_password"], "/root/initial.passwd")
-
         if not payload["blog_public"]:
             payload["blog_public"] = "unchecked"
         required_config = set(("user_name", "admin_email"))
diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
index 1840a4c..29d8f81 100644
--- a/tests/unit/test_charm.py
+++ b/tests/unit/test_charm.py
@@ -4,6 +4,8 @@ import copy
 import mock
 import unittest
 
+from unittest.mock import Mock
+
 from charm import WordpressCharm, create_wordpress_secrets, gather_wordpress_secrets
 from wordpress import WORDPRESS_SECRETS
 from ops import testing
@@ -124,3 +126,16 @@ class TestWordpressCharm(unittest.TestCase):
             }
         }
         self.assertEqual(self.harness.charm.make_pod_resources(), expected)
+
+    def test_on_get_initial_password_action(self):
+        action_event = Mock()
+        # First test with no initial password set.
+        with mock.patch.object(self.harness.charm, "_get_initial_password") as get_initial_password:
+            get_initial_password.return_value = ""
+            self.harness.charm._on_get_initial_password_action(action_event)
+            self.assertEqual(action_event.fail.call_args, mock.call("Initial password has not been set yet."))
+        # Now test with initial password set.
+        with mock.patch.object(self.harness.charm, "_get_initial_password") as get_initial_password:
+            get_initial_password.return_value = "passwd"
+            self.harness.charm._on_get_initial_password_action(action_event)
+            self.assertEqual(action_event.set_results.call_args, mock.call({"password": "passwd"}))
diff --git a/tests/unit/test_wordpress.py b/tests/unit/test_wordpress.py
index f204cac..92712e7 100644
--- a/tests/unit/test_wordpress.py
+++ b/tests/unit/test_wordpress.py
@@ -94,13 +94,10 @@ class WordpressTest(unittest.TestCase):
     def test__init__(self):
         self.assertEqual(self.test_wordpress.model_config, self.test_model_config)
 
-    @mock.patch("wordpress.password_generator", side_effect=dummy_password_generator)
-    def test_first_install(self, password_generator_func):
+    def test_first_install(self):
         mocked_call_wordpress = mock.MagicMock(name="call_wordpress", return_value=True)
-        mocked__write_initial_password = mock.MagicMock(name="_write_initial_password", return_value=None)
         mocked_wordpress_configured = mock.MagicMock(name="wordpress_configured", return_value=True)
         self.test_wordpress.call_wordpress = mocked_call_wordpress
-        self.test_wordpress._write_initial_password = mocked__write_initial_password
         self.test_wordpress.wordpress_configured = mocked_wordpress_configured
 
         test_payload = {
@@ -112,10 +109,7 @@ class WordpressTest(unittest.TestCase):
             'admin_email': 'root@xxxxxxxxxxxxxxxxxxx',
             'weblog_title': 'Test Blog',
         }
-        self.test_wordpress.first_install(self.test_service_ip)
-
-        # Test that we wrote initial admin credentials inside the operator pod.
-        self.test_wordpress._write_initial_password.assert_called_with(TEST_GENERATED_PASSWORD, "/root/initial.passwd")
+        self.test_wordpress.first_install(self.test_service_ip, TEST_GENERATED_PASSWORD)
 
         # Test that we POST'd our initial configuration options to the wordpress API.
         self.test_wordpress.call_wordpress.assert_called_with(
@@ -128,7 +122,7 @@ class WordpressTest(unittest.TestCase):
         self.test_wordpress.model_config["initial_settings"] = (
             "user_name: admin\n" "weblog_title: Test Blog\n" "blog_public: False"
         )
-        self.test_wordpress.first_install(self.test_service_ip)
+        self.test_wordpress.first_install(self.test_service_ip, TEST_GENERATED_PASSWORD)
         self.test_wordpress.call_wordpress.assert_not_called()
 
     def test_wordpress_configured(self):