wordpress-charmers team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

Review: Approve

All looks good. Some minor comments inline. first_install needs to explicitly return False on failure, not None. The tests I just reviewed will need to cope, but everything else should work fine since call sites just cast the None to a boolean False in any case.

Diff comments:

> diff --git a/src/charm.py b/src/charm.py
> index 7f2eb7c..ddb402e 100755
> --- a/src/charm.py
> +++ b/src/charm.py
> @@ -104,31 +84,47 @@ class WordpressK8sCharm(CharmBase):
>  
>          self.state.set_default(_spec=None)
>  
> +        self.wordpress = Wordpress(self.model.config)
> +
>      def on_config_changed(self, event):
>          is_valid = self.is_valid_config()
>          if not is_valid:
>              return
>  
>          self.configure_pod()
> -
> -        if self.state.init and self.model.unit.is_leader() and not self.wordpress_configured():
> +        if self.state.init:
>              self.on.wp_initialise.emit()
>  
>      def on_wp_initialise(self, event):
> -        ready = self.install_ready()
> -        if not ready:
> -            # Until k8s supports telling Juju our pod is available we need to defer initial
> -            # site setup for a subsequent update-status or config-changed hook to complete.
> -            # https://github.com/canonical/operator/issues/214
> -            self.model.unit.status = WaitingStatus("Waiting for pod to be ready")
> +        wordpress_needs_configuring = False
> +        pod_alive = self.is_service_up() and self.model.unit.is_leader()

I'd swap these around, checking for leadership first, since is_service_up will be more expensive and more prone to failure.

> +        if pod_alive:
> +            wordpress_configured = self.wordpress.wordpress_configured(self.get_service_ip())
> +            wordpress_needs_configuring = self.state.init and not wordpress_configured

Everytime I see self.init I get confused, because I always do this flag backwards (self.initialized, defaulting to False, whereas you have self.init for needs initialization, defaulting to True). I think this is my problem, not yours :) Or maybe better to be explicit and call it needs_initializing.

> +        else:
> +            msg = "Workpress workload pod is not ready"
> +            logger.info(msg)
> +            self.model.unit.status = WaitingStatus(msg)
>              return
>  
> -        installed = self.first_install()
> -        if not installed:
> -            return
> +        if wordpress_needs_configuring:
> +            msg = "Wordpress needs configuration"
> +            logger.info(msg)
> +            self.model.unit.status = MaintenanceStatus(msg)
> +            installed = self.wordpress.first_install(self.get_service_ip())
> +            if not installed:
> +                msg = "Failed to configure wordpress"
> +                logger.info(msg)
> +                self.model.unit.status = BlockedStatus(msg)
> +                return
> +
> +            self.state.init = False
> +            logger.info("Wordpress configured and initialised")
> +            self.model.unit.status = ActiveStatus()
>  
> -        logger.info("Wordpress installed and initialised")
> -        self.state.init = False
> +        else:
> +            logger.info("Wordpress workload pod is ready and configured")
> +            self.model.unit.status = ActiveStatus()
>  
>      def configure_pod(self):
>          spec = self.make_pod_spec()
> diff --git a/src/wordpress.py b/src/wordpress.py
> new file mode 100644
> index 0000000..171e2cb
> --- /dev/null
> +++ b/src/wordpress.py
> @@ -0,0 +1,135 @@
> +#!/usr/bin/env python3
> +
> +import logging
> +import re
> +import secrets
> +import string
> +import subprocess
> +from urllib.parse import urlparse, urlunparse
> +from yaml import safe_load
> +
> +logger = logging.getLogger()
> +
> +
> +def import_requests():
> +    # Workaround until https://github.com/canonical/operator/issues/156 is fixed.
> +    try:
> +        import requests
> +    except ImportError:
> +        subprocess.check_call(['apt-get', 'update'])
> +        subprocess.check_call(['apt-get', '-y', 'install', 'python3-requests'])
> +        import requests
> +
> +    return requests
> +
> +
> +def password_generator():
> +    alphabet = string.ascii_letters + string.digits
> +    return ''.join(secrets.choice(alphabet) for i in range(24))
> +
> +
> +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)

