Skip to content

Commit

Permalink
argspec - fix validating type for required options that are None (ans…
Browse files Browse the repository at this point in the history
…ible#79677)

* Only bypass type validation for null parameters if the default is None. A default is mutually exclusive with required.

* Prevent coercing None to str type. Fail the type check instead.
  • Loading branch information
s-hertel authored Apr 17, 2023
1 parent 964e678 commit 694c11d
Show file tree
Hide file tree
Showing 6 changed files with 140 additions and 6 deletions.
2 changes: 2 additions & 0 deletions changelogs/fragments/79677-fix-argspec-type-check.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
bugfixes:
- module/role argument spec - validate the type for options that are None when the option is required or has a non-None default (https://github.com/ansible/ansible/issues/79656).
2 changes: 1 addition & 1 deletion lib/ansible/module_utils/common/parameters.py
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,7 @@ def _validate_argument_types(argument_spec, parameters, prefix='', options_conte
continue

value = parameters[param]
if value is None:
if value is None and not spec.get('required') and spec.get('default') is None:
continue

wanted_type = spec.get('type')
Expand Down
2 changes: 1 addition & 1 deletion lib/ansible/module_utils/common/validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ def check_type_str(value, allow_conversion=True, param=None, prefix=''):
if isinstance(value, string_types):
return value

if allow_conversion:
if allow_conversion and value is not None:
return to_native(value, errors='surrogate_or_strict')

msg = "'{0!r}' is not a string and conversion is not allowed".format(value)
Expand Down
2 changes: 1 addition & 1 deletion test/integration/targets/apt_repository/tasks/apt.yml
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@
- assert:
that:
- result is failed
- result.msg == 'Please set argument \'repo\' to a non-empty value'
- result.msg.startswith("argument 'repo' is of type <class 'NoneType'> and we were unable to convert to str")

- name: Test apt_repository with an empty value for repo
apt_repository:
Expand Down
9 changes: 9 additions & 0 deletions test/integration/targets/roles_arg_spec/roles/c/meta/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,15 @@ argument_specs:
main:
short_description: Main entry point for role C.
options:
c_dict:
type: "dict"
required: true
c_int:
type: "int"
required: true
c_list:
type: "list"
required: true
c_raw:
type: "raw"
required: true
129 changes: 126 additions & 3 deletions test/integration/targets/roles_arg_spec/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,10 @@
hosts: localhost
gather_facts: false
vars:
c_dict: {}
c_int: 1
c_list: []
c_raw: ~
a_str: "some string"
a_int: 42
tasks:
Expand All @@ -157,6 +160,125 @@
include_role:
name: c

- name: "New play to reset vars: Test nested role including/importing role fails with null required options"
hosts: localhost
gather_facts: false
vars:
a_main_spec:
a_str:
required: true
type: "str"
c_main_spec:
c_int:
required: true
type: "int"
c_list:
required: true
type: "list"
c_dict:
required: true
type: "dict"
c_raw:
required: true
type: "raw"
# role c calls a's main and alternate entrypoints
a_str: ''
c_dict: {}
c_int: 0
c_list: []
c_raw: ~
tasks:
- name: test type coercion fails on None for required str
block:
- name: "Test import_role of role C (missing a_str)"
import_role:
name: c
vars:
a_str: ~
- fail:
msg: "Should not get here"
rescue:
- debug:
var: ansible_failed_result
- name: "Validate import_role failure"
assert:
that:
# NOTE: a bug here that prevents us from getting ansible_failed_task
- ansible_failed_result.argument_errors == [error]
- ansible_failed_result.argument_spec_data == a_main_spec
vars:
error: >-
argument 'a_str' is of type <class 'NoneType'> and we were unable to convert to str:
'None' is not a string and conversion is not allowed
- name: test type coercion fails on None for required int
block:
- name: "Test import_role of role C (missing c_int)"
import_role:
name: c
vars:
c_int: ~
- fail:
msg: "Should not get here"
rescue:
- debug:
var: ansible_failed_result
- name: "Validate import_role failure"
assert:
that:
# NOTE: a bug here that prevents us from getting ansible_failed_task
- ansible_failed_result.argument_errors == [error]
- ansible_failed_result.argument_spec_data == c_main_spec
vars:
error: >-
argument 'c_int' is of type <class 'NoneType'> and we were unable to convert to int:
<class 'NoneType'> cannot be converted to an int
- name: test type coercion fails on None for required list
block:
- name: "Test import_role of role C (missing c_list)"
import_role:
name: c
vars:
c_list: ~
- fail:
msg: "Should not get here"
rescue:
- debug:
var: ansible_failed_result
- name: "Validate import_role failure"
assert:
that:
# NOTE: a bug here that prevents us from getting ansible_failed_task
- ansible_failed_result.argument_errors == [error]
- ansible_failed_result.argument_spec_data == c_main_spec
vars:
error: >-
argument 'c_list' is of type <class 'NoneType'> and we were unable to convert to list:
<class 'NoneType'> cannot be converted to a list
- name: test type coercion fails on None for required dict
block:
- name: "Test import_role of role C (missing c_dict)"
import_role:
name: c
vars:
c_dict: ~
- fail:
msg: "Should not get here"
rescue:
- debug:
var: ansible_failed_result
- name: "Validate import_role failure"
assert:
that:
# NOTE: a bug here that prevents us from getting ansible_failed_task
- ansible_failed_result.argument_errors == [error]
- ansible_failed_result.argument_spec_data == c_main_spec
vars:
error: >-
argument 'c_dict' is of type <class 'NoneType'> and we were unable to convert to dict:
<class 'NoneType'> cannot be converted to a dict
- name: "New play to reset vars: Test nested role including/importing role fails"
hosts: localhost
Expand All @@ -171,13 +293,15 @@
required: true
type: "int"

c_int: 100
c_list: []
c_dict: {}
c_raw: ~
tasks:
- block:
- name: "Test import_role of role C (missing a_str)"
import_role:
name: c
vars:
c_int: 100

- fail:
msg: "Should not get here"
Expand All @@ -202,7 +326,6 @@
include_role:
name: c
vars:
c_int: 200
a_str: "some string"

- fail:
Expand Down

0 comments on commit 694c11d

Please sign in to comment.