Fix real-deployment blockers and scope down to router-only VMs

Confirmed working against a real VK Cloud PROD deployment (3 routers,
19 resources, apply succeeded end to end). Fixes found along the way:

- provider "vkcs" was never configured (versions.tf) - username/password/
  project_id/region were declared but wired to nothing; added auth_url and
  user_domain_name to complete it.
- Nova keypairs are per-user, not per-project - added an optional
  vkcs_compute_keypair resource (var.ssh_public_key) so Terraform can
  register a keypair under the deploying service account itself.
- router_priv_port used a hand-computed fixed_ip offset that collided with
  VKCS's own auto-created service ports on each network (observed: a
  "network:dns" port) - now left unset so Neutron's IPAM auto-assigns,
  which is collision-free by construction.
- vkcs_compute_instance set image_id at the top level while also booting
  from a volume via block_device - the provider docs say not to do this;
  Nova echoes back a sentinel string for image_id on a volume-booted
  server, which Terraform read as drift on a ForceNew attribute and
  wanted to destroy+recreate every already-created instance on every
  subsequent plan.
- private_network_cidrs bumped from /29 to /28 - too tight once the
  platform's own reserved ports are accounted for.

Also removed the priv_srv_01/02/03 demo instances and the LAN network/
security group only they used - this deployment provisions router VMs
only, confirmed with the user.

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

terraform/tests/test_terraform_delivery.py

@@ -97,7 +97,9 @@ def find_data_sources(doc, dtype):
 REQUIRED_FILES = [
     "main.tf",
     "variables.tf",
+    "versions.tf",
     "terraform.tfvars",
+    "prod.auto.tfvars.example",
     "scripts/network-init.sh.tpl",
 ]
 
@@ -286,7 +288,7 @@ def test_default_security_group_id_override_accepted(override, should_pass):
 # ---------------------------------------------------------------------------
 
 
-@pytest.mark.parametrize("relpath", ["main.tf", "variables.tf", "images.tf"])
+@pytest.mark.parametrize("relpath", ["main.tf", "variables.tf", "versions.tf", "images.tf"])
 def test_hcl_file_parses(relpath):
     # images.tf is intentionally fully commented out (disabled helper data
     # source) - it must still parse cleanly, just possibly to an empty doc.
@@ -294,6 +296,86 @@ def test_hcl_file_parses(relpath):
     assert isinstance(doc, dict)
 
 
+# ---------------------------------------------------------------------------
+# versions.tf: the provider is actually configured (auth variables used to
+# be declared in variables.tf but never wired to anything - regression
+# guard against that gap reappearing)
+# ---------------------------------------------------------------------------
+
+
+def test_provider_vkcs_wires_all_auth_variables():
+    versions = load_tf("versions.tf")
+    provider_blocks = versions.get("provider", [])
+    vkcs_provider = None
+    for block in provider_blocks:
+        if "vkcs" in block:
+            vkcs_provider = block["vkcs"]
+    assert vkcs_provider is not None, "expected a provider \"vkcs\" block in versions.tf"
+
+    expected = {
+        "auth_url": "${var.auth_url}",
+        "username": "${var.username}",
+        "password": "${var.password}",
+        "project_id": "${var.project_id}",
+        "region": "${var.region}",
+        "user_domain_name": "${var.user_domain_name}",
+    }
+    for attr, expr in expected.items():
+        assert vkcs_provider.get(attr) == [expr], (
+            f"provider \"vkcs\" must set {attr} = var.{attr} - "
+            f"auth variables must not be left orphaned/unwired"
+        )
+
+
+@pytest.mark.parametrize("name", ["auth_url", "user_domain_name", "region"])
+def test_auth_variable_has_a_default(name):
+    v = find_variable(load_tf("variables.tf"), name)
+    assert v is not None, f"variable {name} is missing"
+    assert "default" in v, f"{name} should have a sane default so it doesn't have to be set explicitly for the common case"
+
+
+# ---------------------------------------------------------------------------
+# 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)
+# ---------------------------------------------------------------------------
+
+
+def test_gitignore_excludes_auto_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"
+    )
+
+
+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)."""
+    result = subprocess.run(
+        ["git", "check-ignore", "terraform/prod.auto.tfvars.example"],
+        cwd=REPO_ROOT,
+        capture_output=True,
+        text=True,
+    )
+    assert result.returncode != 0, "prod.auto.tfvars.example 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()
+    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"
+        )
+
+
 # ---------------------------------------------------------------------------
 # variables.tf: the scaling knobs exist as expected
 # ---------------------------------------------------------------------------
@@ -319,17 +401,21 @@ def test_private_network_cidrs_variable():
     )
 
 
-def test_router_count_not_pinned_in_tfvars():
-    """TF_VAR_router_count only takes effect if terraform.tfvars doesn't set
-    an active (non-comment) value for it."""
+def test_router_count_pinned_in_tfvars_is_a_valid_number():
+    """terraform.tfvars now pins a real router_count for this project's
+    actual PROD deployment (a deliberate choice, confirmed with the user) -
+    a tfvars-file value always beats a TF_VAR_ environment variable in
+    Terraform's precedence order, so TF_VAR_router_count no longer has any
+    effect while this stays set. Just sanity-check it's a positive integer,
+    not that it's absent."""
     tfvars_text = (TERRAFORM_DIR / "terraform.tfvars").read_text()
     active_lines = [
         line for line in tfvars_text.splitlines() if not line.strip().startswith("#")
     ]
