Merge ~mthaddon/charm-k8s-discourse/+git/charm-k8s-discourse:redis-relation into charm-k8s-discourse:master

[Tom Haddon]

Add a redis relation, using the new redis-k8s library, but continue to support static redis config as well

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

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

[🤖 prod-jenkaas-is (prod-jenkaas-is)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-is (prod-jenkaas-is)]

FAILED: Continuous integration, rev:74fc0f5f551c691645b3296da782f1c13824a9d4
https://jenkins.canonical.com/is/job/lp-charm-k8s-discourse-ci/18/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/is/job/lp-charm-test/87/
    None: https://jenkins.canonical.com/is/job/lp-update-mp/152390/

Click here to trigger a rebuild:
https://jenkins.canonical.com/is/job/lp-charm-k8s-discourse-ci/18//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-is (prod-jenkaas-is)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-is (prod-jenkaas-is)]

FAILED: Continuous integration, rev:08af48d0f81729925d90a738113c9fc544122564
https://jenkins.canonical.com/is/job/lp-charm-k8s-discourse-ci/19/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/is/job/lp-charm-test/88/
    None: https://jenkins.canonical.com/is/job/lp-update-mp/152655/

Click here to trigger a rebuild:
https://jenkins.canonical.com/is/job/lp-charm-k8s-discourse-ci/19//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-is (prod-jenkaas-is)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-is (prod-jenkaas-is)]

PASSED: Continuous integration, rev:2ef40686420066f05e3d98432a308d1c8e702d57
https://jenkins.canonical.com/is/job/lp-charm-k8s-discourse-ci/20/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/is/job/lp-charm-test/90/
    None: https://jenkins.canonical.com/is/job/lp-update-mp/157128/

Click here to trigger a rebuild:
https://jenkins.canonical.com/is/job/lp-charm-k8s-discourse-ci/20//rebuild

review: Approve (continuous-integration)

[Joel Sing (jsing)]

LGTM - see various minor comments inline.

review: Approve (+1)

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

Change successfully merged at revision ab22995f6d560f85d94adec0d2a9d4d1b8813221

diff --git a/Makefile b/Makefile
index 95aa3b7..6a7bbe1 100644
--- a/Makefile
+++ b/Makefile
@@ -12,7 +12,7 @@ lint: blacken
 
 # We actually use the build directory created by charmcraft,
 # but the .charm file makes a much more convenient sentinel.
-unittest: discourse.charm
+unittest: discourse-k8s.charm
 	@tox -e unit
 
 test: lint unittest
@@ -21,7 +21,7 @@ clean:
 	@echo "Cleaning files"
 	@git clean -fXd
 
