Merge ~hloeung/content-cache-charm:sites-persistent-ports-kv into content-cache-charm:master

[Haw Loeung]

Don't shuffling sites and port-pair mappings - LP:1865945

When sites are added or removed, site to port-pair mappings are reshuffled and re-allocated sequentially. This can cause issues if either HAProxy or Nginx fails to reload where sites end up talking to incorrect backends. Instead, we want to keep sites to port-pair mappings persistent.

[Joel Sing (jsing)]

Quick pass, few comments, but general approach seems fine.

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

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

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

Unable to determine commit message from repository - please click "Set commit message" and enter the commit message manually.

[Stuart Bishop (stub)]

Code looks good. A few inline comments, but likely nothing requiring a rereview.

review: Approve

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

Change successfully merged at revision c3721b36276e5b48cc656cd547961bc38bc00cf3

diff --git a/reactive/content_cache.py b/reactive/content_cache.py
index 450c63f..08f9bbc 100644
--- a/reactive/content_cache.py
+++ b/reactive/content_cache.py
@@ -12,7 +12,7 @@ import yaml
 from charms import reactive
 from charms.layer import status
 from charmhelpers import context
-from charmhelpers.core import hookenv, host
+from charmhelpers.core import hookenv, host, unitdata
 from charmhelpers.contrib.charmsupport import nrpe
 
 from lib import utils
@@ -500,26 +500,95 @@ def check_haproxy_alerts():
     reactive.set_flag('nagios-nrpe-telegraf.configured')
 
 
+def cleanout_sites(site_ports_map, sites):
+    new_site_ports_map = {}
+    for site, site_conf in site_ports_map.items():
+        if site not in sites:
+            continue
+
+        site_map = {'locations': {}}
+        site_map['cache_port'] = site_conf['cache_port']
+        for location, loc_conf in site_conf.get('locations', {}).items():
+            site_map['locations'][location] = loc_conf
+
+        new_site_ports_map[site] = site_map
+
+    return new_site_ports_map
+
+
+def allocated_ports(site_ports_map):
+    allocated_ports = []
+    for site, site_conf in site_ports_map.items():
+        allocated_ports.append(site_conf['cache_port'])
+        for location, loc_conf in site_conf.get('locations', {}).items():
+            if 'backend_port' not in loc_conf:
+                continue
+            allocated_ports.append(loc_conf['backend_port'])
+    return sorted(allocated_ports)
+
+
+def ports_map_lookup(ports_map, site, base_port, blacklist_ports=None, key=None):
+    if key:
+        (unused_port, port) = utils.next_port_pair(0, base_port, blacklist_ports=blacklist_ports)
+    else:
+        (port, unused_port) = utils.next_port_pair(base_port, 0, blacklist_ports=blacklist_ports)
+
+    if site not in ports_map:
+        return port
+
+    if key:
+        if 'locations' not in ports_map[site] or key not in ports_map[site]['locations']:
+            return port
+        return ports_map[site]['locations'][key].get('backend_port', port)
+    else:
+        return ports_map[site].get('cache_port', port)
+
+
 def sites_from_config(sites_yaml, sites_secrets=None, blacklist_ports=None):
     conf = yaml.safe_load(sites_yaml)
     sites = interpolate_secrets(conf, sites_secrets)
     cache_port = 0
     backend_port = 0
     new_sites = {}
+    existing_site_ports_map = unitdata.kv().get('existing_site_ports_map', {})
+    new_site_ports_map = {}
+    if not blacklist_ports:
+        blacklist_ports = []
+
+    blacklist_ports += allocated_ports(existing_site_ports_map)
+    # We need to clean out sites and backends that no longer
+    # exists. This should happen after we've built a list of ports to
+    # blacklist to ensure that we don't reuse one for a site that's
+    # being or been removed.
+    existing_site_ports_map = cleanout_sites(existing_site_ports_map, sites)
     for site, site_conf in sites.items():
         if not site_conf:
             continue
