Add validation for OpenTofu compute and login variables (#674)

* optional home vol using vol size

* use home_volume_provisioning

* automatically modify nfs configuration depending on home volume

* remove dead tf code

* wip: add nodegroup typedefs

* failing - can't define standard and extra with different parameters!

* just check compute keys

* move validation into nodegroup module

* add validation for login too

* use node module defaults for login vars

* show valid keys in validation

* catchup validation with PR693

* fixup broken merge from main

* Fix docs wording

Co-authored-by: Matt Anson <matta@stackhpc.com>

---------

Co-authored-by: Matt Anson <matta@stackhpc.com>

Author: sjpb

environments/skeleton/{{cookiecutter.environment}}/tofu/compute.tf

@@ -24,16 +24,38 @@ module "compute" {
 
   # optionally set for group:
   networks = concat(var.cluster_networks, lookup(each.value, "extra_networks", []))
-  extra_volumes = lookup(each.value, "extra_volumes", {})
-  compute_init_enable = lookup(each.value, "compute_init_enable", [])
-  ignore_image_changes = lookup(each.value, "ignore_image_changes", false)
-  match_ironic_node = lookup(each.value, "match_ironic_node", false)
-  availability_zone = lookup(each.value, "availability_zone", "nova")
+  # here null means "use module var default"
+  extra_volumes = lookup(each.value, "extra_volumes", null)
+  compute_init_enable = lookup(each.value, "compute_init_enable", null)
+  ignore_image_changes = lookup(each.value, "ignore_image_changes", null)
+  match_ironic_node = lookup(each.value, "match_ironic_node", null)
+  availability_zone = lookup(each.value, "availability_zone", null)
 
   # computed
   # not using openstack_compute_instance_v2.control.access_ip_v4 to avoid
   # updates to node metadata on deletion/recreation of the control node:
   control_address = openstack_networking_port_v2.control[var.cluster_networks[0].network].all_fixed_ips[0]
   security_group_ids = [for o in data.openstack_networking_secgroup_v2.nonlogin: o.id]
   baremetal_nodes = data.external.baremetal_nodes.result
+  
+  # input dict validation:
+  group_name = each.key
+  group_keys = keys(each.value)
+  allowed_keys = [
+    "nodes",
+    "flavor",
+    "image_id",
+    "extra_networks",
+    "vnic_types",
+    "compute_init_enable",
+    "ignore_image_changes",
+    "volume_backed_instances",
+    "root_volume_size",
+    "root_volume_type",
+    "extra_volumes",
+    "match_ironic_node",
+    "availability_zone",
+    "gateway_ip",
+    "nodename_template",
+  ]
 }

environments/skeleton/{{cookiecutter.environment}}/tofu/login.tf

@@ -24,11 +24,12 @@ module "login" {
 
   # optionally set for group
   networks = concat(var.cluster_networks, lookup(each.value, "extra_networks", []))
-  extra_volumes = lookup(each.value, "extra_volumes", {})
-  fip_addresses = lookup(each.value, "fip_addresses", [])
-  fip_network = lookup(each.value, "fip_network", "")
-  match_ironic_node = lookup(each.value, "match_ironic_node", false)
-  availability_zone = lookup(each.value, "availability_zone", "nova")
+  # here null means "use module var default"
+  extra_volumes = lookup(each.value, "extra_volumes", null)
+  fip_addresses = lookup(each.value, "fip_addresses", null)
+  fip_network = lookup(each.value, "fip_network", null)
+  match_ironic_node = lookup(each.value, "match_ironic_node", null)
+  availability_zone = lookup(each.value, "availability_zone", null)
 
   # can't be set for login
   compute_init_enable = []
@@ -40,4 +41,25 @@ module "login" {
   control_address = openstack_networking_port_v2.control[var.cluster_networks[0].network].all_fixed_ips[0]
   security_group_ids = [for o in data.openstack_networking_secgroup_v2.login: o.id]
   baremetal_nodes = data.external.baremetal_nodes.result
+
+  # input dict validation:
+  group_name = each.key
+  group_keys = keys(each.value)
+  allowed_keys = [
+    "nodes",
+    "flavor",
+    "image_id",
+    "extra_networks",
+    "vnic_types",
+    "volume_backed_instances",
+    "root_volume_size",
+    "root_volume_type",
+    "extra_volumes",
+    "fip_addresses",
+    "fip_network",
+    "match_ironic_node",
+    "availability_zone",
+    "gateway_ip",
+    "nodename_template",
+  ]
 }

environments/skeleton/{{cookiecutter.environment}}/tofu/node_group/variables.tf

@@ -62,8 +62,13 @@ variable "extra_volumes" {
             size: Size of volume in GB
         **NB**: The order in /dev is not guaranteed to match the mapping
         EOF
-    type = any
+    type = map(
+        object({
+            size = number
+        })
+    )
     default = {}
+    nullable = false
 }
 
 variable "security_group_ids" {
@@ -79,17 +84,18 @@ variable "compute_init_enable" {
     type = list(string)
     description = "Groups to activate for ansible-init compute rebuilds"
     default = []
+    nullable = false
 }
 
 variable "ignore_image_changes" {
     type = bool
     description = "Whether to ignore changes to the image_id parameter"
     default = false
+    nullable = false
 }
 
 variable "networks" {
     type = list(map(string))
-    default = []
 }
 
 variable "fip_addresses" {
@@ -100,6 +106,7 @@ variable "fip_addresses" {
         allocated to the project.
     EOT
     default = []
+    nullable = false
 }
 
 variable "fip_network" {
@@ -109,18 +116,21 @@ variable "fip_network" {
         networks are defined.
     EOT
     default = ""
+    nullable = false
 }
 
 variable "match_ironic_node" {
     type = bool
     description = "Whether to launch instances on the Ironic node of the same name as each cluster node"
     default = false
+    nullable = false
 }
 
 variable "availability_zone" {
     type = string
     description = "Name of availability zone - ignored unless match_ironic_node is true"
     default = "nova"
+    nullable = false
 }
 
 variable "baremetal_nodes" {
@@ -137,3 +147,25 @@ variable "nodename_template" {
     type = string
     default = ""
 }
+
+variable "group_name" {
+    type = string
+}
+
+variable "group_keys" {
+    type = list
+    validation {
+      condition = length(setsubtract(var.group_keys, var.allowed_keys)) == 0
+      error_message = <<-EOT
+        Node group '${var.group_name}' contains invalid key(s) ${
+        join(", ", setsubtract(var.group_keys, var.allowed_keys))}.
+        
+        Valid keys are ${join(", ", var.allowed_keys)}.
+    EOT
+    }
+}
+
+variable "allowed_keys" {
+    type = list
+    # don't provide a default here as allowed keys may depend on module use
+}

environments/skeleton/{{cookiecutter.environment}}/tofu/variables.tf

@@ -71,7 +71,7 @@ variable "cluster_image_id" {
 }
 
 variable "compute" {
-    type = any
+
     description = <<-EOF
         Mapping defining homogenous groups of compute nodes. Groups are used
         in Slurm partition definitions.
@@ -100,6 +100,8 @@ variable "compute" {
             gateway_ip: Address to add default route via
             nodename_template: Overrides variable cluster_nodename_template
     EOF
+    default = {}
+    type = any # can't do any better; TF type constraints can't cope with heterogeneous inner mappings
 }
 
 variable "environment_root" {