address code reviews

Author: florentianayuwono

charmcraft.yaml

@@ -1,7 +1,7 @@
 # Copyright 2025 Canonical Ltd.
 # See LICENSE file for licensing details.
 # This file configures Charmcraft.
-# See https://canonical-charmcraft.readthedocs-hosted.com/stable/howto/manage-charmcraft/ 
+# See https://canonical-charmcraft.readthedocs-hosted.com/stable/howto/manage-charmcraft/
 # for guidance.
 
 type: charm
@@ -25,7 +25,7 @@ description: |
 
 base: ubuntu@24.04
 build-base: ubuntu@24.04
-platforms: 
+platforms:
   amd64:
     build-on: [amd64]
     build-for: [amd64]
@@ -48,7 +48,7 @@ config:
 
         For example: "1.2.3.4:8888" or "1.2.3.4".
         If no proxy is specified, the default proxy is the principal charm's juju-https-proxy or juju-http-proxy relation data.
-        If no port is specified, the default port value of 3128 will be used.
+        If no port is specified, the default port value of 80 will be used.
       type: string
     exclude-addresses-from-proxy:
       description: Comma-separated list of IP or hostname addresses that should bypass the proxy.

src/aproxy.py

@@ -36,13 +36,13 @@
 logger = logging.getLogger(__name__)
 
 # Files and constants
-NFT_CONF_DIR = Path("/etc/aproxy")
+NFT_CONF_DIR = Path("/opt/aproxy-charm")
 NFT_CONF_FILE = NFT_CONF_DIR / "nftables.conf"
 SYSTEMD_UNIT_PATH = Path("/etc/systemd/system/aproxy-nftables.service")
 APROXY_LISTEN_PORT = 8443
 APROXY_SNAP_NAME = "aproxy"
 APROXY_SNAP_CHANNEL = "edge"
-DEFAULT_PROXY_PORT = 3128
+DEFAULT_PROXY_PORT = 80
 RELATION_NAME = "juju-info"
 
 # ^\.?             : one leading dot allowed
@@ -55,6 +55,12 @@
     r"^\.?(?!-)(?!.*--)(?!.*-$)([a-zA-Z0-9-]{1,63}\.)*([a-zA-Z0-9-]{1,63})\.?$"
 )
 
+# ^                : start of the string
+# [a-zA-Z]         : match letter
+# [a-zA-Z0-9+\-.]* : match letters, digits, +, -, ., zero or more times
+# ://              : match literal "://"
+URI_SCHEME_PREFIX_RE = re.compile(r"^[a-zA-Z][a-zA-Z0-9+\-.]*://")
+
 
 class AproxyConfig(BaseModel):
     """Configuration model for aproxy charm.
@@ -63,7 +69,7 @@ class AproxyConfig(BaseModel):
         model_config: Pydantic config to forbid extra fields.
         proxy_address: The target proxy address (hostname or IP).
         proxy_port: The target proxy port.
-        no_proxy: Comma-separated list of IPs, CIDRs, or hostnames to
+        exclude_addresses: Comma-separated list of IPs, CIDRs, or hostnames to
             exclude from interception.
         intercept_ports_list: List of ports to intercept as strings.
     """
@@ -72,7 +78,7 @@ class AproxyConfig(BaseModel):
 
     proxy_address: str
     proxy_port: int = DEFAULT_PROXY_PORT
-    no_proxy: List[str] = []
+    exclude_addresses: List[str] = []
     intercept_ports_list: List[str]
 
     @classmethod
@@ -106,10 +112,12 @@ def from_charm(cls, charm: ops.CharmBase) -> "AproxyConfig":
                 ) from exc
 
         # Parse no proxy list
-        no_proxy = []
-        raw_no_proxy = str(conf.get("exclude-addresses-from-proxy", ""))
-        if raw_no_proxy:
-            no_proxy = [entry.strip() for entry in raw_no_proxy.split(",") if entry.strip()]
+        exclude_addresses = []
+        raw_exclude_addresses = str(conf.get("exclude-addresses-from-proxy", ""))
+        if raw_exclude_addresses:
+            exclude_addresses = [
+                entry.strip() for entry in raw_exclude_addresses.split(",") if entry.strip()
+            ]
 
         # Parse intercept ports
         intercept_ports_raw = str(conf.get("intercept-ports", ""))
@@ -120,7 +128,7 @@ def from_charm(cls, charm: ops.CharmBase) -> "AproxyConfig":
             return cls(
                 proxy_address=proxy_address,
                 proxy_port=proxy_port,
-                no_proxy=no_proxy,
+                exclude_addresses=exclude_addresses,
                 intercept_ports_list=intercept_ports_list,
             )
         except ValidationError as exc:
@@ -146,25 +154,25 @@ def _validate_proxy_port(cls, proxy_port: int) -> int:
             raise ValueError(f"proxy port must be between 1 and 65535 instead of {proxy_port}")
         return proxy_port
 
-    @field_validator("no_proxy")
-    def _validate_no_proxy(cls, no_proxy: List[str]) -> List[str]:
-        """Validate no_proxy entries are valid IPs, CIDRs, or hostnames."""
-        valid_no_proxy = []
-        for entry in no_proxy:
+    @field_validator("exclude_addresses")
+    def _validate_exclude_addresses(cls, exclude_addresses: List[str]) -> List[str]:
+        """Validate exclude_addresses entries are valid IPs, CIDRs, or hostnames."""
+        valid_exclude_addresses = []
+        for entry in exclude_addresses:
             entry = entry.strip()
             if not entry:
                 continue
             try:
                 # Check if it's a valid IP or CIDR
                 ipaddress.ip_network(entry, strict=False)
-                valid_no_proxy.append(entry)
+                valid_exclude_addresses.append(entry)
             except ValueError as exc:
                 # If not an IP/CIDR, check if it's a valid hostname
                 if HOSTNAME_PATTERN.match(entry):
-                    valid_no_proxy.append(entry)
+                    valid_exclude_addresses.append(entry)
                 else:
-                    raise ValueError(f"invalid no_proxy entry {entry}") from exc
-        return valid_no_proxy
+                    raise ValueError(f"invalid exclude_addresses entry {entry}") from exc
+        return valid_exclude_addresses
 
     @field_validator("intercept_ports_list")
     def _validate_and_merge_ports(cls, ports: List[str]) -> List[str]:
@@ -228,8 +236,8 @@ def _get_principal_proxy_address(cls) -> str:
 
         proxy_conf = https_proxy or http_proxy or ""
 
-        # Strip any leading http:// or https://
-        return re.sub(r"^https?://", "", proxy_conf)
+        # Strip prefix like http://, https://, socks5h://, ftp://, etc.
+        return URI_SCHEME_PREFIX_RE.sub("", proxy_conf)
 
 
 class AproxyManager:
@@ -276,14 +284,23 @@ def configure_target_proxy(self) -> None:
         Raises:
             ConnectionError: If the target proxy is not reachable.
         """
+        snap_cache = snap.SnapCache()
+        aproxy_snap = snap_cache[APROXY_SNAP_NAME]
+
+        # Check if current config is the same as the target config
+        current_proxy = aproxy_snap.get("proxy")
         target_proxy = f"{self.config.proxy_address}:{self.config.proxy_port}"
+        if current_proxy == target_proxy:
+            logger.info("Proxy is already set to %s, skipping reconfiguration", target_proxy)
+            return
+
+        # Check if target proxy is reachable
         if not self._is_proxy_reachable(self.config.proxy_address, self.config.proxy_port):
             logger.error("Proxy is not reachable at %s", target_proxy)
             raise ConnectionError(f"Proxy is not reachable at {target_proxy}")
 
         logger.info("Configuring snap: proxy=%s", target_proxy)
-        snap_cache = snap.SnapCache()
-        snap_cache[APROXY_SNAP_NAME].set({"proxy": target_proxy})
+        aproxy_snap.set({"proxy": target_proxy})
 
     def _is_proxy_reachable(self, host: str, port: int = DEFAULT_PROXY_PORT) -> bool:
         """Check if the target proxy is reachable on the specified port.
@@ -342,7 +359,7 @@ def _render_nft_rules(self) -> str:
             [
                 "127.0.0.0/8",  # private loopback range
             ]
-            + self.config.no_proxy
+            + self.config.exclude_addresses
         )
 
         return f"""#!/usr/sbin/nft -f

tests/unit/conftest.py

@@ -45,6 +45,29 @@ def set(self, config: dict):
         args = [f"{k}={v}" for k, v in config.items()]
         subprocess.run(["snap", "set", "aproxy"] + args, check=True)  # nosec
 
+    def get(self, key: str, default=""):
+        """Simulate getting snap configuration using subprocess.
+
+        Args:
+            key: Configuration key to retrieve.
+            default: Default value if key is not set.
+
+        Returns:
+            The value of the configuration key or default if not set.
+        """
+        if not self.present:
+            return default
+
+        result = subprocess.run(
+            ["snap", "get", "aproxy", key],
+            check=False,
+            capture_output=True,
+            text=True,
+        )  # nosec
+
+        value = result.stdout.strip() if result.stdout else default
+        return value if value else default
+
 
 @pytest.fixture(autouse=True)
 def fake_snap(monkeypatch):

tests/unit/test_aproxy_charm_unit.py

@@ -27,9 +27,7 @@ def test_install_with_proxy_config_should_succeed(patch_proxy_check):
 
     out = ctx.run(ctx.on.install(), state)
 
-    assert out.unit_status == testing.ActiveStatus(
-        "Service ready on target proxy target.proxy:3128"
-    )
+    assert out.unit_status == testing.ActiveStatus("Service ready on target proxy target.proxy:80")
 
 
 def test_install_without_proxy_config_should_fail(patch_proxy_check):
@@ -78,9 +76,7 @@ def test_start_proxy_reachable_should_succeed(patch_proxy_check):
 
     out = ctx.run(ctx.on.start(), state)
 
-    assert out.unit_status == testing.ActiveStatus(
-        "Service ready on target proxy target.proxy:3128"
-    )
+    assert out.unit_status == testing.ActiveStatus("Service ready on target proxy target.proxy:80")
 
 
 def test_start_proxy_unreachable_should_fail(patch_proxy_check):
@@ -96,7 +92,7 @@ def test_start_proxy_unreachable_should_fail(patch_proxy_check):
     out = ctx.run(ctx.on.start(), state)
 
     assert out.unit_status == testing.BlockedStatus(
-        "Failed to configure aproxy: Proxy is not reachable at target.proxy:3128"
+        "Failed to configure aproxy: Proxy is not reachable at target.proxy:80"
     )
 
 
@@ -148,7 +144,7 @@ def test_config_changed_should_succeed(patch_proxy_check):
     out = ctx.run(ctx.on.config_changed(), state)
 
     assert out.unit_status == testing.ActiveStatus(
-        "Service ready on target proxy modified.proxy:3128"
+        "Service ready on target proxy modified.proxy:80"
     )
 
 
@@ -182,7 +178,7 @@ def test_config_changed_with_unreachable_proxy_should_fail(patch_proxy_check):
     out = ctx.run(ctx.on.config_changed(), state)
 
     assert out.unit_status == testing.BlockedStatus(
-        "Failed to configure aproxy: Proxy is not reachable at modified.proxy:3128"
+        "Failed to configure aproxy: Proxy is not reachable at modified.proxy:80"
     )