-        (cache_port, unused_backend_port) = utils.next_port_pair(
-            cache_port, backend_port, blacklist_ports=blacklist_ports
-        )
+        site_ports_map = {'locations': {}}
+        cache_port = ports_map_lookup(existing_site_ports_map, site, cache_port, blacklist_ports)
         site_conf['cache_port'] = cache_port
+        site_ports_map['cache_port'] = cache_port
+        # With the new port allocated, make sure it's blacklisted so it doesn't
+        # get reused later.
+        blacklist_ports.append(cache_port)
+
         for location, loc_conf in site_conf.get('locations', {}).items():
-            if loc_conf and loc_conf.get('backends'):
-                (unused_cache_port, backend_port) = utils.next_port_pair(
-                    cache_port, backend_port, blacklist_ports=blacklist_ports
-                )
-                loc_conf['backend_port'] = backend_port
+            if not loc_conf or not loc_conf.get('backends'):
+                continue
+            location_map = {}
+            backend_port = ports_map_lookup(existing_site_ports_map, site, backend_port, blacklist_ports, key=location)
+            loc_conf['backend_port'] = backend_port
+            location_map['backend_port'] = backend_port
+
+            # With the new port allocated, make sure it's blacklisted so it doesn't
+            # get reused later.
+            blacklist_ports.append(backend_port)
+            site_ports_map['locations'][location] = location_map
+
         new_sites[site] = site_conf
+        new_site_ports_map[site] = site_ports_map
+
+    unitdata.kv().set('existing_site_ports_map', new_site_ports_map)
     return new_sites
 
 
diff --git a/tests/unit/files/config_test_sites_map.txt b/tests/unit/files/config_test_sites_map.txt
new file mode 100644
index 0000000..7158e24
--- /dev/null
+++ b/tests/unit/files/config_test_sites_map.txt
@@ -0,0 +1,127 @@
+site1.local:
+    cache_port: 6080
+    locations:
+        /:
+            backend_port: 8080
+            backends:
+            - 127.0.1.10:80
+            - 127.0.1.11:80
+            - 127.0.1.12:80
+            origin-headers:
+            -   X-Origin-Key: Sae6oob2aethuosh
+            -   X-Some-Header-1: something one two three
+            -   X-Some-Header-2: something:one:two:three
+            signed-url-hmac-key: SrRorTsImr92B6FfSKBFrSIiR5NYGS+gdjd8oGoVH44=
+    port: 80
+site2.local:
+    cache_port: 6081
+    locations:
+        /:
+            backend-check-method: GET
+            backend-check-path: /check/
+            backend-maxconn: 1024
+            backend-tls: true
+            backend_port: 8081
+            backends:
+            - 127.0.1.10:443
+            - 127.0.1.11:443
+            - 127.0.1.12:443
+        /my-local-content/:
+            extra-config:
+            - root /var/www/html
+        /my-local-content2/:
+            extra-config:
+            - root /var/www/html
+    redirect-http-to-https: true
+    tls-cert-bundle-path: /etc/haproxy/site2-bundle.crt
+site3.local:
+    cache_port: 6082
+    locations:
+        /:
+            backend-options:
+            - forwardfor except 127.0.0.1
+            - forceclose
+            backend_port: 8082
+            backends:
+            - 127.0.1.10:80
+            - 127.0.1.11:80
+            - 127.0.1.12:80
+            cache-maxconn: 4096
+site4.local:
+    cache_port: 6083
+    locations:
+        /:
+            extra-config:
+            - autoindex on
+        /ubuntu/pool/:
+            extra-config:
+            - autoindex on
+            - auth_request /auth
+site5:
+    cache_port: 6084
+    locations:
+        /:
+            backend_port: 8083
+            backends:
+            - 127.0.1.10:80
+        /auth:
+            backend-path: /auth-check/
+            backend_port: 8084
+            backends:
+            - 127.0.1.11:80
+            cache-background-update: None
+            cache-min-uses: None
+            cache-validity: 200 401 1h
+            modifier: '='
+    site-name: site5.local
+site6.local:
+    cache_port: 6085
+    locations:
+        /:
+            backend-tls: true
+            backend_port: 8085
+            backends:
+            - 127.0.1.10:443
+site7.local:
+    cache_port: 6086
+    locations:
+        /:
+            backend-tls: false
+            backend_port: 8086
+            backends:
+            - 127.0.1.10:80
+    port: 444
+    tls-cert-bundle-path: /etc/haproxy/site7-bundle.crt
+site8.local:
+    cache_port: 6087
+    locations:
+        /:
+            backend-tls: false
+            backend_port: 8087
+            backends:
+            - 127.0.1.10:80
+        /auth:
+            backend-tls: true
+            backend_port: 8088
+            backends:
+            - 127.0.1.10:443
+            site-name: auth.site8.local
+    port: 444
+    tls-cert-bundle-path: /etc/haproxy/site8-bundle.crt
+site9.local:
+    cache_port: 6088
+    locations:
+        /:
+            backend-inter-time: 1m
+            backend-tls: false
+            backend_port: 8089
+            backends:
+            - 127.0.1.15:80
+            extra-config:
+            - proxy_force_ranges off
+        /private/content:
+            extra-config:
+            - root /srv/example1.com/content/
+            - autoindex on
+            - auth_request /auth
+            nagios-expect: 401 Unauthorized
diff --git a/tests/unit/test_content_cache.py b/tests/unit/test_content_cache.py
index 4cbbf05..1abaff0 100644
--- a/tests/unit/test_content_cache.py
+++ b/tests/unit/test_content_cache.py
@@ -7,12 +7,14 @@ from unittest import mock
 
 import freezegun
 import jinja2
