From 4fd0edec2401dc9a1a692c85f40ab3aba69d9c6a Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Tue, 4 Aug 2026 16:20:41 +0200 Subject: [PATCH] Add drift check for retired release references Retirement removes a release from the supported range, but osism/defaults keeps two kinds of reference to it that nothing fails on afterwards: - a version gate -- openstack_version in ['A', 'B'] -- selecting a value for particular releases. Once a listed release is gone that branch can never be taken again. - a per-release file -- all/010-.yml -- a backward-compat layer extending the 001 mirror for an older supported release. Its own header says to delete it when that release leaves the range, and nothing checks that it was. Both are dead rather than broken, so nothing prompts the cleanup. A gate on victoria has sat in the image catalogue since roughly 2021 unnoticed, which is the argument for a check rather than a habit. Nothing covered this: every other defaults-reading plugin asks whether a var name exists upstream (kolla_image_orphan, kolla_enablement_orphan), whether an orphaned enable flag anchors it (kolla_orphan_config), or whether a value matches upstream and only in all/001 (kolla_mirror_verbatim). The staleness here is in neither the name nor the value but in the gate condition, which nothing read. This is the mirror image of kolla_source_ref_phase: that plugin catches a reference that should have moved forward when a release changed phase, this one a reference that should have been dropped when a release was retired. Both failure modes are silent, and both went years unnoticed. Findings are advisory. A gate on a release that can no longer be deployed may still be load-bearing for an upgrade *from* it -- the deploy scripts pick the key-value store by reading the release from the running kolla-ansible image label, precisely to handle the mid-upgrade state -- so "outside the supported range" means "confirm this is still needed", not "this is a defect". The nightly job stays green and deliberate keeps go in the allowlist. Scope is osism/defaults only, and that is a deliberate limit. The same redis/valkey cutover is gated in shell case statements in testbed, metalbox and container-image-kolla-ansible, so covering them would find three more today -- but a case parser has to handle ;;& and ;&, nested case, arms sharing a line with code, quoting and heredocs, and a subtle mis-parse yields a silent false negative, which is the failure this check exists to prevent. It would also be the only shell-parsing code in the tool. kolla_source_ref_phase made the same call for the hardcoded requirements ref in container-images-kolla scripts/002-generate.sh. Those copies are covered by the retirement checklist in the guide, which names each file (osism/osism.github.io#1047). The defaults side needs no such judgement: the repository uses exactly one idiom, `in` and its `not in` form with a bracketed list of quoted literals, and every all/*.yml file is valid YAML whose jinja lives only in values. parse_version_gates therefore matches one stable construct inside safe_load'ed strings and ignores the rest of the expression rather than trying to understand jinja. Values are walked recursively, since a gate can sit inside a map or a list rather than a scalar. The all/010-.yml convention now has one definition, enablement.per_release_file, used both by groupvars_home -- which routes dropped keys into these files -- and by the check that retires them, so the two cannot disagree about the name. Ordered next to kolla_orphan_config: both report dead content in osism/defaults, one for services upstream removed and one for releases we no longer support. Verified against the live repositories: reports mariadb_image in all/002-images-kolla.yml as the single advisory finding, and stays silent on the three all/010-*.yml files, whose releases are all still supported. 422 tests pass; flake8 and black clean. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- src/drift-config.yml | 1 + src/osism_drift/drift/__init__.py | 2 + .../drift/kolla_version_gate_orphan.py | 136 ++++++++++++ src/osism_drift/enablement.py | 76 ++++++- .../test_enablement_version_gates.py | 75 +++++++ .../test_kolla_version_gate_orphan.py | 198 ++++++++++++++++++ tests/kolla_drift/test_plugin_registry.py | 1 + 7 files changed, 488 insertions(+), 1 deletion(-) create mode 100644 src/osism_drift/drift/kolla_version_gate_orphan.py create mode 100644 tests/kolla_drift/test_enablement_version_gates.py create mode 100644 tests/kolla_drift/test_kolla_version_gate_orphan.py diff --git a/src/drift-config.yml b/src/drift-config.yml index 0c3288d2..c54d6a1c 100644 --- a/src/drift-config.yml +++ b/src/drift-config.yml @@ -32,3 +32,4 @@ plugins: kolla_image_orphan: {enabled: true} kolla_secrets_orphan: {enabled: true} kolla_source_ref_phase: {enabled: true} + kolla_version_gate_orphan: {enabled: true} diff --git a/src/osism_drift/drift/__init__.py b/src/osism_drift/drift/__init__.py index 10bf903c..39cba0e4 100644 --- a/src/osism_drift/drift/__init__.py +++ b/src/osism_drift/drift/__init__.py @@ -14,6 +14,7 @@ kolla_source_ref_phase, kolla_version_chain_inner, kolla_version_chain_upstream, + kolla_version_gate_orphan, release_vs_manager, role_shadows, role_unpinned, @@ -26,6 +27,7 @@ kolla_groupvars_missing, kolla_mirror_verbatim, kolla_orphan_config, + kolla_version_gate_orphan, kolla_image_orphan, kolla_secrets_orphan, kolla_enablement_build, diff --git a/src/osism_drift/drift/kolla_version_gate_orphan.py b/src/osism_drift/drift/kolla_version_gate_orphan.py new file mode 100644 index 00000000..27f9bec4 --- /dev/null +++ b/src/osism_drift/drift/kolla_version_gate_orphan.py @@ -0,0 +1,136 @@ +"""kolla_version_gate_orphan: osism/defaults still names a retired release. + +Retirement removes a release from the supported range, but osism/defaults keeps +two kinds of reference to it that nothing fails on afterwards: + +- a **version gate** -- `openstack_version in ['A', 'B']` -- selecting a value + for particular releases. Once a listed release is gone, that branch can never + be taken again. +- a **per-release file** -- `all/010-.yml` -- a backward-compat layer + extending the 001 mirror for an older supported release. Its own header says + to delete it when that release leaves the range. + +Both are dead rather than broken, so nothing prompts the cleanup: a `victoria` +gate sat in the image catalogue for years unnoticed. This compares every +reference against the supported range (the release/latest/openstack-*.yml file +set) and reports the ones that name a release outside it. + +The mirror image of kolla_source_ref_phase: that plugin catches a reference that +should have moved *forward* when a release changed phase, this one a reference +that should have been *dropped* when a release was retired. + +Findings are advisory. A gate on a release that can no longer be deployed may +still be load-bearing for an upgrade *from* it, so "outside the supported range" +means "confirm this is still needed", not "this is a defect". Deliberate keeps +belong in the allowlist. + +Scope is deliberately osism/defaults only. The same redis/valkey cutover is +gated in shell `case` statements in testbed, metalbox and +container-image-kolla-ansible, but a `case` parser would have to handle `;;&`, +`;&`, nested case, arms sharing a line, quoting and heredocs -- and a subtle +mis-parse there yields a silent false negative, the failure this check exists to +prevent. Those copies are covered by the retirement checklist in the guide +instead, which names each file. Same call as kolla_source_ref_phase made for the +hardcoded requirements ref in container-images-kolla scripts/002-generate.sh. +""" + +from osism_drift import enablement, source +from osism_drift.model import DriftEntry + +NAME = "kolla_version_gate_orphan" +DESCRIPTION = ( + "Flag osism/defaults version gates and per-release compat files that name an " + "OpenStack release outside the supported range, so their content is dead." +) +INPUT_FILES = [ + ("defaults", "all/*.yml"), + ("release", "latest/openstack-*.yml (the supported release range)"), +] +SUMMARY = ( + "{n} version gates naming a release outside the supported range, so the " + "branch they select can no longer be taken:" +) +REMEDIATION = ( + "drop the retired release from the gate; if that leaves the condition " + "matching nothing, remove the conditional and keep the value that remains. " + "Allowlist a gate deliberately kept for upgrades from that release." +) +# Per-entry overrides for the file findings, so they render as their own block: +# deleting a whole file is a different action from editing an expression. +FILE_SUMMARY = ( + "{n} per-release compat files for a release outside the supported range, " + "which their own header says to delete at this point:" +) +FILE_REMEDIATION = ( + "delete the file. It exists only to extend the 001 mirror for an older " + "release while that release is supported; once it leaves the range every key " + "in the file is dead. Allowlist it if it is deliberately kept." +) + +EXPECTED_SRC = "osism/release latest/openstack-*.yml (supported releases)" +_DEFAULTS_DIR = "all" + + +def stale_releases(named, supported) -> list: + """The `named` releases that are not in `supported`, order preserved.""" + return [r for r in named if r not in supported] + + +def _entry(image, found, found_src, supported, summary, remediation): + """One advisory finding: `image` still names `found`, which is unsupported.""" + return DriftEntry( + plugin=NAME, + image=image, + alias=image, + expected=", ".join(sorted(supported)), + found=found, + expected_src=EXPECTED_SRC, + found_src=found_src, + summary=summary, + remediation=remediation, + severity="advisory", + ) + + +def run(config, allowlist, verbose: bool = False) -> list[DriftEntry]: + """Return advisory drifts for defaults references to unsupported releases.""" + supported = set(enablement.release_range(config)) + drifts = [] + for filename in sorted(source.list_dir("defaults", _DEFAULTS_DIR, config)): + if not filename.endswith(".yml"): + continue + + target = enablement.per_release_file_target(filename) + if target is not None and target not in supported: + drifts.append( + allowlist.apply( + _entry( + image=filename, + found=target, + found_src=f"osism/defaults {_DEFAULTS_DIR}/", + supported=supported, + summary=FILE_SUMMARY, + remediation=FILE_REMEDIATION, + ) + ) + ) + + body = source.read("defaults", f"{_DEFAULTS_DIR}/{filename}", config) + gates = enablement.parse_version_gates(body) + for var, named in sorted(gates.items()): + dead = stale_releases(named, supported) + if not dead: + continue + drifts.append( + allowlist.apply( + _entry( + image=var, + found=", ".join(dead), + found_src=f"osism/defaults {_DEFAULTS_DIR}/{filename}", + supported=supported, + summary=SUMMARY, + remediation=REMEDIATION, + ) + ) + ) + return drifts diff --git a/src/osism_drift/enablement.py b/src/osism_drift/enablement.py index 13c4a637..813bb3f2 100644 --- a/src/osism_drift/enablement.py +++ b/src/osism_drift/enablement.py @@ -6,6 +6,8 @@ build keys use hyphens) so every cross-space comparison is on one form. """ +import re + import yaml from osism_drift import secrets_map, source @@ -16,6 +18,78 @@ def canon(name: str) -> str: return name.replace("-", "_") +# The one idiom osism/defaults uses to make a value depend on the release. The +# `not in` form is matched too: whether a named release is still supported does +# not depend on the sense of the test. +_VERSION_GATE = re.compile(r"openstack_version\s+(?:not\s+)?in\s*\[([^\]]*)\]") +_GATE_LITERAL = re.compile(r"""['"]\s*([^'"]+?)\s*['"]""") + + +def _value_strings(value): + """Yield every string inside `value`, walking mappings and lists. + + A gate can sit anywhere in a var's value, not only in a scalar: the defaults + carry nested structures (container-dimension maps, option lists). + """ + if isinstance(value, str): + yield value + elif isinstance(value, dict): + for item in value.values(): + yield from _value_strings(item) + elif isinstance(value, list): + for item in value: + yield from _value_strings(item) + + +def parse_version_gates(body: bytes) -> dict: + """{var: [release, ...]} for every openstack_version gate in a defaults file. + + Jinja lives only in values, which safe_load returns as strings, so the gate + is matched textually inside them rather than evaluated. Only the literals + inside the bracket list are returned; everything else in the expression -- + other conditions, filters, nested parentheses -- is ignored on purpose, + keeping the parse to one stable construct instead of trying to understand + jinja. A var whose value holds no gate is absent from the result. + """ + data = yaml.safe_load(body) or {} + if not isinstance(data, dict): + return {} + gates = {} + for key, value in data.items(): + if not isinstance(key, str): + continue + releases = [] + for text in _value_strings(value): + for listed in _VERSION_GATE.findall(text): + releases.extend(_GATE_LITERAL.findall(listed)) + if releases: + gates[key] = list(dict.fromkeys(releases)) + return gates + + +_PER_RELEASE_PREFIX = "010-" + + +def per_release_file(release: str) -> str: + """The all/ path holding backward-compat keys for `release` (parent spec D8). + + One definition of the convention, so the check that fills these files + (groupvars_home) and the check that retires them cannot disagree about the + name. + """ + return f"all/{_PER_RELEASE_PREFIX}{release}.yml" + + +def per_release_file_target(filename: str) -> str | None: + """The release an all/ `filename` carries compat keys for, else None. + + Inverse of per_release_file, over a bare filename as list_dir yields it. + """ + if not filename.startswith(_PER_RELEASE_PREFIX) or not filename.endswith(".yml"): + return None + return filename[len(_PER_RELEASE_PREFIX) : -len(".yml")] or None + + def parse_enable_flags(body: bytes) -> dict: """{service_id: raw_value} for every enable_ in an OSISM vars file.""" data = yaml.safe_load(body) or {} @@ -340,7 +414,7 @@ def groupvars_home(key, newest, newest_keys, dropped_map): return ("all/001-kolla-defaults.yml", f"upstream defines it at {newest}") L = dropped_map.get(key) if L: - return (f"all/010-{L}.yml", f"upstream dropped by {newest}; last in {L}") + return (per_release_file(L), f"upstream dropped by {newest}; last in {L}") return None diff --git a/tests/kolla_drift/test_enablement_version_gates.py b/tests/kolla_drift/test_enablement_version_gates.py new file mode 100644 index 00000000..e09f2f65 --- /dev/null +++ b/tests/kolla_drift/test_enablement_version_gates.py @@ -0,0 +1,75 @@ +from osism_drift import enablement + + +def test_parses_a_gate_into_its_listed_releases(): + body = b""" +enable_redis: "{{ 'yes' if openstack_version in ['2024.1', '2024.2'] else 'no' }}" +""" + assert enablement.parse_version_gates(body) == { + "enable_redis": ["2024.1", "2024.2"] + } + + +def test_parses_the_not_in_form_the_same_way(): + """Whether a named release is still supported does not depend on the sense.""" + body = b"enable_x: \"{{ 'a' if openstack_version not in ['2024.1'] else 'b' }}\"\n" + assert enablement.parse_version_gates(body) == {"enable_x": ["2024.1"]} + + +def test_parses_a_named_release_literal(): + body = b"mariadb_image: \"{{ 'mariadb' if openstack_version in ['victoria'] }}\"\n" + assert enablement.parse_version_gates(body) == {"mariadb_image": ["victoria"]} + + +def test_ignores_the_rest_of_the_expression(): + """Only the bracket list is read; other conditions and filters are not parsed.""" + body = ( + b"horizon_listen_port: \"{{ '8080' if (enable_haproxy | bool and " + b"openstack_version not in ['2024.1', '2025.1']) else horizon_port }}\"\n" + ) + assert enablement.parse_version_gates(body) == { + "horizon_listen_port": ["2024.1", "2025.1"] + } + + +def test_finds_a_gate_nested_inside_a_structured_value(): + """Values are not always scalars; a gate can sit inside a map or a list.""" + body = b""" +dims: + ulimits: + nofile: "{{ 1024 if openstack_version in ['2024.2'] else 2048 }}" +opts: + - "{{ 'x' if openstack_version in ['victoria'] else 'y' }}" +""" + assert enablement.parse_version_gates(body) == { + "dims": ["2024.2"], + "opts": ["victoria"], + } + + +def test_var_without_a_gate_is_absent(): + body = b'plain: "{{ docker_image_url }}nova-api"\nother: 5\n' + assert enablement.parse_version_gates(body) == {} + + +def test_deduplicates_repeated_literals_preserving_order(): + body = ( + b"v: \"{{ 'a' if openstack_version in ['2025.1', '2024.1'] else " + b"('b' if openstack_version in ['2024.1'] else 'c') }}\"\n" + ) + assert enablement.parse_version_gates(body) == {"v": ["2025.1", "2024.1"]} + + +def test_empty_and_non_mapping_documents_yield_nothing(): + assert enablement.parse_version_gates(b"") == {} + assert enablement.parse_version_gates(b"- a\n- b\n") == {} + + +def test_per_release_file_round_trips(): + assert enablement.per_release_file("2024.2") == "all/010-2024.2.yml" + assert enablement.per_release_file_target("010-2024.2.yml") == "2024.2" + + +def test_per_release_file_target_rejects_other_files(): + for name in ("001-kolla-defaults.yml", "099-kolla.yml", "010-.yml", "010-2024.2"): + assert enablement.per_release_file_target(name) is None, name diff --git a/tests/kolla_drift/test_kolla_version_gate_orphan.py b/tests/kolla_drift/test_kolla_version_gate_orphan.py new file mode 100644 index 00000000..17c3b577 --- /dev/null +++ b/tests/kolla_drift/test_kolla_version_gate_orphan.py @@ -0,0 +1,198 @@ +"""Isolated tmp_path trees on purpose: the shared fixtures/defaults/all +directory is read whole by several other plugins, so a file with a version gate +added there would change their results too. +""" + +import pytest +from osism_drift.config import Allowlist, AllowEntry, Config, PluginCfg, Remote +from osism_drift.drift import kolla_version_gate_orphan as plugin + +SUPPORTED = ("2024.1", "2025.1") + + +def _tree(tmp_path, files, releases=SUPPORTED): + """Write `files` into /defaults/all and return a Config reading it.""" + d = tmp_path / "defaults" / "all" + d.mkdir(parents=True) + for name, text in files.items(): + (d / name).write_text(text) + return Config( + remote=Remote("https://raw/", "https://api/", "main", "osism"), + base_dirs=(str(tmp_path),), + release_version="latest", + plugins={plugin.NAME: PluginCfg(enabled=True)}, + releases=releases, + ) + + +def _run(tmp_path, files, allowlist=None, releases=SUPPORTED): + cfg = _tree(tmp_path, files, releases) + return plugin.run(cfg, allowlist or Allowlist(())) + + +# --- version gates ----------------------------------------------------------- + + +def test_flags_a_gate_naming_a_release_outside_the_supported_range(tmp_path): + drifts = _run( + tmp_path, + { + "002-images.yml": "mariadb_image: \"{{ 'a' if openstack_version in " + "['victoria'] else 'b' }}\"\n" + }, + ) + assert len(drifts) == 1 + d = drifts[0] + assert (d.plugin, d.image, d.found) == (plugin.NAME, "mariadb_image", "victoria") + assert d.found_src == "osism/defaults all/002-images.yml" + assert d.expected == "2024.1, 2025.1" + + +def test_silent_when_every_named_release_is_supported(tmp_path): + drifts = _run( + tmp_path, + { + "099.yml": "enable_redis: \"{{ 'yes' if openstack_version in " + "['2024.1', '2025.1'] else 'no' }}\"\n" + }, + ) + assert drifts == [] + + +def test_reports_only_the_dead_releases_of_a_mixed_gate(tmp_path): + """A gate can name both a supported and a retired release.""" + drifts = _run( + tmp_path, + { + "099.yml": "enable_x: \"{{ 'y' if openstack_version in " + "['2024.1', '2023.2'] else 'n' }}\"\n" + }, + ) + assert [d.found for d in drifts] == ["2023.2"] + + +def test_flags_a_not_in_gate_too(tmp_path): + drifts = _run( + tmp_path, + { + "099.yml": "p: \"{{ 'a' if openstack_version not in ['2023.2'] else 'b' }}\"\n" + }, + ) + assert [d.image for d in drifts] == ["p"] + + +def test_ignores_files_that_are_not_yaml(tmp_path): + drifts = _run( + tmp_path, + {"README.md": "openstack_version in ['victoria']\n"}, + ) + assert drifts == [] + + +# --- per-release compat files ------------------------------------------------ + + +def test_flags_a_per_release_file_for_a_retired_release(tmp_path): + """The file's own header says to delete it once the release leaves the range.""" + drifts = _run(tmp_path, {"010-2023.2.yml": "swift_rsync_port: '10873'\n"}) + assert len(drifts) == 1 + d = drifts[0] + assert (d.image, d.found) == ("010-2023.2.yml", "2023.2") + assert d.found_src == "osism/defaults all/" + assert d.summary == plugin.FILE_SUMMARY + + +def test_silent_for_a_per_release_file_of_a_supported_release(tmp_path): + assert _run(tmp_path, {"010-2024.1.yml": "k: v\n"}) == [] + + +def test_a_retired_per_release_file_can_also_carry_a_stale_gate(tmp_path): + """Both kinds are reported; they call for different actions.""" + drifts = _run( + tmp_path, + { + "010-2023.2.yml": "e: \"{{ 'a' if openstack_version in ['2023.2'] " + "else 'b' }}\"\n" + }, + ) + assert sorted(d.image for d in drifts) == ["010-2023.2.yml", "e"] + + +# --- severity and allowlist -------------------------------------------------- + + +def test_findings_are_advisory_so_the_nightly_job_stays_green(tmp_path): + """A gate on an unsupported release may still serve upgrades from it.""" + drifts = _run( + tmp_path, + {"099.yml": "g: \"{{ 'a' if openstack_version in ['victoria'] else 'b' }}\"\n"}, + ) + assert [d.severity for d in drifts] == ["advisory"] + + +def test_a_deliberate_gate_can_be_allowlisted(tmp_path): + allowlist = Allowlist( + (AllowEntry(plugin=plugin.NAME, image="g", reason="needed for upgrades"),) + ) + drifts = _run( + tmp_path, + {"099.yml": "g: \"{{ 'a' if openstack_version in ['victoria'] else 'b' }}\"\n"}, + allowlist=allowlist, + ) + assert [d.allowlisted for d in drifts] == [True] + + +# --- pure helper ------------------------------------------------------------- + + +def test_stale_releases_keeps_order_and_drops_supported(): + assert plugin.stale_releases( + ["2025.1", "victoria", "2024.1", "2023.2"], {"2024.1", "2025.1"} + ) == ["victoria", "2023.2"] + + +# --- report ------------------------------------------------------------------ + + +def test_report_names_the_variable_the_release_and_the_file(tmp_path): + from osism_drift import report + + drifts = _run( + tmp_path, + { + "002-images.yml": "mariadb_image: \"{{ 'a' if openstack_version in " + "['victoria'] else 'b' }}\"\n" + }, + ) + text = "\n".join(report.format_text(drifts, [plugin])) + assert "mariadb_image" in text + assert "002-images.yml" in text + assert "outside the supported range" in " ".join(text.split()) + + +def test_gate_and_file_findings_render_as_separate_blocks(tmp_path): + """Editing an expression and deleting a file are different actions.""" + from osism_drift import report + + drifts = _run( + tmp_path, + { + "010-2023.2.yml": "k: v\n", + "099.yml": "g: \"{{ 'a' if openstack_version in ['victoria'] " + "else 'b' }}\"\n", + }, + ) + text = "\n".join(report.format_text(drifts, [plugin])) + assert text.count(plugin.NAME) == 2 + assert "delete the file" in " ".join(text.split()) + + +@pytest.fixture +def registered(): + from osism_drift.drift import KOLLA_PLUGINS + + return [p.NAME for p in KOLLA_PLUGINS] + + +def test_plugin_is_registered(registered): + assert plugin.NAME in registered diff --git a/tests/kolla_drift/test_plugin_registry.py b/tests/kolla_drift/test_plugin_registry.py index b9728fb9..e612467f 100644 --- a/tests/kolla_drift/test_plugin_registry.py +++ b/tests/kolla_drift/test_plugin_registry.py @@ -78,6 +78,7 @@ def test_plugins_in_lifecycle_order(): "kolla_groupvars_missing", "kolla_mirror_verbatim", "kolla_orphan_config", + "kolla_version_gate_orphan", "kolla_image_orphan", "kolla_secrets_orphan", "kolla_enablement_build",