wordpress-charmers team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> diff --git a/src/charm.py b/src/charm.py
> new file mode 100755
> index 0000000..5204372
> --- /dev/null
> +++ b/src/charm.py
> @@ -0,0 +1,346 @@
> +#!/usr/bin/env python3
> +
> +import io
> +import re
> +import secrets
> +import string
> +import subprocess
> +import sys
> +from pprint import pprint
> +from urllib.parse import urlparse, urlunparse
> +from yaml import safe_load
> +
> +sys.path.append("lib")
> +
> +from ops.charm import CharmBase, CharmEvents  # NoQA: E402
> +from ops.framework import EventBase, EventSource, StoredState  # NoQA: E402
> +from ops.main import main  # NoQA: E402
> +from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus, WaitingStatus  # NoQA: E402
> +
> +import logging  # NoQA: E402

This is stdlib and goes in the first batch of imports.

> +
> +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

Normally I just stick this in the main imports section, so call sites don't need to bother with 'requests = import_requests()'. But what you have is arguably better, avoiding import side effects and all.

> +
> +
> +def password_generator():
> +    alphabet = string.ascii_letters + string.digits
> +    return ''.join(secrets.choice(alphabet) for i in range(24))
> +
> +
> +def generate_pod_config(config, secured=True):
> +    """Kubernetes pod config generator.
> +
> +    generate_pod_config generates Kubernetes deployment config.
> +    If the secured keyword is set then it will return a sanitised copy
> +    without exposing secrets.
> +    """
> +    pod_config = {}
> +    if config["container_config"].strip():
> +        pod_config = safe_load(config["container_config"])
> +
> +    pod_config["WORDPRESS_DB_HOST"] = config["db_host"]
> +    pod_config["WORDPRESS_DB_NAME"] = config["db_name"]
> +    pod_config["WORDPRESS_DB_USER"] = config["db_user"]
> +    if config.get("wp_plugin_openid_team_map"):
> +        pod_config["WP_PLUGIN_OPENID_TEAM_MAP"] = config["wp_plugin_openid_team_map"]
> +
> +    if secured:
> +        return pod_config
> +
> +    # Add secrets from charm config
> +    pod_config["WORDPRESS_DB_PASSWORD"] = config["db_password"]
> +    if config.get("wp_plugin_akismet_key"):
> +        pod_config["WP_PLUGIN_AKISMET_KEY"] = config["wp_plugin_akismet_key"]
> +
> +    return pod_config
> +
> +
> +class WordpressInitialiseEvent(EventBase):
> +    """Custom event for signalling Wordpress initialisation.
> +
> +    WordpressInitialiseEvent allows us to signal the handler for
> +    the initial Wordpress setup logic.
> +    """
> +
> +    pass
> +
> +
> +class WordpressCharmEvents(CharmEvents):
> +    """Register custom charm events.
> +
> +    WordpressCharmEvents registeres the custom WordpressInitialiseEvent
> +    event to the charm.
> +    """
> +
> +    wp_initialise = EventSource(WordpressInitialiseEvent)
> +
> +
> +class WordpressK8sCharm(CharmBase):
> +    state = StoredState()
> +    # Override the default list of event handlers with our WordpressCharmEvents subclass.
> +    on = WordpressCharmEvents()
> +
> +    def __init__(self, *args):
> +        super().__init__(*args)
> +
> +        self.framework.observe(self.on.start, self.on_start)
> +        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.website_relation_changed, self.on_website_relation_changed)
> +        self.framework.observe(self.on.wp_initialise, self.on_wp_initialise)
> +
> +        self.state.set_default(_init=True)
> +        self.state.set_default(_started=False)
> +        self.state.set_default(_valid=False)
> +        self.state.set_default(_configured=False)

Personally, since this is not intended to be subclassed or used as an API, I'd drop the leading underscores from all the above. If these are considered private, so is self._state itself and methods like self._on_config_changed().

> +
> +    def on_website_relation_changed(self, event):
> +        logger.info("on_website_relation_changed fired!!! {}".format(event))
> +
> +    def on_start(self, event):
> +        self.state._started = True
> +
> +    def on_config_changed(self, event):
> +        if not self.state._started:
> +            event.defer()
> +            return

You can drop the above check-and-def, and the on_start method above it entirely. Juju guarantees the install and start hooks are called first. With operator framework charms, the hooks/config-changed file doesn't even exist until it is created by the start hook (or install hook for non-k8s charms).

