diff --git a/osism/tasks/conductor/sonic/config_generator.py b/osism/tasks/conductor/sonic/config_generator.py index 76d72e432..b011893fa 100644 --- a/osism/tasks/conductor/sonic/config_generator.py +++ b/osism/tasks/conductor/sonic/config_generator.py @@ -90,11 +90,27 @@ "VERSIONS", ) -# Tables inherited from the device's image-provided base config_db.json: -# their existing content is preserved across regen because the generator does -# not emit it itself (see the ownership model on generate_sonic_config). +# Tables inherited from the device's image-provided base config_db.json: not +# dropped on regen, so their base content is preserved, while the generator +# updates only selected fields in place (DEVICE_METADATA localhost attributes, +# VERSIONS.DATABASE.VERSION) rather than rebuilding them wholesale (see the +# ownership model on generate_sonic_config). INHERITED_TABLE_KEYS = ("DEVICE_METADATA", "VERSIONS") +# Tables read from the device's image-provided base config_db.json but never +# modified by the generator: their values are consumed (via a defensive +# config.get(...), so a missing table is tolerated). They are neither dropped +# nor written, so an image-provided table passes through to the output +# unchanged -- the same output behaviour as a pass-through table, the +# difference being only that the generator depends on this one as an input. +# Distinct from inherited tables, which the generator also writes selected +# fields into; the distinction is read-only vs. read-and-update, not the access +# syntax (inherited tables are read via config.get() too). Empty for now; the +# first consumer is the gNMI listen port, read from TELEMETRY. Must stay +# disjoint from OWNED_TABLE_KEYS -- an image-consumed table dropped up front +# would always read back empty. +IMAGE_CONSUMED_TABLE_KEYS = () + # Owned tables that are also scaffolded: every scaffold key except the # inherited ones. The orchestrator setdefault-creates these up front, so # downstream helpers can index into them unconditionally. @@ -146,25 +162,36 @@ def generate_sonic_config(device, hwsku, device_as_mapping=None, config_version= dict: Minimal SONiC configuration dictionary Config ownership model: - This generator owns the tables listed in OWNED_TABLE_KEYS; operators - must not make manual adjustments to them. On every regen those tables - are rebuilt from scratch from NetBox data and hardcoded SONiC policy: - their base content is dropped up front, so neither pre-existing values - nor entries removed from NetBox survive. Operator customizations to - owned tables must be modeled in NetBox or expressed as generator - policy, not applied directly to config_db.json. - - Tables that are neither owned nor inherited are not emitted by the - generator and pass through unchanged from the device's base - config_db.json. The generator does not currently police them, so they - are not a supported place for operator customizations either. - The generator builds on the device's image-provided base - config_db.json. A few fields it does not itself emit are inherited - from that base rather than regenerated — currently the - DEVICE_METADATA localhost device attributes (e.g. the device type) - and the DATABASE schema VERSION. These come from the image, not from - operator hand-edits. + config_db.json and classifies every table it touches into one of four + categories. Operator hand-edits to config_db.json are unsupported in + all of them: customizations must be modeled in NetBox or expressed as + generator policy, never applied directly to the file. + + - Owned (OWNED_TABLE_KEYS): fully owned by the generator and rebuilt + from scratch every regen from NetBox data and hardcoded SONiC + policy. Their base content is dropped up front, so neither + pre-existing values nor entries removed from NetBox survive. + - Inherited (INHERITED_TABLE_KEYS): not dropped on regen, so the + image base content is preserved, while the generator updates + selected fields in place — currently DEVICE_METADATA localhost + attributes (hostname, hwsku, platform, mac) and the + VERSIONS.DATABASE.VERSION. Scaffold-created when absent, so they + always exist at access time even on a fresh base config. + - Image-consumed (IMAGE_CONSUMED_TABLE_KEYS): read from the image + base config but never modified by the generator. The defining + property is behavioural (read-only), not the access syntax: + inherited tables are also read via config.get(), so .get() alone + does not distinguish the two. Empty for now. + - Pass-through: every table the generator never references. Left + untouched and unmanaged; not a supported place for operator + customizations either. + + A static guard test (see test_config_generator_ownership.py) parses + this module and fails the build if it references a table that is not + owned, inherited, or image-consumed — so a newly handled table cannot + silently fall into the unpoliced pass-through tier and reintroduce + stale config. """ # Get port configuration for the HWSKU port_config = get_port_config(hwsku) diff --git a/tests/unit/tasks/conductor/sonic/test_config_generator_ownership.py b/tests/unit/tasks/conductor/sonic/test_config_generator_ownership.py index 860fc61bb..a2e95d2e7 100644 --- a/tests/unit/tasks/conductor/sonic/test_config_generator_ownership.py +++ b/tests/unit/tasks/conductor/sonic/test_config_generator_ownership.py @@ -15,9 +15,13 @@ test_config_generator_orchestrator.py for BGP_GLOBALS['default']. """ +import ast +from pathlib import Path from types import SimpleNamespace +from osism.tasks.conductor.sonic import config_generator from osism.tasks.conductor.sonic.config_generator import ( + IMAGE_CONSUMED_TABLE_KEYS, INHERITED_TABLE_KEYS, ON_DEMAND_OWNED_TABLE_KEYS, OWNED_TABLE_KEYS, @@ -250,3 +254,246 @@ def test_scaffolded_and_on_demand_owned_are_disjoint(self): be miscategorised about how it comes into existence. """ assert set(SCAFFOLDED_OWNED_TABLE_KEYS).isdisjoint(ON_DEMAND_OWNED_TABLE_KEYS) + + def test_image_consumed_and_owned_are_disjoint(self): + """No table is both image-consumed and owned. + + Image-consumed tables are read defensively from the image base config + (via config.get) and never dropped. Owned tables are dropped up front + and rebuilt. A table in both would be dropped before it is read, so the + consuming helper would always see an empty table -- the dependency the + image-consumed category exists to document would silently break. + """ + assert set(IMAGE_CONSUMED_TABLE_KEYS).isdisjoint(OWNED_TABLE_KEYS) + + def test_image_consumed_and_inherited_are_disjoint(self): + """No table is both image-consumed and inherited. + + Image-consumed tables are read but never modified; inherited tables + are read and have selected fields updated in place. The two are + mutually exclusive by definition (modified vs. not), so a table in + both is a contradictory classification. With the owned/inherited and + owned/image-consumed invariants above, this completes pairwise + disjointness across all three classified categories. + """ + assert set(IMAGE_CONSUMED_TABLE_KEYS).isdisjoint(INHERITED_TABLE_KEYS) + + +# --------------------------------------------------------------------------- +# Static guard: every referenced table is classified +# --------------------------------------------------------------------------- + + +_CONFIG_METHODS = ("get", "setdefault", "pop", "update") + +# Calls allowed on the RHS of `config = ...` as a whole-config base load: they +# take the prior config or a file as input and introduce no literal table keys +# of their own. Any other call (merge(config, {...}), dict(config, X={})) could +# carry a literal key the collector never reads, so the backstop rejects it. +_BASE_LOAD_CALLS = frozenset({"copy.deepcopy", "copy.copy", "json.load", "json.loads"}) + + +def _is_config(node): + return isinstance(node, ast.Name) and node.id == "config" + + +def _literal_dict_keys(node): + """String-literal keys of a dict-display node ({...}); [] for anything else. + + A ``**spread`` entry has a None key and is skipped, so {**config, "X": ...} + yields just ["X"]. + """ + if not isinstance(node, ast.Dict): + return [] + return [ + key.value + for key in node.keys + if isinstance(key, ast.Constant) and isinstance(key.value, str) + ] + + +def _config_table_keys_referenced_in_source(): + """Collect every top-level config_db table the generator references. + + Parses config_generator.py and records the string-literal table name of + every reference made through a local variable named ``config``: + - subscripts (``config["X"]``), + - the defensive accessors (``config.get/setdefault/pop("X", ...)``) -- a + read is a real dependency on a table, so reads are collected the same + as writes; that is how an image-consumed table such as TELEMETRY is + caught, + - update() in every literal form: ``config.update({"X": ...})``, + ``config.update(X=...)`` (keyword), ``config.update(**{"X": ...})``, + - dict-merge assignments with a literal operand: ``config |= {"X": ...}``, + ``config = config | {"X": ...}``, ``config = {**config, "X": ...}``. + + Keys that are not string literals are skipped, because the table name is + not knowable statically: dynamic subscripts (``config[some_var]``) and + merges from a non-literal mapping (``config.update(some_dict)``). Any other + way of mutating config would also hide keys, so it does not slip past + silently -- test_config_mutations_are_statically_analyzable rejects it. + Returns a set of table names. + """ + tree = ast.parse(Path(config_generator.__file__).read_text()) + referenced = set() + + for node in ast.walk(tree): + # config["X"] + if isinstance(node, ast.Subscript): + key = node.slice + if ( + _is_config(node.value) + and isinstance(key, ast.Constant) + and isinstance(key.value, str) + ): + referenced.add(key.value) + elif isinstance(node, ast.Call): + func = node.func + if isinstance(func, ast.Attribute) and _is_config(func.value): + # config.get/setdefault/pop("X", ...) + if ( + func.attr in ("get", "setdefault", "pop") + and node.args + and isinstance(node.args[0], ast.Constant) + and isinstance(node.args[0].value, str) + ): + referenced.add(node.args[0].value) + # config.update({"X": ...}) / .update(X=...) / .update(**{"X": ...}) + elif func.attr == "update": + if node.args: + referenced.update(_literal_dict_keys(node.args[0])) + for keyword in node.keywords: + if keyword.arg is not None: + referenced.add(keyword.arg) + else: + referenced.update(_literal_dict_keys(keyword.value)) + # config |= {"X": ...} + elif isinstance(node, ast.AugAssign): + if _is_config(node.target) and isinstance(node.op, ast.BitOr): + referenced.update(_literal_dict_keys(node.value)) + # config = {**config, "X": ...} / config = config | {"X": ...} + elif isinstance(node, ast.Assign): + if any(_is_config(target) for target in node.targets): + value = node.value + if isinstance(value, ast.Dict): + referenced.update(_literal_dict_keys(value)) + elif isinstance(value, ast.BinOp) and isinstance(value.op, ast.BitOr): + referenced.update(_literal_dict_keys(value.left)) + referenced.update(_literal_dict_keys(value.right)) + + return referenced + + +def _unanalyzable_config_mutations(): + """Find config mutations whose literal keys the collector cannot read. + + The collector above understands a fixed set of mutation forms. Any other + way of writing to the top-level ``config`` dict could introduce a table the + guard never sees, so this backstop locates them and the guard rejects them. + Returns a list of (lineno, source) pairs. + + Flagged: a method call ``config.(...)`` whose method is outside the + recognized set, and a reassignment ``config = `` whose right-hand + side is not a dict display, a dict-merge (``a | b``), or one of the + whitelisted base-load calls (copy.deepcopy / copy.copy / json.load / + json.loads). A merge via an arbitrary call -- ``config = merge(config, + {...})``, ``config = dict(config, X={})`` -- is therefore flagged, not + silently allowed. + + Not flagged -- the one accepted blind spot, a dynamic key in an otherwise + recognized form (``config[var]``, ``config.setdefault(var, {})``, + ``config.pop(var, None)``, ``config.update(name)``). The table name is not + a literal at the call site; the generator relies on this legitimately for + the up-front scaffold and drop loops, and resolving it would need data-flow + analysis. + """ + tree = ast.parse(Path(config_generator.__file__).read_text()) + offenders = [] + + for node in ast.walk(tree): + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and _is_config(node.func.value) + and node.func.attr not in _CONFIG_METHODS + ): + offenders.append((node.lineno, ast.unparse(node))) + elif isinstance(node, ast.Assign) and any( + _is_config(target) for target in node.targets + ): + value = node.value + allowed = ( + isinstance(value, ast.Dict) + or (isinstance(value, ast.BinOp) and isinstance(value.op, ast.BitOr)) + or ( + isinstance(value, ast.Call) + and ast.unparse(value.func) in _BASE_LOAD_CALLS + ) + ) + if not allowed: + offenders.append((node.lineno, ast.unparse(node))) + + return offenders + + +class TestStaticTableReferenceGuard: + """Every config_db table the generator touches must be classified. + + The taxonomy constants above are a hand-maintained allowlist; nothing + forces a newly handled table into one of them. This guard parses the + generator source and fails when it references a table that is neither + owned, inherited, nor image-consumed -- the omission that lets a new table + fall into the unpoliced pass-through tier and accumulate stale config. It + is static rather than runtime so it catches tables that emit only when + NetBox carries their data (a generate-and-inspect test would miss them + unless the fixture happened to trigger every table). + """ + + def test_every_referenced_table_is_classified(self): + classified = ( + set(OWNED_TABLE_KEYS) + | set(INHERITED_TABLE_KEYS) + | set(IMAGE_CONSUMED_TABLE_KEYS) + ) + referenced = _config_table_keys_referenced_in_source() + unclassified = referenced - classified + + assert not unclassified, ( + "config_generator.py references these config_db tables, but the " + "ownership model does not classify them: " + + ", ".join(sorted(unclassified)) + + ".\n\n" + "Every table the generator touches must be placed in exactly one " + "category (see the generate_sonic_config docstring):\n" + " - owned, rebuilt from NetBox/policy every regen -> add to " + "ON_DEMAND_OWNED_TABLE_KEYS (or TOP_LEVEL_SCAFFOLD_KEYS if it is " + "created up front by the orchestrator)\n" + " - image base content preserved, with selected fields updated " + "in place -> add to INHERITED_TABLE_KEYS\n" + " - read from the image but never modified -> add to " + "IMAGE_CONSUMED_TABLE_KEYS\n" + "Leaving a table unclassified lets pre-existing operator or image " + "content survive a regen, the stale-config bug the ownership model " + "exists to prevent." + ) + + def test_config_mutations_are_statically_analyzable(self): + """No config mutation can hide a literal table from the collector. + + The classification guard only catches tables it can see. This backstop + keeps that "every literal reference" guarantee honest: every write to + the config dict must use a form whose keys the collector reads, so a + new mutation idiom (e.g. config.replace(...), config = merge(...)) cannot + smuggle an unclassified table past the guard. The fix is to use a + supported form (subscript / get/setdefault/pop / update / |=) or to + teach the collector the new one. + """ + offenders = _unanalyzable_config_mutations() + assert not offenders, ( + "config_generator.py mutates the config dict in forms the ownership " + "guard cannot statically read for table keys:\n" + + "\n".join(f" line {lineno}: {source}" for lineno, source in offenders) + + '\nUse a supported form (config["X"] = ..., config.get/setdefault/' + "pop, config.update, config |= {...}) or extend " + "_config_table_keys_referenced_in_source to understand the new one." + )