fix(garm): deliver aproxy bootstrap as pre-install script so it runs before GARM's install wrapper

charms/garm/src/charm_state.py

@@ -102,6 +102,7 @@ def from_databag(cls, data: Mapping[str, str]) -> "RunnerConfig":
         )
         values["otel_collector_endpoint"] = _strip_newlines(values["otel_collector_endpoint"])
         values["dockerhub_mirror"] = _strip_newlines(values["dockerhub_mirror"])
+        values["runner_http_proxy"] = _strip_newlines(values["runner_http_proxy"])
         return cls(**values)
 
     def has_config(self) -> bool:

charms/garm/src/runner_template.py

@@ -4,18 +4,26 @@
 """Render runner-behaviour options into a GARM runner-install template.
 
 GARM stores the runner-install script as a server-side template that a
-scaleset references by ``template_id``. The six operator-facing runner options
-(docker mirror, proxy/aproxy, telemetry, pre-job hook) are delivered by copying
-GARM's system ``github_linux`` template and injecting two blocks right after the
+scaleset references by ``template_id``. Three of the six operator-facing runner
+options (docker mirror, telemetry, pre-job hook) are delivered by copying GARM's
+system ``github_linux`` template and injecting two blocks right after the
 shebang — before the base template's ``set -e`` — so a best-effort step can never
 abort the whole bootstrap:
 
   * a *pre-install* block that runs as root before the runner is installed
-    (aproxy, docker registry mirror, static host prep), and
+    (docker registry mirror, static host prep), and
   * a *runner job hooks* block that writes the GitHub job-start hook and the
     runner ``env`` file under ``/home/runner/actions-runner`` (GARM hardcodes the
     ``runner`` user and that path).
 
+The proxy/aproxy option is *not* injected into this template: GARM prepends a
+compiled-in wrapper script that curls this template from the GARM metadata API
+before any of its content runs, so proxy bootstrap embedded in the template
+could never help that curl succeed on a proxy-only network. It is instead
+rendered by :func:`render_aproxy_pre_install_script` and delivered as a
+``pre_install_scripts`` extra-spec entry (see ``scaleset_reconciler``), which
+GARM runs, as root, before the wrapper.
+
 The reconciler creates/updates the per-scaleset template with the bytes returned
 by :func:`build_template_data` and only does so when :meth:`RunnerConfig.has_config`
 is true.
@@ -29,6 +37,7 @@
 import json
 import pathlib
 import shlex
+import urllib.parse
 
 import jinja2
 
@@ -102,12 +111,11 @@ def render_pre_install(config: RunnerConfig) -> str:
     Returns:
         A bash snippet, terminated by a newline.
     """
-    aproxy = _render_aproxy(config) if config.runner_http_proxy else ""
     dockerhub = (
         _render_dockerhub_mirror(config.dockerhub_mirror) if config.dockerhub_mirror else ""
     )
     return _PRE_INSTALL_TEMPLATE.render(
-        aproxy=aproxy, dockerhub=dockerhub, static_prep=_render_static_host_prep()
+        dockerhub=dockerhub, static_prep=_render_static_host_prep()
     )
 
 
@@ -156,6 +164,60 @@ def render_pre_job_hooks(config: RunnerConfig) -> str:
     )
 
 
+def render_aproxy_pre_install_script(config: RunnerConfig) -> str:
+    """Render the standalone aproxy bootstrap script.
+
+    The script installs aproxy, points it and the snap store at the configured
+    proxy, and writes an nftables DNAT ruleset that transparently redirects
+    egress traffic to aproxy's listener, both for locally-initiated connections
+    (``output`` chain) and forwarded ones (``prerouting`` chain).
+
+    Args:
+        config: The runner options (uses proxy, exclude addresses, redirect ports).
+
+    Returns:
+        A complete best-effort bash script (shebang first line, deliberately no
+        ``set -e``) configuring aproxy as a transparent forward proxy.
+    """
+    # Values are already validated at parse time (RunnerConfig.from_databag); only
+    # shell-safe embedding (quoting, the empty-string split guard below) happens here.
+    ports = [t for t in config.aproxy_redirect_ports.split(",") if t] or ["80", "443"]
+    nft_ports = ", ".join(ports)
+    excludes = [t for t in config.aproxy_exclude_addresses.split(",") if t]
+    exclude_elements = ", ".join(["127.0.0.0/8", *excludes])
+    # `\$default-ipv4` stays escaped so the literal two characters `$default-ipv4`
+    # land in the file, which nft itself resolves against the `define` above when
+    # it loads the file.
+    dnat_rule = (
+        f"ip daddr != @exclude tcp dport {{ {nft_ports} }} counter dnat to \\$default-ipv4:54969"
+    )
+    return _APROXY_TEMPLATE.render(
+        proxy=shlex.quote(config.runner_http_proxy),
+        proxy_host_port=shlex.quote(_proxy_host_port(config.runner_http_proxy)),
+        exclude_elements=exclude_elements,
+        dnat_rule=dnat_rule,
+    )
+
+
+def _proxy_host_port(proxy: str) -> str:
+    """Return the bare authority (``host:port``, optionally ``user:pass@``) of a proxy.
+
+    aproxy rejects a full URL for its ``proxy=`` setting, unlike ``snap set
+    system proxy.http/https=``, so it needs the scheme stripped. The URL
+    ``netloc`` is returned verbatim, preserving userinfo and IPv6 brackets that
+    ``urlsplit().hostname`` would drop. Accepts a URL (``http://host:port``) or
+    an already-bare ``host:port``.
+
+    Args:
+        proxy: The configured proxy value, with or without a URL scheme.
+
+    Returns:
+        The proxy authority with any scheme and path removed.
+    """
+    candidate = proxy if "//" in proxy else f"//{proxy}"
+    return urllib.parse.urlsplit(candidate).netloc
+
+
 def _render_pre_job_hook_body(config: RunnerConfig, otel_endpoint: str) -> str:
     """Render the contents of the pre-job hook file (the GitHub job-start hook).
 
@@ -207,40 +269,6 @@ def _render_custom_pre_job_script(script: str) -> str:
     return _CUSTOM_PRE_JOB_SCRIPT_TEMPLATE.render(delim=delim, script=script)
 
 
-def _render_aproxy(config: RunnerConfig) -> str:
-    """Render aproxy install + nftables redirect rules.
-
-    aproxy listens on ``:54969`` and an nftables DNAT ruleset (written to
-    ``/etc/nftables.conf``) redirects egress traffic to it, both for
-    locally-initiated connections (``output`` chain) and forwarded ones
-    (``prerouting`` chain).
-
-    Args:
-        config: The runner options (uses proxy, exclude addresses, redirect ports).
-
-    Returns:
-        A bash snippet configuring aproxy as a transparent forward proxy.
-    """
-    # Values are already validated at parse time (RunnerConfig.from_databag); only
-    # shell-safe embedding (quoting, the empty-string split guard below) happens here.
-    ports = [t for t in config.aproxy_redirect_ports.split(",") if t] or ["80", "443"]
-    nft_ports = ", ".join(ports)
-    excludes = [t for t in config.aproxy_exclude_addresses.split(",") if t]
-    exclude_elements = ", ".join(["127.0.0.0/8", *excludes])
-    # `\$default-ipv4` stays escaped so the literal two characters `$default-ipv4`
-    # land in the file, which nft itself resolves against the `define` above when
-    # it loads the file.
-    dnat_rule = (
-        f"ip daddr != @exclude tcp dport {{ {nft_ports} }} counter dnat to \\$default-ipv4:54969"
-    )
-
-    return _APROXY_TEMPLATE.render(
-        proxy=shlex.quote(config.runner_http_proxy),
-        exclude_elements=exclude_elements,
-        dnat_rule=dnat_rule,
-    )
-
-
 def _render_dockerhub_mirror(mirror: str) -> str:
     """Render Docker daemon config pointing at the registry mirror.
 

charms/garm/src/scaleset_reconciler.py

@@ -14,14 +14,19 @@
 from garm_client.models.scale_set import ScaleSet
 from garm_client.models.template import Template
 from garm_client.models.update_scale_set_params import UpdateScaleSetParams
-from runner_template import build_template_data
+from runner_template import build_template_data, render_aproxy_pre_install_script
 
 logger = logging.getLogger(__name__)
 
 # GARM seeds a non-editable system template per forge/OS; we copy this one to
 # build per-scaleset runner templates carrying the operator's runner options.
 SYSTEM_TEMPLATE_NAME = "github_linux"
 
+# GARM runs pre-install scripts in lexicographic key order; "00-aproxy" sorts
+# before the configurator's "pre_install.sh", so the proxy is up before any
+# operator-supplied script runs.
+APROXY_SCRIPT_NAME = "00-aproxy"
+
 
 @dataclass
 class ScalesetSpec:
@@ -345,9 +350,6 @@ def _to_create_params(spec: ScalesetSpec) -> CreateScaleSetParams:
         Raises:
             ValidationError: If the spec data fails Pydantic model validation.
         """
-        extra_specs: dict[str, object] = {}
-        if spec.pre_install_scripts:
-            extra_specs["pre_install_scripts"] = spec.pre_install_scripts
         return CreateScaleSetParams.model_validate(
             {
                 "name": spec.name,
@@ -361,7 +363,7 @@ def _to_create_params(spec: ScalesetSpec) -> CreateScaleSetParams:
                 "enabled": True,
                 "labels": sorted(spec.labels),
                 "github_runner_group": spec.runner_group or None,
-                "extra_specs": extra_specs or None,
+                "extra_specs": _effective_extra_specs(spec) or None,
                 "template_id": spec.template_id,
             }
         )
@@ -404,18 +406,21 @@ def _maybe_update(self, observed: ScaleSet, spec: ScalesetSpec, template_id: int
             logger.debug("Scaleset %s is up to date", spec.name)
             return
 
-        extra_specs: dict[str, object] = {}
-        if spec.pre_install_scripts:
-            extra_specs["pre_install_scripts"] = spec.pre_install_scripts
-
+        # UpdateScaleSetParams omits None fields (exclude_none), so None can only
+        # leave extra_specs untouched, never clear them. Send an explicit empty
+        # dict when the desired specs are empty but the scaleset still carries
+        # some (e.g. a proxy was unset) — otherwise a stale aproxy script would
+        # persist and _needs_update would loop forever trying to converge.
+        desired_extra = _effective_extra_specs(spec)
+        extra_specs = desired_extra or ({} if observed.extra_specs else None)
         params = UpdateScaleSetParams(
             image=spec.image,
             flavor=spec.flavor,
             min_idle_runners=spec.min_idle_runners,
             max_runners=spec.max_runners,
             enabled=True,
             runner_group=spec.runner_group or None,
-            extra_specs=extra_specs or None,
+            extra_specs=extra_specs,
             template_id=spec.template_id,
         )
         # Send template_id when the scaleset has, or had, a custom template — a 0
@@ -431,14 +436,53 @@ def _maybe_update(self, observed: ScaleSet, spec: ScalesetSpec, template_id: int
 
     @staticmethod
     def _needs_update(observed: ScaleSet, spec: ScalesetSpec, template_id: int) -> bool:
-        observed_scripts = (observed.extra_specs or {}).get("pre_install_scripts", {})
+        # GARM round-trips extra_specs exactly as sent, so the observed script
+        # values are base64 like the desired ones — compare them encoded.
+        observed_extra = observed.extra_specs or {}
+        desired_extra = _effective_extra_specs(spec)
         return (
             observed.image != spec.image
             or observed.flavor != spec.flavor
             or observed.max_runners != spec.max_runners
             or observed.min_idle_runners != spec.min_idle_runners
             or observed.enabled is not True
             or observed.github_runner_group != (spec.runner_group or None)
-            or observed_scripts != spec.pre_install_scripts
+            or observed_extra.get("pre_install_scripts", {})
+            != desired_extra.get("pre_install_scripts", {})
+            or bool(observed_extra.get("disable_updates"))
+            != bool(desired_extra.get("disable_updates"))
             or (observed.template_id or 0) != template_id
         )
+
+
+def _effective_extra_specs(spec: ScalesetSpec) -> dict[str, object]:
+    """Build the scaleset extra_specs a spec should produce.
+
+    Single source of truth for create, update, and drift detection: combines the
+    operator-supplied pre-install scripts with the charm's aproxy bootstrap and
+    the ``disable_updates`` flag when a runner proxy is configured.
+
+    Args:
+        spec: The desired scaleset.
+
+    Returns:
+        The extra_specs dict, with all script values base64-encoded (GARM decodes
+        ``pre_install_scripts`` as ``map[string][]byte``); empty when the spec
+        yields no extra specs.
+    """
+    scripts = dict(spec.pre_install_scripts)
+    extra_specs: dict[str, object] = {}
+    if spec.runner_config.runner_http_proxy:
+        # The aproxy bootstrap must run before GARM's compiled-in install wrapper
+        # (which needs egress to fetch the runner template), hence a pre-install
+        # script rather than template content.
+        scripts[APROXY_SCRIPT_NAME] = render_aproxy_pre_install_script(spec.runner_config)
+        # cloud-init's apt upgrade runs before any pre-install script, so it can
+        # never use the proxy — skip it instead of timing out on every mirror.
+        extra_specs["disable_updates"] = True
+    if scripts:
+        extra_specs["pre_install_scripts"] = {
+            name: base64.b64encode(content.encode("utf-8")).decode("utf-8")
+            for name, content in scripts.items()
+        }
+    return extra_specs

charms/garm/src/templates/aproxy.j2

@@ -1,8 +1,40 @@
-# Transparently forward runner egress through the configured HTTP proxy.
-snap install aproxy --edge
-snap set aproxy proxy={{ proxy }} listen=:54969
-nft delete table ip aproxy 2>/dev/null || true
-cat << EOF > /etc/nftables.conf
+#!/bin/bash
+# aproxy proxy bootstrap, delivered as a GARM pre-install script so it runs
+# before the runner-install wrapper. cloud-init runs each pre-install script as
+# its own runcmd entry and continues past a non-zero exit, so strict mode here
+# would not protect GARM's sequence anyway -- a failure already surfaces
+# downstream as the wrapper's curl to the GARM API failing. Instead we log each
+# step (to /var/log/cloud-init-output.log and the serial console) so a broken
+# proxy bootstrap is diagnosable, and we install the nftables redirect only once
+# aproxy is actually listening: a DNAT to a dead :54969 would black-hole all
+# :80/:443 egress instead of leaving the box on whatever path it had.
+
+log() { echo "[aproxy-bootstrap] $*"; }
+
+# snapd itself needs the proxy to reach the snap store before aproxy exists.
+snap set system proxy.http={{ proxy }}
+snap set system proxy.https={{ proxy }}
+
+log "installing aproxy snap"
+# A non-zero exit is expected when aproxy is already baked into the image; only
+# bail if the snap is genuinely absent afterwards.
+if ! snap install aproxy --edge && ! snap list aproxy >/dev/null 2>&1; then
+    log "ERROR: aproxy install failed and the snap is not present; skipping redirect"
+    exit 0
+fi
+
+if ! snap set aproxy proxy={{ proxy_host_port }} listen=:54969; then
+    log "ERROR: failed to point aproxy at the upstream proxy; skipping redirect"
+    exit 0
+fi
+
+# Only redirect once aproxy is actually listening; otherwise the DNAT would
+# black-hole all :80/:443 egress by sending it to a dead port. Runner VMs are
+# single-use and never reboot, so the ruleset is piped straight into the kernel
+# rather than persisted to a file and reloaded by nftables.service on boot.
+if snap services aproxy 2>/dev/null | grep -q active; then
+    log "aproxy is listening; applying nftables redirect"
+    nft -f - << EOF
 define default-ipv4 = $(ip route get $(ip route show 0.0.0.0/0 | grep -oP 'via \K\S+') | grep -oP 'src \K\S+')
 table ip aproxy {
       set exclude {
@@ -20,5 +52,6 @@ table ip aproxy {
       }
 }
 EOF
-systemctl enable nftables.service
-nft -f /etc/nftables.conf
+else
+    log "ERROR: aproxy service is not active; leaving egress unchanged (no redirect)"
+fi

charms/garm/src/templates/pre_install.j2

@@ -1,8 +1,5 @@
 
 # ===== charm-injected pre-install setup (runs as root) =====
-{% if aproxy %}
-{{ aproxy }}
-{% endif %}
 {% if dockerhub %}
 {{ dockerhub }}
 {% endif %}