fix(validation): accept an empty string as the unset value of an int

plugins/action/validate_inputs.py

@@ -2,26 +2,29 @@
 
 Each spec file's ``argument_specs.inputs.options`` declares top-level variables. Every one the host
 defines is checked without Ansible's lenient coercion: a string is never a list, bool or dict. A dict
-option whose only sub-option is named ``<name>`` is a collection keyed by name; each entry is checked
+option whose only sub-option is a placeholder such as ``<name>`` is a collection keyed by name; each
-against that sub-option, and a null entry means the entry is not managed. Messages name the key path
+entry is checked against that sub-option, and a null entry means the entry is not managed. A ``raw``
+option that declares ``options`` is a role toggle: ``true``, ``false``, or the dict those options
+describe. Messages name the key path
 and the types involved but never a value, so secrets cannot leak into job output.
 """
 
+import glob
 from collections.abc import Mapping
 
 from ansible.errors import AnsibleError
 from ansible.parsing.vault import EncryptedString
 from ansible.plugins.action import ActionBase
 
-KEYED = "<name>"
 SPEC_KEYS = {"type", "required", "default", "choices", "elements", "options", "description", "no_log"}
 UNRESOLVED = object()
 
 # Numbers pass where a string is declared and digit strings where an int is: the roles read those
-# through `| string` and `| int` unchanged. A string posing as a list, bool or dict never passes.
+# through `| string` and `| int` unchanged. An empty string is the contract's unset int, as in
+# `system.vlan: ""`. A string posing as a list, bool or dict never passes.
 TYPES = {
     "str": lambda v: isinstance(v, (str, int, float, EncryptedString)) and not isinstance(v, bool),
-    "int": lambda v: (isinstance(v, int) and not isinstance(v, bool)) or (isinstance(v, str) and v.isdigit()),
+    "int": lambda v: (isinstance(v, int) and not isinstance(v, bool)) or (isinstance(v, str) and (v == "" or v.isdigit())),
     "float": lambda v: isinstance(v, (int, float)) and not isinstance(v, bool),
     "bool": lambda v: isinstance(v, bool),
     "list": lambda v: isinstance(v, list),
@@ -72,15 +75,21 @@ class _Validator:
         if value is UNRESOLVED:
             return
         kind = spec.get("type", "str")
+        if kind == "raw" and "options" in spec and not isinstance(value, (bool, Mapping)):
+            self.errors.append(f"{path}: expected bool or dict, got {_kind(value)}")
+            return
         if not TYPES[kind](value):
-            self.errors.append(f"{path}: expected {kind}, got {_kind(value)}")
+            keyed = len(spec.get("options", {})) == 1 and next(iter(spec["options"])).startswith("<")
+            self.errors.append(f"{path}: expected {'dict keyed by name' if keyed else kind}, got {_kind(value)}")
             return
         if "choices" in spec and value not in spec["choices"]:
             shown = "" if spec.get("no_log") else f", got {value!r}"
             self.errors.append(f"{path}: must be one of {', '.join(map(str, spec['choices']))}{shown}")
-        if kind == "dict" and "options" in spec:
+        # Keyed on the value, not the declared type, so a `raw` option ("false, or a dict of...") is
+        # still checked whenever it holds a dict or a list.
+        if "options" in spec and isinstance(value, Mapping):
             self.check_options(spec["options"], value, path)
-        elif kind == "list" and "elements" in spec:
+        elif "elements" in spec and isinstance(value, list):
             element = {"type": spec["elements"]}
             if "options" in spec:
                 element["options"] = spec["options"]
@@ -88,14 +97,16 @@ class _Validator:
                 self.check(element, item, f"{path}[{index}]")
 
     def check_options(self, options, value, path):
-        if set(options) == {KEYED}:
+        if len(options) == 1 and next(iter(options)).startswith("<"):
+            entry_spec = next(iter(options.values()))
             for name, entry in value.items():
                 if entry is not None:
-                    self.check(options[KEYED], entry, f"{path}.{name}")
+                    self.check(entry_spec, entry, f"{path}.{name}")
             return
         for key in value:
             if key not in options:
-                self.errors.append(f"{path}.{key}: unsupported key, {path} accepts {', '.join(sorted(options))}")
+                accepted = ", ".join(sorted(options)) or "no keys"
+                self.errors.append(f"{path}.{key}: unsupported key, {path} accepts {accepted}")
         for key, spec in options.items():
             if key in value:
                 self.check(spec, value[key], f"{path}.{key}")
@@ -110,10 +121,25 @@ class ActionModule(ActionBase):
         result = super().run(tmp, task_vars)
         task_vars = task_vars or {}
         validator = _Validator(self._templar)
-        for path in self._task.args["specs"]:
+        declared = {}
+        paths = []
+        # A pattern (`.../*/roles/*/meta/argument_specs.yml`) picks up every role's spec without a list
+        # to keep in sync; one that matches nothing is a broken path, not an empty set.
+        specs = self._task.args["specs"]
+        if not isinstance(specs, list):
+            raise AnsibleError(f"specs must be a list of spec file paths, got {type(specs).__name__}")
+        for entry in specs:
+            matches = sorted(glob.glob(entry)) if glob.has_magic(entry) else [entry]
+            if not matches:
+                raise AnsibleError(f"no spec file matches {entry}")
+            paths.extend(matches)
+        for path in paths:
             options = self._loader.load_from_file(path)["argument_specs"]["inputs"]["options"]
             _check_spec(options, path)
             for name, spec in options.items():
+                if name in declared:
+                    raise AnsibleError(f"{name} is declared by both {declared[name]} and {path}")
+                declared[name] = path
                 if name in task_vars:
                     validator.check(spec, task_vars[name], name)
                 elif spec.get("required"):

roles/global_defaults/meta/argument_specs.yml

@@ -9,6 +9,14 @@ argument_specs:
       rhel_iso: {type: str}
       custom_iso: {type: bool}
       thirdparty_tasks: {type: str}
+      partitioning_efi_size_mib: {type: int}
+      partitioning_boot_size_mib: {type: int}
+      partitioning_separate_boot: {type: bool}
+      partitioning_boot_fs_fstype: {type: str}
+      partitioning_use_full_disk: {type: bool}
+      cleanup_verify_boot: {type: bool}
+      cleanup_boot_timeout: {type: int}
+      cleanup_remove_on_failure: {type: bool}
       note: {type: str}
       hypervisor:
         type: dict
@@ -35,10 +43,15 @@ argument_specs:
           filesystem: {type: str, choices: [btrfs, ext4, xfs]}
           name: {type: str}
           source: {type: str}
-          id: {type: str}
+          id: {type: int}
           cpus: {type: int}
           memory: {type: int}
           balloon: {type: int}
+          cores: {type: int}
+          config: {type: str, choices: [none, current, pending]}
+          network_info: {type: bool}
+          network_config: {type: str}
+          guest_path: {type: str}
           path: {type: str}
           timezone: {type: str}
           locale: {type: str}
@@ -48,9 +61,9 @@ argument_specs:
             type: dict
             options:
               bridge: {type: str}
-              vlan: {type: str}
+              vlan: {type: int}
               ip: {type: str}
-              prefix: {type: str}
+              prefix: {type: int}
               gateway: {type: str}
               dns:
                 type: dict
@@ -63,9 +76,9 @@ argument_specs:
                 options:
                   name: {type: str}
                   bridge: {type: str}
-                  vlan: {type: str}
+                  vlan: {type: int}
                   ip: {type: str}
-                  prefix: {type: str}
+                  prefix: {type: int}
                   gateway: {type: str}
                   extra_addresses: {type: list, elements: str}
                   parent: {type: str}
@@ -80,12 +93,12 @@ argument_specs:
             type: list
             elements: dict
             options:
-              size: {type: float}
+              size: {type: int}
               device: {type: str}
               storage: {type: str}
               format: {type: str}
-              bus: {type: str}
+              bus: {type: str, choices: ["", scsi, virtio, sata, ide]}
-              slot: {type: str}
+              slot: {type: int}
               path: {type: str}
               target: {type: str}
               mount:

tests/inputs/fixture_spec.yml

@@ -21,6 +21,11 @@ argument_specs:
           secret:
             type: str
             no_log: true
+          gate:
+            type: raw
+            options:
+              port:
+                type: int
           disks:
             type: list
             elements: dict
@@ -31,7 +36,7 @@ argument_specs:
           users:
             type: dict
             options:
-              "<name>":
+              "<user>":
                 type: dict
                 options:
                   password:

tests/inputs/inputs_fixtures.yml

@@ -16,12 +16,18 @@ inputs_fixtures:
         retired: ~
     expect: []
 
+  - name: empty-int-is-unset
+    value:
+      name: web01
+      count: ""
+    expect: []
+
   - name: list-for-dict
     value:
       name: web01
       users:
         - name: alice
-    expect: ["fixture.users: expected dict, got list"]
+    expect: ["fixture.users: expected dict keyed by name, got list"]
 
   - name: comma-string-for-list
     value:
@@ -39,7 +45,7 @@ inputs_fixtures:
     value:
       name: web01
       userz: {}
-    expect: ["fixture.userz: unsupported key, fixture accepts count, disks, enabled, kind, name, secret, servers, users"]
+    expect: ["fixture.userz: unsupported key, fixture accepts count, disks, enabled, gate, kind, name, secret, servers, users"]
 
   - name: wrong-element-type
     value:
@@ -84,5 +90,33 @@ inputs_fixtures:
       enabled: "true"
       userz: {}
     expect:
-      - "fixture.userz: unsupported key, fixture accepts count, disks, enabled, kind, name, secret, servers, users"
+      - "fixture.userz: unsupported key, fixture accepts count, disks, enabled, gate, kind, name, secret, servers, users"
       - "fixture.enabled: expected bool, got str"
+
+  - name: raw-gate-false
+    value:
+      name: web01
+      gate: false
+    expect: []
+
+  - name: raw-gate-dict-checked
+    value:
+      name: web01
+      gate:
+        port: http
+        portz: 1
+    expect:
+      - "fixture.gate.portz: unsupported key, fixture.gate accepts port"
+      - "fixture.gate.port: expected int, got str"
+
+  - name: raw-gate-true
+    value:
+      name: web01
+      gate: true
+    expect: []
+
+  - name: raw-gate-string-rejected
+    value:
+      name: web01
+      gate: "yes"
+    expect: ["fixture.gate: expected bool or dict, got str"]

tests/inputs/test_inputs.yml

@@ -14,6 +14,79 @@
         loop_var: inf
         label: "{{ inf.name }}"
 
+    - name: Expand a spec pattern
+      validate_inputs:
+        specs:
+          - "{{ playbook_dir }}/fixture_*.yml"
+      vars:
+        fixture:
+          name: web01
+          servers: [10.0.0.1]
+      register: inputs_pattern_result
+
+    - name: Assert the pattern matched the fixture spec
+      ansible.builtin.assert:
+        that:
+          - inputs_pattern_result is succeeded
+        success_msg: "[inputs pattern] OK"
+
+    - name: Refuse a pattern that matches nothing
+      block:
+        - name: Run the validator with a pattern that matches no file
+          validate_inputs:
+            specs:
+              - "{{ playbook_dir }}/nothing_*.yml"
+          register: inputs_nomatch_result
+      rescue:
+        - name: Keep the no-match failure
+          ansible.builtin.set_fact:
+            inputs_nomatch_result: "{{ ansible_failed_result }}"
+
+    - name: Assert the empty pattern was refused
+      ansible.builtin.assert:
+        that:
+          - inputs_nomatch_result.msg is search('no spec file matches')
+        fail_msg: "[inputs no-match] got {{ inputs_nomatch_result.msg | default('') }}"
+        success_msg: "[inputs no-match] OK"
+
+    - name: Refuse a spec list given as a string
+      block:
+        - name: Run the validator with specs as a string
+          validate_inputs:
+            specs: "{{ playbook_dir }}/fixture_spec.yml"
+          register: inputs_string_result
+      rescue:
+        - name: Keep the string-specs failure
+          ansible.builtin.set_fact:
+            inputs_string_result: "{{ ansible_failed_result }}"
+
+    - name: Assert the string was refused
+      ansible.builtin.assert:
+        that:
+          - inputs_string_result.msg is search('specs must be a list')
+        fail_msg: "[inputs string] got {{ inputs_string_result.msg | default('') }}"
+        success_msg: "[inputs string] OK"
+
+    - name: Refuse a variable declared by two specs
+      block:
+        - name: Run the validator with the fixture spec twice
+          validate_inputs:
+            specs:
+              - "{{ playbook_dir }}/fixture_spec.yml"
+              - "{{ playbook_dir }}/fixture_spec.yml"
+          register: inputs_duplicate_result
+      rescue:
+        - name: Keep the duplicate-declaration failure
+          ansible.builtin.set_fact:
+            inputs_duplicate_result: "{{ ansible_failed_result }}"
+
+    - name: Assert the duplicate declaration was refused
+      ansible.builtin.assert:
+        that:
+          - inputs_duplicate_result.msg is search('fixture is declared by both')
+        fail_msg: "[inputs duplicate] got {{ inputs_duplicate_result.msg | default('') }}"
+        success_msg: "[inputs duplicate] OK"
+
     - name: Validate a host var holding values that cannot be decrypted
       block:
         - name: Run the validator on the vaulted host var

tests/users/users_fixtures.yml

@@ -52,7 +52,7 @@ users_fixtures:
         - name: ops
           password: pw
     expect:
-      fail: "system.users: expected dict, got list"
+      fail: "system.users: expected dict keyed by name, got list"
 
   - name: list-rejected-precomputed
     precomputed: true
@@ -61,7 +61,7 @@ users_fixtures:
         - name: ops
           password: pw
     expect:
-      fail: "system.users: expected dict, got list"
+      fail: "system.users: expected dict keyed by name, got list"
 
   - name: scalar-entry-rejected
     system: