sonic: Enforce config table ownership statically

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)

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.<m>(...)`` whose method is outside the
+    recognized set, and a reassignment ``config = <expr>`` 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."
+        )