wordpress-charmers team mailing list archive

[Tom Haddon <tom.haddon@xxxxxxxxxxxxx>]

Have added some comments

Diff comments:

> diff --git a/src/charm.py b/src/charm.py
> new file mode 100755
> index 0000000..7505ed0
> --- /dev/null
> +++ b/src/charm.py
> @@ -0,0 +1,289 @@
> +#!/usr/bin/env python3
> +
> +import io
> +import re
> +import subprocess
> +import sys
> +from pprint import pprint
> +from time import sleep
> +from urllib.parse import urlparse, urlunparse
> +from yaml import safe_load
> +
> +sys.path.append("lib")
> +
> +from ops.charm import CharmBase  # NoQA: E402
> +from ops.framework import StoredState  # NoQA: E402
> +from ops.main import main  # NoQA: E402
> +from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus  # NoQA: E402
> +
> +import logging  # NoQA: E402
> +
> +logger = logging.getLogger()
> +
> +
> +class Wordpress(CharmBase):
> +    state = StoredState()
> +
> +    def __init__(self, *args):
> +        super().__init__(*args)
> +        for event in (
> +            self.on.start,
> +            self.on.config_changed,
> +        ):
> +            self.framework.observe(event, self)
> +
> +    def on_start(self, event):
> +        logger.info("Here we go...")

I think this was just to test logging. It certainly doesn't seem like the text we'd want here in the end.

> +        # There may be a nicer way to do this!
> +        # https://github.com/canonical/operator/issues/156
> +        subprocess.check_call(['apt-get', 'update'])
> +        subprocess.check_call(['apt-get', '-y', 'install', 'python3-requests', 'pwgen'])
> +        # Initialise states
> +        self.state._started = True
> +        self.state._valid = False
> +        self.state._configured = False
> +        self.state._init = True
> +
> +    def on_config_changed(self, event):
> +        if not self.state._valid:
> +            valid = True
> +            config = self.model.config
> +            for k in ["image", "db_host", "db_name", "db_user", "db_password"]:
> +                if config[k].strip() == "":
> +                    self.model.unit.status = BlockedStatus("{!r} config is required".format(k))
> +                    valid = False
> +                    logger.info("config invalid")

Would be nice to say what config is expected, and what's missing

> +                    return event.defer()
> +            self.state._valid = valid
> +
> +        if not self.state._configured:
> +            return self.configure_pod()
> +
> +        if self.state._init:
> +            if not self.install_ready():
> +                logger.info("not ready to install yet")
> +                # Until k8s supports telling Juju our pod is available we need to defer loop
> +                # until it's ready.
> +                return event.defer()
> +                
> +            self.first_install()
> +
> +    def configure_pod(self):  # NoQA: C901

I don't think this needs the C901 exception does it?

> +        # only the leader can set_spec()
> +        if self.model.unit.is_leader():
> +            spec = self.make_pod_spec()
> +            self.model.unit.status = MaintenanceStatus("Configuring container")
> +            self.model.pod.set_spec(spec)
> +            self.state._configured = True
> +
> +    def make_pod_spec(self):
> +        config = self.model.config
> +        container_config = self.sanitized_container_config()
> +        if container_config is None:
> +            return  # Status already set
> +
> +        ports = [
> +            {"name": name, "containerPort": int(port), "protocol": "TCP"}
> +            for name, port in [addr.split(":", 1) for addr in config["ports"].split()]
> +        ]
> +
> +        # PodSpec v1? https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.13/#podspec-v1-core
> +        spec = {
> +            "containers": [
> +                {
> +                    "name": self.app.name,
> +                    "imageDetails": {"imagePath": config["image"]},
> +                    "ports": ports,
> +                    "config": container_config,
> +                    "readinessProbe": {"exec": {"command": ["/usr/bin/curl", "http://localhost"]}},
> +                }
> +            ]
> +        }
> +        out = io.StringIO()
> +        pprint(spec, out)

Is this still needed? Just for debugging?