-    assert not any(re.match(r"^\s*router_count\s*=", line) for line in active_lines), (
-        "router_count must not be set in terraform.tfvars, or the "
-        "TF_VAR_router_count environment variable would be shadowed"
-    )
+    matches = [line for line in active_lines if re.match(r"^\s*router_count\s*=", line)]
+    assert matches, "expected router_count to be pinned in terraform.tfvars for this deployment"
+    value = int(matches[0].split("=", 1)[1].strip())
+    assert value >= 1
 
 
 def test_private_network_cidrs_is_set_in_tfvars():
@@ -357,6 +443,24 @@ def test_router_resource_uses_count_variable():
     assert instances["router"]["count"] == ["${var.router_count}"]
 
 
+def test_compute_instances_do_not_set_top_level_image_id():
+    """Regression guard: the provider docs say 'Do not specify [image_id]
+    if booting from a volume' - doing so anyway caused every already-created
+    instance to be flagged for destroy+recreate on the next plan, because
+    Nova reports back a sentinel string ("Attempt to boot from volume - no
+    image supplied") instead of echoing the image UUID for a volume-booted
+    server, which Terraform then sees as configuration drift on a
+    ForceNew attribute. The image only belongs inside block_device."""
+    main = load_tf("main.tf")
+    for name, attrs in find_resources(main, "vkcs_compute_instance"):
+        assert "image_id" not in attrs, (
+            f"vkcs_compute_instance.{name} must not set top-level image_id "
+            f"when booting from a volume via block_device"
+        )
+        assert attrs["block_device"][0]["source_type"] == ["image"]
+        assert "uuid" in attrs["block_device"][0]
+
+
 def test_no_legacy_hardcoded_router_resources():
     main = load_tf("main.tf")
     instance_names = {name for name, _ in find_resources(main, "vkcs_compute_instance")}
@@ -371,6 +475,22 @@ def test_no_legacy_hardcoded_router_resources():
     )
 
 
+def test_deployment_is_router_only():
+    """This deployment provisions only the router VMs - the demo's
+    priv_srv_01/02/03 instances and the shared LAN network/security group
+    that only they used were removed as out of scope (confirmed with the
+    user)."""
+    main = load_tf("main.tf")
+    instance_names = {name for name, _ in find_resources(main, "vkcs_compute_instance")}
+    assert instance_names == {"router"}, f"expected only the router instance, found {instance_names}"
+
+    network_names = {name for name, _ in find_resources(main, "vkcs_networking_network")}
+    assert "lan_net" not in network_names
+
+    secgroup_names = {name for name, _ in find_resources(main, "vkcs_networking_secgroup")}
+    assert secgroup_names == {"router_sg"}, f"expected only router_sg, found {secgroup_names}"
+
+
 def test_private_roles_local_driven_by_variable():
     main = load_tf("main.tf")
     private_roles = find_local(main, "private_roles")
@@ -437,12 +557,109 @@ def test_all_instances_use_default_security_group_local():
         )
 
 
