From faf6618cf0470eccc7990264a270bda24ed20821 Mon Sep 17 00:00:00 2001 From: Blair Hamilton Date: Tue, 18 Aug 2026 15:59:45 -0400 Subject: [PATCH 1/2] feat: k5s-stack-namespaces hook, and put it under the install guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two sibling k5s lane overlays that declare the same `namespace:` fail silently: `k5s up` on one server-side-applies over the other's objects, no error is raised, the pods go Ready, and the rig runs a blend of two lanes' configuration. On a perf rig that means every number it reports was measured against a universe nobody described. The mistake has one shape — a new lane starts as a copy of an existing one and its namespace is not changed with everything else. - Assert uniqueness only, never a naming convention. A repo's lane names are its own business; pinning `perf-rig-` would be one repo's convention wearing an org hook's clothes. - Read files as written rather than through k5s's `extends:` resolution. Two lanes that both restate a shared namespace is the collision; resolving first would hide it. An overlay declaring no namespace inherits the base stack's — the normal shape for a lane that only adjusts load — and is skipped. - Compare every stack in the changed files' directories, not just the changed set. A collision is a property of the whole set, and the lane that already owned the namespace is usually not in the commit that collides with it. Wire it into the `hook-install` CI job as well: that job exists because `service-yaml-check` shipped for months uninstallable, and a new `language: python` hook added outside the guard reintroduces exactly the gap the guard was built to close. The fixture grows sibling overlays with distinct namespaces so the try-repo run exercises the grouping path instead of passing on an empty file set. --- .github/workflows/ci.yml | 11 ++- .pre-commit-hooks.yaml | 16 ++++ README.md | 35 ++++++++ pinpredict_hooks/k5s_stack_namespaces.py | 104 +++++++++++++++++++++++ pyproject.toml | 1 + tests/test_k5s_stack_namespaces.py | 94 ++++++++++++++++++++ 6 files changed, 260 insertions(+), 1 deletion(-) create mode 100644 pinpredict_hooks/k5s_stack_namespaces.py create mode 100644 tests/test_k5s_stack_namespaces.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 88718b5..998784d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -52,6 +52,7 @@ jobs: stevedore-release-scope --help > /dev/null service-yaml-check > /dev/null no-production-newtonsoft --help > /dev/null + k5s-stack-namespaces > /dev/null - name: try-repo each hook against a fixture consumer run: | @@ -77,6 +78,14 @@ jobs: image: pinpredict/oms YAML + # Sibling k5s lane overlays with distinct namespaces, so + # k5s-stack-namespaces exercises its grouping path rather than + # passing on an empty file set. + mkdir -p "$fixture/overlays" + printf 'namespace: rig-base\n' > "$fixture/k5s.yaml" + printf 'namespace: rig-5x\n' > "$fixture/overlays/perf-5x.yaml" + printf 'namespace: rig-10x\n' > "$fixture/overlays/perf-10x.yaml" + # A tracked, clean production source so no-production-newtonsoft has # something to actually scan — with zero matching files it would pass # without exercising the git enumeration path. @@ -86,7 +95,7 @@ jobs: git -c init.defaultBranch=main init -q --template= "$fixture" git -C "$fixture" add -A - for hook in stevedore-release-scope service-yaml-check no-production-newtonsoft; do + for hook in stevedore-release-scope service-yaml-check no-production-newtonsoft k5s-stack-namespaces; do echo "::group::try-repo $hook" (cd "$fixture" && pre-commit try-repo "$GITHUB_WORKSPACE" "$hook" --all-files) echo "::endgroup::" diff --git a/.pre-commit-hooks.yaml b/.pre-commit-hooks.yaml index 8f81d9b..3465da6 100644 --- a/.pre-commit-hooks.yaml +++ b/.pre-commit-hooks.yaml @@ -68,3 +68,19 @@ language: script files: '(^|/)go\.mod$|(^|/)\.tool-versions$' pass_filenames: false + +- id: k5s-stack-namespaces + name: k5s stack overlays declare unique namespaces + description: | + Fails when two sibling k5s stack overlays declare the same `namespace:`. A + new lane is usually a copy of an existing one, and a namespace left + unchanged makes `k5s up` server-side-apply over the other lane's objects + with no error — Ready pods running a blend of two lanes' configuration, + which for a perf rig means every number it reports is measured against a + universe nobody described. Asserts uniqueness only, never a naming + convention; overlays that declare no namespace (inheriting the base stack's) + are skipped. Compares every stack in the changed files' directories, not + just the changed set, since a collision is a property of the whole set. + entry: k5s-stack-namespaces + language: python + files: '(^|/)(k5s\.yaml|komp\.yaml|overlays/.*\.ya?ml)$' diff --git a/README.md b/README.md index cfc9568..bd2ed82 100644 --- a/README.md +++ b/README.md @@ -14,6 +14,7 @@ repos. | `stevedore-release-scope` | Assert that onboarding or retiring a service does not widen the shared image build contract: named services pair an `.stevedore.yaml` image id with a name-matching sibling chart, `docker-release` and `chart-release` receive the same `only:` selector, and `change_detection.shared_paths` carries the all-image signal without listing paths every onboarding touches. | `.stevedore.yaml`, `.github/workflows/ci.yml`, `charts/*/Chart.yaml` | | `no-production-newtonsoft` | Reject Newtonsoft.Json references in production .NET sources: a case-insensitive scan of every tracked source and build file, permitted only under the `--allow-prefix` paths (the approved test/benchmark projects) and on the one central `PackageVersion` line. Static half only — the transitive package-graph half needs `dotnet restore` and stays in the consumer's CI. | every commit (whole-tree scan) | | `check-go-version-sync` | Fails when a `go.mod` `go` directive and the governing `.tool-versions` `golang` pin drift apart. | `go.mod`, `.tool-versions` | +| `k5s-stack-namespaces` | Fails when two sibling k5s stack overlays declare the same `namespace:`. A new lane is usually a copy of an existing one, and a namespace left unchanged makes `k5s up` server-side-apply over the other lane's objects with no error — Ready pods running a blend of two lanes' config. Asserts uniqueness only, never a naming convention. | `k5s.yaml`, `komp.yaml`, `overlays/*.yaml` | ## Using a hook @@ -77,6 +78,40 @@ console script (`pinpredict_hooks/service_yaml_check.py`) with the paths — there is no `hooks/service-yaml-check.py` to run directly, for the packaging reason spelled out under [Repository layout](#repository-layout). +### `k5s-stack-namespaces` + +```yaml + - repo: https://github.com/pinpredict/pre-commit-hooks + rev: v0.5.0 + hooks: + - id: k5s-stack-namespaces +``` + +No arguments. A k5s rig is a base stack plus lane overlays +(`k5s up -f overlays/perf-10x.yaml`), and a lane targeting a shared cluster +declares its own `namespace:` so bringing one lane up cannot converge the rig +another lane is standing in. + +What it catches is quiet: a new lane starts as a copy of an existing one, and if +its `namespace:` is not changed with everything else, `k5s up` server-side-applies +over the old lane's objects. Nothing errors — the pods are Ready and the rig is +running a blend of two lanes' configuration. For a perf rig that means every +number it reports was measured against a universe nobody described. + +Two deliberate choices: + +- **Uniqueness only, never a naming convention.** A repo's lane names are its own + business; pinning a pattern like `perf-rig-` would be one repo's + convention wearing an org hook's clothes. +- **Files are read as written, not through k5s's `extends:` resolution.** Two + lanes that both *restate* a shared namespace is the collision; resolving first + would hide it. An overlay that declares no namespace at all — inheriting the + base stack's, the normal shape for a lane that only adjusts load — is skipped. + +It compares every stack in the changed files' **directories**, not just the +changed set: a collision is a property of the whole set, and the lane that already +owned the namespace is usually not part of the commit that collides with it. + ## Releasing Bump the version in `pyproject.toml` **in the same PR as the hook change**, diff --git a/pinpredict_hooks/k5s_stack_namespaces.py b/pinpredict_hooks/k5s_stack_namespaces.py new file mode 100644 index 0000000..1dd0306 --- /dev/null +++ b/pinpredict_hooks/k5s_stack_namespaces.py @@ -0,0 +1,104 @@ +#!/usr/bin/env python3 +"""Assert that sibling k5s stack overlays do not share a namespace. + +A k5s rig is a base stack plus lane overlays (`k5s up -f overlays/perf-10x.yaml`). +Each lane that targets a shared cluster declares its own `namespace:` so that +bringing one lane up cannot converge the rig another lane is standing in. + +That is easy to get wrong in exactly one way, and the way is quiet. A new lane +starts life as a copy of an existing one; if its `namespace:` is not changed with +everything else, `k5s up` on the new lane server-side-applies over the old lane's +objects. Nothing errors. The pods are Ready, the rig looks healthy, and it is +running a blend of two lanes' configuration — for a perf rig, that means every +number it reports is measured against a universe no one described. + +Generic on purpose: it asserts uniqueness, never a naming convention. A repo's +lane names are its own business, and a hook that pinned `perf-rig-` would +be one repo's convention wearing an org hook's clothes. + +A runtime suffix does not remove the need for this. k5s can append a per-engineer +suffix (`namespaceSuffix: user`), which separates PEOPLE — but two lanes declaring +the same base still resolve to the same namespace for any one engineer, so the +declared values must be distinct regardless. The two mechanisms cover different +axes and neither substitutes for the other. + +Overlays that declare NO namespace are skipped, not flagged: inheriting the base +stack's namespace is the normal shape for a lane that only adjusts load, and it +is `k5s down`-safe because there is only ever one such rig. +""" + +from __future__ import annotations + +import sys +from collections import defaultdict +from pathlib import Path + +from ._common import MISSING, emit, load_yaml + +PROG = "k5s-stack-namespaces" + + +def namespace_of(path: Path) -> str | None: + """Return the `namespace:` a stack file declares, or None. + + Reads the file as written — deliberately NOT through k5s's merge or + `extends:` resolution. Two overlays that resolve to the same namespace only + because they share a fragment are the case this must catch, and resolving + first would hide it. + """ + doc = load_yaml(path, PROG) + if doc is MISSING or not isinstance(doc, dict): + return None + ns = doc.get("namespace") + return ns if isinstance(ns, str) and ns else None + + +def check(paths: list[Path]) -> list[str]: + """Group the given stacks by declared namespace and report any collision.""" + by_namespace: dict[str, list[Path]] = defaultdict(list) + for path in sorted(set(paths)): + ns = namespace_of(path) + if ns is not None: + by_namespace[ns].append(path) + + failures = [] + for ns, owners in sorted(by_namespace.items()): + if len(owners) > 1: + listed = ", ".join(str(p) for p in owners) + failures.append( + f"namespace {ns!r} is declared by {len(owners)} stacks ({listed}) — " + f"`k5s up` on one would server-side-apply over the other's objects " + f"with no error, leaving a rig that blends both configurations. " + f"Give each its own namespace." + ) + return failures + + +def main(argv: list[str] | None = None) -> int: + # Defaulted so setuptools can wire this as a console script while a direct + # `python -m` invocation still works. + if argv is None: + argv = sys.argv[1:] + + # pre-commit passes only the CHANGED files, but a collision is a property of + # the whole SET — a lane whose namespace was already taken is invisible if the + # file that took it is not in this commit. So the changed files only tell us + # which directories to look at; every stack in those directories is compared. + changed = [Path(a) for a in argv] + if not changed: + return 0 + scope: set[Path] = set() + for path in changed: + for sibling in path.parent.glob("*.y*ml"): + if sibling.is_file(): + scope.add(sibling) + + failures = check(sorted(scope)) + if failures: + emit(failures, PROG) + return 1 + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/pyproject.toml b/pyproject.toml index a5ffcec..101b12e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -18,6 +18,7 @@ dependencies = ["PyYAML>=6"] stevedore-release-scope = "pinpredict_hooks.stevedore_release_scope:main" service-yaml-check = "pinpredict_hooks.service_yaml_check:main" no-production-newtonsoft = "pinpredict_hooks.no_production_newtonsoft:main" +k5s-stack-namespaces = "pinpredict_hooks.k5s_stack_namespaces:main" [tool.setuptools] packages = ["pinpredict_hooks"] diff --git a/tests/test_k5s_stack_namespaces.py b/tests/test_k5s_stack_namespaces.py new file mode 100644 index 0000000..db49e18 --- /dev/null +++ b/tests/test_k5s_stack_namespaces.py @@ -0,0 +1,94 @@ +#!/usr/bin/env python3 +"""Regression tests for the k5s-stack-namespaces hook. + +Plain files in a temp dir — no git repo, no subprocess — so the suite stays fast +enough to run as a pre-commit hook in this repo itself. +""" + +from __future__ import annotations + +import sys +import tempfile +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +from pinpredict_hooks.k5s_stack_namespaces import main # noqa: E402 + + +def lane(ns: str | None, extends: str | None = None) -> str: + body = "services:\n oms:\n image: img\n" + if ns is not None: + body = f"namespace: {ns}\n" + body + if extends is not None: + body = f"extends:\n - {extends}\n" + body + return body + + +class K5sStackNamespacesTest(unittest.TestCase): + def setUp(self) -> None: + self._tmp = tempfile.TemporaryDirectory() + self.dir = Path(self._tmp.name) / "overlays" + self.dir.mkdir(parents=True) + self.addCleanup(self._tmp.cleanup) + + def write(self, name: str, body: str) -> Path: + path = self.dir / name + path.write_text(body, encoding="utf-8") + return path + + def test_unique_namespaces_pass(self) -> None: + a = self.write("perf-5x.yaml", lane("perf-rig-5x")) + self.write("perf-10x.yaml", lane("perf-rig-10x")) + self.assertEqual(main([str(a)]), 0) + + def test_duplicate_namespace_fails_and_names_both_files(self) -> None: + # The realistic mistake: perf-20x started as a copy of perf-10x. + a = self.write("perf-10x.yaml", lane("perf-rig-10x")) + self.write("perf-20x.yaml", lane("perf-rig-10x")) + self.assertEqual(main([str(a)]), 1) + + def test_collision_is_caught_when_only_the_new_file_is_staged(self) -> None: + """A collision is a property of the whole SET, not the changed files. + + pre-commit passes only what this commit touched, so the lane that already + owned the namespace is usually absent from argv. Comparing just the staged + set would pass every time the older file is untouched — i.e. always. + """ + self.write("perf-10x.yaml", lane("perf-rig-10x")) + new = self.write("perf-20x.yaml", lane("perf-rig-10x")) + self.assertEqual(main([str(new)]), 1) + + def test_overlay_without_a_namespace_is_skipped(self) -> None: + """Inheriting the base stack's namespace is the normal lane shape.""" + a = self.write("perf-5x.yaml", lane(None)) + self.write("perf-10x.yaml", lane(None)) + self.assertEqual(main([str(a)]), 0) + + def test_shared_namespace_via_extends_is_still_caught(self) -> None: + """Files are read AS WRITTEN, not through k5s's extends resolution. + + Two lanes that end up in one namespace because they both inherit it from a + shared fragment is the same collision; resolving first would hide it. Here + the fragment declares the namespace and neither lane overrides it, so + neither lane declares one — nothing to compare, and the hook stays quiet. + Restating it in both lanes is what it catches. + """ + self.write("_shared.yaml", "namespace: perf-rig\n") + a = self.write("perf-5x.yaml", lane("perf-rig", extends="_shared.yaml")) + self.write("perf-10x.yaml", lane("perf-rig", extends="_shared.yaml")) + self.assertEqual(main([str(a)]), 1) + + def test_no_files_is_a_clean_noop(self) -> None: + self.assertEqual(main([]), 0) + + def test_malformed_yaml_is_fatal_not_skipped(self) -> None: + """Silently passing on unparseable YAML is how a guard stops guarding.""" + bad = self.write("perf-5x.yaml", "namespace: [unclosed\n") + with self.assertRaises(SystemExit): + main([str(bad)]) + + +if __name__ == "__main__": + unittest.main() From a4d906db1c9cda1c7bbd5215aa032b92714228f3 Mon Sep 17 00:00:00 2001 From: Blair Hamilton Date: Tue, 18 Aug 2026 15:59:59 -0400 Subject: [PATCH 2/2] chore(release): bump to 0.5.0 and re-pin the README rev examples MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding a hook is a minor bump, and the Releasing section requires the `pyproject.toml` version to move in the same PR as the hook change: under `language: python` that version is what consumers actually pip-install, so a tag cut without it ships a package whose self-reported version disagrees with the `rev:` it came from. The version was also behind independently — it read 0.3.0 against a repo already tagged v0.4.0, i.e. the exact drift that section warns about, so this closes that gap on the way past rather than carrying it forward. Move the four consumer-facing `rev:` examples and the single "Current release" line to v0.5.0 together, matching the convention set when the stale pins were last fixed. The worked example under Releasing stays at v0.4.0 — it illustrates the procedure rather than pinning a release. --- README.md | 10 +++++----- pyproject.toml | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index bd2ed82..d079d5e 100644 --- a/README.md +++ b/README.md @@ -23,12 +23,12 @@ Reference this repo from a consumer's `.pre-commit-config.yaml`: ```yaml repos: - repo: https://github.com/pinpredict/pre-commit-hooks - rev: v0.4.0 # bump to upgrade + rev: v0.5.0 # bump to upgrade hooks: - id: check-go-version-sync ``` -**Current release: `v0.4.0`.** Pin an explicit tag rather than a branch; +**Current release: `v0.5.0`.** Pin an explicit tag rather than a branch; `pre-commit autoupdate` rewrites the `rev:` to the latest tag when you want to move. @@ -179,7 +179,7 @@ A repo with no `.stevedore.yaml` at all is a clean no-op. ```yaml - repo: https://github.com/pinpredict/pre-commit-hooks - rev: v0.4.0 + rev: v0.5.0 hooks: - id: stevedore-release-scope args: @@ -215,7 +215,7 @@ Nothing is exempt by default — name every permitted prefix: ```yaml - repo: https://github.com/pinpredict/pre-commit-hooks - rev: v0.4.0 + rev: v0.5.0 hooks: - id: no-production-newtonsoft args: @@ -271,7 +271,7 @@ same toolchain. Each module's governing pin is the nearest ancestor ```yaml - repo: https://github.com/pinpredict/pre-commit-hooks - rev: v0.4.0 + rev: v0.5.0 hooks: - id: check-go-version-sync ``` diff --git a/pyproject.toml b/pyproject.toml index 101b12e..42beef8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "pinpredict-pre-commit-hooks" -version = "0.3.0" +version = "0.5.0" description = "Shared pre-commit hooks used across PinPredict repositories" readme = "README.md" requires-python = ">=3.9"