Add router_networks: fixed-IP router interfaces into external networks (mvm-s3)

mvm-s3 is a separate VK Cloud project whose admin pre-created two private
networks/subnets with a known IP per router. Unify project-managed
(private_network_cidrs, IPAM-assigned) and externally-owned (router_networks,
fixed-IP) private interfaces into one local.router_interfaces so both share
the existing port/dynamic-network mechanism instead of duplicating it.

Switch from implicit *.auto.tfvars loading to explicit -var-file per
environment (now two share this terraform/ directory) plus a dedicated
Terraform workspace for mvm-s3, so PROD's state and credentials are never
touched by mvm-s3 applies.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GHfG9FgpMrGdrvC1QUewTw

terraform/tests/test_terraform_delivery.py

@@ -55,6 +55,23 @@ CHECKOV_BIN = (
     else shutil.which("checkov")
 )
 
+# The real mvm-s3 shape (two externally-owned, fixed-IP networks - matches
+# terraform/mvm-s3.tfvars), reused by several tests below.
+MVM_S3_ROUTER_NETWORKS = {
+    "primary": {
+        "network_id": "25532efe-2931-4666-a090-0da3a6f18224",
+        "subnet_id": "1fa5357c-1f11-4d20-89f4-b27a4fb56e6e",
+        "cidr": "172.16.252.8/29",
+        "ip_addresses": ["172.16.252.11", "172.16.252.12", "172.16.252.13", "172.16.252.14"],
+    },
+    "backup": {
+        "network_id": "2b4cc25f-55a1-4d36-a3a2-5b16414af31a",
+        "subnet_id": "5eacbc86-e15d-4c49-8704-547a719acc23",
+        "cidr": "172.16.252.0/29",
+        "ip_addresses": ["172.16.252.3", "172.16.252.4", "172.16.252.5", "172.16.252.6"],
+    },
+}
+
 
 def load_tf(relpath):
     with open(TERRAFORM_DIR / relpath) as f:
@@ -99,7 +116,9 @@ REQUIRED_FILES = [
     "variables.tf",
     "versions.tf",
     "terraform.tfvars",
-    "prod.auto.tfvars.example",
+    "prod.secrets.tfvars.example",
+    "mvm-s3.tfvars",
+    "mvm-s3.secrets.tfvars.example",
     "scripts/network-init.sh.tpl",
 ]
 
@@ -136,15 +155,18 @@ def test_terraform_fmt_clean():
 
 
 @pytest.mark.parametrize(
-    "router_count,private_network_cidrs",
+    "router_count,private_network_cidrs,router_networks",
     [
-        (None, None),  # whatever terraform.tfvars already commits to
-        (1, ["10.90.0.0/29"]),
-        (4, ["10.90.0.0/28", "10.90.0.16/28", "10.90.0.32/28"]),
+        (None, None, None),  # whatever terraform.tfvars already commits to
+        (1, ["10.90.0.0/29"], None),
+        (4, ["10.90.0.0/28", "10.90.0.16/28", "10.90.0.32/28"], None),
+        # mvm-s3-shaped: no project-managed private networks, two
+        # externally-owned fixed-IP ones instead.
+        (4, [], MVM_S3_ROUTER_NETWORKS),
     ],
 )
 def test_terraform_init_and_validate_against_real_provider_schema(
-    router_count, private_network_cidrs
+    router_count, private_network_cidrs, router_networks
 ):
     """Real `terraform init` + `terraform validate` against the actual vkcs
     provider, using the project-local filesystem-mirror copy of the
@@ -157,9 +179,10 @@ def test_terraform_init_and_validate_against_real_provider_schema(
     does not enforce the variable validation{} blocks - see
     test_variable_validations_are_enforced_by_plan for that).
 
-    Parametrized over router_count/private_network_cidrs (set via TF_VAR_*,
-    exactly how horizontal scaling is meant to be driven) to prove the
-    delivery actually resolves at other scales, not just the defaults.
+    Parametrized over router_count/private_network_cidrs/router_networks
+    (set via TF_VAR_*, exactly how horizontal scaling and the mvm-s3-style
+    fixed-IP deployment are meant to be driven) to prove the delivery
+    actually resolves at other scales/shapes, not just the defaults.
     """
     assert TERRAFORM_BIN, "no terraform binary found (checked venv/bin and PATH)"
     if not CLI_CONFIG_FILE.is_file():