+import yaml
 
 # We also need to mock up charms.layer so we can run unit tests without having
 # to build the charm and pull in layers such as layer-status.
 sys.modules['charms.layer'] = mock.MagicMock()
 
 from charms.layer import status  # NOQA: E402
+from charmhelpers.core import unitdata  # NOQA: E402
 
 # Add path to where our reactive layer lives and import.
 sys.path.append(os.path.dirname(os.path.dirname(os.path.dirname(os.path.realpath(__file__)))))
@@ -25,6 +27,8 @@ class TestCharm(unittest.TestCase):
         self.maxDiff = None
         self.tmpdir = tempfile.mkdtemp(prefix='charm-unittests-')
         self.addCleanup(shutil.rmtree, self.tmpdir)
+        os.environ['UNIT_STATE_DB'] = os.path.join(self.tmpdir, '.unit-state.db')
+        unitdata.kv().set('existing_site_ports_map', {})
 
         self.charm_dir = os.path.dirname(os.path.dirname(os.path.dirname(os.path.realpath(__file__))))
 
@@ -774,6 +778,108 @@ site5.local:
         }
         self.assertEqual(want, content_cache.sites_from_config(config_yaml))
 
+    def test_allocated_ports(self):
+        with open('tests/unit/files/config_test_sites_map.txt', 'r', encoding='utf-8') as f:
+            config_yaml = f.read()
+
+        sites = yaml.safe_load(config_yaml)
+        sites_list = list(sites.keys())
+
+        want = [
+            6080,
+            8080,
+            6081,
+            8081,
+            6082,
+            8082,
+            6083,
+            6084,
+            8083,
+            8084,
+            6085,
+            8085,
+            6086,
+            8086,
+            6087,
+            8087,
+            8088,
+            6088,
+            8089,
+        ]
+        self.assertEqual(sorted(want), content_cache.allocated_ports(sites))
+
+        want = []
+        self.assertEqual(want, content_cache.allocated_ports({}))
+
+        new = {}
+        new[sites_list[1]] = sites[sites_list[1]]
+        new[sites_list[len(sites_list) - 1]] = sites[sites_list[len(sites_list) - 1]]
+        want = [6081, 6088, 8081, 8089]
+        self.assertEqual(want, content_cache.allocated_ports(new))
+
+    def test_sites_from_config_no_reshuffling(self):
+        with open('tests/unit/files/config_test_sites_map.txt', 'r', encoding='utf-8') as f:
+            config_yaml = f.read()
+
+        sites = yaml.safe_load(config_yaml)
+        sites_list = list(sites.keys())
+
+        want = sites
+        self.assertEqual(want, content_cache.sites_from_config(config_yaml))
+
+        # Remove all except second and last site for testing. Check to
+        # make sure it's correct and ports aren't reshuffled.
+        new = {}
+        new[sites_list[1]] = sites[sites_list[1]]
+        new[sites_list[len(sites_list) - 1]] = sites[sites_list[len(sites_list) - 1]]
+        config_yaml = yaml.safe_dump(new, indent=4, default_flow_style=False)
+        want = new
+        self.assertEqual(want, content_cache.sites_from_config(config_yaml))
+
+        # Add two sites back and make sure the existing two aren't reshuffled.
+        new = {}
+        new[sites_list[1]] = sites[sites_list[1]]
+        new[sites_list[len(sites_list) - 1]] = sites[sites_list[len(sites_list) - 1]]
+        new[sites_list[0]] = sites[sites_list[0]]
+        new[sites_list[2]] = sites[sites_list[2]]
+        config_yaml = yaml.safe_dump(new, indent=4, default_flow_style=False)
+        want = new
+        self.assertEqual(want, content_cache.sites_from_config(config_yaml))
+
+        # Add new site somewhere in the middle.
+        new = {}
+        new[sites_list[1]] = sites[sites_list[1]]
+        new[sites_list[len(sites_list) - 1]] = sites[sites_list[len(sites_list) - 1]]
+        new[sites_list[0]] = sites[sites_list[0]]
+        new[sites_list[2]] = sites[sites_list[2]]
+        new['site11'] = {'locations': {'/': {'backend-tls': True, 'backends': ['127.0.1.10:443']}}}
+        config_yaml = yaml.safe_dump(new, indent=4, default_flow_style=False)
+        want = new
+        want['site11']['cache_port'] = 6083
+        want['site11']['locations']['/']['backend_port'] = 8083
+        self.assertEqual(want, content_cache.sites_from_config(config_yaml))
+
+        # Add a new site at the start, in the middle, and at the
+        # end. We also want to make sure we don't recycle and reuse a
+        # port for a site that's just been or is being removed.
+        new = {}
+        new[sites_list[1]] = sites[sites_list[1]]
+        new[sites_list[len(sites_list) - 1]] = sites[sites_list[len(sites_list) - 1]]
+        new[sites_list[0]] = sites[sites_list[0]]
+        new[sites_list[2]] = sites[sites_list[2]]
+        new['site0'] = {'locations': {'/': {'backend-tls': True, 'backends': ['127.0.1.10:443']}}}
+        new['site666'] = {'locations': {'/': {'backend-tls': True, 'backends': ['127.0.1.10:443']}}}
+        new['zzz'] = {'locations': {'/': {'backend-tls': True, 'backends': ['127.0.1.10:443']}}}
+        config_yaml = yaml.safe_dump(new, indent=4, default_flow_style=False)
+        want = new
+        want['site0']['cache_port'] = 6084
+        want['site0']['locations']['/']['backend_port'] = 8084
+        want['site666']['cache_port'] = 6085
+        want['site666']['locations']['/']['backend_port'] = 8085
+        want['zzz']['cache_port'] = 6089
+        want['zzz']['locations']['/']['backend_port'] = 8090
+        self.assertEqual(want, content_cache.sites_from_config(config_yaml))
+
     def test_sites_from_config_blacklist_ports(self):
         blacklist_ports = [6080, 8080]
         config_yaml = '''