fix tests and refactor k8s _is_patched

config.yaml

@@ -19,7 +19,7 @@ options:
         "external-dns.alpha.kubernetes.io/hostname=example.com,service.beta.kubernetes.io/aws-load-balancer-type=nlb"
 
       Ensure the annotations are correctly formatted and adhere to Kubernetes' syntax and character set : https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/#syntax-and-character-set
-      invalid values will result in the annotations being reset to None, removed from the LoadBalancer, and all previously set annotations will be lost.
+      Invalid values will result in LoadBalancer being removed and all previously set annotations will be lost.
     type: string
   basic_auth_user:
     description: |

src/charm.py

@@ -209,13 +209,12 @@ def __init__(self, *args):
             ),
         )
 
-        annotations = cast(Optional[str], self.config.get("loadbalancer_annotations", None))
         self.lb_patch = KubernetesLoadBalancer(
             name=f"{self.app.name}-lb",
             namespace=self.model.name,
             field_manager=self.app.name,
             ports=self._service_ports,
-            additional_annotations=annotations,
+            additional_annotations=self._loadbalancer_annotations,
             additional_labels={"app.kubernetes.io/name": self.app.name},
             additional_selectors={"app.kubernetes.io/name": self.app.name},
         )
@@ -335,6 +334,14 @@ def _basic_auth_user(self) -> Optional[str]:
         """
         return cast(Optional[str], self.config.get("basic_auth_user", None))
 
+    @property
+    def _loadbalancer_annotations(self) -> Optional[str]:
+        """A comma-separated list of annotations to apply to the LoadBalancer service.
+
+        The input string is expected to be in the format: "key1=value1,key2=value2,key3=value3".
+        """
+        return cast(Optional[str], self.config.get("loadbalancer_annotations", None))
+
     def _on_forward_auth_config_changed(self, event: AuthConfigChangedEvent):
         if self._is_forward_auth_enabled:
             if self.forward_auth.is_ready():

src/k8s_lb_manager.py

@@ -5,7 +5,6 @@
 
 import logging
 import re
-import time
 from typing import Dict, List, Optional
 
 from lightkube import ApiError, Client
@@ -114,11 +113,11 @@ def reconcile(self):
             existing_service = self.client.get(Service, name=self.name, namespace=self.namespace)
 
             # Patch if differences exist
-            if existing_service != service:
+            if not self._is_patched(old_service=existing_service, new_service=service):
                 self.client.patch(Service, name=self.name, obj=service, patch_type=PatchType.APPLY)
                 logger.info(f"Patched LoadBalancer {self.name} in namespace {self.namespace}")
             else:
-                logger.info(f"No changes for LoadBalancer {self.name}")
+                logger.debug(f"No changes for LoadBalancer {self.name}")
         except ApiError as e:
             # Create the service if it doesn't exist
             if e.status.code == 404:
@@ -127,6 +126,28 @@ def reconcile(self):
             else:
                 logger.info(f"Failed to create LoadBalancer {self.name}: {e}")
 
+    def _is_patched(self, old_service: Service, new_service: Service) -> bool:
+        """Reports if the service has already been patched.
+
+        Returns:
+            bool: A boolean indicating if the service patch has been applied.
+        """
+        new_ports = [(p.port, p.targetPort) for p in new_service.spec.ports]  # type: ignore[attr-defined]
+        old_ports = [
+            (p.port, p.targetPort) for p in old_service.spec.ports  # type: ignore[attr-defined]
+        ]
+        ports_match = new_ports == old_ports
+
+        new_annotations = (
+            new_service.metadata.annotations or {}  # pyright: ignore[reportOptionalMemberAccess]
+        )
+        old_annotations = (
+            old_service.metadata.annotations or {}  # pyright: ignore[reportOptionalMemberAccess]
+        )
+        annotations_match = new_annotations == old_annotations
+
+        return ports_match and annotations_match
+
     def _annotations_valid(self) -> bool:
         """Check if the annotations are valid.
 
@@ -135,54 +156,8 @@ def _annotations_valid(self) -> bool:
         if self.additional_annotations is None:
             logger.error("Annotations are invalid or could not be parsed.")
             return False