@@ -174,6 +197,8 @@ def test_terraform_init_and_validate_against_real_provider_schema(
         env["TF_VAR_router_count"] = str(router_count)
     if private_network_cidrs is not None:
         env["TF_VAR_private_network_cidrs"] = json.dumps(private_network_cidrs)
+    if router_networks is not None:
+        env["TF_VAR_router_networks"] = json.dumps(router_networks)
 
     with tempfile.TemporaryDirectory() as tmp:
         tmp_path = Path(tmp)
@@ -252,7 +277,7 @@ def _plan_variables_only(var_overrides):
     "cidrs,should_pass",
     [
         (["10.90.0.0/29", "10.90.0.8/29"], True),
-        ([], False),  # must contain at least one CIDR
+        ([], True),  # optional now - a router_networks-only deployment (e.g. mvm-s3) sets none
         (["not-a-cidr"], False),  # must be a valid IPv4 CIDR
         (["10.90.0.0/29", "10.90.0.0/29"], False),  # must be unique
     ],
@@ -262,6 +287,27 @@ def test_private_network_cidrs_validation_is_enforced(cidrs, should_pass):
     assert ok == should_pass, f"unexpected result for private_network_cidrs={cidrs!r}:\n{output}"
 
 
+def _router_networks_case(**override):
+    net = json.loads(json.dumps(MVM_S3_ROUTER_NETWORKS))  # deep copy
+    net["primary"].update(override)
+    return net
+
+
+@pytest.mark.parametrize(
+    "router_networks,should_pass",
+    [
+        (MVM_S3_ROUTER_NETWORKS, True),
+        ({}, True),  # optional - a private_network_cidrs-only deployment (e.g. PROD) sets none
+        (_router_networks_case(cidr="not-a-cidr"), False),  # cidr must be a valid IPv4 CIDR
+        (_router_networks_case(ip_addresses=["not-an-ip"]), False),  # ip must be a valid IPv4 address
+        (_router_networks_case(ip_addresses=["10.0.0.1"]), False),  # ip must fall inside its own cidr
+    ],
+)
+def test_router_networks_validation_is_enforced(router_networks, should_pass):
+    ok, output = _plan_variables_only({"TF_VAR_router_networks": json.dumps(router_networks)})
+    assert ok == should_pass, f"unexpected result for router_networks={router_networks!r}:\n{output}"
+
+
 @pytest.mark.parametrize("count,should_pass", [(2, True), (0, False), (-1, False)])
 def test_router_count_validation_is_enforced(count, should_pass):
     ok, output = _plan_variables_only({"TF_VAR_router_count": str(count)})
@@ -335,44 +381,53 @@ def test_auth_variable_has_a_default(name):
 
 
 # ---------------------------------------------------------------------------
-# Real secrets must never land in the git-tracked terraform.tfvars template -
-# they belong in a gitignored *.auto.tfvars overlay instead (see
-# prod.auto.tfvars.example)
+# Real secrets must never land in a git-tracked tfvars file - each
+# environment's creds belong in its own gitignored *.secrets.tfvars, passed
+# explicitly via -var-file (see *.secrets.tfvars.example). Two environments
+# now share this terraform/ directory (PROD and mvm-s3), so unlike the old
+# single-environment *.auto.tfvars convention, nothing here may rely on
+# Terraform's automatic tfvars loading for secrets.
 # ---------------------------------------------------------------------------
 
 
-def test_gitignore_excludes_auto_tfvars_overlay():
+def test_gitignore_excludes_secrets_tfvars_overlay():
     gitignore_text = (REPO_ROOT / ".gitignore").read_text()
-    assert "*.auto.tfvars" in gitignore_text, (
-        "*.auto.tfvars must be gitignored - real credentials are meant to be "
-        "layered on top of terraform.tfvars via such a file, never committed"
+    assert "*.secrets.tfvars" in gitignore_text, (
+        "*.secrets.tfvars must be gitignored - real per-environment "
+        "credentials are meant to be passed via such a file with an "
+        "explicit -var-file, never committed"
     )
 
 
-def test_prod_auto_tfvars_example_is_not_gitignored():
-    """The *.example file documents the overlay pattern and must ship in the
-    repo (unlike the real *.auto.tfvars it documents)."""
+@pytest.mark.parametrize(
+    "relpath", ["prod.secrets.tfvars.example", "mvm-s3.secrets.tfvars.example"]
+)
+def test_secrets_tfvars_example_is_not_gitignored(relpath):
+    """Each *.example file documents the -var-file pattern for one
+    environment and must ship in the repo (unlike the real *.secrets.tfvars
+    it documents)."""
     result = subprocess.run(
-        ["git", "check-ignore", "terraform/prod.auto.tfvars.example"],
+        ["git", "check-ignore", f"terraform/{relpath}"],
         cwd=REPO_ROOT,
         capture_output=True,
         text=True,
     )
-    assert result.returncode != 0, "prod.auto.tfvars.example must NOT be gitignored"
+    assert result.returncode != 0, f"{relpath} must NOT be gitignored"
 
 
-def test_tfvars_template_has_no_auth_credentials():
-    """terraform.tfvars is a committed, anonymized template - auth_url/
-    user_domain_name/real credentials belong in a gitignored *.auto.tfvars
-    overlay, not here."""
-    tfvars_text = (TERRAFORM_DIR / "terraform.tfvars").read_text()
+@pytest.mark.parametrize("relpath", ["terraform.tfvars", "mvm-s3.tfvars"])
+def test_committed_tfvars_have_no_auth_credentials(relpath):
+    """terraform.tfvars and mvm-s3.tfvars are committed, non-secret shape
+    templates - auth_url/user_domain_name/real credentials belong in each
+    environment's gitignored *.secrets.tfvars overlay, not here."""
+    tfvars_text = (TERRAFORM_DIR / relpath).read_text()
     active_lines = [
         line for line in tfvars_text.splitlines() if not line.strip().startswith("#")
     ]
     for forbidden in ("auth_url", "user_domain_name"):
         assert not any(re.match(rf"^\s*{forbidden}\s*=", line) for line in active_lines), (
-            f"{forbidden} should not be set in the committed terraform.tfvars "
-            f"template - use a gitignored *.auto.tfvars overlay instead"
+            f"{forbidden} should not be set in the committed {relpath} "
+            f"template - use a gitignored *.secrets.tfvars overlay instead"
         )
 
 
@@ -392,12 +447,27 @@ def test_private_network_cidrs_variable():
     v = find_variable(load_tf("variables.tf"), "private_network_cidrs")
     assert v is not None, "variable private_network_cidrs is missing"
     assert v["type"] == ["${list(string)}"]
-    assert "default" not in v, (
-        "private_network_cidrs must NOT have a default - the admin is "
-        "required to pass it explicitly"
+    assert v.get("default") == [[]], (
+        "private_network_cidrs must default to [] - a deployment that only "
+        "uses var.router_networks (e.g. mvm-s3) needs no project-managed "
+        "private networks at all"
+    )
+    assert len(v.get("validation", [])) >= 2, (
+        "expected validations for: valid CIDR syntax, uniqueness"
+    )
+
+
+def test_router_networks_variable():
+    v = find_variable(load_tf("variables.tf"), "router_networks")
+    assert v is not None, "variable router_networks is missing"
+    assert v.get("default") == [{}], (
+        "router_networks must default to {} - a deployment that only uses "
+        "var.private_network_cidrs (e.g. PROD) needs no externally-owned "
+        "fixed-IP networks at all"
     )
     assert len(v.get("validation", [])) >= 3, (
-        "expected validations for: non-empty, valid CIDR syntax, uniqueness"
+        "expected validations for: valid CIDR syntax, valid IPv4 addresses, "
+        "each address falling inside its own cidr"
     )
 
 
@@ -419,8 +489,8 @@ def test_router_count_pinned_in_tfvars_is_a_valid_number():
 
 
 def test_private_network_cidrs_is_set_in_tfvars():
-    """Unlike router_count, private_network_cidrs has no default, so
-    terraform.tfvars must actively set it for a working example deployment."""
+    """private_network_cidrs defaults to [], but PROD's terraform.tfvars
+    still pins its real project-managed networks explicitly."""
     tfvars_text = (TERRAFORM_DIR / "terraform.tfvars").read_text()
     active_lines = [
         line for line in tfvars_text.splitlines() if not line.strip().startswith("#")
@@ -471,7 +541,7 @@ def test_no_legacy_hardcoded_router_resources():
     port_names = {name for name, _ in find_resources(main, "vkcs_networking_port")}
     assert port_names.isdisjoint({"lan_port1", "lan_port2"}), (
         "found legacy hardcoded lan_port1/lan_port2 instead of the "
-        "for_each-based router_priv_port"
+        "for_each-based router_iface_port"
     )
 
 
@@ -491,21 +561,40 @@ def test_deployment_is_router_only():
     assert secgroup_names == {"router_sg"}, f"expected only router_sg, found {secgroup_names}"
 
 
-def test_private_roles_local_driven_by_variable():
+def test_router_interfaces_local_merges_both_network_sources():
+    """locals.router_interfaces unifies the two ways a router can get a
+    private interface: project-managed (private_network_cidrs, network/
+    subnet created by this Terraform) and externally-owned, fixed-IP
+    (router_networks, referenced by UUID only - e.g. the mvm-s3
+    environment)."""
     main = load_tf("main.tf")
-    private_roles = find_local(main, "private_roles")
-    assert private_roles == [
-        '${[for idx in range(length(var.private_network_cidrs)) : "priv${idx + 1}"]}'
-    ], "locals.private_roles must be generated from length(var.private_network_cidrs)"
+    router_interfaces = find_local(main, "router_interfaces")
+    assert router_interfaces is not None, "locals.router_interfaces is missing"
+    expr = router_interfaces[0]
+    assert expr.startswith("${merge("), "router_interfaces must be built via merge(...)"
+    assert "for role , cidr in local.private_network_cidr" in expr, (
+        "router_interfaces must include the project-managed private_network_cidrs roles"
+    )
+    assert "for role , net in var.router_networks" in expr, (
+        "router_interfaces must include the externally-owned router_networks roles"
+    )
 
 
-def test_router_network_blocks_scale_with_private_roles():
+def test_router_interface_roles_local_is_keys_of_router_interfaces():
+    main = load_tf("main.tf")
+    assert find_local(main, "router_interface_roles") == [
+        "${keys(local.router_interfaces)}"
+    ], "locals.router_interface_roles must be keys(local.router_interfaces)"
+
+
+def test_router_network_blocks_scale_with_router_interface_roles():
     main = load_tf("main.tf")
     router = dict(find_resources(main, "vkcs_compute_instance"))["router"]
     dynamic_network = router["dynamic"][0]["network"]
-    assert dynamic_network["for_each"] == ["${local.private_roles}"], (
-        "the dynamic private network blocks must iterate local.private_roles "
-        "so the NIC count scales with the number of private_network_cidrs entries"
+    assert dynamic_network["for_each"] == ["${local.router_interface_roles}"], (
+        "the dynamic private network blocks must iterate "
+        "local.router_interface_roles so the NIC count scales with both "
+        "private_network_cidrs and router_networks entries"
     )
 
 
@@ -622,23 +711,41 @@ def test_router_priv_subnet_has_dhcp_disabled():
     )
 
 
-def test_router_priv_port_leaves_ip_address_unset():
-    """Regression guard: a hand-computed fixed_ip.ip_address collided with
-    VKCS's own auto-created service ports on the network (observed: a
-    "network:dns" port silently consuming an address) during a real PROD
-    deployment. Leaving ip_address unset lets Neutron's IPAM auto-assign
-    one, which is guaranteed collision-free; network-init.sh.tpl matches
-    interfaces by which declared CIDR their live IP falls into, not by an
-    exact expected IP, so this doesn't need to be known in advance."""
+def test_router_iface_port_ip_address_is_conditional_on_fixed_ips():
+    """Regression guard + new behaviour, in one place: a hand-computed
+    fixed_ip.ip_address collided with VKCS's own auto-created service ports
+    on the network (observed: a "network:dns" port silently consuming an
+    address) during a real PROD deployment - so for project-managed roles
+    (private_network_cidrs) ip_address must stay null and let Neutron's IPAM
+    auto-assign. But externally-owned roles (router_networks, e.g. mvm-s3)
+    have their IP pre-agreed by another project's admin, so those must set
+    it explicitly. Both cases are driven by the same conditional expression."""
     main = load_tf("main.tf")
     ports = dict(find_resources(main, "vkcs_networking_port"))
-    assert "router_priv_port" in ports
-    fixed_ip = ports["router_priv_port"]["fixed_ip"][0]
-    assert "ip_address" not in fixed_ip, (
-        "router_priv_port.fixed_ip must not set ip_address - let Neutron's "
-        "IPAM auto-assign to avoid colliding with platform-reserved ports"
-    )
+    assert "router_iface_port" in ports
+    fixed_ip = ports["router_iface_port"]["fixed_ip"][0]
     assert "subnet_id" in fixed_ip
+    assert fixed_ip["ip_address"] == [
+        "${local.router_interfaces[each.value[1]].fixed_ips == None ? None : "
+        "local.router_interfaces[each.value[1]].fixed_ips[each.value[0]]}"
+    ], (
+        "router_iface_port.fixed_ip.ip_address must stay null when the "
+        "role's fixed_ips is null (project-managed roles, IPAM auto-assign) "
+        "and pick the per-router static IP otherwise (router_networks roles)"
+    )
+
+
+def test_router_iface_port_has_length_precondition_for_fixed_ips():
+    """router_networks.ip_addresses must cover every router - a precondition
+    (not just a variable validation{}) gives a clear per-role error at plan
+    time instead of an out-of-range index crash."""
+    main = load_tf("main.tf")
+    ports = dict(find_resources(main, "vkcs_networking_port"))
+    port = ports["router_iface_port"]
+    assert "lifecycle" in port, "router_iface_port must declare a lifecycle.precondition"
+    precondition = port["lifecycle"][0]["precondition"][0]
+    assert "fixed_ips" in precondition["condition"][0]
+    assert "length(" in precondition["condition"][0]
 
 
 # ---------------------------------------------------------------------------
@@ -673,7 +780,7 @@ def test_tfvars_private_network_cidrs_do_not_overlap_and_have_room_for_routers()
     router_count = _configured_router_count()
     for net in networks:
         # ip_address is left unset on each port (Neutron IPAM auto-assigns -
-        # see router_priv_port in main.tf), so there's no fixed per-router
+        # see router_iface_port in main.tf), so there's no fixed per-router
         # offset to reserve room for - but VKCS auto-creates its own service
         # ports on the network (observed: one "network:dns" port consuming
         # an address), so there must be room for router_count routers plus
@@ -684,6 +791,69 @@ def test_tfvars_private_network_cidrs_do_not_overlap_and_have_room_for_routers()
         )
 
 
+# ---------------------------------------------------------------------------
+# mvm-s3.tfvars: sanity-check the externally-owned network/IP values shipped
+# for this environment (no project-managed private networks at all here)
+# ---------------------------------------------------------------------------
+
+
+def _mvm_s3_router_count():
+    tfvars_text = (TERRAFORM_DIR / "mvm-s3.tfvars").read_text()
+    for line in tfvars_text.splitlines():
+        if line.strip().startswith("#"):
+            continue
+        m = re.match(r"^\s*router_count\s*=\s*(\d+)", line)
+        if m:
+            return int(m.group(1))
+    return find_variable(load_tf("variables.tf"), "router_count")["default"][0]
+
+
+def test_mvm_s3_tfvars_disables_project_managed_private_networks():
+    tfvars = load_tf("mvm-s3.tfvars")
+    assert tfvars["private_network_cidrs"][0] == [], (
+        "mvm-s3 must not create any project-managed private network - both "
+        "of its private interfaces come from router_networks instead"
+    )
+
+
+def test_mvm_s3_tfvars_router_networks_matches_the_diagram():
+    """Regression guard: mvm-s3.tfvars must keep shipping exactly the
+    network/subnet UUIDs and per-router IPs from the admin's diagram."""
+    tfvars = load_tf("mvm-s3.tfvars")
+    assert tfvars["router_networks"][0] == MVM_S3_ROUTER_NETWORKS
+
+
+def test_mvm_s3_tfvars_router_networks_ip_addresses_cover_router_count():
+    tfvars = load_tf("mvm-s3.tfvars")
+    router_networks = tfvars["router_networks"][0]
+    router_count = _mvm_s3_router_count()
+    assert router_networks, "mvm-s3.tfvars must set router_networks"
+    for role, net in router_networks.items():
+        assert len(net["ip_addresses"]) >= router_count, (
+            f"router_networks[{role!r}].ip_addresses has fewer entries "
+            f"than router_count={router_count}"
+        )
+
+
+def test_mvm_s3_tfvars_router_networks_ips_are_valid_and_dont_overlap():
+    tfvars = load_tf("mvm-s3.tfvars")
+    router_networks = tfvars["router_networks"][0]
+    networks = []
+    for role, net in router_networks.items():
+        cidr = ipaddress.ip_network(net["cidr"])
+        networks.append(cidr)
+        for ip in net["ip_addresses"]:
+            assert ipaddress.ip_address(ip) in cidr, (
+                f"router_networks[{role!r}] ip {ip} does not fall inside {cidr}"
+            )
+        assert len(net["ip_addresses"]) == len(set(net["ip_addresses"])), (
+            f"router_networks[{role!r}].ip_addresses has duplicate entries"
+        )
+    for i, a in enumerate(networks):
+        for b in networks[i + 1 :]:
+            assert not a.overlaps(b), f"{a} overlaps {b} in mvm-s3.tfvars router_networks"
+
+
 # ---------------------------------------------------------------------------
 # network-init.sh.tpl: only the intended Terraform interpolations remain
 # un-escaped, and the rendered result is syntactically valid bash