-discourse.charm: src/*.py requirements.txt
+discourse-k8s.charm: src/*.py requirements.txt
 	charmcraft build
 
 build-image:
diff --git a/README.md b/README.md
index 09898e4..b49813d 100644
--- a/README.md
+++ b/README.md
@@ -15,22 +15,13 @@ Discourse than the one currently deployed.
 For details on using Kubernetes with Juju [see here](https://juju.is/docs/kubernetes), and for
 details on using Juju with MicroK8s for easy local testing [see here](https://juju.is/docs/microk8s-cloud).
 
-To get started with a test environment, first deploy Redis in an IaaS model:
-
-    juju deploy cs:~redis-charmers/redis  # Note the deployed IP as ${REDIS_IP}
-
-Now deploy the Discourse and PostgreSQL charms within a Juju Kubernetes model
-as follows:
-
-    juju deploy cs:~postgresql-charmers/postgresql-k8s postgresql
-    juju deploy cs:~discourse-charmers/discourse-k8s discourse \
-      --config redis_host=${REDIS_IP} \
-      --config developer_emails="[e-mail]" \
-      --config external_hostname="foo.internal" \
-      --config smtp_address="127.0.0.1" \
-      --config smtp_domain="foo.internal"
-    juju add-relation discourse postgresql:db-admin
-    juju expose discourse
+To deploy into a Juju K8s model:
+
+    juju deploy postgresql-k8s
+    juju deploy redis-k8s
+    juju deploy discourse-k8s
+    juju relate discourse-k8s postgresql-k8s:db-admin
+    juju relate discourse-k8s redis-k8s
 
 Once the deployment is completed and the "discourse" workload state in `juju
 status` has changed to "active" you can visit http://{$discourse_ip}:3000 in a
diff --git a/config.yaml b/config.yaml
index a55dafd..85f0677 100644
--- a/config.yaml
+++ b/config.yaml
@@ -18,7 +18,7 @@ options:
   external_hostname:
     type: string
     description: "External hostname this discourse instance should respond to"
-    default: ""
+    default: "foo.internal"
   enable_cors:
     type: boolean
     description: "Enable Cross-origin Resource Sharing (CORS) at the application level (required for SSO)"
@@ -30,15 +30,15 @@ options:
   developer_emails:
     type: string
     description: "Comma delimited list of email addresses that should have developer level access"
-    default: ""
+    default: "[e-mail]"
   smtp_domain:
     type: string
     description: "Hostname that email sent by this discourse should appear to come from"
-    default: ""
+    default: "foo.internal"
   smtp_address:
     type: string
     description: "Hostname / IP that should be used to send SMTP mail"
-    default: ""
+    default: "127.0.0.1"
   smtp_port:
     type: int
     description: "Port to use when connecting to SMTP server"
diff --git a/lib/charms/redis_k8s/v0/redis.py b/lib/charms/redis_k8s/v0/redis.py
 new file mode 100644
index 0000000..70787b6
--- /dev/null
+++ b/lib/charms/redis_k8s/v0/redis.py
@@ -0,0 +1,97 @@
+"""Library for the redis relation.
+
+This library contains the Requires and Provides classes for handling the
+redis interface.
+
+Import `RedisRequires` in your charm by adding the following to `src/charm.py`:
+```
+from charms.redis_k8s.v0.redis import RedisRequires
+
+# In your charm's `__init__` method.
+self.redis = RedisRequires(self, self._stored)
+```
+And then wherever you need to reference the relation data:
+```
+for redis_unit in self._stored.redis_relation:
+    redis_host = self._stored.redis_relation[redis_unit]["hostname"]
+    redis_port = self._stored.redis_relation[redis_unit]["port"]
+```
+"""
+import logging
+
+from ops.charm import CharmEvents
+from ops.framework import EventBase, EventSource, Object
+
+# The unique Charmhub library identifier, never change it
+LIBID = "fe18a608cec5465fa5153e419abcad7b"
+
+# Increment this major API version when introducing breaking changes
+LIBAPI = 0
+
+# Increment this PATCH version before using `charmcraft publish-lib` or reset
+# to 0 if you are raising the major API version
+LIBPATCH = 1
+
+logger = logging.getLogger(__name__)
+
+
+class RedisRelationUpdatedEvent(EventBase):
+    pass
+
+
+class RedisRelationCharmEvents(CharmEvents):
+    redis_relation_updated = EventSource(RedisRelationUpdatedEvent)
+
+
+class RedisRequires(Object):
+    def __init__(self, charm, _stored):
+        # Define a constructor that takes the charm and it's StoredState
+        super().__init__(charm, "redis")
+        self.framework.observe(charm.on.redis_relation_changed, self._on_relation_changed)
+        self.framework.observe(charm.on.redis_relation_broken, self._on_relation_broken)
+        self._stored = _stored
+        self.charm = charm
+
+    def _on_relation_changed(self, event):
+        if not event.unit:
+            return
+
+        hostname = event.relation.data[event.unit].get("hostname")
+        port = event.relation.data[event.unit].get("port")
+        # Store some data from the relation in local state
+        self._stored.redis_relation[event.relation.id] = {"hostname": hostname, "port": port}
+
+        # Trigger an event that our charm can react to.
+        self.charm.on.redis_relation_updated.emit()
+
+    def _on_relation_broken(self, event):
+        # Remove the unit data from local state
+        self._stored.redis_relation.pop(event.relation.id, None)
+
+        # Trigger an event that our charm can react to.
+        self.charm.on.redis_relation_updated.emit()
+
+
+class RedisProvides(Object):
+    def __init__(self, charm, port):
+        super().__init__(charm, "redis")
+        self.framework.observe(charm.on.redis_relation_changed, self._on_relation_changed)
+        self._port = port
+
+    def _on_relation_changed(self, event):
+        if not self.model.unit.is_leader():
+            logger.debug("Relation changes ignored by non-leader")
+            return
+
+        event.relation.data[self.model.unit]['hostname'] = str(self._bind_address(event))
+        event.relation.data[self.model.unit]['port'] = str(self._port)
+        # The reactive Redis charm exposes also 'password'. When tackling
+        # https://github.com/canonical/redis-operator/issues/7 add 'password'
+        # field so that it matches the exposed interface information from it.
+        # event.relation.data[self.unit]['password'] = ''
+
+    def _bind_address(self, event):
+        relation = self.model.get_relation(event.relation.name, event.relation.id)
+        if address := self.model.get_binding(relation).network.bind_address:
+            return address
+        return self.app.name
diff --git a/metadata.yaml b/metadata.yaml
index 82e1aa8..0c8beb2 100644
--- a/metadata.yaml
+++ b/metadata.yaml
@@ -1,4 +1,5 @@
-name: discourse
+name: discourse-k8s
+display-name: Discourse
 summary: Discourse is the 100% open source discussion platform built for the next decade of the Internet.
 description: |
   Discourse is the 100% open source discussion platform built for the next decade of the Internet.
@@ -15,6 +16,8 @@ series:
   - kubernetes
 min-juju-version: 2.8.0
 requires:
+  redis:
+    interface: redis
   db:
     interface: pgsql
     limit: 1
diff --git a/src/charm.py b/src/charm.py
index d8f210b..0ef9f01 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -2,7 +2,13 @@
 # Copyright 2020 Canonical Ltd.
 # See LICENSE file for licensing details.
 
+import logging
+
 import ops.lib
+from charms.redis_k8s.v0.redis import (
+    RedisRelationCharmEvents,
+    RedisRequires,
+)
 from ops.charm import CharmBase
 from ops.main import main
 from ops.framework import StoredState
@@ -10,6 +16,9 @@ from ops.framework import StoredState
 from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus, WaitingStatus
 
 
+logger = logging.getLogger(__name__)
+
+
 pgsql = ops.lib.use("pgsql", 1, "[e-mail]")
 
 THROTTLE_LEVELS = {
@@ -52,7 +61,7 @@ def create_discourse_pod_config(config):
         'DISCOURSE_SMTP_USER_NAME': config['smtp_username'],
         'DISCOURSE_SMTP_PASSWORD': config['smtp_password'],
         'DISCOURSE_REDIS_HOST': config['redis_host'],
-        'DISCOURSE_REDIS_PORT': 6379,
+        'DISCOURSE_REDIS_PORT': config['redis_port'],
         'DISCOURSE_ENABLE_CORS': config['enable_cors'],
         'DISCOURSE_CORS_ORIGIN': config['cors_origin'],
         'DISCOURSE_REFRESH_MAXMIND_DB_DURING_PRECOMPILE_DAYS': "0",
@@ -161,7 +170,7 @@ def get_pod_spec(app_name, config):
     return pod_spec
 
 
-def check_for_config_problems(config):
+def check_for_config_problems(config, stored):
     """Check if there are issues with the juju config.
 
     - Primarily looks for missing config options using check_for_missing_config_fields()
@@ -169,7 +178,7 @@ def check_for_config_problems(config):
     - Returns a list of errors if any were found.
     """
     errors = []
-    missing_fields = check_for_missing_config_fields(config)
+    missing_fields = check_for_missing_config_fields(config, stored)
 
     if missing_fields:
         errors.append('Required configuration missing: {}'.format(" ".join(missing_fields)))
@@ -183,7 +192,7 @@ def check_for_config_problems(config):
     return errors
 
 
-def check_for_missing_config_fields(config):
+def check_for_missing_config_fields(config, stored):
     """Check for missing fields in juju config.
 
     - Returns a list of required fields that are either not present
@@ -194,13 +203,18 @@ def check_for_missing_config_fields(config):
     needed_fields = [
         'db_name',
         'smtp_address',
-        'redis_host',
         'cors_origin',
         'developer_emails',
         'smtp_domain',
         'discourse_image',
         'external_hostname',
     ]
+    # See if Redis connection information has been provided via a relation.
+    redis_hostname = None
+    for redis_unit in stored.redis_relation:
+        redis_hostname = stored.redis_relation[redis_unit]["hostname"]
+    if not redis_hostname:
+        needed_fields.append("redis_host")
     for key in needed_fields:
         if not config.get(key):
             missing_fields.append(key)
@@ -209,6 +223,7 @@ def check_for_missing_config_fields(config):
 
 
 class DiscourseCharm(CharmBase):
+    on = RedisRelationCharmEvents()
     stored = StoredState()
 
     def __init__(self, *args):
@@ -220,7 +235,13 @@ class DiscourseCharm(CharmBase):
         super().__init__(*args)
 
         self.stored.set_default(
-            db_name=None, db_user=None, db_password=None, db_host=None, has_db_relation=False, has_db_credentials=False
+            db_name=None,
+            db_user=None,
+            db_password=None,
+            db_host=None,
+            has_db_relation=False,
+            has_db_credentials=False,
+            redis_relation={},
         )
         self.framework.observe(self.on.leader_elected, self.configure_pod)
         self.framework.observe(self.on.config_changed, self.configure_pod)
@@ -230,6 +251,9 @@ class DiscourseCharm(CharmBase):
         self.framework.observe(self.db.on.database_relation_joined, self.on_database_relation_joined)
         self.framework.observe(self.db.on.master_changed, self.on_database_changed)
 
+        self.redis = RedisRequires(self, self.stored)
+        self.framework.observe(self.on.redis_relation_updated, self.configure_pod)
+
     def check_config_is_valid(self, config):
         """Check that the provided config is valid.
 
@@ -238,7 +262,7 @@ class DiscourseCharm(CharmBase):
         - Sets model status as appropriate.
         """
         valid_config = True
-        errors = check_for_config_problems(config)
+        errors = check_for_config_problems(config, self.stored)
 
         # Set status if we have a bad config.
         if errors:
@@ -266,6 +290,15 @@ class DiscourseCharm(CharmBase):
 
         - Configures pod using pod_spec generated from config.
         """
+        # Get redis connection information from config but allow overriding
+        # via a relation.
+        redis_hostname = self.config["redis_host"]
+        redis_port = 6379
+        for redis_unit in self.stored.redis_relation:
+            redis_hostname = self.stored.redis_relation[redis_unit]["hostname"]
+            redis_port = self.stored.redis_relation[redis_unit]["port"]
+            logging.debug("Got redis connection details from relation of %s:%s", redis_hostname, redis_port)
+
         # Set our status while we get configured.
         self.model.unit.status = MaintenanceStatus('Configuring pod')
 
@@ -282,6 +315,8 @@ class DiscourseCharm(CharmBase):
             config["db_user"] = self.stored.db_user
             config["db_password"] = self.stored.db_password
             config["db_host"] = self.stored.db_host
+            config["redis_host"] = redis_hostname
+            config["redis_port"] = redis_port
             # Get our spec definition.
             if self.check_config_is_valid(config):
                 # Get pod spec using our app name and config
diff --git a/tests/unit/fixtures/config_valid_complete.yaml b/tests/unit/fixtures/config_valid_complete.yaml
index 7815983..946388d 100644
--- a/tests/unit/fixtures/config_valid_complete.yaml
+++ b/tests/unit/fixtures/config_valid_complete.yaml
@@ -12,6 +12,7 @@ config:
     image_user: 'someuser'
     max_body_size: 25
     redis_host: 10.9.89.197
+    redis_port: 6379
     smtp_address: 167.89.123.58
     smtp_authentication: login
     smtp_domain: example.com
@@ -51,7 +52,7 @@ pod_config:
 pod_spec:
     version: 3
     containers:
-    - name: discourse
+    - name: discourse-k8s
       envConfig:
         DISCOURSE_CORS_ORIGIN: '*'
         DISCOURSE_DEVELOPER_EMAILS: [e-mail]
@@ -100,14 +101,14 @@ pod_spec:
                 nginx.ingress.kubernetes.io/session-cookie-max-age: '3600'
                 nginx.ingress.kubernetes.io/session-cookie-name: 'DISCOURSE_AFFINITY'
                 nginx.ingress.kubernetes.io/session-cookie-samesite: 'Lax'
-              name: discourse-ingress
+              name: discourse-k8s-ingress
               spec:
                 rules:
                   - host: discourse.local
                     http:
                         paths:
                          - backend:
-                             serviceName: discourse
+                             serviceName: discourse-k8s
                              servicePort: 3000
                            path: '/'
                 tls:
diff --git a/tests/unit/fixtures/config_valid_no_saml_fingerprint.yaml b/tests/unit/fixtures/config_valid_no_saml_fingerprint.yaml
index 4f7365a..cc76606 100644
--- a/tests/unit/fixtures/config_valid_no_saml_fingerprint.yaml
+++ b/tests/unit/fixtures/config_valid_no_saml_fingerprint.yaml
@@ -11,6 +11,7 @@ config:
     image_pass: ''
     image_user: ''
     redis_host: 10.9.89.197
+    redis_port: 6379
     smtp_address: 167.89.123.58
     smtp_authentication: login
     smtp_domain: example.com
diff --git a/tests/unit/fixtures/config_valid_no_saml_target_url.yaml b/tests/unit/fixtures/config_valid_no_saml_target_url.yaml
index 2f31a4f..1364998 100644
--- a/tests/unit/fixtures/config_valid_no_saml_target_url.yaml
+++ b/tests/unit/fixtures/config_valid_no_saml_target_url.yaml
@@ -11,6 +11,7 @@ config:
     image_pass: ''
     image_user: ''
     redis_host: 10.9.89.197
+    redis_port: 6379
     smtp_address: 167.89.123.58
     smtp_authentication: login
     smtp_domain: example.com
diff --git a/tests/unit/fixtures/config_valid_no_tls.yaml b/tests/unit/fixtures/config_valid_no_tls.yaml
index 1f7e021..8179e1a 100644
--- a/tests/unit/fixtures/config_valid_no_tls.yaml
+++ b/tests/unit/fixtures/config_valid_no_tls.yaml
@@ -12,6 +12,7 @@ config:
     image_user: ''
     max_body_size: 20
     redis_host: 10.9.89.197
+    redis_port: 6379
     smtp_address: 167.89.123.58
     smtp_authentication: login
     smtp_domain: example.com
@@ -51,7 +52,7 @@ pod_config:
 pod_spec:
     version: 3
     containers:
-    - name: discourse
+    - name: discourse-k8s
       envConfig:
         DISCOURSE_CORS_ORIGIN: '*'
         DISCOURSE_DEVELOPER_EMAILS: [e-mail]
@@ -99,13 +100,13 @@ pod_spec:
                 nginx.ingress.kubernetes.io/session-cookie-name: 'DISCOURSE_AFFINITY'
                 nginx.ingress.kubernetes.io/session-cookie-samesite: 'Lax'
                 nginx.ingress.kubernetes.io/ssl-redirect: 'false'
-              name: discourse-ingress
+              name: discourse-k8s-ingress
               spec:
                 rules:
                   - host: discourse.local
                     http:
                         paths:
                          - backend:
-                             serviceName: discourse
+                             serviceName: discourse-k8s
                              servicePort: 3000
                            path: '/'
diff --git a/tests/unit/fixtures/config_valid_with_tls.yaml b/tests/unit/fixtures/config_valid_with_tls.yaml
index 8e9e225..a4eff75 100644
--- a/tests/unit/fixtures/config_valid_with_tls.yaml
+++ b/tests/unit/fixtures/config_valid_with_tls.yaml
@@ -11,6 +11,7 @@ config:
     image_pass: 'discourse123'
     image_user: 'discourse_role'
     redis_host: 10.9.89.197
+    redis_port: 6379
     smtp_address: 167.89.123.58
     smtp_authentication: login
     smtp_domain: example.com
diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
index 20fe84e..40da7db 100644
--- a/tests/unit/test_charm.py
+++ b/tests/unit/test_charm.py
@@ -56,7 +56,7 @@ class TestDiscourseK8sCharmHooksDisabled(unittest.TestCase):
             if config_key.startswith('config_valid_'):
                 config_valid = self.harness.charm.check_config_is_valid(self.configs[config_key]['config'])
                 pod_config = create_discourse_pod_config(self.configs[config_key]['config'])
-                self.assertEqual(config_valid, True, 'Valid config is not recognized as valid')
+                self.assertEqual(config_valid, True, 'Valid config {} is not recognized as valid'.format(config_key))
                 self.assertEqual(
                     pod_config,
                     self.configs[config_key]['pod_config'],
@@ -77,7 +77,9 @@ class TestDiscourseK8sCharmHooksDisabled(unittest.TestCase):
         for config_key in self.configs:
             if config_key.startswith('config_invalid_'):
                 config_valid = self.harness.charm.check_config_is_valid(self.configs[config_key]['config'])
-                missing_fields = check_for_missing_config_fields(self.configs[config_key]['config'])
+                stored = SimpleNamespace()
+                stored.redis_relation = {}
+                missing_fields = check_for_missing_config_fields(self.configs[config_key]['config'], stored)
                 self.assertEqual(config_valid, False, 'Invalid config is not recognized as invalid')
                 self.assertEqual(
                     missing_fields,
@@ -91,6 +93,8 @@ class TestDiscourseK8sCharmHooksDisabled(unittest.TestCase):
         test_config['db_user'] = 'discourse_m'
         test_config['db_password'] = 'a_real_password'
         test_config['db_host'] = '10.9.89.237'
+        test_config['redis_host'] = '10.9.89.197'
+        test_config['redis_port'] = 6379
         db_event = SimpleNamespace()
         db_event.master = SimpleNamespace()
         db_event.master.user = 'discourse_m'