-
-        logger.info("Annotations are valid.")
         return True
 
-    def is_loadbalancer_ready(self) -> bool:
-        """Wait for the LoadBalancer to be ready and return its status.
-
-        :return: True if the LoadBalancer is ready within the timeout period, False otherwise.
-        """
-        timeout = 60  # Default timeout of 300 seconds
-        check_interval = 10
-        attempts = timeout // check_interval
-
-        for _ in range(attempts):
-            lb_status = self._get_lb_external_address
-            if lb_status:
-                logger.info(f"LoadBalancer {self.name} is ready with address: {lb_status}")
-                return True
-
-            logger.warning(f"LoadBalancer {self.name} not ready, retrying...")
-            time.sleep(check_interval)
-
-        logger.error(f"LoadBalancer {self.name} is not ready after {timeout} seconds.")
-        return False
-
-    @property
-    def _get_lb_external_address(self) -> Optional[str]:
-        """Get the external address of the LoadBalancer.
-
-        :return: The external hostname or IP address of the LoadBalancer if available, None otherwise.
-        """
-        try:
-            lb = self.client.get(Service, name=self.name, namespace=self.namespace)
-        except ApiError as e:
-            logger.error(f"Failed to fetch LoadBalancer {self.name}: {e}")
-            return None
-
-        if not (status := getattr(lb, "status", None)):
-            return None
-        if not (load_balancer_status := getattr(status, "loadBalancer", None)):
-            return None
-        if not (ingress_addresses := getattr(load_balancer_status, "ingress", None)):
-            return None
-        if not (ingress_address := ingress_addresses[0]):
-            return None
-
-        return ingress_address.hostname or ingress_address.ip
-
 
 def validate_annotation_key(key: str) -> bool:
     """Validate the annotation key."""

tests/scenario/conftest.py

@@ -11,7 +11,7 @@
 
 @pytest.fixture
 def traefik_charm():
-    with patch("charm.KubernetesServicePatch"):
+    with patch("charm.KubernetesLoadBalancer"):
         with patch("lightkube.core.client.GenericSyncClient"):
             with patch(
                 "charm._get_loadbalancer_status",

tests/scenario/test_setup.py

@@ -51,7 +51,10 @@ def test_start_traefik_no_hostname(*_, traefik_ctx):
         containers=[Container(name="traefik", can_connect=False)],
     )
     out = Context(charm_type=TraefikIngressCharm).run("start", state)
-    assert out.unit_status == ("waiting", "gateway address unavailable")
+    assert out.unit_status == (
+        "blocked",
+        "Traefik load balancer is unable to obtain an IP or hostname from the cluster.",
+    )
 
 
 @patch("charm.TraefikIngressCharm._external_host", PropertyMock(return_value="foo.bar"))

tests/scenario/test_status.py

@@ -3,7 +3,7 @@
 
 from unittest.mock import PropertyMock, patch
 
-from ops import ActiveStatus, WaitingStatus
+from ops import ActiveStatus, BlockedStatus, WaitingStatus
 from scenario import Container, State
 
 
@@ -32,7 +32,9 @@ def test_start_traefik_no_hostname(traefik_ctx, *_):
     out = traefik_ctx.run("start", state)
 
     # THEN unit status is `waiting`
-    assert out.unit_status == WaitingStatus("gateway address unavailable")
+    assert out.unit_status == BlockedStatus(
+        "Traefik load balancer is unable to obtain an IP or hostname from the cluster."
+    )
 
 
 @patch("charm.TraefikIngressCharm._external_host", PropertyMock(return_value="foo.bar"))

tests/scenario/test_workload_version.py

@@ -10,7 +10,7 @@
 from charm import TraefikIngressCharm
 
 
-@patch("charm.KubernetesServicePatch")
+@patch("charm.KubernetesLoadBalancer")
 @patch("lightkube.core.client.GenericSyncClient")
 @patch("charm.TraefikIngressCharm._static_config_changed", PropertyMock(return_value=False))
 @patch("charm.TraefikIngressCharm._external_host", PropertyMock(return_value="foo.bar"))