> +
> +        if not self.state._valid:
> +            config = self.model.config
> +            want = ("image", "db_host", "db_name", "db_user", "db_password")
> +            missing = [k for k in want if config[k].rstrip() == ""]
> +            if missing:
> +                message = " ".join(missing)
> +                logger.info("Missing required config: {}".format(message))
> +                self.model.unit.status = BlockedStatus("{} config is required".format(message))
> +                event.defer()
> +                return
> +
> +            self.state._valid = True

The above only checks validity once, and config changes after initial deployment are not checked. Instead, always check validity. And the validity check can be broken out into a small helper method to make unit testing easier.

self.state._valid = self.is_config_valid()
if not self.state._valid:
    event.defer()
    return

> +
> +        if not self.state._configured:
> +            logger.info("Configuring pod")
> +            self.configure_pod()
> +            return

If you do this, something will need to reset self.state._configured to catch charm reconfiguration. Instead, better to drop self.state._configured and just call self.configure_pod() every time. It should be idempotent.

> +
> +        if self.model.unit.is_leader() and self.state._init:
> +            self.on.wp_initialise.emit()

There are a couple of problems here. What happens if the leader changes before the on_wp_initialise() handler has completed successfully? What happens if the leader changes after the on_wp_initialize() handler has completed successfully?

