From 65125f66f828a4ebe6190fa747947d0f791540fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=20C=2E=20Mass=C3=B3n?= Date: Tue, 16 Jul 2024 21:28:33 -0300 Subject: [PATCH 01/10] improve exceptions handling and add can_connect guards --- lib/charms/loki_k8s/v1/loki_push_api.py | 8 +++- src/charm.py | 56 ++++++++++++++++++------- tests/unit/helpers.py | 3 ++ tox.ini | 4 +- 4 files changed, 53 insertions(+), 18 deletions(-) diff --git a/lib/charms/loki_k8s/v1/loki_push_api.py b/lib/charms/loki_k8s/v1/loki_push_api.py index c3c1d086f..c82174032 100644 --- a/lib/charms/loki_k8s/v1/loki_push_api.py +++ b/lib/charms/loki_k8s/v1/loki_push_api.py @@ -527,7 +527,7 @@ def _alert_rules_error(self, event): # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 11 +LIBPATCH = 12 PYDEPS = ["cosl"] @@ -2475,6 +2475,9 @@ def disable_inactive_endpoints( container: Container, active_endpoints: Dict[str, str], topology: JujuTopology ): """Disable forwarding for inactive endpoints by checking against the Pebble plan.""" + if not container.can_connect(): + return + pebble_layer = container.get_plan().to_dict().get("log-targets", None) if not pebble_layer: return @@ -2501,6 +2504,9 @@ def enable_endpoints( container: Container, active_endpoints: Dict[str, str], topology: JujuTopology ): """Enable forwarding for the specified Loki endpoints.""" + if not container.can_connect(): + return + layer = Layer( { # pyright: ignore "log-targets": _PebbleLogClient._build_log_targets( diff --git a/src/charm.py b/src/charm.py index 61b0f63ab..a20e51038 100755 --- a/src/charm.py +++ b/src/charm.py @@ -66,7 +66,7 @@ StatusBase, WaitingStatus, ) -from ops.pebble import Error, Layer, PathError, ProtocolError +from ops.pebble import ConnectionError, Error, ExecError, Layer, PathError, ProtocolError # To keep a tidy debug-log, we suppress some DEBUG/INFO logs from some imported libs, # even when charm logging is set to a lower level. @@ -566,12 +566,16 @@ def _update_config(self, config: dict) -> bool: return False def _update_cert(self): - if not self._loki_container.can_connect(): - return + if self.server_cert.available: + self._push_cert_files() + else: + self._remove_cert_files() - ca_cert_path = Path(self._ca_cert_path) + self._update_ca_certificates() - if self.server_cert.available: + def _push_cert_files(self): + ca_cert_path = Path(self._ca_cert_path) + try: # Save the workload certificates self._loki_container.push( CERT_FILE, @@ -589,21 +593,36 @@ def _update_cert(self): self.server_cert.ca_cert, # pyright: ignore make_dirs=True, ) + except ConnectionError: + logger.error("Could not push cert files. Pebble refused connection. Shutting down?") + return - # Repeat for the charm container. We need it there for loki client requests. - # (and charm tracing and logging TLS) - ca_cert_path.parent.mkdir(exist_ok=True, parents=True) - ca_cert_path.write_text(self.server_cert.ca_cert) # pyright: ignore - else: + # Repeat for the charm container. We need it there for loki client requests. + # (and charm tracing and logging TLS) + ca_cert_path.parent.mkdir(exist_ok=True, parents=True) + ca_cert_path.write_text(self.server_cert.ca_cert) # pyright: ignore + + def _remove_cert_files(self): + ca_cert_path = Path(self._ca_cert_path) + try: self._loki_container.remove_path(CERT_FILE, recursive=True) self._loki_container.remove_path(KEY_FILE, recursive=True) self._loki_container.remove_path(ca_cert_path, recursive=True) + except ConnectionError: + logger.error("Could not remove cert files. Pebble refused connection. Shutting down?") + return - # Repeat for the charm container. - ca_cert_path.unlink(missing_ok=True) + # Repeat for the charm container. + ca_cert_path.unlink(missing_ok=True) - self._loki_container.exec(["update-ca-certificates", "--fresh"]).wait() - subprocess.run(["update-ca-certificates", "--fresh"]) + def _update_ca_certificates(self): + try: + self._loki_container.exec(["update-ca-certificates", "--fresh"]).wait() + subprocess.run(["update-ca-certificates", "--fresh"]) + except (ConnectionError, ExecError) as e: + logger.error("Could not run update-ca-certificates in workload container. %s", e) + except subprocess.SubprocessError as e: + logger.error("Could not run update-ca-certificates in charm container. %s", e) def _alerting_config(self) -> str: """Construct Loki altering configuration. @@ -779,7 +798,14 @@ def _loki_version(self) -> Optional[str]: """ if not self._loki_container.can_connect(): return None - version_output, _ = self._loki_container.exec(["/usr/bin/loki", "-version"]).wait_output() + try: + version_output, _ = self._loki_container.exec( + ["/usr/bin/loki", "-version"] + ).wait_output() + except ExecError as e: + logger.error("Error retrieving Loki version: %s", e) + return None + # Output looks like this: # loki, version 2.4.1 (branch: HEAD, ... result = re.search(r"version (\d*\.\d*\.\d*)", version_output) diff --git a/tests/unit/helpers.py b/tests/unit/helpers.py index 10ffbceb1..094aaa65c 100644 --- a/tests/unit/helpers.py +++ b/tests/unit/helpers.py @@ -23,3 +23,6 @@ def __init__(self, args): def wait_output(self): return ("v0.1.0", "") + + def wait(self): + return ("v0.1.0", "") diff --git a/tox.ini b/tox.ini index f3bcf0796..fe9b0e5a7 100644 --- a/tox.ini +++ b/tox.ini @@ -33,7 +33,7 @@ deps = black ruff commands = - ruff --fix {[vars]all_path} + ruff check --fix {[vars]all_path} black {[vars]all_path} # codespell pinned cause version 2.3.0 mistakenly considers joined words such as "assertIn" invalid @@ -46,7 +46,7 @@ deps = commands = codespell {[vars]lib_path} codespell . --skip .git --skip .tox --skip build --skip lib --skip venv --skip .mypy_cache - ruff {[vars]all_path} + ruff check {[vars]all_path} black --check --diff {[vars]all_path} [testenv:static-{charm,lib}] From e8373045fd0af51a6f1d7197e01b7ac9e2314e8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=20C=2E=20Mass=C3=B3n?= Date: Sat, 20 Jul 2024 00:20:07 -0300 Subject: [PATCH 02/10] always remove certs to avoid they stay on disk forever --- src/charm.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/charm.py b/src/charm.py index a20e51038..260f2df91 100755 --- a/src/charm.py +++ b/src/charm.py @@ -566,10 +566,10 @@ def _update_config(self, config: dict) -> bool: return False def _update_cert(self): + self._remove_cert_files() + if self.server_cert.available: self._push_cert_files() - else: - self._remove_cert_files() self._update_ca_certificates() From f41a236c9e422adc459fb8aee86d3f693fa1e581 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=20C=2E=20Mass=C3=B3n?= Date: Mon, 22 Jul 2024 15:45:39 -0300 Subject: [PATCH 03/10] revert changes in charm.py. See #434 --- src/charm.py | 56 ++++++++++++++-------------------------------------- 1 file changed, 15 insertions(+), 41 deletions(-) diff --git a/src/charm.py b/src/charm.py index 260f2df91..61b0f63ab 100755 --- a/src/charm.py +++ b/src/charm.py @@ -66,7 +66,7 @@ StatusBase, WaitingStatus, ) -from ops.pebble import ConnectionError, Error, ExecError, Layer, PathError, ProtocolError +from ops.pebble import Error, Layer, PathError, ProtocolError # To keep a tidy debug-log, we suppress some DEBUG/INFO logs from some imported libs, # even when charm logging is set to a lower level. @@ -566,16 +566,12 @@ def _update_config(self, config: dict) -> bool: return False def _update_cert(self): - self._remove_cert_files() - - if self.server_cert.available: - self._push_cert_files() - - self._update_ca_certificates() + if not self._loki_container.can_connect(): + return - def _push_cert_files(self): ca_cert_path = Path(self._ca_cert_path) - try: + + if self.server_cert.available: # Save the workload certificates self._loki_container.push( CERT_FILE, @@ -593,36 +589,21 @@ def _push_cert_files(self): self.server_cert.ca_cert, # pyright: ignore make_dirs=True, ) - except ConnectionError: - logger.error("Could not push cert files. Pebble refused connection. Shutting down?") - return - # Repeat for the charm container. We need it there for loki client requests. - # (and charm tracing and logging TLS) - ca_cert_path.parent.mkdir(exist_ok=True, parents=True) - ca_cert_path.write_text(self.server_cert.ca_cert) # pyright: ignore - - def _remove_cert_files(self): - ca_cert_path = Path(self._ca_cert_path) - try: + # Repeat for the charm container. We need it there for loki client requests. + # (and charm tracing and logging TLS) + ca_cert_path.parent.mkdir(exist_ok=True, parents=True) + ca_cert_path.write_text(self.server_cert.ca_cert) # pyright: ignore + else: self._loki_container.remove_path(CERT_FILE, recursive=True) self._loki_container.remove_path(KEY_FILE, recursive=True) self._loki_container.remove_path(ca_cert_path, recursive=True) - except ConnectionError: - logger.error("Could not remove cert files. Pebble refused connection. Shutting down?") - return - # Repeat for the charm container. - ca_cert_path.unlink(missing_ok=True) + # Repeat for the charm container. + ca_cert_path.unlink(missing_ok=True) - def _update_ca_certificates(self): - try: - self._loki_container.exec(["update-ca-certificates", "--fresh"]).wait() - subprocess.run(["update-ca-certificates", "--fresh"]) - except (ConnectionError, ExecError) as e: - logger.error("Could not run update-ca-certificates in workload container. %s", e) - except subprocess.SubprocessError as e: - logger.error("Could not run update-ca-certificates in charm container. %s", e) + self._loki_container.exec(["update-ca-certificates", "--fresh"]).wait() + subprocess.run(["update-ca-certificates", "--fresh"]) def _alerting_config(self) -> str: """Construct Loki altering configuration. @@ -798,14 +779,7 @@ def _loki_version(self) -> Optional[str]: """ if not self._loki_container.can_connect(): return None - try: - version_output, _ = self._loki_container.exec( - ["/usr/bin/loki", "-version"] - ).wait_output() - except ExecError as e: - logger.error("Error retrieving Loki version: %s", e) - return None - + version_output, _ = self._loki_container.exec(["/usr/bin/loki", "-version"]).wait_output() # Output looks like this: # loki, version 2.4.1 (branch: HEAD, ... result = re.search(r"version (\d*\.\d*\.\d*)", version_output) From 733407d8917641d77381e958987e533e5f334810 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=20C=2E=20Mass=C3=B3n?= Date: Mon, 22 Jul 2024 19:17:05 -0300 Subject: [PATCH 04/10] add comment --- src/charm.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/charm.py b/src/charm.py index 61b0f63ab..b8fdb58db 100755 --- a/src/charm.py +++ b/src/charm.py @@ -566,6 +566,9 @@ def _update_config(self, config: dict) -> bool: return False def _update_cert(self): + # If Pebble is not ready, we do not proceed. + # This code will end up running anyway when Pebble is ready, because + # it will eventually be called from the `_configure()` method. if not self._loki_container.can_connect(): return From 3e58f7e2de3eb796473ddd4898d0d5b3d89c2148 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=20C=2E=20Mass=C3=B3n?= Date: Tue, 23 Jul 2024 15:02:02 -0300 Subject: [PATCH 05/10] move can_connect() guard to `_update_logging` --- lib/charms/loki_k8s/v1/loki_push_api.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/charms/loki_k8s/v1/loki_push_api.py b/lib/charms/loki_k8s/v1/loki_push_api.py index c82174032..53a10e1ca 100644 --- a/lib/charms/loki_k8s/v1/loki_push_api.py +++ b/lib/charms/loki_k8s/v1/loki_push_api.py @@ -2475,9 +2475,6 @@ def disable_inactive_endpoints( container: Container, active_endpoints: Dict[str, str], topology: JujuTopology ): """Disable forwarding for inactive endpoints by checking against the Pebble plan.""" - if not container.can_connect(): - return - pebble_layer = container.get_plan().to_dict().get("log-targets", None) if not pebble_layer: return @@ -2504,9 +2501,6 @@ def enable_endpoints( container: Container, active_endpoints: Dict[str, str], topology: JujuTopology ): """Enable forwarding for the specified Loki endpoints.""" - if not container.can_connect(): - return - layer = Layer( { # pyright: ignore "log-targets": _PebbleLogClient._build_log_targets( @@ -2568,6 +2562,9 @@ def _update_logging(self, _): return for container in self._charm.unit.containers.values(): + if not container.can_connect(): + continue + self._update_endpoints(container, loki_endpoints) def _retrieve_endpoints_from_relation(self) -> dict: From 471bc44dbbe1502e4fa12bbefc4190fe16551ec3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=20C=2E=20Mass=C3=B3n?= <939888+Abuelodelanada@users.noreply.github.com> Date: Wed, 24 Jul 2024 10:37:23 -0300 Subject: [PATCH 06/10] Update lib/charms/loki_k8s/v1/loki_push_api.py Co-authored-by: Leon <82407168+sed-i@users.noreply.github.com> --- lib/charms/loki_k8s/v1/loki_push_api.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/charms/loki_k8s/v1/loki_push_api.py b/lib/charms/loki_k8s/v1/loki_push_api.py index 53a10e1ca..7f8372c47 100644 --- a/lib/charms/loki_k8s/v1/loki_push_api.py +++ b/lib/charms/loki_k8s/v1/loki_push_api.py @@ -2562,10 +2562,9 @@ def _update_logging(self, _): return for container in self._charm.unit.containers.values(): - if not container.can_connect(): - continue - - self._update_endpoints(container, loki_endpoints) + if container.can_connect(): + self._update_endpoints(container, loki_endpoints) + # else: `_update_endpoints` will be called on pebble-ready anyway. def _retrieve_endpoints_from_relation(self) -> dict: loki_endpoints = {} From 0e06f877fb1efd7bb35a6c79676fbced312dabb5 Mon Sep 17 00:00:00 2001 From: Leon <82407168+sed-i@users.noreply.github.com> Date: Thu, 25 Jul 2024 17:15:53 -0400 Subject: [PATCH 07/10] Use binary pip packages for loki-tester charm --- tests/integration/loki-tester/charmcraft.yaml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/integration/loki-tester/charmcraft.yaml b/tests/integration/loki-tester/charmcraft.yaml index 048d45441..5210557dd 100644 --- a/tests/integration/loki-tester/charmcraft.yaml +++ b/tests/integration/loki-tester/charmcraft.yaml @@ -8,3 +8,12 @@ bases: run-on: - name: "ubuntu" channel: "20.04" +parts: + charm: + charm-binary-python-packages: + # For v2.tls_certificates + - cryptography + - jsonschema + + # For v1.alertmanager_dispatch & v1.tracing + - pydantic>=2 From 1347e217d80a65ee70eabadfebbca249ab15ff6e Mon Sep 17 00:00:00 2001 From: Leon <82407168+sed-i@users.noreply.github.com> Date: Thu, 25 Jul 2024 17:16:14 -0400 Subject: [PATCH 08/10] Update charmcraft.yaml --- tests/integration/loki-tester/charmcraft.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/integration/loki-tester/charmcraft.yaml b/tests/integration/loki-tester/charmcraft.yaml index 5210557dd..b07ebcde1 100644 --- a/tests/integration/loki-tester/charmcraft.yaml +++ b/tests/integration/loki-tester/charmcraft.yaml @@ -17,3 +17,5 @@ parts: # For v1.alertmanager_dispatch & v1.tracing - pydantic>=2 + + - cosl From a3ff0ed2fa2d919cf0812b4497947db4c427f2b3 Mon Sep 17 00:00:00 2001 From: Leon <82407168+sed-i@users.noreply.github.com> Date: Thu, 25 Jul 2024 17:20:54 -0400 Subject: [PATCH 09/10] Update charmcraft.yaml --- tests/integration/log-proxy-tester/charmcraft.yaml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/integration/log-proxy-tester/charmcraft.yaml b/tests/integration/log-proxy-tester/charmcraft.yaml index 8ca1cc387..191cde371 100644 --- a/tests/integration/log-proxy-tester/charmcraft.yaml +++ b/tests/integration/log-proxy-tester/charmcraft.yaml @@ -13,3 +13,13 @@ parts: charm: build-packages: - git + + charm-binary-python-packages: + # For v2.tls_certificates + - cryptography + - jsonschema + + # For v1.alertmanager_dispatch & v1.tracing + - pydantic>=2 + + - cosl From 163af596657d09ea90b839fd050448f69727f629 Mon Sep 17 00:00:00 2001 From: Leon <82407168+sed-i@users.noreply.github.com> Date: Thu, 25 Jul 2024 17:21:11 -0400 Subject: [PATCH 10/10] Update charmcraft.yaml --- tests/integration/log-forwarder-tester/charmcraft.yaml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/integration/log-forwarder-tester/charmcraft.yaml b/tests/integration/log-forwarder-tester/charmcraft.yaml index 8ca1cc387..191cde371 100644 --- a/tests/integration/log-forwarder-tester/charmcraft.yaml +++ b/tests/integration/log-forwarder-tester/charmcraft.yaml @@ -13,3 +13,13 @@ parts: charm: build-packages: - git + + charm-binary-python-packages: + # For v2.tls_certificates + - cryptography + - jsonschema + + # For v1.alertmanager_dispatch & v1.tracing + - pydantic>=2 + + - cosl