Allow subspec defaults to be processed when the parent argument is not supplied

[sivel]

SUMMARY

There has been un unnoticed or unreported bug in handling defaults within a subspec when the parent argument is not provided.

An example module:

    module = AnsibleModule(argument_spec={
        'one': {
            'type': 'dict',
            'options': {
                'two': {
                    'default': True,
                    'type': 'bool',
                }
            }
        }
    })

When called like:

- suboptions_test:

It would result in module.params['one'] == None which is not expected.

Many modules have worked around this in various ways, often needing to duplicate the defaults.

This PR addresses this issue, and ensures that defaults from options are processed when the top level argument is omitted.

This now would produce:

module.params['one']['two'] == True

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

lib/ansible/module_utils/basic.py

ANSIBLE VERSION

devel

ADDITIONAL INFORMATION

cc @gundalow @Qalthos @rcarrillocruz @caphrim007

You have been CCed, because you are responsible for modules, particularly network modules that utilize the provider argument which may be affected by this change. Please test and let me know if this is problematic.

[sivel]

A quick look indicates that some of the workarounds may not be fully compatible with this fix.

[ganeshrn]

If the parent value is not given in that case child values shouldn't be initialized to default (at least that's how network data model works).

In this particular case if the subspec has an option with required=True and if parent value is not given as input this change will initialize child option value to default and will result in required condition failure. This might break the existing modules.

[caphrim007]

The way I always thought of the subspec defaults and requires as working is that they would only be applicable when someone specified the top-level arg.

As such, I think that would break the modules I maintain. It would certainly change the behavior of the tasks that use this method.

This would be a problem and would likely result in me continuing to pull more and more functionality into the module execution code because I continually cant rely on argument spec functionality

[caphrim007]

In other words, I parrot @ganeshrn's remarks

[sivel]

I do understand that this will require changes to existing modules.

And I understand you have concerns. However...

Let's say that the top level option is required=False (in my example this is one). And all of the sub options have defaults and are also required=False (example is two). Regardless of the specifics of provider, isn't it unexpected that you cannot just rely on the defaults? As it is one will be None, but to match how default works, I would expect one to have a value of {'two': True}.

As it is, there is no way to just rely on the defaults.

As a result, this requires authors to do exactly what you said you would do. Pull more functionality into the module, because this requires workarounds or duplication of data to allow defaults to actually work.

I sincerely believe the way that subspec works currently is broken, as evidenced by all the work arounds.

That being said, I want to understand the concerns more. Words are unclear. I'd be interested to see specifics where you believe this would cause issues. There may be ways to extend this to meet the needs.

[sivel]

An example of a workaround that was submitted to get around this issue #31774

[sivel]

And maybe a solution is to make this configurable, if the problem will require too much up front work.

[cben]

So the current functionality some modules rely on is ability to tell if parent arg is completely omitted, by it being set to None?
May I suggest then they would opt-in to this by explicitly setting default=None on the parent param?
IOW, omitting parent arg would result in parent's default if set, or a dict with child defaults otherwise.

[caphrim007]

The way I would read your example is

"two" is populated if and only if "one" is specified.

module:
  one:

Would cause "two"'s default to be set

module:

Would cause "two" to be None

The places where I use this are cases where a value may be required for creation, but not for update or delete. Say, a parameters called is_remote that we're considering adding to the bigip_gtm_pool module.

That module has a members param. It is a suboption that has additional params server and virtual_server.

The is_remote is required due to a inconsistency in the F5 product.

With this change, whether you specified any virtual_server values or server values, there would always be an is_remote value in that dict. So we could not easily check if "no members" we're specified like we do now (checking for None)

Today, we only find the members param (and everything under it relevant) if it itself is specified.

Ex.

bigip_gtm_pool:
  members:

Or

bigip_gtm_pool:

Didn't specify members, don't need to worry about processing members

bigip_gtm_pool:
  members:
    - server: foo
      virtual_server: bar

Gets the default of is_remote because members we're specified.

This "fix" would change that. So we would end up removing our usage of argument spec; making argument spec, and the docs, even more useless to us, and bake that logic into the module classes themselves.

[caphrim007]

@cben, the default is already none, and Ansible has linting logic that rejects modules when you specify a none default

[sivel]

@caphrim007 I am seeing that there are 2 sets of functionality here that could be useful here. In my specific example, I would expect two to be set in both:

module:

and

module:
  one:

But based on your need, it sounds like, there may be cases where this is unwanted.

I'm thinking of adding an apply_defaults option (defaulted to False) to the parent, that allows the behavior I describe here. Which would give users a way to use this API without workarounds, when this functionality is desired.

Thoughts?

[sivel]

I just pushed a change with apply_defaults

[caphrim007]

@sivel my first inclination when I was CC'd on this was that both parties might be satisfied with a new reserved arg.

Since you mention it, yes, I think that would be workable

[ganeshrn]

@sivel +1 for apply_defaults

[ansibot]

cc @dharmabumstead
click here for bot help

[sivel]

@cben I just pushed a change to switch manageiq_connection over to use apply_defaults. Just wanted your confirmation on this change.

Thanks!

[ansibot]

cc @abellotti @gtanzillo @mattclay @yaacov @zgalor
click here for bot help

[ansibot]

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/remote_management/manageiq/manageiq_provider.py:0:0: E326 Value for "choices" from the argument_spec ([]) for "api_version" does not match the documentation (['v2', 'v3'])

click here for bot help

[ansibot]

cc @dkorn
click here for bot help

[cben]

+100 for added documentation alone!

manageiq modules changes LGTM.
Some minor suggestions to the logic, driven by my fear of _handle_options 😉

[cben]

cosmetic: if you move apply_defaults to the end, the difference from the next input will become immediately obvious :)

[cben]

I think you can extract if spec is None: continue to be first, above the apply_defaults check, and the rest would be simpler.
This branch is untested, consider adding a test case with apply_defaults but no options.

After that, I think lifting if params.get(k) is None to be the outer check might (?) make things even clearer:

spec = v.get('options', None)
if spec is None:
    continue
if params.get(k) is None:
    if v.get('apply_defaults', False):
        params[k] = {}  # allows individual option defaults to apply
    else:
        continue

(untested so likely buggy. and "clearer" is of course subjective, your call...)

[cben]

Q: Does default work together with options, for cases where top-level arg is not supplied?
Is apply_defaults simply shorthand for a default of corresponding dict (in your example giving top_level a default=dict(second_level=True))?

If that is true, perhaps implementation doesn't need to complicate _handle_options further, could derive an "effective default" without even looking at given params.

[sivel]

Q: Does default work together with options, for cases where top-level arg is not supplied?

yes

Is apply_defaults simply shorthand for a default of corresponding dict (in your example giving top_level a default=dict(second_level=True))?

Yes, effectively the goal here, is that a user should not have to duplicate default options from the suboptions, to allow this functionality to work.

A future linting changing will ensure a module does not supply both apply_defaults and default together on the same argument.