> +        logger.info("Container environment config (sans secrets) <<EOM\n{}\nEOM".format(out.getvalue()))
> +
> +        # If we need credentials (secrets) for our image, add them to the spec after logging
> +        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"]
> +
> +        config_with_secrets = self.full_container_config()
> +        if config_with_secrets is None:
> +            return None  # Status already set
> +        container_config.update(config_with_secrets)
> +
> +        return spec
> +
> +    def sanitized_container_config(self):
> +        """Container config without secrets"""
> +        config = self.model.config
> +        if config["container_config"].strip() == "":
> +            container_config = {}
> +        else:
> +            container_config = safe_load(config["container_config"])
> +            if not isinstance(container_config, dict):
> +                self.model.unit.status = BlockedStatus("container_config is not a YAML mapping")
> +                return None
> +        container_config["WORDPRESS_DB_HOST"] = config["db_host"]
> +        container_config["WORDPRESS_DB_NAME"] = config["db_name"]
> +        container_config["WORDPRESS_DB_USER"] = config["db_user"]
> +        if config.get("wp_plugin_openid_team_map"):
> +            container_config["WP_PLUGIN_OPENID_TEAM_MAP"] = config["wp_plugin_openid_team_map"]
> +        return container_config
> +
> +    def full_container_config(self):
> +        """Container config with secrets"""
> +        config = self.model.config
> +        container_config = self.sanitized_container_config()
> +        if container_config is None:
> +            return None
> +        if config["container_secrets"].strip() == "":
> +            container_secrets = {}
> +        else:
> +            container_secrets = safe_load(config["container_secrets"])
> +            if not isinstance(container_secrets, dict):
> +                self.model.unit.status = BlockedStatus("container_secrets is not a YAML mapping")
> +                return None
> +        container_config.update(container_secrets)
> +        # Add secrets from charm config
> +        container_config["WORDPRESS_DB_PASSWORD"] = config["db_password"]
> +        if config.get("wp_plugin_akismet_key"):
> +            container_config["WP_PLUGIN_AKISMET_KEY"] = config["wp_plugin_akismet_key"]
> +        return container_config
> +
> +    def install_ready(self):
> +        ready = True
> +        config = self.model.config
> +        if not self.is_pod_up("website"):
> +            logger.info("Pod not yet ready - retrying")
> +            ready = False
> +
> +        try:
> +            self.is_vhost_ready()
> +        except:
> +            logger.info("Wordpress vhost is not yet listening - retrying")
> +            ready = False
> +
> +        if not config["initial_settings"]:
> +            logger.info("No initial_setting provided or wordpress already configured. Skipping first install.")
> +            logger.info("{} {}".format(self.state._configured, config["initial_settings"]))
> +            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 = subprocess.check_output(["/usr/bin/pwgen", "-s", "24", "1"]).decode().rstrip()

Feels like it might be better to do this in python rather than an external program. Would make testing easier if nothing else.

> +        payload = {
> +            "admin_password": admin_password,
> +            "blog_public": "checked",
> +            "Submit": "submit",
> +        }
> +        payload.update(safe_load(config["initial_settings"]))
> +        payload["admin_password2"] = 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: {0}".format(e))
> +            return
> +        # TODO: write_file
> +        # host.write_file(os.path.join("/root/", "initial.passwd"), payload["admin_password"], perms=0o400)

Yup, let's do this via StoredState for now, and file the issue about using Kubernetes secrets (and then include that in a comment so we can come back and use it once it's available).

> +        logger.info("wp installed")
> +        self.state._init = False
> +        self.model.unit.status = ActiveStatus()
> +        return True
> +
> +    def call_wordpress(self, uri, redirects=True, payload={}, _depth=1):
> +        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."""
> +        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."""
> +        import requests
> +        rv = True
> +
> +        # Check if we have WP code deployed at all
> +        try:
> +            r = self.call_wordpress("/wp-login.php", redirects=False)
> +        except requests.exceptions.ConnectionError:
> +            logger.info("call_wordpress() returned requests.exceptions.ConnectionError")
> +            rv = False
> +        if r is None:
> +            logger.info("call_wordpress() returned None")
> +            rv = False
> +        if hasattr(r, "status_code") and r.status_code in (403, 404):
> +            logger.info("call_wordpress() returned status {}".format(r.status_code))
> +            rv = False
> +
> +        return rv
> +
> +    def get_service_ip(self, endpoint):
> +        try:
> +            return str(self.model.get_binding(endpoint).network.ingress_addresses[0])
> +        except Exception as e:
> +            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(Wordpress)

This is obviously very bike-sheddy, but I feel like I tend to have seen more class names suffixed with "Charm" than not - i.e. WordpressCharm.

> +

Can likely remove this empty line. We should check lint settings on this file at some point.



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