Merge ~pjdc/charm-k8s-mattermost/+git/charm-k8s-mattermost:control-source into charm-k8s-mattermost:master

[Paul Collins]

add ingress_whitelist_source_range setting, set nginx.ingress.kubernetes.io/ssl-redirect to false when site_url scheme is "http"

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

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

[Tom Haddon (mthaddon)]

Do we think we're going to need this option? Happy to approve if so (although some config validation would be good), but I'm not sure when we'd be using this, and any code/config we can avoid is a good thing.

review: Needs Information

[Paul Collins (pjdc)]

I'm certainly planning to use it while our deployment is being validated -- due to the shared ingress there's no other practical way (unless Mattermost has an internal setting) to restrict access.

As for validation, I suppose we could do something like split the value on commas and confirm that each element is a valid CIDR before updating the pod spec.

[Tom Haddon (mthaddon)]

> I'm certainly planning to use it while our deployment is being validated --
> due to the shared ingress there's no other practical way (unless Mattermost
> has an internal setting) to restrict access.

Ack.

> As for validation, I suppose we could do something like split the value on
> commas and confirm that each element is a valid CIDR before updating the pod
> spec.

Yeah, let's add that in please.

[Paul Collins (pjdc)]

> > As for validation, I suppose we could do something like split the value on
> > commas and confirm that each element is a valid CIDR before updating the pod
> > spec.
>
> Yeah, let's add that in please.

Pushed. Results in the following:

Unit Workload Agent Address Ports Message
mattermost/2* blocked idle 10.1.1.142 8065/TCP ingress_whitelist_source_range: invalid network(s): 10.2.4.9/24

review: Needs Resubmitting

[Tom Haddon (mthaddon)]

Two comments inline, neither of them are blocking.

[Paul Collins (pjdc)]

> Two comments inline, neither of them are blocking.

I've rebased on origin with S3 landed, reworked the check based on your suggestions, integrated the S3 required-settings check into the check, and added some tests for check_ranges.

review: Needs Resubmitting

[Paul Collins (pjdc)]

Yielding the following:

Unit Workload Agent Address Ports Message
mattermost/1* blocked idle 10.1.1.23 8065/TCP required setting(s) empty: s3_bucket; ingress_whitelist_source_range: invalid network(s): 91.189.92.242/25

[Tom Haddon (mthaddon)]

LGTM, thx

review: Approve

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

Change successfully merged at revision 627cbc0db4e21161ca05bea83bb5b5373035d9cf

diff --git a/config.yaml b/config.yaml
index 4799770..2d6bf21 100644
--- a/config.yaml
+++ b/config.yaml
@@ -3,6 +3,13 @@ options:
         type: boolean
         description: Set the Mattermost log level to DEBUG, otherwise INFO.
         default: false
+    ingress_whitelist_source_range:
+        type: string
+        description: |
+          A comma-separated list of CIDRs to store in the ingress.kubernetes.io/whitelist-source-range annotation.
+
+          This can be used to lock down access to Mattermost based on source IP address.
+        default: ''
     open_server:
         type: boolean
         description: Allows users to sign up from the root page without an invite
diff --git a/src/charm.py b/src/charm.py
index 888969f..52e7034 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -3,6 +3,8 @@
 import sys
 sys.path.append('lib')  # noqa: E402
 
