Avoid internal error when validating argspec #81631
Open
+17
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ansibot
added
bug
mkrizek
removed
the
needs_triage
mkrizek
reviewed
Sep 5, 2023
| @@ -838,6 +838,9 @@ def env_fallback(*args, **kwargs): | |||
|
|
|||
| def set_fallbacks(argument_spec, parameters): | |||
| no_log_values = set() | |||
| if not isinstance(parameters, Mapping): | |||
| # The wrong type is handled somewhere else | |||
| return no_log_values | |||
There was a problem hiding this comment.
This shouldn't be silently ignored. It should be an error at "somewhere else" or here directly.
There was a problem hiding this comment.
This should raise a TypeError if any of the arguments is invalid. But yeah, the input validation must happen in the outer layer, in the calling code somewhere.
There was a problem hiding this comment.
Actually the caller checked for the right type, but only after calling this function. I've switched the order.
|
The reason that CI fails is that the last task in the That no longer happens since this PR fixes that crash, now it results in which contains the secret value |
|
The reason why I'll push a change that disables the broken test once CI completes. This should be fixed in another PR. If you want to keep the double type checking with raising |
ansibot
removed
ci_verified
s-hertel
reviewed
Sep 8, 2023
Comment on lines
+46
to
+48
| when: false # TODO FIXME: The code that should prevent 'AGROUND' ending up in the output | ||
| # has been broken for some time; its misbehavior was masked by | ||
| # the bug fixed in https://github.com/ansible/ansible/pull/81631. |
There was a problem hiding this comment.
I think the underlying bug is in the outer validate method with how we use _list_no_log_values. An error anywhere in the spec prevents marking any of the options as no_log (and we stop at the first error). Maybe we could accumulate the errors non-fatally the first time.
diff --git a/lib/ansible/module_utils/common/arg_spec.py b/lib/ansible/module_utils/common/arg_spec.py
index d9f716efce..44548c71ec 100644
--- a/lib/ansible/module_utils/common/arg_spec.py
+++ b/lib/ansible/module_utils/common/arg_spec.py
@@ -201,10 +201,10 @@ class ArgumentSpecValidator:
'collection_name': deprecation.get('collection_name'),
})
- try:
- result._no_log_values.update(_list_no_log_values(self.argument_spec, result._validated_parameters))
- except TypeError as te:
- result.errors.append(NoLogError(to_native(te)))
+ no_log_introspection_errors = []
+ result._no_log_values.update(_list_no_log_values(self.argument_spec, result._validated_parameters, no_log_introspection_errors))
+ for error in no_log_introspection_errors:
+ result.errors.append(NoLogError(error))
try:
result._deprecations.extend(_list_deprecations(self.argument_spec, result._validated_parameters))
diff --git a/lib/ansible/module_utils/common/parameters.py b/lib/ansible/module_utils/common/parameters.py
index 386eb875ce..4e146683c5 100644
--- a/lib/ansible/module_utils/common/parameters.py
+++ b/lib/ansible/module_utils/common/parameters.py
@@ -305,7 +305,7 @@ def _list_deprecations(argument_spec, parameters, prefix=''):
return deprecations
-def _list_no_log_values(argument_spec, params):
+def _list_no_log_values(argument_spec, params, errors=None):
"""Return set of no log values
:arg argument_spec: An argument spec dictionary
@@ -324,6 +324,9 @@ def _list_no_log_values(argument_spec, params):
try:
no_log_values.update(_return_datastructure_name(no_log_object))
except TypeError as e:
+ if errors is not None:
+ errors.append('Failed to convert "%s": %s' % (arg_name, to_native(e)))
+ continue
raise TypeError('Failed to convert "%s": %s' % (arg_name, to_native(e)))
# Get no_log values from suboptions
@@ -342,9 +345,19 @@ def _list_no_log_values(argument_spec, params):
@@ -342,9 +345,19 @@ def _list_no_log_values(argument_spec, params):
# Validate dict fields in case they came in as strings
if isinstance(sub_param, string_types):
- sub_param = check_type_dict(sub_param)
+ try:
+ sub_param = check_type_dict(sub_param)
+ except TypeError as e:
+ if errors is not None:
+ errors.append(to_native(e))
+ continue
+ raise
if not isinstance(sub_param, Mapping):
+ if errors is not None:
+ errors.append("Value '{1}' in the sub parameter field '{0}' must by a {2}, "
+ "not '{1.__class__.__name__}'".format(arg_name, sub_param, wanted_type))
+ continue
raise TypeError("Value '{1}' in the sub parameter field '{0}' must by a {2}, "
"not '{1.__class__.__name__}'".format(arg_name, sub_param, wanted_type))We also call _list_no_log_values in _validate_sub_spec, but since _list_no_log_values is recursive I don't think that's necessary if the outer scope is fixed.
There was a problem hiding this comment.
#81672 contains a slightly modified version of your patch.
I think we should fix that first then. |
ansibot
added
the
stale_ci
|
2.18 task- there are several related things that could use some attention, and some we've already fixed in other branches |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
Reported in ansible-collections/community.docker#680.
When passing a boolean or number to a parameter that expects a dictionary, there is an exception in the fallback handling since it assumes that the value is a dictionary.
Fixes this and adds a regression test.
ISSUE TYPE