Improvements (#55)

* Add actions to control the new workload

* Fix static analysis issues

* Add tests

* Address comments

* Remove merge artefact

* Fix http test

* Address comments

* Test django_allowed_hosts config

* Lint fix

* Regroup install of custom snap in install snap fn

* Fix constants with new snap

* Fix typing

* Address comments

* Fix import

* Don't block the charm if charmed-bind-snap cannot be found

* Revert to the old way of getting the units IP

* Remove mixin

* Remove django configuration

* Don't open port 8080

* Remove unrelated tests, fix options.yaml

* Remove empty config file

* Add log when installing custom snap

* Remove unused code

* Simplify snap_install command

* Address comments

* Address comments

* Fix

metadata.yaml

@@ -8,7 +8,6 @@ issues: https://github.com/canonical/bind-operator/issues
 maintainers: 
   - https://launchpad.net/~canonical-is-devops
 source: https://github.com/canonical/bind-operator
-
 description: |
   A [Juju](https://juju.is/) [charm](https://juju.is/docs/olm/charmed-operators)
   deploying and managing a DNS server Integrator on Kubernetes and bare metal.
@@ -32,3 +31,8 @@ provides:
 peers:
   bind-peers:
     interface: bind-instance
+resources:
+  charmed-bind-snap:
+    type: file
+    filename: charmed-bind.snap
+    description: charmed-bind snap file for debugging purposes

pyproject.toml

@@ -51,6 +51,9 @@ module = "tests.*"
 [tool.pylint]
 disable = "wrong-import-order"
 
+[tool.pylint.tests]
+disable = ["too-many-arguments"]
+
 [tool.pytest.ini_options]
 minversion = "6.0"
 log_cli_level = "INFO"

src/bind.py

@@ -7,6 +7,7 @@
 import os
 import pathlib
 import shutil
+import subprocess  # nosec
 import tempfile
 import time
 
@@ -104,15 +105,17 @@ def stop(self) -> None:
             logger.error(error_msg)
             raise StopError(error_msg) from e
 
-    def setup(self, unit_name: str) -> None:
+    def setup(self, unit_name: str, snap_path: str | None) -> None:
         """Prepare the machine.
 
         Args:
             unit_name: The name of the current unit
+            snap_path: The path to the snap to install, can be blank.
         """
         self._install_snap_package(
             snap_name=constants.DNS_SNAP_NAME,
             snap_channel=constants.SNAP_PACKAGES[constants.DNS_SNAP_NAME]["channel"],
+            snap_path=snap_path,
         )
         self._install_bind_reload_service(unit_name)
         # We need to put the service zone in place so we call
@@ -227,28 +230,40 @@ def update_zonefiles_and_reload(
         logger.debug("Update and reload duration (ms): %s", (time.time_ns() - start_time) / 1e6)
 
     def _install_snap_package(
-        self, snap_name: str, snap_channel: str, refresh: bool = False
+        self, snap_name: str, snap_channel: str, snap_path: str | None, refresh: bool = False
     ) -> None:
         """Installs snap package.
 
         Args:
             snap_name: the snap package to install
             snap_channel: the snap package channel
+            snap_path: The path to the snap to install, can be blank.
             refresh: whether to refresh the snap if it's already present.
 
         Raises:
             InstallError: when encountering a SnapError or a SnapNotFoundError
         """
         try:
-            snap_cache = snap.SnapCache()
-            snap_package = snap_cache[snap_name]
-
-            if not snap_package.present or refresh:
-                snap_package.ensure(snap.SnapState.Latest, channel=snap_channel)
-
-        except (snap.SnapError, snap.SnapNotFoundError) as e:
+            # If a snap resource was not given, install the snap as published on snapcraft
+            if snap_path is None or snap_path == "":
+                snap_cache = snap.SnapCache()
+                snap_package = snap_cache[snap_name]
+
+                if not snap_package.present or refresh:
+                    snap_package.ensure(snap.SnapState.Latest, channel=snap_channel)
+            else:
+                # Installing the charm via subprocess.
+                # Calling subprocess here is not a security issue.
+                logger.info(
+                    "Installing from custom charmed-bind snap located: %s",
+                    snap_path,
+                )
+                subprocess.check_output(
+                    ["sudo", "snap", "install", snap_path, "--dangerous"]
+                )  # nosec
+        except (snap.SnapError, snap.SnapNotFoundError, subprocess.CalledProcessError) as e:
             error_msg = f"An exception occurred when installing {snap_name}. Reason: {e}"
-            logger.error(error_msg)
+            logger.exception(error_msg)
             raise InstallError(error_msg) from e
 
     def _zones_to_files_content(self, zones: list[models.Zone]) -> dict[str, str]:

src/charm.py

@@ -63,10 +63,19 @@ def __init__(self, *args: typing.Any):
         self.framework.observe(
             self.on[constants.PEER].relation_joined, self._on_peer_relation_joined
         )
-        self.unit.open_port("tcp", 8080)  # ACL API
         self.unit.open_port("tcp", 53)  # Bind DNS
         self.unit.open_port("udp", 53)  # Bind DNS
 
+        # Record if the `charmed-bind-snap` resource is defined.
+        # Using this can be useful when debugging locally
+        # More information about resources:
+        # https://juju.is/docs/sdk/resources#heading--add-a-resource-to-a-charm
+        self.snap_path: str | None = None
+        try:
+            self.snap_path = str(self.model.resources.fetch("charmed-bind-snap"))
+        except ops.ModelError as e:
+            logger.warning(e)
+
     def _on_reload_bind(self, _: events.ReloadBindEvent) -> None:
         """Handle periodic reload bind event.
 
@@ -147,7 +156,7 @@ def _on_config_changed(self, _: ops.ConfigChangedEvent) -> None:
     def _on_install(self, _: ops.InstallEvent) -> None:
         """Handle install."""
         self.unit.status = ops.MaintenanceStatus("Preparing bind")
-        self.bind.setup(self.unit.name)
+        self.bind.setup(self.unit.name, self.snap_path)
 
     def _on_start(self, _: ops.StartEvent) -> None:
         """Handle start."""
@@ -160,7 +169,7 @@ def _on_stop(self, _: ops.StopEvent) -> None:
     def _on_upgrade_charm(self, _: ops.UpgradeCharmEvent) -> None:
         """Handle upgrade-charm."""
         self.unit.status = ops.MaintenanceStatus("Upgrading dependencies")
-        self.bind.setup(self.unit.name)
+        self.bind.setup(self.unit.name, self.snap_path)
 
     def _on_leader_elected(self, _: ops.LeaderElectedEvent) -> None:
         """Handle leader-elected event."""

src/constants.py

@@ -8,7 +8,7 @@
 SNAP_PACKAGES = {
     DNS_SNAP_NAME: {"channel": "edge"},
 }
-DNS_CONFIG_DIR = f"/var/snap/{DNS_SNAP_NAME}/current/etc/bind"
+DNS_CONFIG_DIR = f"/var/snap/{DNS_SNAP_NAME}/common/bind"
 
 ZONE_SERVICE_NAME = "service.test"
 

tests/conftest.py

@@ -11,6 +11,7 @@ def pytest_addoption(parser):
         parser: Pytest parser.
     """
     parser.addoption("--charm-file", action="store", default=None)
+    parser.addoption("--charmed-bind-snap-file", action="store", default=None)
     parser.addoption(
         "--use-existing",
         action="store_true",

tests/integration/conftest.py

@@ -43,11 +43,18 @@ async def app_fixture(
         yield model.applications[app_name]
         return
 
+    resources = {}
+
+    if pytestconfig.getoption("--charmed-bind-snap-file"):
+        resources.update({"charmed-bind-snap": pytestconfig.getoption("--charmed-bind-snap-file")})
+
     if charm := pytestconfig.getoption("--charm-file"):
-        application = await model.deploy(f"./{charm}", application_name=app_name)
+        application = await model.deploy(
+            f"./{charm}", application_name=app_name, resources=resources
+        )
     else:
         charm = await ops_test.build_charm(".")
-        application = await model.deploy(charm, application_name=app_name)
+        application = await model.deploy(charm, application_name=app_name, resources=resources)
 
     await model.wait_for_idle(apps=[application.name], status="active")
 

tests/integration/helpers.py

@@ -4,24 +4,23 @@
 
 """Helper functions for the integration tests."""
 
-# Ignore duplicate code from the helpers (they can be in the charm also)
-# pylint: disable=duplicate-code
-# Ignore functions having too many arguments (will be removed in the future)
-# pylint: disable=too-many-positional-arguments
-
 import json
+import logging
 import pathlib
 import random
 import string
 import tempfile
 import time
 
+import juju.application
 import ops
 from pytest_operator.plugin import OpsTest
 
 import constants
 import models
 
+logger = logging.getLogger(__name__)
+
 
 class ExecutionError(Exception):
     """Exception raised when execution fails.
@@ -83,8 +82,8 @@ async def run_on_unit(ops_test: OpsTest, unit_name: str, command: str) -> str:
     return stdout
 
 
-# pylint: disable=too-many-arguments
 async def push_to_unit(
+    *,
     ops_test: OpsTest,
     unit: ops.model.Unit,
     source: str,
@@ -142,7 +141,7 @@ async def dispatch_to_unit(
 
 
 async def generate_anycharm_relation(
-    app: ops.model.Application,
+    app: juju.application.Application,
     ops_test: OpsTest,
     any_charm_name: str,
     dns_entries: list[models.DnsEntry],
@@ -200,10 +199,10 @@ async def change_anycharm_relation(
         dns_entries: List of DNS entries for any-charm
     """
     await push_to_unit(
-        ops_test,
-        anyapp_unit,
-        json.dumps([e.model_dump(mode="json") for e in dns_entries]),
-        "/srv/dns_entries.json",
+        ops_test=ops_test,
+        unit=anyapp_unit,
+        source=json.dumps([e.model_dump(mode="json") for e in dns_entries]),
+        destination="/srv/dns_entries.json",
     )
 
     # fire reload-data event
@@ -241,7 +240,9 @@ async def dig_query(
     return result
 
 
-async def get_active_unit(app: ops.model.Application, ops_test: OpsTest) -> ops.model.Unit | None:
+async def get_active_unit(
+    app: juju.application.Application, ops_test: OpsTest
+) -> ops.model.Unit | None:
     """Get the current active unit if it exists.
 
     Args:
@@ -251,7 +252,7 @@ async def get_active_unit(app: ops.model.Application, ops_test: OpsTest) -> ops.
     Returns:
         The current active unit if it exists, None otherwise
     """
-    for unit in app.units:  # type: ignore
+    for unit in app.units:
         # We take `[1]` because `[0]` is the return code of the process
         data = json.loads((await ops_test.juju("show-unit", unit.name, "--format", "json"))[1])
         relations = data[unit.name]["relation-info"]
@@ -271,7 +272,9 @@ async def get_active_unit(app: ops.model.Application, ops_test: OpsTest) -> ops.
     return None
 
 
-async def check_if_active_unit_exists(app: ops.model.Application, ops_test: OpsTest) -> bool:
+async def check_if_active_unit_exists(
+    app: juju.application.Application, ops_test: OpsTest
+) -> bool:
     """Check if an active unit exists and is reachable.
 
     Args:
@@ -281,8 +284,7 @@ async def check_if_active_unit_exists(app: ops.model.Application, ops_test: OpsT
     Returns:
         The current active unit if it exists, None otherwise
     """
-    # Application actually does have units
-    unit = app.units[0]  # type: ignore
+    unit = app.units[0]
     # We take `[1]` because `[0]` is the return code of the process
     data = json.loads((await ops_test.juju("show-unit", unit.name, "--format", "json"))[1])
     relations = data[unit.name]["relation-info"]
@@ -319,3 +321,22 @@ async def force_reload_bind(ops_test: OpsTest, unit: ops.model.Unit):
     """
     restart_cmd = f"sudo snap restart --reload {constants.DNS_SNAP_NAME}"
     await run_on_unit(ops_test, unit.name, restart_cmd)
+
+
+async def get_unit_ips(ops_test: OpsTest, unit: ops.model.Unit) -> list[str]:
+    """Retrieve unit ip addresses.
+
+    Args:
+        ops_test: The ops test framework instance
+        unit: the bind unit to force reload
+
+    Returns:
+        list of units ip addresses.
+    """
+    _, status, _ = await ops_test.juju("status", "--format", "json")
+    status = json.loads(status)
+    units = status["applications"][unit.name.split("/")[0]]["units"]
+    ip_list = []
+    for key in sorted(units.keys(), key=lambda n: int(n.split("/")[-1])):
+        ip_list.append(units[key]["public-address"])
+    return ip_list