+from ipaddress import ip_network
+
 from urllib.parse import urlparse
 
 from ops.charm import (
@@ -17,6 +19,7 @@ from ops.framework import (
 from ops.main import main
 from ops.model import (
     ActiveStatus,
+    BlockedStatus,
     MaintenanceStatus,
     WaitingStatus,
 )
@@ -42,6 +45,19 @@ class MattermostCharmEvents(CharmEvents):
     db_master_available = EventSource(MattermostDBMasterAvailableEvent)
 
 
+def check_ranges(ranges, name):
+    if ranges:
+        networks = ranges.split(',')
+        invalid_networks = []
+        for network in networks:
+            try:
+                ip_network(network)
+            except ValueError:
+                invalid_networks.append(network)
+        if invalid_networks:
+            return '{}: invalid network(s): {}'.format(name, ', '.join(invalid_networks))
+
+
 class MattermostK8sCharm(CharmBase):
 
     state = StoredState()
@@ -101,6 +117,19 @@ class MattermostK8sCharm(CharmBase):
 
         # TODO(pjdc): Emit event when we add support for read replicas.
 
+    def _check_for_config_problems(self):
+        problems = []
+
+        missing = self._missing_charm_settings()
+        if missing:
+            problems.append('required setting(s) empty: {}'.format(', '.join(sorted(missing))))
+
+        ranges = self.model.config['ingress_whitelist_source_range']
+        if ranges:
+            problems.append(check_ranges(ranges, 'ingress_whitelist_source_range'))
+
+        return '; '.join(filter(None, problems))
+
     def _make_pod_spec(self):
         mattermost_image_details = self.mattermost_image.fetch()
         pod_config = self._make_pod_config()
@@ -177,6 +206,7 @@ class MattermostK8sCharm(CharmBase):
         if not site_url:
             return None
         parsed = urlparse(site_url)
+        annotations = {}
 
         if parsed.scheme.startswith('http'):
             ingress = {
@@ -205,6 +235,15 @@ class MattermostK8sCharm(CharmBase):
                 tls_secret_name = self.model.config['tls_secret_name']
                 if tls_secret_name:
                     ingress['spec']['tls'][0]['secretName'] = tls_secret_name
+            else:
+                annotations['nginx.ingress.kubernetes.io/ssl-redirect'] = 'false'
+
+            ingress_whitelist_source_range = self.model.config['ingress_whitelist_source_range']
+            if ingress_whitelist_source_range:
+                annotations['nginx.ingress.kubernetes.io/whitelist-source-range'] = ingress_whitelist_source_range
+
+            if annotations:
+                ingress['annotations'] = annotations
 
             return {
                 "kubernetesResources": {
@@ -222,13 +261,12 @@ class MattermostK8sCharm(CharmBase):
             self.unit.status = ActiveStatus()
             return
 
-        missing = self._missing_charm_settings()
-        if missing:
-            self.unit.status = WaitingStatus('Some required settings are empty: {}'.format(', '.join(sorted(missing))))
+        problems = self._check_for_config_problems()
+        if problems:
+            self.unit.status = BlockedStatus(problems)
             return
 
         self.unit.status = MaintenanceStatus('Configuring pod')
-
         pod_spec = self._make_pod_spec()
 
         # Due to https://github.com/canonical/operator/issues/293 we
diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
index 97fbede..17e7464 100644
--- a/tests/unit/test_charm.py
+++ b/tests/unit/test_charm.py
@@ -7,7 +7,11 @@ import sys
 sys.path.append('lib')  # noqa: E402
 sys.path.append('src')  # noqa: E402
 
-from charm import MattermostK8sCharm
+from charm import (
+    MattermostK8sCharm,
+    check_ranges,
+)
+
 from ops import testing
 
 CONFIG_NO_S3_SETTINGS_S3_ENABLED = {
@@ -23,6 +27,10 @@ CONFIG_NO_S3_SETTINGS_S3_DISABLED_NO_DEFAULTS = {
     's3_enabled': False,
 }
 
+RANGE_BAD = '10.242.0.0/8,91.189.92.242/25'
+RANGE_GOOD = '10.0.0.0/8,91.189.92.128/25'
+RANGE_MIXED = '10.242.0.0/8,91.189.92.128/25'
+
 
 class TestMattermostK8sCharm(unittest.TestCase):
 
@@ -39,3 +47,15 @@ class TestMattermostK8sCharm(unittest.TestCase):
         self.harness.charm.model.config = copy.deepcopy(CONFIG_NO_S3_SETTINGS_S3_DISABLED_NO_DEFAULTS)
         expected = []
         self.assertEqual(sorted(self.harness.charm._missing_charm_settings()), expected)
+
+    def test_check_ranges_bad(self):
+        expected = 'range_bad: invalid network(s): 10.242.0.0/8, 91.189.92.242/25'
+        self.assertEqual(check_ranges(RANGE_BAD, 'range_bad'), expected)
+
+    def test_check_ranges_good(self):
+        expected = None
+        self.assertEqual(check_ranges(RANGE_GOOD, 'range_good'), expected)
+
+    def test_check_ranges_mixed(self):
+        expected = 'range_mixed: invalid network(s): 10.242.0.0/8'
+        self.assertEqual(check_ranges(RANGE_MIXED, 'range_mixed'), expected)