+# ---------------------------------------------------------------------------
+# main.tf: SSH keypair - Nova keypairs are per-user, not per-project, so a
+# keypair uploaded under a different account is invisible to the deploying
+# one. var.ssh_public_key lets Terraform register it itself.
+# ---------------------------------------------------------------------------
+
+
+def test_ssh_public_key_variable_defaults_to_null():
+    v = find_variable(load_tf("variables.tf"), "ssh_public_key")
+    assert v is not None, "variable ssh_public_key is missing"
+    assert v["default"] == [None], "ssh_public_key should default to null (register nothing unless supplied)"
+
+
+def test_keypair_resource_conditionally_registers_public_key():
+    main = load_tf("main.tf")
+    keypairs = dict(find_resources(main, "vkcs_compute_keypair"))
+    assert "router" in keypairs, "expected a vkcs_compute_keypair.router resource"
+    kp = keypairs["router"]
+    assert kp["count"] == ["${var.ssh_public_key != None ? 1 : 0}"], (
+        "the keypair should only be created when ssh_public_key is actually supplied"
+    )
+    assert kp["name"] == ["${var.ssh_key_name}"]
+    assert kp["public_key"] == ["${var.ssh_public_key}"]
+
+
+def test_ssh_key_name_local_prefers_registered_keypair():
+    main = load_tf("main.tf")
+    value = find_local(main, "ssh_key_name")
+    assert value == [
+        "${var.ssh_public_key != None ? vkcs_compute_keypair.router[0].name : var.ssh_key_name}"
+    ], (
+        "locals.ssh_key_name must use the keypair Terraform registers itself "
+        "when ssh_public_key is supplied, falling back to var.ssh_key_name "
+        "(an already-existing keypair) otherwise"
+    )
+
+
+def test_all_instances_use_ssh_key_name_local():
+    main = load_tf("main.tf")
+    for name, attrs in find_resources(main, "vkcs_compute_instance"):
+        assert attrs["key_pair"] == ["${local.ssh_key_name}"], (
+            f"vkcs_compute_instance.{name} must reference local.ssh_key_name, "
+            f"not var.ssh_key_name directly, so it depends on the keypair "
+            f"resource when one is registered"
+        )
+
+
+# ---------------------------------------------------------------------------
+# main.tf: router private subnets don't need Neutron's DHCP service - with
+# config_drive = true, cloud-init learns each interface's address from
+# config-drive metadata rather than an actual DHCP exchange, and
+# network-init.sh.tpl re-pins it deterministically to ethN afterwards
+# ---------------------------------------------------------------------------
+
+
+def test_router_priv_subnet_has_dhcp_disabled():
+    main = load_tf("main.tf")
+    subnets = dict(find_resources(main, "vkcs_networking_subnet"))
+    assert "router_priv_subnet" in subnets
+    assert subnets["router_priv_subnet"]["enable_dhcp"] == [False], (
+        "router_priv_subnet must have enable_dhcp = false - no in-guest "
+        "DHCP client is ever used on these interfaces"
+    )
+
+
+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."""
+    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 "subnet_id" in fixed_ip
+
+
 # ---------------------------------------------------------------------------
 # terraform.tfvars example CIDRs: sanity-check the values actually shipped
 # (no auto-carving anymore - these come straight from the admin/example)
 # ---------------------------------------------------------------------------
 
 
+def _configured_router_count():
+    """The router_count actually in effect: whatever terraform.tfvars pins,
+    else the variable's default (a tfvars value always beats the default)."""
+    tfvars_text = (TERRAFORM_DIR / "terraform.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_tfvars_private_network_cidrs_do_not_overlap_and_have_room_for_routers():
     tfvars = load_tf("terraform.tfvars")
     raw_cidrs = tfvars["private_network_cidrs"][0]
@@ -453,13 +670,17 @@ def test_tfvars_private_network_cidrs_do_not_overlap_and_have_room_for_routers()
         for b in networks[i + 1 :]:
             assert not a.overlaps(b), f"{a} overlaps {b} in terraform.tfvars"
 
-    router_count_default = find_variable(load_tf("variables.tf"), "router_count")["default"][0]
+    router_count = _configured_router_count()
     for net in networks:
-        # offset scheme: cidrhost(cidr, router_index + 2) for router_index in
-        # 0..router_count-1, so we need at least router_count + 2 addresses.
-        assert net.num_addresses >= router_count_default + 2, (
-            f"{net} has too few addresses for {router_count_default} routers "
-            f"(offset scheme needs router_count + 2)"
+        # 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
+        # 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
+        # at least one such reservation, on top of network/gateway/broadcast.
+        assert net.num_addresses >= router_count + 3, (
+            f"{net} has too few addresses for {router_count} routers plus "
+            f"platform-reserved ports (e.g. VKCS's network:dns service port)"
         )