fix: credential refresh on config change (#56)

* Refactor integration tests (#49)

* feat: make integration test idempotent
* refactor: encapsulate logic for tests, moving connect-to-minio logic into a reusable helper so it can be used by multiple tests
* refactor: add arg for access/secret_key in connect_client_to_server
* feat: make connection test attempts auto-retry using tenacity.retry
* fix: unpin black to fix linting/formatting

* fix: credential refresh on config change (#50)

* feat: add hash of config to env vars

Fixes #47

This adds a hash of the current config to environment variables in order to trigger creating a new pod whenever config changes.  This ensures minio will restart whenever someone does `juju config minio access-key=newKey` or similar.  This also adds unit tests for the new code

Implementation:
* refactor handling the default value for secret-key (moved to `_get_secret_key()`) to make it reusable
* add `_generate_config_hash()` to create the hash of the current config

* fix: _get_secret_key try/except caught wrong exception

Fixes bug where we catch errors about missing data in self._stored.  Was using `except KeyError`, but `self._stored` raises an `AttributeError` if data is not available.

* fix: linting/formatting

* feat: Add integration test for credential refresh

* fix: test_refresh_credentials

Updates test_refresh_credentials to use the refactored connect_client_to_server.

src/charm.py

@@ -6,6 +6,7 @@
 from random import choices
 from string import ascii_uppercase, digits
 from base64 import b64encode
+from hashlib import sha256
 
 from oci_image import OCIImageResource, OCIImageResourceError
 from ops.charm import CharmBase
@@ -26,7 +27,8 @@ def __init__(self, *args):
         super().__init__(*args)
         self.log = logging.getLogger()
 
-        self._stored.set_default(secret_key=_gen_pass())
+        # Random salt used for hashing config
+        self._stored.set_default(hash_salt=_gen_pass())
 
         self.image = OCIImageResource(self, "oci-image")
 
@@ -53,9 +55,11 @@ def main(self, event):
             self.model.unit.status = error.status
             return
 
-        secret_key = self.model.config["secret-key"] or self._stored.secret_key
+        secret_key = self._get_secret_key()
         self._send_info(interfaces, secret_key)
 
+        configmap_hash = self._generate_config_hash()
+
         self.model.unit.status = MaintenanceStatus("Setting pod spec")
         self.model.pod.set_spec(
             {
@@ -79,6 +83,11 @@ def main(self, event):
                             "minio-secret": {
                                 "secret": {"name": f"{self.model.app.name}-secret"}
                             },
+                            # This hash forces a restart for pods whenever we change the config.
+                            # This would ideally be a spec.template.metadata.annotation rather
+                            # than an environment variable, but we cannot use that using podspec.
+                            # (see https://stackoverflow.com/questions/37317003/restart-pods-when-configmap-updates-in-kubernetes/51421527#51421527)  # noqa E403
+                            "configmap-hash": configmap_hash,
                         },
                     }
                 ],
@@ -135,6 +144,17 @@ def _send_info(self, interfaces, secret_key):
                 }
             )
 
+    def _generate_config_hash(self):
+        """Returns a hash of the current config state"""
+        # Add a randomly generated salt to the config to make it hard to reverse engineer the
+        # secret-key from the password.
+        salt = self._stored.hash_salt
+        all_config = tuple(
+            str(self.model.config[name]) for name in sorted(self.model.config.keys())
+        ) + (salt,)
+        config_hash = sha256(".".join(all_config).encode("utf-8"))
+        return config_hash.hexdigest()
+
     def _get_minio_args(self):
         model_mode = self.model.config["mode"]
         if model_mode == "server":
@@ -165,6 +185,23 @@ def _get_minio_args_gateway(self):
                 BlockedStatus,
             )
 
+    def _get_secret_key(self):
+        """Returns the secret key set by config, else returns the randomly generated secret"""
+        config_secret = self.model.config["secret-key"]
+        if config_secret != "":
+            # Use secret specified in config
+            secret = config_secret
+        else:
+            try:
+                # Try to use a randomly generated default key from the past
+                secret = self._stored.secret_key
+            except AttributeError:
+                # Create and store a randomly generated default key to reuse in future
+                secret = _gen_pass()
+                self._stored.set_default(secret_key=secret)
+
+        return secret
+
     def _with_console_address(self, minio_args):
         console_port = str(self.model.config["console-port"])
         return [*minio_args, "--console-address", ":" + console_port]

