From 0e28d6fa2874487fa78f236ff163d1bbbe9dd311 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Wed, 10 Jun 2026 13:52:24 +0200 Subject: [PATCH] sonic: Enforce config table ownership statically The SONiC config generator owns selected config_db.json tables, but the ownership registry was a hand-maintained allowlist with no enforcement. Nothing forced a newly handled table into it, so a table the generator references but does not classify would fall into the unpoliced pass-through tier and reintroduce the stale-config bug the ownership model exists to prevent. The existing taxonomy tests only check relationships between the registered constants, so they cannot catch an omission. The model also lived only in a docstring far from the helpers that must honour it. Add a static guard test that parses config_generator.py and collects every top-level config_db table referenced on the config dict: config["X"] subscripts, the defensive config.get/setdefault/pop("X") accessors (a .get() read is a real dependency on a table), and every literal-keyed merge form -- config.update({"X": ...}), config.update(X=...), config.update(**{"X": ...}), config |= {"X": ...}, config = config | {"X": ...}, and config = {**config, "X": ...}. The test fails when any referenced table is not classified as owned, inherited, or image-consumed, naming the offending table and the list to add it to. It is static rather than runtime because some tables emit only when NetBox carries their data -- as forthcoming features will (e.g. the BFD and EVPN support proposed in open PRs, not yet in tree) -- which a generate-and-inspect test would miss unless its fixture happened to trigger every table. So that coverage cannot erode silently, a second guard rejects any config mutation the collector cannot read: a method call outside the recognized set, or a reassignment whose right-hand side is not a dict display, a dict-merge, or a whitelisted base load (copy.deepcopy and friends). A merge through an arbitrary call -- config = merge(config, {...}), config = dict(config, X={}) -- is rejected rather than allowed to hide a table. The one accepted blind spot is a dynamic key in an otherwise recognized form (config[var], config.setdefault(var, {}), config.update(name)): the table name is not a literal at the call site, the generator relies on this for the up-front scaffold and drop loops, and resolving it would need data-flow analysis. Introduce a fourth ownership category, IMAGE_CONSUMED_TABLE_KEYS, for tables read from the image base config but never modified by the generator (so they pass through to the output unchanged, like a pass-through table, the difference being only that the generator depends on one as an input). The defining property is behavioural (read-only), not the access syntax: inherited tables are read via config.get() too, so .get() alone does not distinguish them. It is empty for now; the first consumer is the gNMI listen port read from TELEMETRY. Disjointness invariants pin that an image-consumed table is never also owned (which would drop it up front and leave the consuming helper reading an empty table) nor inherited (read-only contradicts read-and-update), completing pairwise disjointness across the three classified categories. Rewrite the generate_sonic_config ownership-model docstring around the same four categories (owned / inherited / image-consumed / pass-through) and point it at the guard test, so the documented model and the enforced model stay in sync. The inherited category, in both the docstring and the INHERITED_TABLE_KEYS comment, is described as preserving image content while updating selected fields in place (e.g. the DEVICE_METADATA localhost attributes and VERSIONS.DATABASE.VERSION), correcting earlier wording that said inherited content is emitted unchanged or not emitted by the generator at all. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Roger Luethi --- .../tasks/conductor/sonic/config_generator.py | 69 +++-- .../sonic/test_config_generator_ownership.py | 247 ++++++++++++++++++ 2 files changed, 295 insertions(+), 21 deletions(-) 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." + )