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",