If on_wp_initialise is idempotent, it is easier. If we need to ensure it only runs once (ie. calling it won't destroy a production deployment), it becomes much trickier as we need to use leadership settings (now apparently deprecated) or a peer relation. I've opened https://github.com/canonical/operator/issues/215 because the peer relation would be a lot more cumbersome. Or perhaps we can ask wordpress if it has been initialised, either by having the initialisation create a page we can try to retrieve, or a setting we can check that we know gets changed from a default setting.

> +
> +    def on_wp_initialise(self, event):
> +        if not self.state._init:
> +            event.defer()
> +            return

You should remove the above check-and-defer. You already check before emitting the wp_initialise event, and you shouldn't defer events that will never be run (deferred forever), since they will still be emitted and deferred every hook forever and worse case you blow a stack if you accumulate too many.

> +
> +        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.
> +            self.model.unit.status = WaitingStatus("Waiting for pod to be ready")
> +            event.defer()
> +            return
> +
> +        installed = self.first_install()
> +        if not installed:
> +            event.defer()
> +            return
> +
> +        logger.info("Wordpress installed and initialised")
> +        self.state._init = False
> +
> +    def configure_pod(self):
> +        # only the leader can set_spec()
> +        if self.model.unit.is_leader():
> +            spec = self.make_pod_spec()
> +            self.model.unit.status = MaintenanceStatus("Configuring pod")
> +            self.model.pod.set_spec(spec)
> +            logger.info("Pod configured")
> +            self.state._configured = True
> +            self.model.unit.status = MaintenanceStatus("Pod configured")

Since per above configure_pod() is being called every config-changed and update-status hook, we should only call pod.set_spec() if the spec has actually changed (unless Juju checks this for us).

def configure_pod(self):
    spec = self.make_pod_spec()
    if spec != self.state._spec:  # Initialized in __init__
        self.state._spec = spec
        if self.model.unit.is_leader():
            self.model.unit.status = MaintenanceStatus("Configuring pod")
            self.model.pod.set_spec(spec)
            self.model.unit.status = MaintenanceStatus("Pod configured")  # Need to ensure active status later!
        else:
            logger.info("spec changes ignored by non-leader")
    else:
        logger.info("pod spec unchanged")

> +
> +    def make_pod_spec(self):
> +        config = self.model.config
> +        full_pod_config = generate_pod_config(config, secured=False)
> +        secure_pod_config = generate_pod_config(config, secured=True)
> +
> +        ports = [
> +            {"name": name, "containerPort": int(port), "protocol": "TCP"}
> +            for name, port in [addr.split(":", 1) for addr in config["ports"].split()]
> +        ]
> +
> +        spec = {
> +            "containers": [
> +                {
> +                    "name": self.app.name,
> +                    "imageDetails": {"imagePath": config["image"]},
> +                    "ports": ports,
> +                    "config": secure_pod_config,
> +                    "readinessProbe": {"exec": {"command": ["/bin/cat", "/srv/wordpress-helpers/.ready"]}},
> +                }
> +            ]
> +        }
> +
> +        out = io.StringIO()
> +        pprint(spec, out)
> +        logger.info("Kubernetes Pod spec config (sans secrets) <<EOM\n{}\nEOM".format(out.getvalue()))
> +
> +        if config.get("image_user") and config.get("image_pass"):
> +            spec.get("containers")[0].get("imageDetails")["username"] = config["image_user"]
> +            spec.get("containers")[0].get("imageDetails")["password"] = config["image_pass"]

In a future branch, we should look at using the recommended approach of attaching the image info as a Juju resource. I only did it this way originally because I got to avoid an experimental reactive layer I didn't want to decipher.

> +
> +        secure_pod_config.update(full_pod_config)
> +
> +        return spec
> +
> +    def install_ready(self):
> +        ready = True
> +        config = self.model.config
> +        if not self.is_pod_up("website"):
> +            logger.info("Pod not yet ready - retrying")

Probably drop the 'retrying' here and a few lines ago, since you are not looping and retrying. Any retry will happen next hook.

> +            ready = False
> +
> +        try:
> +            if not self.is_vhost_ready():
> +                ready = False
> +        except Exception as e:
> +            logger.info("Wordpress vhost is not yet listening - retrying: {}".format(e))
> +            ready = False
> +
> +        if not config["initial_settings"]:
> +            logger.info("No initial_setting provided. Skipping first install.")
> +            ready = False
> +
> +        return ready
> +
> +    def first_install(self):
> +        """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"]
> +
> +        # Until juju run-action supports operator pods we must drop the initial
> +        # admin password as a file in the workload pod.

I don't understand this comment. Isn't /root/initial.password on the operator pod? Or what is the workload pod in this context?

I'm wondering if we should require the password to be provided in config, and only start generating passwords in future versions when we can write an action to retrieve them. I also don't think we can rely on /root/initial.password sticking around for ever, as it will be lost if the operator pod is restarted for any reason. You could get away with storing it in $CHARM_DIR, which is stateful IIRC, but then when retrieving it you need to worry about which $CHARM_DIR was the leader's.

> +        with open("/root/initial.passwd", "w") as f:
> +            f.write(payload["admin_password"])
> +
> +        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
> +        try:
> +            self.call_wordpress("/wp-admin/install.php?step=2", redirects=True, payload=payload)
> +        except Exception as e:
> +            logger.info("failed to call_wordpress: {}".format(e))
> +            return
> +
> +        if not self.wordpress_configured():
> +            self.model.unit.status = BlockedStatus("Failed to install wordpress")
> +
> +        self.model.unit.status = ActiveStatus()
> +        return True
> +
> +    def call_wordpress(self, 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
> +        service_ip = self.get_service_ip("website")
> +        if service_ip:
> +            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(o.path, redirects=redirects, payload=payload, _depth=_depth + 1)
> +            else:
> +                return r
> +        else:
> +            logger.info("Error getting service IP")
> +            return False
> +
> +    def wordpress_configured(self):
> +        """Check whether first install has been completed."""
> +        requests = import_requests()
> +
> +        # Check whether pod is deployed
> +        if not self.is_pod_up("website"):
> +            return False
> +        # Check if we have WP code deployed at all
> +        if not self.is_vhost_ready():
> +            return False
> +        # We have code on disk, check if configured
> +        try:
> +            r = self.call_wordpress("/", 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")
> +            self.model.unit.status = BlockedStatus("MySQL database setup failed, we likely have no wp-config.php")
> +            return False
> +        else:
> +            return True
> +
> +    def is_vhost_ready(self):
> +        """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("/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
> +
> +    def get_service_ip(self, endpoint):
> +        try:
> +            return str(self.model.get_binding(endpoint).network.ingress_addresses[0])
> +        except Exception:
> +            logger.info("We don't have any ingress addresses yet")
> +
> +    def is_pod_up(self, endpoint):
> +        """Check to see if the pod of a relation is up"""
> +        return self.get_service_ip(endpoint) or False
> +
> +
> +if __name__ == "__main__":
> +    main(WordpressK8sCharm)


-- 
https://code.launchpad.net/~tcuthbert/charm-k8s-wordpress/+git/charm-k8s-wordpress/+merge/381502
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.