These small helper functions make testing easier, good call. Mocking open is confusing.

> +
> +    def first_install(self, service_ip):
> +        """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",
> +            "Submit": "submit",
> +        }
> +        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"))
> +        missing = required_config.difference(payload.keys())
> +        if missing:
> +            logger.info("Error: missing wordpress settings: {}".format(missing))
> +            return

Since this returns True on success, it should return False on failure. Not None. Returning None makes it look like the function returns nothing, and for this and similar reasons never return mixed types.

> +        try:
> +            self.call_wordpress(service_ip, "/wp-admin/install.php?step=2", redirects=True, payload=payload)
> +        except Exception as e:
> +            logger.info("failed to call_wordpress: {}".format(e))
> +            return

return False

> +
> +        if not self.wordpress_configured(service_ip):
> +            return

return False

> +
> +        return True
> +
> +    def call_wordpress(self, service_ip, uri, redirects=True, payload={}, _depth=1):
> +        requests = import_requests()
> +
> +        max_depth = 10
> +        if _depth > max_depth:
> +            logger.info("Redirect loop detected in call_worpress()")
> +            raise RuntimeError("Redirect loop detected in call_worpress()")
> +        config = self.model_config
> +        headers = {"Host": config["blog_hostname"]}
> +        url = urlunparse(("http", service_ip, uri, "", "", ""))
> +        if payload:
> +            r = requests.post(url, allow_redirects=False, headers=headers, data=payload, timeout=30)
> +        else:
> +            r = requests.get(url, allow_redirects=False, headers=headers, timeout=30)
> +        if redirects and r.is_redirect:
> +            # Recurse, but strip the scheme and host first, we need to connect over HTTP by bare IP
> +            o = urlparse(r.headers.get("Location"))
> +            return self.call_wordpress(service_ip, o.path, redirects=redirects, payload=payload, _depth=_depth + 1)
> +        else:
> +            return r
> +
> +    def wordpress_configured(self, service_ip):
> +        """Check whether first install has been completed."""
> +        requests = import_requests()
> +
> +        # We have code on disk, check if configured
> +        try:
> +            r = self.call_wordpress(service_ip, "/", redirects=False)
> +        except requests.exceptions.ConnectionError:
> +            return False
> +
> +        if r.status_code == 302 and re.match("^.*/wp-admin/install.php", r.headers.get("location", "")):
> +            return False
> +        elif r.status_code == 302 and re.match("^.*/wp-admin/setup-config.php", r.headers.get("location", "")):
> +            logger.info("MySQL database setup failed, we likely have no wp-config.php")
> +            return False
> +        elif r.status_code in (500, 403, 404):
> +            raise RuntimeError("unexpected status_code returned from Wordpress")
> +
> +        return True
> +
> +    def is_vhost_ready(self, service_ip):
> +        """Check whether wordpress is available using http."""
> +        requests = import_requests()
> +
> +        rv = True
> +        # Check if we have WP code deployed at all
> +        try:
> +            r = self.call_wordpress(service_ip, "/wp-login.php", redirects=False)
> +            if r is None:
> +                logger.error("call_wordpress() returned None")
> +                rv = False
> +            if hasattr(r, "status_code") and r.status_code in (403, 404):
> +                logger.info("Wordpress returned an unexpected status {}".format(r.status_code))
> +                rv = False
> +        except requests.exceptions.ConnectionError:
> +            logger.info("HTTP vhost is not ready yet")
> +            rv = False
> +
> +        return rv


-- 
https://code.launchpad.net/~tcuthbert/charm-k8s-wordpress/+git/charm-k8s-wordpress/+merge/381798
Your team Wordpress Charmers is requested to review the proposed merge of ~tcuthbert/charm-k8s-wordpress/+git/charm-k8s-wordpress:operator into charm-k8s-wordpress:master.