test-requirements.txt

@@ -1,8 +1,9 @@
 # Copyright 2021 Canonical Ltd.
 # See LICENSE file for licensing details.
 
-pytest
-pyyaml
 flake8
 black
 flake8-copyright<0.3
+pytest
+pyyaml
+tenacity<8.1

tests/integration/test_charm.py

@@ -5,8 +5,9 @@
 from pathlib import Path
 
 import pytest
-import yaml
 from pytest_operator.plugin import OpsTest
+from tenacity import Retrying, stop_after_delay, wait_exponential
+import yaml
 
 log = logging.getLogger(__name__)
 
@@ -37,25 +38,40 @@ async def test_build_and_deploy(ops_test: OpsTest):
     await ops_test.model.wait_for_idle(timeout=60 * 10)
 
 
-async def test_connect_client_to_server(ops_test: OpsTest):
-    """
-    Tests a deployed MinIO app by trying to connect to it from a pod and do trivial actions with it
+async def connect_client_to_server(
+    ops_test: OpsTest, application, access_key=None, secret_key=None
+):
+    """Connects to the minio server using a minio client. raising a ConnectionError if failed
+    Args:
+        ops_test: fixture
+        application: Minio application to connect to
+        access_key (str): (Optional) access-key for minio login.  If omitted, will be pulled from
+                          application's config
+        secret_key (str): (Optional) secret-key for minio login.  If omitted, will be pulled from
+                          application's config
+
+    Returns:
+        None
     """
-
-    application = ops_test.model.applications[APP_NAME]
     config = await application.get_config()
+
+    if access_key is None:
+        access_key = config["access-key"]["value"]
+    if secret_key is None:
+        secret_key = config["secret-key"]["value"]
+
     port = config["port"]["value"]
     alias = "ci"
     bucket = "testbucket"
     service_name = APP_NAME
     model_name = ops_test.model_name
-    log.info(f"ops_test.model_name = {ops_test.model_name}")
 
     url = f"http://{service_name}.{model_name}.svc.cluster.local:{port}"
 
     minio_cmd = (
-        f"mc alias set {alias} {url} {MINIO_CONFIG['access-key']} {MINIO_CONFIG['secret-key']}"
+        f"mc alias set {alias} {url} {access_key} {secret_key}"
         f"&& mc mb {alias}/{bucket}"
+        f"&& mc rb {alias}/{bucket}"
     )
 
     kubectl_cmd = (
@@ -77,9 +93,37 @@ async def test_connect_client_to_server(ops_test: OpsTest):
 
     ret_code, stdout, stderr = await ops_test.run(*kubectl_cmd)
 
-    assert (
-        ret_code == 0
-    ), f"Test returned code {ret_code} with stdout:\n{stdout}\nstderr:\n{stderr}"
+    if ret_code != 0:
+        raise ConnectionError(
+            f"Connection to Minio returned code {ret_code} with stdout:\n{stdout}\n"
+            f"stderr:\n{stderr}."
+        )
+    else:
+        return
+
+
+async def test_connect_client_to_server(ops_test: OpsTest):
+    """
+    Tests a deployed MinIO by connecting with mc (MinIO client) via a Pod.
+    """
+
+    application = ops_test.model.applications[APP_NAME]
+
+    for attempt in retry_for_60_seconds:
+        log.info(
+            f"Test attempting to connect to minio using mc client (attempt "
+            f"{attempt.retry_state.attempt_number})"
+        )
+        with attempt:
+            await connect_client_to_server(ops_test=ops_test, application=application)
+
+
+# Helper to retry calling a function over 60 seconds
+retry_for_60_seconds = Retrying(
+    wait=wait_exponential(multiplier=1, min=1, max=10),
+    stop=stop_after_delay(60),
+    reraise=True,
+)
 
 
 async def test_connect_to_console(ops_test: OpsTest):
@@ -118,3 +162,36 @@ async def test_connect_to_console(ops_test: OpsTest):
     assert (
         ret_code == 0
     ), f"Test returned code {ret_code} with stdout:\n{stdout}\nstderr:\n{stderr}"
+
+
+async def test_refresh_credentials(ops_test: OpsTest):
+    """Tests that changing access/secret correctly gets reflected in workload
+
+    Note: This test is not idempotent - it leaves the charm with different credentials than how it
+          started.  We could move credential changing and resetting to a fixture so it always
+          restored them if that becomes a problem.
+
+    Note: Untested here is whether setting the config to the current value (eg: a no-op) correctly
+          avoids restarting the workload.
+    """
+    # Update credentials in deployed Minio's config
+    application = ops_test.model.applications[APP_NAME]
+    old_config = await application.get_config()
+    config = {
+        "access-key": old_config["access-key"]["value"] + "modified",
+        "secret-key": old_config["secret-key"]["value"] + "modified",
+    }
+    await application.set_config(config)
+
+    for attempt in retry_for_60_seconds:
+        log.info(
+            f"Test attempting to connect to minio using mc client (attempt "
+            f"{attempt.retry_state.attempt_number})"
+        )
+        with attempt:
+            await connect_client_to_server(
+                ops_test=ops_test,
+                application=application,
+                access_key=config["access-key"],
+                secret_key=config["secret-key"],
+            )

tests/unit/test_charm.py

@@ -1,5 +1,6 @@
 # Copyright 2021 Canonical Ltd.
 # See LICENSE file for licensing details.
+from unittest.mock import MagicMock, PropertyMock
 
 import pytest
 import yaml
@@ -309,3 +310,72 @@ def test_minio_console_port_args(harness):
         "--console-address",
         ":9999",
     ]
+
+
+@pytest.mark.parametrize(
+    "config,hash_salt,expected_hash",
+    [
+        (  # Standard working case
+            {"access-key": "access-key-value", "secret-key": "secret-key-value"},
+            "hash-salt",
+            "9b25665b7652ab845b909c718f6c66dccb0946a7ef8ddd607b85198d6deabe5f",
+        ),
+        (  # Vary the sorted order of keys
+            {
+                "x_last_alphabetical_order": "access-key-value",
+                "secret-key": "secret-key-value",
+            },
+            "hash-salt",
+            "a33e4682a38734508a9c8cb6971761636fefb0225eab0cb897443f1cf1317a07",
+        ),
+        (  # Vary the access-key
+            {"access-key": "access-key-value1", "secret-key": "secret-key-value"},
+            "hash-salt",
+            "162ba72393a4993626d553f2e64255f0998a70ef1b8ed4ea73652920d014898d",
+        ),
+        (  # Vary the salt
+            {"access-key": "access-key-value", "secret-key": "secret-key-value"},
+            "hash-salt1",
+            "82c0d902422d085cfc5d5d652d7ebd78175042705542fe7db9866a259bd06528",
+        ),
+    ],
+)
+def test_generate_config_hash(config, hash_salt, expected_hash, harness):
+    ##################
+    # Setup test
+
+    harness.begin()
+
+    # Mock config to use a controlled subset of keys.  This avoids the expected hash changing
+    # whenever someone adds a new config option
+
+    # Use tuple(generator) instead of generator directly - if we use the bare generator directly
+    # it'll raise an exception in update_config because update_config will be editing the object
+    # we're taking keys() from
+    old_keys = tuple(harness.charm.config.keys())
+    harness.update_config(unset=old_keys)
+    assert len(harness.charm.config.keys()) == 0, "Failed to delete default config keys"
+
+    # Avoid triggering config_changed as hooks may have unexpected failures due to reduced config
+    # data
+    with harness.hooks_disabled():
+        harness.update_config(config)
+
+    # Mock away _stored with known values
+    harness.charm._stored = MagicMock()
+    mocked_salt = PropertyMock(return_value=hash_salt)
+    type(harness.charm._stored).hash_salt = mocked_salt
+
+    ##################
+    # Execute test
+
+    hashed_config = harness.charm._generate_config_hash()
+
+    ##################
+    # Check results
+    assert expected_hash == hashed_config
+
+
+# TODO: test get_secret_key
+# TODO: How can I test whether the hash/password gets randomly generated if respective config is
+#  omitted?  Or can/should I at all?