Merge ~nick-moffitt/content-cache-charm:open-ports into content-cache-charm:master

[Nick Moffitt]

open_port to fix LP:1825320

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

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

[Joel Sing (jsing)]

This only opens ports - don't we also need to close them?

review: Needs Information

[Nick Moffitt (nick-moffitt)]

When would we close them? And when would we have the information about which ports to close?

[Joel Sing (jsing)]

LGTM with one nit comment inline.

It would be good to have test coverage of the open_port/close_port calls though (follow up is fine).

review: Approve (+1)

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

Change successfully merged at revision 9a84bd3cf23a415ad640a5564f9cb570f1fcc2d6

diff --git a/reactive/content_cache.py b/reactive/content_cache.py
index 88509a5..305ea0a 100644
--- a/reactive/content_cache.py
+++ b/reactive/content_cache.py
@@ -154,6 +154,8 @@ def configure_haproxy():
         status.blocked('list of sites provided is invalid')
         return
 
+    old_ports = set(hookenv.opened_ports())
+    opened_ports = set()
     # We need to slot in the caching layer here.
     new_conf = {}
     for site, site_conf in sites.items():
@@ -171,6 +173,12 @@ def configure_haproxy():
             new_conf[cached_site]['tls-cert-bundle-path'] = tls_cert_bundle_path
 
         new_conf[cached_site]['port'] = site_conf.get('port') or default_port
+        try:
+            new_port = int(new_conf[cached_site]['port'])
+        except ValueError as e:
+            hookenv.log('Only integer ports are supported: {}'.format(e))
+        hookenv.open_port(new_port)
+        opened_ports.add(new_port)
 
         # XXX: Reduce complexity here
 
@@ -225,6 +233,9 @@ def configure_haproxy():
             if not new_conf[cached_site]['locations']:
                 new_conf[cached_site]['locations'][location] = new_cached_loc_conf
 
+    for obsolete_port in old_ports.difference(opened_ports):
+        hookenv.close_port(obsolete_port)
+
     if haproxy.monitoring_password:
         rendered_config = haproxy.render(new_conf, monitoring_password=haproxy.monitoring_password)
     else:
diff --git a/tests/unit/test_content_cache.py b/tests/unit/test_content_cache.py
index e1390c0..4752787 100644
--- a/tests/unit/test_content_cache.py
+++ b/tests/unit/test_content_cache.py
@@ -221,13 +221,18 @@ site1.local:
 
         with mock.patch('lib.haproxy.HAProxyConf.conf_file', new_callable=mock.PropertyMock) as mock_conf_file:
             mock_conf_file.return_value = os.path.join(self.tmpdir, 'haproxy.cfg')
-            with mock.patch('charmhelpers.core.host.pwgen', return_value="biometricsarenotsecret"):
-              content_cache.configure_haproxy()
+            with mock.patch('charmhelpers.core.host.pwgen', return_value="biometricsarenotsecret"), \
+                 mock.patch('charmhelpers.core.hookenv.opened_ports', return_value=[443]), \
+                 mock.patch('charmhelpers.core.hookenv.open_port'), mock.patch('charmhelpers.core.hookenv.close_port'):
+                content_cache.configure_haproxy()
             self.assertFalse(service_start_or_restart.assert_called_with('haproxy'))
 
             # Again, this time should be no change so no need to restart HAProxy
             service_start_or_restart.reset_mock()
-            content_cache.configure_haproxy()
+            with mock.patch('charmhelpers.core.hookenv.opened_ports', return_value=[443]),\
+                 mock.patch('charmhelpers.core.hookenv.open_port'), \
+                 mock.patch('charmhelpers.core.hookenv.close_port'):
+                content_cache.configure_haproxy()
             self.assertFalse(service_start_or_restart.assert_not_called())
 
             with open('tests/unit/files/content_cache_rendered_haproxy_test_output.txt', 'r', encoding='utf-8') as f: