diff --git a/libs/svcforge_core/svcforge_core/adapters/helm.py b/libs/svcforge_core/svcforge_core/adapters/helm.py index efab270..1ed697a 100644 --- a/libs/svcforge_core/svcforge_core/adapters/helm.py +++ b/libs/svcforge_core/svcforge_core/adapters/helm.py @@ -28,6 +28,7 @@ from collections.abc import Sequence from pathlib import Path from typing import Any, Protocol +import httpx import yaml from pydantic import BaseModel, ConfigDict, Field @@ -55,6 +56,24 @@ _STDERR_TAIL_BYTES = 2048 MANAGED_BY_LABEL = "app.kubernetes.io/managed-by" MANAGED_BY_VALUE = "svcforge" +# Where the kubelet mounts the pod's ServiceAccount. Their presence is also how this module +# decides it is running inside the cluster: in-cluster gets the fast release read below, +# anything else falls back to shelling helm. +_SA_DIR = Path("/var/run/secrets/kubernetes.io/serviceaccount") +_SA_TOKEN = _SA_DIR / "token" +_SA_CA = _SA_DIR / "ca.crt" + +# helm's own label on every release secret it writes. Pairing it with MANAGED_BY_LABEL is +# what separates svcforge's releases from the rest of the cluster's. +_HELM_OWNER_LABEL = "owner=helm" + +# The states `helm list` shows by default. Pushed into the selector so superseded revisions +# never leave the API server: this cluster had 96 release secrets of which 71 were +# superseded, so the filter is most of the win. +_LIVE_STATUSES = "status in (deployed,failed,pending-install,pending-upgrade,pending-rollback)" + +_API_TIMEOUT_S = 15.0 + class HelmError(SvcforgeError, RuntimeError): """Non-zero exit. str(self) is the stderr tail that lands in instances.error. @@ -258,24 +277,30 @@ class HelmProvisioner: cert-manager are all reported as orphans svcforge is failing to account for, every sweep. They are not orphans. They were never svcforge's to know about. - What this does NOT fix is cost, and that was measured rather than assumed: - unscoped 23 releases in 4392ms, scoped to 0 releases in 3988ms. `--selector` is not - pushed down as a server-side label selector — helm fetches and decompresses every - release secret in the cluster regardless, then filters what it already parsed. The - saving is around 10%, not the order of magnitude the shape of the flag suggests. + Cost is why this does not shell out to helm when it does not have to. `--selector` is + NOT pushed down as a server-side selector — helm fetches and decompresses every + release secret in the cluster regardless, then filters what it already parsed. + Measured: unscoped 23 releases in 4392ms, scoped to 0 in 3988ms, a saving of about + 10%, not the order of magnitude the flag's shape suggests. - That matters because this call was timing out at 330s on every tick, with the CPU - request mutated to 0 by a cluster policy, against ~4s given real CPU. Scoping does - not rescue that. A check that never completes reports no drift, which looks exactly - like no drift existing, so the timeout needs its own fix — CPU for the container, or - reading the release secrets directly with a label selector, which the API server - answers in 141ms because it never decompresses anything. + That was not academic. With the CPU request mutated to 0 by a cluster policy, the + call took over 330s and timed out on every tick, against ~4s given real CPU. A check + that never completes reports no drift, which looks exactly like no drift existing. + + In-cluster this reads the release secrets directly instead. Measured from a pod on + this cluster: 21ms and 31KB against helm's 4392ms, because nothing is decompressed + and no release payload crosses the wire. See `_list_releases_via_api`. Out of cluster there is no + ServiceAccount to authenticate with, so it falls back to helm — which keeps the e2e + suite, and anyone running this from a laptop, working unchanged. Releases provisioned before the label existed will not match, so the first sweep after this ships sees them as missing and re-provisions. That is safe by construction — provisioning is `helm upgrade --install` against a deterministic release name — and the re-provision is what applies the label. """ + via_api = await self._list_releases_via_api() + if via_api is not None: + return via_api argv = self._base_argv( "list", "--all-namespaces", @@ -293,6 +318,104 @@ class HelmProvisioner: raise HelmError(f"helm list returned {type(parsed).__name__}, expected a list") return [ReleaseInfo.model_validate(row) for row in parsed] + async def _list_releases_via_api(self) -> list[ReleaseInfo] | None: + """Release names and namespaces read straight off helm's release secrets. + + Returns None when there is no in-cluster ServiceAccount to authenticate with, which + is the caller's signal to fall back to helm. + + This exists because helm's own list is expensive for a reason this caller does not + need. helm gunzips every release payload to build its table; the only fields anyone + here reads are name and namespace: + + reconciler/main.py live = {(r.name, r.namespace) for r in releases} + worker/handlers.py releases = {r.name for r in await ...list_releases()} + + Both live in the secret's labels and its metadata, so nothing has to be decompressed. + That is the whole difference between ~141ms and ~4.4s, and it is why this survives a + container whose CPU request was mutated to 0. + + Three details carry the correctness: + + * `PartialObjectMetadataList` in the Accept header asks the API server for metadata + only. Without it the response carries every release's gzipped manifest — megabytes + of payload fetched purely to be thrown away, which is the cost this method exists + to avoid. + * The status selector drops superseded revisions server-side: 96 release secrets on + this cluster, 25 of them live. The states kept are the ones `helm list` shows by + default, so a failed release still counts as existing — it does exist, and + treating it as missing would have the reconciler re-provision on top of it. + * helm writes one secret per revision, so a release can still appear more than once + — 20 releases here had up to 10 revisions each. The newest `version` label wins. + Skipping this would report a release that exists as several, which for the + reconciler's set difference is harmless, and for anything counting releases is not. + + `chart`, `status`, `revision` and `app_version` on the returned ReleaseInfo are the + subset the labels give away free. `chart` in particular is empty rather than wrong, + because the chart name lives only in the compressed payload. Narrowing the model + instead would have been honest about this method and dishonest about the helm path, + which does populate them. + """ + try: + token = _SA_TOKEN.read_text(encoding="utf-8").strip() + except OSError: + return None + host, port = ( + os.environ.get("KUBERNETES_SERVICE_HOST"), + os.environ.get("KUBERNETES_SERVICE_PORT_HTTPS", "443"), + ) + if not host: + return None + + selector = f"{_HELM_OWNER_LABEL},{MANAGED_BY_LABEL}={MANAGED_BY_VALUE},{_LIVE_STATUSES}" + try: + async with httpx.AsyncClient(verify=str(_SA_CA), timeout=_API_TIMEOUT_S) as client: + resp = await client.get( + f"https://{host}:{port}/api/v1/secrets", + params={"labelSelector": selector}, + headers={ + "authorization": f"Bearer {token}", + "accept": ("application/json;as=PartialObjectMetadataList;g=meta.k8s.io;v=v1"), + }, + ) + resp.raise_for_status() + items: Any = resp.json().get("items", []) + except (httpx.HTTPError, json.JSONDecodeError) as exc: + # Raise, do not fall back to helm. The fallback is for "there is no + # ServiceAccount here", which is a fact about the environment and is known + # before any request goes out. This is different: the API server was reachable + # and something went wrong, and quietly retrying through helm would swap a + # visible error for the 330s timeout this method exists to remove, on a + # container that OOMs while helm parses. The tick logs check.failed and the + # next one tries again in 60s. + raise HelmError(f"listing release secrets failed: {exc}") from exc + + newest: dict[tuple[str, str], tuple[int, ReleaseInfo]] = {} + for item in items: + meta = item.get("metadata") or {} + labels = meta.get("labels") or {} + name, namespace = labels.get("name"), meta.get("namespace") + if not name or not namespace: + continue # not a release secret this method understands; leave it alone + try: + revision = int(labels.get("version", 0)) + except ValueError: + revision = 0 + key = (name, namespace) + if key in newest and newest[key][0] >= revision: + continue + newest[key] = ( + revision, + ReleaseInfo( + name=name, + namespace=namespace, + chart="", + status=labels.get("status", ""), + revision=max(revision, 0), + ), + ) + return [info for _, info in newest.values()] + class _ValuesFile: """Context manager yielding a path to a values.yaml written from a dict. diff --git a/tests/unit/test_helm_argv.py b/tests/unit/test_helm_argv.py index 4ab816b..8e13723 100644 --- a/tests/unit/test_helm_argv.py +++ b/tests/unit/test_helm_argv.py @@ -12,9 +12,12 @@ here is that the two sides of the label agree, and that neither drops out under from __future__ import annotations from pathlib import Path +from typing import Any +import httpx import pytest +from svcforge_core.adapters import helm from svcforge_core.adapters.helm import ( MANAGED_BY_LABEL, MANAGED_BY_VALUE, @@ -91,3 +94,137 @@ async def test_the_label_written_is_the_label_read(monkeypatch: pytest.MonkeyPat await prov.list_releases() assert _pair(seen[0], "--labels") == _pair(seen[1], "--selector") + + +def _meta(name: str, ns: str, version: str, status: str = "deployed") -> dict[str, object]: + """One PartialObjectMetadata item as the API server returns it for a release secret.""" + return { + "metadata": { + "name": f"sh.helm.release.v1.{name}.v{version}", + "namespace": ns, + "labels": {"name": name, "owner": "helm", "status": status, "version": version}, + } + } + + +class _FakeResponse: + def __init__(self, items: list[dict[str, object]]) -> None: + self._items = items + + def raise_for_status(self) -> None: + return None + + def json(self) -> dict[str, object]: + return {"items": self._items} + + +def _fake_api( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, items: list[dict[str, object]] +) -> dict[str, Any]: + """Stand in for the in-cluster ServiceAccount and the API call it authenticates. + + The token is a real file at a repointed constant rather than a patched method: the + module decides it is in-cluster by whether that file reads, so faking the decision at + the filesystem keeps the test honest about what it is exercising. + """ + captured: dict[str, Any] = {} + + token = tmp_path / "token" + token.write_text("tok", encoding="utf-8") + monkeypatch.setattr(helm, "_SA_TOKEN", token) + monkeypatch.setattr(helm, "_SA_CA", tmp_path / "ca.crt") + monkeypatch.setenv("KUBERNETES_SERVICE_HOST", "10.96.0.1") + monkeypatch.setenv("KUBERNETES_SERVICE_PORT_HTTPS", "443") + + class _Client: + def __init__(self, **kw: object) -> None: + captured["client_kwargs"] = kw + + async def __aenter__(self) -> _Client: + return self + + async def __aexit__(self, *exc: object) -> None: + return None + + async def get(self, url: str, **kw: object) -> _FakeResponse: + captured["url"] = url + captured.update(kw) + return _FakeResponse(items) + + monkeypatch.setattr(httpx, "AsyncClient", _Client) + return captured + + +@pytest.mark.asyncio +async def test_api_read_collapses_a_release_to_its_newest_revision( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """helm writes one secret per revision. Ten revisions is one release, not ten.""" + _fake_api( + monkeypatch, + tmp_path, + [_meta("acme-redis", "tenant-acme", v) for v in ("1", "2", "10", "9")], + ) + + out = await HelmProvisioner().list_releases() + + assert [(r.name, r.namespace) for r in out] == [("acme-redis", "tenant-acme")] + # 10, not 9: string ordering would pick "9" and quietly report a stale revision. + assert out[0].revision == 10 + + +@pytest.mark.asyncio +async def test_api_read_keeps_same_named_releases_in_different_namespaces( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Two tenants may both call their instance `redis`. Namespace is part of the identity.""" + _fake_api(monkeypatch, tmp_path, [_meta("redis", "tenant-a", "1"), _meta("redis", "tenant-b", "1")]) + + out = await HelmProvisioner().list_releases() + + assert {(r.name, r.namespace) for r in out} == {("redis", "tenant-a"), ("redis", "tenant-b")} + + +@pytest.mark.asyncio +async def test_api_read_asks_for_metadata_only_and_scopes_the_selector( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """The Accept header is the difference between metadata and megabytes of gzipped payload.""" + cap = _fake_api(monkeypatch, tmp_path, []) + + await HelmProvisioner().list_releases() + + assert "PartialObjectMetadataList" in cap["headers"]["accept"] + selector = cap["params"]["labelSelector"] + assert "owner=helm" in selector + assert f"{MANAGED_BY_LABEL}={MANAGED_BY_VALUE}" in selector + assert "superseded" not in selector # dropped server-side by asking only for live states + assert cap["url"].endswith("/api/v1/secrets") + + +@pytest.mark.asyncio +async def test_api_read_ignores_secrets_that_are_not_releases( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """A malformed or unrelated secret must not become a phantom release.""" + _fake_api( + monkeypatch, + tmp_path, + [{"metadata": {"namespace": "x", "labels": {}}}, _meta("real", "tenant-a", "1")], + ) + + out = await HelmProvisioner().list_releases() + + assert [r.name for r in out] == ["real"] + + +@pytest.mark.asyncio +async def test_no_service_account_falls_back_to_helm(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + """Out of cluster there is nothing to authenticate with, so e2e and laptops keep working.""" + seen = _capture(monkeypatch) + + monkeypatch.setattr(helm, "_SA_TOKEN", tmp_path / "absent") + + await HelmProvisioner(kubeconfig=Path("/dev/null")).list_releases() + + assert seen and seen[0][:2] == ["helm", "list"]