From af39152f416c9992d01d95bbecb63b418c283716 Mon Sep 17 00:00:00 2001 From: Jordan Barrett Date: Tue, 7 Nov 2023 14:16:05 +0200 Subject: [PATCH 1/2] parse controller API port from apiaddresses in agent.conf - move flake8 to end of run_tests file, so unit tests always run --- run_tests | 2 +- src/charm.py | 27 ++++++++++++++-- tests/test_charm.py | 76 +++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 99 insertions(+), 6 deletions(-) diff --git a/run_tests b/run_tests index e47ee64..9c0bf7a 100755 --- a/run_tests +++ b/run_tests @@ -11,6 +11,6 @@ if [ -n "$PYTHONPATH" ]; then fi export PYTHONPATH="src:lib$PYTHONPATH" -flake8 coverage run --source=src -m unittest -v "$@" coverage report -m +flake8 diff --git a/src/charm.py b/src/charm.py index 7c96368..302014b 100755 --- a/src/charm.py +++ b/src/charm.py @@ -5,13 +5,15 @@ import controlsocket import logging import secrets +import urllib.parse import yaml from charms.prometheus_k8s.v0.prometheus_scrape import MetricsEndpointProvider from ops.charm import CharmBase from ops.framework import StoredState from ops.charm import RelationJoinedEvent, RelationDepartedEvent from ops.main import main -from ops.model import ActiveStatus, BlockedStatus, Relation +from ops.model import ActiveStatus, BlockedStatus, ErrorStatus, Relation +from typing import List logger = logging.getLogger(__name__) @@ -79,6 +81,12 @@ def _on_metrics_endpoint_relation_created(self, event: RelationJoinedEvent): self.control_socket.add_metrics_user(username, password) # Set up Prometheus scrape config + try: + api_port = self.api_port() + except AgentConfException as e: + self.unit.status = ErrorStatus(f"can't read controller API port from agent.conf: {e}") + return + metrics_endpoint = MetricsEndpointProvider( self, jobs=[{ @@ -86,7 +94,7 @@ def _on_metrics_endpoint_relation_created(self, event: RelationJoinedEvent): "scheme": "https", "static_configs": [{ "targets": [ - f'*:{self.api_port()}' + f'*:{api_port}' ] }], "basic_auth": { @@ -116,7 +124,16 @@ def _agent_conf(self, key: str): def api_port(self) -> str: """Return the port on which the controller API server is listening.""" - return self._agent_conf('apiport') + api_addresses = self._agent_conf('apiaddresses') + if not api_addresses: + raise AgentConfException("agent.conf key 'apiaddresses' missing") + if not isinstance(api_addresses, List): + raise AgentConfException("agent.conf key 'apiaddresses' is not a list") + + parsed_url = urllib.parse.urlsplit('//' + api_addresses[0]) + if not parsed_url.port: + raise AgentConfException("api address doesn't include port") + return parsed_url.port def ca_cert(self) -> str: """Return the controller's CA certificate.""" @@ -136,5 +153,9 @@ def generate_password() -> str: return secrets.token_urlsafe(16) +class AgentConfException(Exception): + """Raised when there are errors reading info from agent.conf.""" + + if __name__ == "__main__": main(JujuControllerCharm) diff --git a/tests/test_charm.py b/tests/test_charm.py index 0585206..3d51315 100644 --- a/tests/test_charm.py +++ b/tests/test_charm.py @@ -3,12 +3,36 @@ import os import unittest -from charm import JujuControllerCharm +from charm import JujuControllerCharm, AgentConfException +from ops import ErrorStatus from ops.testing import Harness from unittest.mock import mock_open, patch agent_conf = ''' -apiport: 17070 +apiaddresses: +- localhost:17070 +cacert: fake +''' + +agent_conf_apiaddresses_missing = ''' +cacert: fake +''' + +agent_conf_apiaddresses_not_list = ''' +apiaddresses: + foo: bar +cacert: fake +''' + +agent_conf_ipv4 = ''' +apiaddresses: +- "127.0.0.1:17070" +cacert: fake +''' + +agent_conf_ipv6 = ''' +apiaddresses: +- "[::1]:17070" cacert: fake ''' @@ -88,6 +112,54 @@ def test_metrics_endpoint_relation(self, mock_remove_user, mock_add_user, harness.remove_relation(relation_id) mock_remove_user.assert_called_once_with(f'juju-metrics-r{relation_id}') + @patch("builtins.open", new_callable=mock_open, read_data=agent_conf_apiaddresses_missing) + def test_apiaddresses_missing(self, _): + harness = Harness(JujuControllerCharm) + self.addCleanup(harness.cleanup) + harness.begin() + + with self.assertRaisesRegex(AgentConfException, "agent.conf key 'apiaddresses' missing"): + harness.charm.api_port() + + @patch("builtins.open", new_callable=mock_open, read_data=agent_conf_apiaddresses_not_list) + def test_apiaddresses_not_list(self, _): + harness = Harness(JujuControllerCharm) + self.addCleanup(harness.cleanup) + harness.begin() + + with self.assertRaisesRegex( + AgentConfException, "agent.conf key 'apiaddresses' is not a list" + ): + harness.charm.api_port() + + @patch("builtins.open", new_callable=mock_open, read_data=agent_conf_apiaddresses_missing) + @patch("controlsocket.Client.add_metrics_user") + def test_apiaddresses_missing_status(self, *_): + harness = Harness(JujuControllerCharm) + self.addCleanup(harness.cleanup) + harness.begin() + + harness.add_relation('metrics-endpoint', 'prometheus-k8s') + self.assertEqual(harness.charm.unit.status, ErrorStatus( + "can't read controller API port from agent.conf: agent.conf key 'apiaddresses' missing" + )) + + @patch("builtins.open", new_callable=mock_open, read_data=agent_conf_ipv4) + def test_apiaddresses_ipv4(self, _): + harness = Harness(JujuControllerCharm) + self.addCleanup(harness.cleanup) + harness.begin() + + self.assertEqual(harness.charm.api_port(), 17070) + + @patch("builtins.open", new_callable=mock_open, read_data=agent_conf_ipv6) + def test_apiaddresses_ipv6(self, _): + harness = Harness(JujuControllerCharm) + self.addCleanup(harness.cleanup) + harness.begin() + + self.assertEqual(harness.charm.api_port(), 17070) + class MockBinding: def __init__(self, address): From 970373c7b38d4beff88448f53cc5a6ed22aab5d8 Mon Sep 17 00:00:00 2001 From: Joseph Phillips Date: Mon, 8 Jan 2024 12:28:24 +0100 Subject: [PATCH 2/2] Updates status determination so that logic from upstream instead uses thing single method. --- src/charm.py | 42 +++++++++++++++++++++++------------------- tests/test_charm.py | 8 +++----- 2 files changed, 26 insertions(+), 24 deletions(-) diff --git a/src/charm.py b/src/charm.py index 828d5ac..e059d0c 100755 --- a/src/charm.py +++ b/src/charm.py @@ -38,7 +38,7 @@ def __init__(self, *args): self.on.dbcluster_relation_changed, self._on_dbcluster_relation_changed) self.control_socket = controlsocket.Client( - socket_path="/var/lib/juju/control.socket") + socket_path='/var/lib/juju/control.socket') self.framework.observe( self.on.metrics_endpoint_relation_created, self._on_metrics_endpoint_relation_created) self.framework.observe( @@ -49,9 +49,11 @@ def _on_collect_status(self, event: CollectStatusEvent): event.add_status(BlockedStatus( 'multiple possible DB bind addresses; set a suitable dbcluster network binding')) - if self.api_port() is None: - event.add_status(BlockedStatus( - 'charm does not appear to be running on a controller node')) + try: + self.api_port() + except AgentConfException as e: + event.add_status(ErrorStatus( + f"cannot read controller API port from agent configuration: {e}")) event.add_status(ActiveStatus()) @@ -73,9 +75,11 @@ def _on_dashboard_relation_joined(self, event): def _on_website_relation_joined(self, event): """Connect a website relation.""" logger.info('got a new website relation: %r', event) - port = self.api_port() - if port is None: - logger.error('charm does not appear to be running on a controller node') + + try: + api_port = self.api_port() + except AgentConfException as e: + logger.error("cannot read controller API port from agent configuration: {}", e) return address = None @@ -86,7 +90,7 @@ def _on_website_relation_joined(self, event): event.relation.data[self.unit].update({ 'hostname': str(address), 'private-address': str(address), - 'port': str(port) + 'port': str(api_port) }) def _on_metrics_endpoint_relation_created(self, event: RelationJoinedEvent): @@ -98,7 +102,7 @@ def _on_metrics_endpoint_relation_created(self, event: RelationJoinedEvent): try: api_port = self.api_port() except AgentConfException as e: - self.unit.status = ErrorStatus(f"can't read controller API port from agent.conf: {e}") + logger.error("cannot read controller API port from agent configuration: {}", e) return metrics_endpoint = MetricsEndpointProvider( @@ -143,15 +147,6 @@ def _on_dbcluster_relation_changed(self, event): self._stored.db_bind_address = ip event.relation.data[self.unit].update({'db-bind-address': ip}) - def _agent_conf(self, key: str): - """Read a value (by key) from the agent.conf file on disk.""" - unit_name = self.unit.name.replace('/', '-') - agent_conf_path = f'/var/lib/juju/agents/unit-{unit_name}/agent.conf' - - with open(agent_conf_path) as agent_conf_file: - agent_conf = yaml.safe_load(agent_conf_file) - return agent_conf.get(key) - def api_port(self) -> str: """Return the port on which the controller API server is listening.""" api_addresses = self._agent_conf('apiaddresses') @@ -162,13 +157,22 @@ def api_port(self) -> str: parsed_url = urllib.parse.urlsplit('//' + api_addresses[0]) if not parsed_url.port: - raise AgentConfException("api address doesn't include port") + raise AgentConfException("API address does not include port") return parsed_url.port def ca_cert(self) -> str: """Return the controller's CA certificate.""" return self._agent_conf('cacert') + def _agent_conf(self, key: str): + """Read a value (by key) from the agent.conf file on disk.""" + unit_name = self.unit.name.replace('/', '-') + agent_conf_path = f'/var/lib/juju/agents/unit-{unit_name}/agent.conf' + + with open(agent_conf_path) as agent_conf_file: + agent_conf = yaml.safe_load(agent_conf_file) + return agent_conf.get(key) + def metrics_username(relation: Relation) -> str: """ diff --git a/tests/test_charm.py b/tests/test_charm.py index da81801..c62344a 100644 --- a/tests/test_charm.py +++ b/tests/test_charm.py @@ -4,8 +4,7 @@ import os import unittest from charm import JujuControllerCharm, AgentConfException -from ops.model import BlockedStatus, ActiveStatus -from ops import ErrorStatus +from ops.model import BlockedStatus, ActiveStatus, ErrorStatus from ops.testing import Harness from unittest.mock import mock_open, patch @@ -140,9 +139,8 @@ def test_apiaddresses_missing_status(self, *_): harness.begin() harness.add_relation('metrics-endpoint', 'prometheus-k8s') - self.assertEqual(harness.charm.unit.status, ErrorStatus( - "can't read controller API port from agent.conf: agent.conf key 'apiaddresses' missing" - )) + harness.evaluate_status() + self.assertIsInstance(harness.charm.unit.status, ErrorStatus) @patch("builtins.open", new_callable=mock_open, read_data=agent_conf_ipv4) def test_apiaddresses_ipv4(self, _):