From 489613a02039dd2dcb32f1a4caa9f4ce1f0f44da Mon Sep 17 00:00:00 2001 From: Donat Szabo Date: Tue, 15 Sep 2026 17:00:22 +0200 Subject: [PATCH 1/2] feat: Argument spec implementation for ssh role --- meta/argument_specs.yml | 117 ++++++++++++++++ tasks/assert_role_vars.yml | 45 ++++++ tasks/main.yml | 3 + tests/tests_additional_packages.yml | 36 +++-- tests/tests_global_config_mode.yml | 17 ++- tests/tests_invalid_input.yml | 207 ++++++++++++++++++++++++++++ 6 files changed, 407 insertions(+), 18 deletions(-) create mode 100644 meta/argument_specs.yml create mode 100644 tasks/assert_role_vars.yml create mode 100644 tests/tests_invalid_input.yml diff --git a/meta/argument_specs.yml b/meta/argument_specs.yml new file mode 100644 index 00000000..356882c5 --- /dev/null +++ b/meta/argument_specs.yml @@ -0,0 +1,117 @@ +# SPDX-License-Identifier: MIT +--- +argument_specs: + main: + short_description: The ssh role. + description: > + The ssh role allows you to configure the OpenSSH client, either + system-wide, using a drop-in configuration file, or for a + specific user. + + This role installs the required packages and generates an SSH + client configuration file from the options given in `ssh`, + including any `Match` and `Host` conditional blocks. Individual + `ssh_` variables (such as `ssh_ForwardX11`) are also + supported and override the corresponding key in `ssh`. + options: + ssh_user: + type: str + default: null + description: > + Whether to manage the global SSH client configuration or a + single user's configuration. When `null`, the role manages + the system-wide configuration. Otherwise, this is the name + of a user whose per-user configuration file + (`~/.ssh/config`) will be managed. The user must already + exist on the managed host. + ssh_skip_defaults: + type: raw + default: auto + description: > + Whether the generated configuration file keeps the OS + default values for options that are not explicitly set. + Accepts `true` to keep OS defaults, `false` to omit them, + or `auto` to let the role decide: defaults are omitted when + writing a drop-in file or a per-user configuration file, and + kept otherwise. + ssh_drop_in_name: + type: str + default: null + description: > + The name used to build the path of the drop-in + configuration file placed in the system-wide SSH client + drop-in directory, following the template + `/etc/ssh/ssh_config.d/{name}.conf`. The suggested format is + `NN-name`, where `NN` is a two-digit number used for + sorting. Ignored on platforms that do not support a drop-in + directory. + ssh: + type: raw + default: [] + description: > + A dict of SSH client configuration options and their + values, keyed by the option name as documented in + `ssh_config(5)`, such as `Compression`, `ControlMaster`, + `GSSAPIAuthentication`, or `Ciphers`. A value can be a + boolean, a string, a number, or a list of strings, depending + on the option. The special keys `Match` and `Host` accept a + single dict or a list of dicts describing conditional + configuration blocks; each dict must contain a `Condition` + key plus any of the same option keys, and is rendered as a + `Match ` or `Host ` block. Accepts an + empty list when no options are being set. + ssh_additional_packages: + type: list + elements: str + default: [] + description: > + List of extra package names to install in addition to the + packages the role installs automatically, such as + `openssh-keysign` or `openssh-askpass`, needed for less + common use cases like host-based authentication. + ssh_config_owner: + type: str + default: null + description: > + The user name or user id that owns the generated + configuration file. When unset, ownership defaults to the + value of `ssh_user`, or `root` when `ssh_user` is not set. + ssh_config_group: + type: str + default: null + description: > + The group name or group id that owns the generated + configuration file. When unset, group ownership defaults to + the value of `ssh_user`, or `root` when `ssh_user` is not + set. + ssh_config_mode: + type: raw + default: null + description: > + The file system permissions for the generated configuration + file. Accepts a string (such as `'0644'`) or an integer. + When unset, mode defaults to `0600` for per-user + configuration or `0644` for system-wide configuration. + ssh_config_file: + type: path + default: null + description: > + The path of the configuration file that will be written by + the role. When unset, the path is derived from + `ssh_drop_in_name` and `ssh_user`. + ssh_backup: + type: bool + default: true + description: > + Whether to create a backup of the existing configuration + file before overwriting it. + ssh_transactional_update_reboot_ok: + type: raw + default: null + description: > + Whether to allow a reboot required by transactional update + systems after installing packages. Set to `true` to allow + the reboot, or `false` to have the role notify that a + reboot is required without performing it. Accepts a boolean + value or `null`. When left unset (`null`), the role fails to + ensure the reboot requirement is not overlooked. diff --git a/tasks/assert_role_vars.yml b/tasks/assert_role_vars.yml new file mode 100644 index 00000000..0d534ce0 --- /dev/null +++ b/tasks/assert_role_vars.yml @@ -0,0 +1,45 @@ +# SPDX-License-Identifier: MIT +--- +- name: Assert ssh_skip_defaults is 'auto' or a boolean + ansible.builtin.assert: + that: + - >- + (ssh_skip_defaults == 'auto') + or (ssh_skip_defaults is sameas true) + or (ssh_skip_defaults is sameas false) + fail_msg: >- + ssh_skip_defaults must be 'auto' or a boolean, + got {{ ssh_skip_defaults | type_debug }} + when: ssh_skip_defaults is defined + +- name: Assert ssh is a dict or an empty list + ansible.builtin.assert: + that: + - >- + ssh is mapping + or (ssh is sequence and ssh is not string + and ssh is not mapping and ssh | length == 0) + fail_msg: >- + ssh must be a dict or an empty list, + got {{ ssh | type_debug }} + when: ssh is defined + +- name: Assert ssh_config_mode is a string or integer + ansible.builtin.assert: + that: + - (ssh_config_mode | type_debug) in ['str', 'int', 'unicode'] + fail_msg: >- + ssh_config_mode must be a string or integer, + got {{ ssh_config_mode | type_debug }} + when: ssh_config_mode is not none + +- name: Assert ssh_transactional_update_reboot_ok is null or a boolean + ansible.builtin.assert: + that: + - >- + (ssh_transactional_update_reboot_ok is none) + or (ssh_transactional_update_reboot_ok is sameas true) + or (ssh_transactional_update_reboot_ok is sameas false) + fail_msg: >- + ssh_transactional_update_reboot_ok must be null or a boolean, + got {{ ssh_transactional_update_reboot_ok | type_debug }} diff --git a/tasks/main.yml b/tasks/main.yml index b6f5e02a..6550d922 100644 --- a/tasks/main.yml +++ b/tasks/main.yml @@ -2,6 +2,9 @@ - name: Set platform/version specific variables include_tasks: set_vars.yml +- name: Validate role parameters + ansible.builtin.include_tasks: assert_role_vars.yml + - name: Ensure required packages are installed package: name: "{{ __ssh_packages + ssh_additional_packages }}" diff --git a/tests/tests_additional_packages.yml b/tests/tests_additional_packages.yml index fc0bbe0c..88f31ec5 100644 --- a/tests/tests_additional_packages.yml +++ b/tests/tests_additional_packages.yml @@ -1,21 +1,29 @@ --- - name: Verify default packages as well as additional are installed hosts: all - vars: - additional_package: >- - {% if ansible_facts['distribution'] in [ 'RedHat', 'CentOS' ] and - ansible_facts['distribution_version'] | int < 9 %} - openssh-ldap - {% elif ansible_facts['os_family'] == 'RedHat' %} - openssh-keycat - {% elif ansible_facts['os_family'] == 'Suse' %} - openssh-helpers - {% elif ansible_facts['os_family'] == 'Alpine' %} - openssh-sk-helper - {% else %} - openssh-tests - {% endif %} tasks: + # NOTE: This must be a set_fact (evaluated now, while ansible_facts is + # still populated) rather than a play var (which is a lazy template). + # run_role_with_clear_facts.yml clears ansible_facts before running the + # role, and the role's argument_specs validation templates every passed + # variable before the role re-gathers facts - a lazy template here would + # fail with "'dict object' has no attribute 'distribution'". + - name: Determine the additional package name for this platform + ansible.builtin.set_fact: + additional_package: >- + {% if ansible_facts['distribution'] in [ 'RedHat', 'CentOS' ] and + ansible_facts['distribution_version'] | int < 9 %} + openssh-ldap + {% elif ansible_facts['os_family'] == 'RedHat' %} + openssh-keycat + {% elif ansible_facts['os_family'] == 'Suse' %} + openssh-helpers + {% elif ansible_facts['os_family'] == 'Alpine' %} + openssh-sk-helper + {% else %} + openssh-tests + {% endif %} + - name: Run role include_tasks: tasks/run_role_with_clear_facts.yml vars: diff --git a/tests/tests_global_config_mode.yml b/tests/tests_global_config_mode.yml index 70b0344a..02707d85 100644 --- a/tests/tests_global_config_mode.yml +++ b/tests/tests_global_config_mode.yml @@ -5,15 +5,24 @@ __ssh_test_backup_files: - /etc/ssh/ssh_config.d/00-ansible.conf - /etc/ssh/ssh_config - # SUSE Minimal VM images do not have the daemon user - __ssh_test_owner: >- - {{ 'nobody' if ansible_facts['os_family'] == 'Suse' - else 'daemon' }} tasks: - name: Backup configuration files include_tasks: tasks/backup.yml + # NOTE: This must be a set_fact (evaluated now, while ansible_facts is + # still populated) rather than a play var (which is a lazy template). + # run_role_with_clear_facts.yml clears ansible_facts before running the + # role, and the role's argument_specs validation templates every passed + # variable before the role re-gathers facts - a lazy template here would + # fail with "'dict object' has no attribute 'os_family'". + - name: Determine expected file owner for this platform + ansible.builtin.set_fact: + # SUSE Minimal VM images do not have the daemon user + __ssh_test_owner: >- + {{ 'nobody' if ansible_facts['os_family'] == 'Suse' + else 'daemon' }} + - name: Run role include_tasks: tasks/run_role_with_clear_facts.yml vars: diff --git a/tests/tests_invalid_input.yml b/tests/tests_invalid_input.yml new file mode 100644 index 00000000..ca2d3e41 --- /dev/null +++ b/tests/tests_invalid_input.yml @@ -0,0 +1,207 @@ +# SPDX-License-Identifier: MIT +--- +- name: Verify invalid parameters are rejected + hosts: all + vars: + __ssh_test_backup_files: + - /etc/ssh/ssh_config.d/00-ansible.conf + - /etc/ssh/ssh_config + tasks: + - name: Backup configuration files + ansible.builtin.include_tasks: tasks/backup.yml + + - name: Run invalid input tests + block: + # ==================================================== + # Section 1: Verify role works with valid defaults + # ==================================================== + - name: Run role with valid defaults + ansible.builtin.include_tasks: tasks/run_role_with_clear_facts.yml + + - name: Mark valid defaults succeeded + ansible.builtin.set_fact: + __valid_defaults_succeeded: true + + - name: Assert role completed with valid defaults + ansible.builtin.assert: + that: + - __valid_defaults_succeeded | default(false) + fail_msg: >- + Role should succeed with default parameters + + # ==================================================== + # Section 2: argument_specs validation (Ansible 2.11+) + # ==================================================== + - name: Run argument specs validation tests + when: ansible_version.full is version("2.11", ">=") + block: + - name: Argument specs reject invalid ssh_backup type + block: + - name: Run role with ssh_backup as a non-boolean string + ansible.builtin.include_tasks: tasks/run_role_with_clear_facts.yml + vars: + ssh_backup: not_a_bool + rescue: + - name: Mark invalid ssh_backup rejected + ansible.builtin.set_fact: + __invalid_input_backup_type_failed: true + + - name: Assert invalid ssh_backup type failed validation + ansible.builtin.assert: + that: + - __invalid_input_backup_type_failed | default(false) + fail_msg: >- + meta/argument_specs should reject ssh_backup values + that are not booleans + + - name: Argument specs reject invalid ssh_additional_packages type + block: + - name: Run role with ssh_additional_packages as a dict + ansible.builtin.include_tasks: tasks/run_role_with_clear_facts.yml + vars: + ssh_additional_packages: + not: a_list + rescue: + - name: Mark invalid ssh_additional_packages rejected + ansible.builtin.set_fact: + __invalid_input_additional_packages_type_failed: true + + - name: Assert invalid ssh_additional_packages type failed validation + ansible.builtin.assert: + that: + - __invalid_input_additional_packages_type_failed | default(false) + fail_msg: >- + meta/argument_specs should reject ssh_additional_packages + values that cannot be converted to a list + + # ==================================================== + # Section 3: assert_role_vars validation (all versions) + # ==================================================== + - name: Assert rejects ssh_skip_defaults with an invalid value + block: + - name: Run role with ssh_skip_defaults as an invalid string + ansible.builtin.include_tasks: tasks/run_role_with_clear_facts.yml + vars: + ssh_skip_defaults: sometimes + rescue: + - name: Mark invalid ssh_skip_defaults rejected + ansible.builtin.set_fact: + __invalid_input_skip_defaults_value_failed: true + when: >- + 'ssh_skip_defaults must be' in + (ansible_failed_result | default({}) | to_json) + + - name: Assert ssh_skip_defaults invalid value failed validation + ansible.builtin.assert: + that: + - __invalid_input_skip_defaults_value_failed | default(false) + fail_msg: >- + tasks/assert_role_vars.yml should reject ssh_skip_defaults + values that are not 'auto' or a boolean + + - name: Assert rejects ssh as a non-empty list + block: + - name: Run role with ssh as a non-empty list + ansible.builtin.include_tasks: tasks/run_role_with_clear_facts.yml + vars: + ssh: + - Compression + - true + rescue: + - name: Mark ssh non-empty list rejected + ansible.builtin.set_fact: + __invalid_input_ssh_list_failed: true + when: >- + 'ssh must be a dict' in + (ansible_failed_result | default({}) | to_json) + + - name: Assert ssh as non-empty list failed validation + ansible.builtin.assert: + that: + - __invalid_input_ssh_list_failed | default(false) + fail_msg: >- + tasks/assert_role_vars.yml should reject ssh when it is a + non-empty list instead of a dict or an empty list + + - name: Assert rejects ssh as a string + block: + - name: Run role with ssh as a string + ansible.builtin.include_tasks: tasks/run_role_with_clear_facts.yml + vars: + ssh: not_a_dict_or_list + rescue: + - name: Mark ssh string rejected + ansible.builtin.set_fact: + __invalid_input_ssh_string_failed: true + when: >- + 'ssh must be a dict' in + (ansible_failed_result | default({}) | to_json) + + - name: Assert ssh as string failed validation + ansible.builtin.assert: + that: + - __invalid_input_ssh_string_failed | default(false) + fail_msg: >- + tasks/assert_role_vars.yml should reject ssh when it is a + string instead of a dict or an empty list + + - name: Assert rejects ssh_config_mode as a boolean + block: + - name: Run role with ssh_config_mode as a boolean + ansible.builtin.include_tasks: tasks/run_role_with_clear_facts.yml + vars: + ssh_config_mode: true + rescue: + - name: Mark ssh_config_mode boolean rejected + ansible.builtin.set_fact: + __invalid_input_config_mode_bool_failed: true + when: >- + 'ssh_config_mode must be' in + (ansible_failed_result | default({}) | to_json) + + - name: Assert ssh_config_mode as boolean failed validation + ansible.builtin.assert: + that: + - __invalid_input_config_mode_bool_failed | default(false) + fail_msg: >- + tasks/assert_role_vars.yml should reject ssh_config_mode + when it is a boolean instead of a string or integer + + - name: Assert rejects ssh_transactional_update_reboot_ok as a string + block: + - name: Run role with ssh_transactional_update_reboot_ok as a string + ansible.builtin.include_tasks: tasks/run_role_with_clear_facts.yml + vars: + ssh_transactional_update_reboot_ok: not_a_bool + rescue: + - name: Mark ssh_transactional_update_reboot_ok string rejected + ansible.builtin.set_fact: + __invalid_input_reboot_ok_string_failed: true + when: >- + 'ssh_transactional_update_reboot_ok must be' in + (ansible_failed_result | default({}) | to_json) + + - name: Assert ssh_transactional_update_reboot_ok as string failed validation + ansible.builtin.assert: + that: + - __invalid_input_reboot_ok_string_failed | default(false) + fail_msg: >- + tasks/assert_role_vars.yml should reject + ssh_transactional_update_reboot_ok when it is a string + instead of null or a boolean + + always: + - name: Clear test facts + ansible.builtin.set_fact: + __valid_defaults_succeeded: + __invalid_input_backup_type_failed: + __invalid_input_additional_packages_type_failed: + __invalid_input_skip_defaults_value_failed: + __invalid_input_ssh_list_failed: + __invalid_input_ssh_string_failed: + __invalid_input_config_mode_bool_failed: + __invalid_input_reboot_ok_string_failed: + tags: tests::cleanup + + - name: Restore configuration files + ansible.builtin.include_tasks: tasks/restore.yml From ff3f86c66c83be868b78871a0b6c6558abd83d8b Mon Sep 17 00:00:00 2001 From: Donat Szabo Date: Wed, 16 Sep 2026 13:22:50 +0200 Subject: [PATCH 2/2] Moved the restore task into the always: block --- tests/tests_invalid_input.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/tests_invalid_input.yml b/tests/tests_invalid_input.yml index ca2d3e41..1521cc79 100644 --- a/tests/tests_invalid_input.yml +++ b/tests/tests_invalid_input.yml @@ -203,5 +203,6 @@ __invalid_input_reboot_ok_string_failed: tags: tests::cleanup - - name: Restore configuration files - ansible.builtin.include_tasks: tasks/restore.yml + - name: Restore configuration files + ansible.builtin.include_tasks: tasks/restore.yml + tags: tests::cleanup