new storage cache config option (#68)

* add new storage-cache config option

* unit test storage-cache config (gotta start somewhere)

* fixup gh action and integration tests

* validate storage-cache config

* unit test has_invalid_config

* only populate [storage][cache] when 'inmemory' is specified

.github/workflows/main.yaml

@@ -12,6 +12,8 @@ jobs:
   lint-unit:
     name: Lint Unit
     uses: charmed-kubernetes/workflows/.github/workflows/lint-unit.yaml@main
+    with:
+      python: "['3.8', '3.9', '3.10', '3.11']"
     needs:
       - call-inclusive-naming-check 
 
@@ -22,14 +24,15 @@ jobs:
       - lint-unit
     steps:
       - name: Check out code
-        uses: actions/checkout@v2
+        uses: actions/checkout@v3
       - name: Setup Python
-        uses: actions/setup-python@v2
+        uses: actions/setup-python@v4
         with:
-          python-version: 3.9
+          python-version: "3.10"
       - name: Setup operator environment
         uses: charmed-kubernetes/actions-operator@main
         with:
           provider: lxd
+          juju-channel: 3/stable
       - name: Run integration test
         run: tox -e integration

config.yaml

@@ -84,6 +84,12 @@ options:
     default:
     description: |
       The HTTPS proxy the registry server should use to access the upstream registry.
+  storage-cache:
+    type: string
+    default: "inmemory"
+    description: |
+      Cache provider for image layer metadata. Valid options are "inmemory" or
+      "disabled".
   storage-delete:
     type: boolean
     default: false

lib/charms/layer/docker_registry.py

@@ -15,6 +15,22 @@
 from charms.reactive.helpers import any_file_changed, data_changed
 
 
+def has_invalid_config():
+    """Checks charm config for invalid values.
+
+    If invalid values are found, return a list of the offending key(s). Otherwise,
+    return an empty list.
+    """
+    charm_config = hookenv.config()
+    bad_config = []
+
+    storage_cache = charm_config.get("storage-cache", "")
+    if storage_cache not in ["inmemory", "disabled"]:
+        bad_config.append("storage-cache")
+
+    return bad_config
+
+
 def configure_registry():
     '''Recreate the docker registry config.yml.'''
     charm_config = hookenv.config()
@@ -108,7 +124,7 @@ def configure_registry():
             'password': charm_config.get('cache-password', ''),
         }
 
-    # storage (https://docs.docker.com/registry/configuration/#storage)
+    # storage (https://distribution.github.io/distribution/about/configuration/#storage)
     # we must have 1 (and only 1) storage driver
     storage = {}
     if charm_config.get('storage-swift-authurl'):
@@ -132,7 +148,6 @@ def configure_registry():
         # If we're not swift, we're local.
         container_registry_path = '/var/lib/registry'
         storage['filesystem'] = {'rootdirectory': container_registry_path}
-        storage['cache'] = {'blobdescriptor': 'inmemory'}
 
         # Local storage is mounted from the host so images persist across
         # registry container restarts.
@@ -143,6 +158,8 @@ def configure_registry():
         storage['delete'] = {'enabled': True}
     if charm_config.get('storage-read-only'):
         storage['maintenance'] = {'readonly': {'enabled': True}}
+    if charm_config.get('storage-cache', 'inmemory') == 'inmemory':
+        storage['cache'] = {'blobdescriptor': 'inmemory'}
     registry_config['storage'] = storage
 
     os.makedirs(os.path.dirname(registry_config_file), exist_ok=True)

reactive/docker_registry.py

@@ -26,11 +26,17 @@
 def start():
     layer.status.maint('Configuring the registry.')
 
-    layer.docker_registry.configure_registry()
-    layer.docker_registry.start_registry()
+    bad_config = layer.docker_registry.has_invalid_config()
+    if bad_config:
+        layer.status.blocked(f"Invalid charm config: {', '.join(bad_config)}")
+        # NB: clear_flag just in case we were called from a handler where it is set.
+        clear_flag('charm.docker-registry.configured')
+    else:
+        layer.docker_registry.configure_registry()
+        layer.docker_registry.start_registry()
 
-    set_flag('charm.docker-registry.configured')
-    report_status()
+        set_flag('charm.docker-registry.configured')
+        report_status()
 
 
 @when('charm.docker-registry.configured')
@@ -65,12 +71,21 @@ def config_changed():
     charm_config = hookenv.config()
     name = charm_config.get('registry-name')
 
-    # If a provider gave us certs and http-host changed, make sure SANs are accurate
+    # If we have certs and http-host changed, ensure we'll refire the
+    # request_certificates handler.
     if (
         is_flag_set('cert-provider.certs.available') and
         charm_config.changed('http-host')
     ):
-        request_certificates()
+        clear_flag('cert-provider.certs.available')
+
+    # If we have connected clients and relevant config has changed, ensure we'll
+    # refire the configure_client handler.
+    if (is_flag_set('charm.docker-registry.client-configured') and
+            any((charm_config.changed('auth-basic-password'),
+                 charm_config.changed('auth-basic-user'),
+                 charm_config.changed('http-host')))):
+        clear_flag('charm.docker-registry.client-configured')
 
     # If our name changed, make sure we stop the old one
     if (
@@ -80,18 +95,7 @@ def config_changed():
         name = charm_config.previous('registry-name')
 
     layer.docker_registry.stop_registry(name=name)
-    layer.docker_registry.configure_registry()
-    layer.docker_registry.start_registry()
-
-    # Now that we reconfigured the registry, inform connected clients if
-    # anything changed that they should know about.
-    if (is_flag_set('charm.docker-registry.client-configured') and
-            any((charm_config.changed('auth-basic-password'),
-                 charm_config.changed('auth-basic-user'),
-                 charm_config.changed('http-host')))):
-        configure_client()
-
-    report_status()
+    start()
 
 
 @when_not("endpoint.docker-registry.joined")

tests/integration/test_docker_registry_integration.py

@@ -46,9 +46,8 @@ async def test_push_image(ops_test):
     for unit in registry_units:
         action = await unit.run_action("push", image="python:3.9-slim", pull=True)
         output = await action.wait()  # wait for result
-        assert output.data.get("status") == "completed"
-        assert output.data.get("results", {}).get("outcome") == "success"
-        assert output.data.get("results", {}).get("raw") == \
+        assert output.status == "completed"
+        assert output.results.get("raw") == \
             f"pushed {unit.public_address}:5000/python:3.9-slim"
 
 
@@ -63,6 +62,5 @@ async def test_image_list(ops_test):
     for unit in registry_units:
         action = await unit.run_action("images", repository="python:3.9-slim")
         output = await action.wait()  # wait for result
-        assert output.data.get("status") == "completed"
-        assert "python" in output.data.get("results", {}).get("output")
-        assert "3.9-slim" in output.data.get("results", {}).get("output")
+        assert output.status == "completed"
+        assert "python" in output.results.get("output")

tests/unit/conftest.py

@@ -1,4 +1,4 @@
 import charms.unit_test
 
 charms.unit_test.patch_reactive()
-charms.unit_test.patch_module('charms.leadership')
+charms.unit_test.patch_module("charms.leadership")

tests/unit/test_docker_registry.py

@@ -1,13 +1,14 @@
 from charms.unit_test import patch_fixture
+from unittest import mock
 
 from charmhelpers.core import host  # patched
 from charms import layer
 
 from reactive import docker_registry as handlers
 
 
-start_registry = patch_fixture('charms.layer.docker_registry.start_registry')
-stop_registry = patch_fixture('charms.layer.docker_registry.stop_registry')
+start_registry = patch_fixture("charms.layer.docker_registry.start_registry")
+stop_registry = patch_fixture("charms.layer.docker_registry.stop_registry")
 
 
 def test_series_upgrade(start_registry, stop_registry):
@@ -28,3 +29,50 @@ def test_series_upgrade(start_registry, stop_registry):
     assert host.service_pause.call_count == 1
     assert host.service_resume.call_count == 1
     assert layer.status.blocked.call_count == 1
+
+
+@mock.patch("charms.layer.docker_registry._configure_local_client")
+@mock.patch("charms.layer.docker_registry.host")
+@mock.patch("charms.layer.docker_registry._write_tls_blobs_to_files")
+@mock.patch("charms.layer.docker_registry.unitdata")
+@mock.patch("charmhelpers.core.hookenv.config")
+@mock.patch("os.makedirs", mock.Mock(return_value=0))
+def test_configure_registry(config, mock_kv, mock_write, mock_host, mock_lc):
+    # storage-cache: invalid value should not result in any cache structure
+    config.return_value = {
+        "log-level": "bananas",
+        "storage-cache": "bananas",
+    }
+    with mock.patch("charms.layer.docker_registry.yaml") as mock_yaml:
+        layer.docker_registry.configure_registry()
+        args, _ = mock_yaml.safe_dump.call_args_list[0]
+        assert "cache" not in args[0]["storage"]
+
+    # storage-cache: valid value should result in a populated cache structure
+    config.return_value = {
+        "log-level": "bananas",
+        "storage-cache": "inmemory",
+    }
+    expected = {
+        "log": {"level": "bananas"},
+        "storage": {"cache": {"blobdescriptor": "inmemory"}},
+    }
+    with mock.patch("charms.layer.docker_registry.yaml") as mock_yaml:
+        layer.docker_registry.configure_registry()
+        args, _ = mock_yaml.safe_dump.call_args_list[0]
+        assert expected["storage"].items() <= args[0]["storage"].items()
+
+
+@mock.patch("charmhelpers.core.hookenv.config")
+def test_has_invalid_config(config):
+    # check valid config is valid
+    config.return_value = {
+        "storage-cache": "disabled",
+    }
+    assert not layer.docker_registry.has_invalid_config()
+
+    # check for bad apples
+    config.return_value = {
+        "storage-cache": "bananas",
+    }
+    assert "storage-cache" in layer.docker_registry.has_invalid_config()

tox.ini

@@ -28,5 +28,5 @@ deps =
     pytest
     pytest-operator
     ipdb
-    juju < 3.0  # https://github.com/juju/python-libjuju/issues/719
+    juju
 commands = pytest --tb native --show-capture=no --log-cli-level=INFO -s {posargs} {toxinidir}/tests/integration