elements='dict' set suboptions to None values

[markuman]

SUMMARY

059cf9e#diff-b6b6dcee788bed9e4eb606cc7bb24cddR2102

This commit introduces elements='dict' which results in None values for suboptions like block_device_mappings or network_interfaces.
With community.aws 1.0.0 everything is fine. Since 1.2.0 it gets broken.
The integration test does not cover it, because it set nor block_device_mappings nor network_interfaces.

@felixfontein and @s-hertel confirms this bug on #ansible-aws

ISSUE TYPE

• Bug Report

COMPONENT NAME

ec2_launch_template
and maybe many more...

ANSIBLE VERSION

ansible --version
ansible 2.10.1
  config file = /home/m/git/lekker/ansible/ansible.cfg
  configured module search path = ['/home/m/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/m/.local/lib/python3.8/site-packages/ansible
  executable location = /home/m/.local/bin/ansible
  python version = 3.8.5 (default, Sep  5 2020, 10:50:12) [GCC 10.2.0]


ansible-galaxy collection list

# /home/m/.ansible/collections/ansible_collections
Collection         Version
------------------ -------
amazon.aws         1.2.0  
ansible.netcommon  1.3.0  
ansible.posix      1.1.0  
community.aws      1.2.0  
google.cloud       1.0.0  
hetzner.hcloud     1.0.0  
markuman.nextcloud 2.0.1  

STEPS TO REPRODUCE

- name: create launch template for autoscaling group cluster-int
  community.aws.ec2_launch_template:
    template_name: cluster-int
    image_id: "{{ ami.images[0].image_id }}"
    instance_type: "{{ launch_template_instance_type }}"
    credit_specification:
      cpu_credits: "{{ CPU_CREDITS | default('standard') }}"
    network_interfaces:
      - associate_public_ip_address: no
        delete_on_termination: yes
        device_index: 0
        groups:
          - "{{ ecsinstanceint.group_id }}"
          - "{{ ECS_SG_1.group_id }}"
          - "{{ ECS_SG_2.group_id }}"
          - "{{ awsservicegroupS3.group_id }}"
          - "{{ elb_net_int_sg.group_id }}"
          - "{{ GLOBAL_SG_1.group_id }}"
          - "{{ GLOBAL_SG_2.group_id }}"
        # subnet_id: subnet-c84506a3
    user_data: "{{ USERDATA | b64encode }}"
    region: "{{ region }}"
    key_name: "{{ keypair }}"
    iam_instance_profile: ecsInstanceRole
    ebs_optimized: yes
    block_device_mappings:
      - device_name: /dev/xvda
        ebs:
          volume_size: 30
          volume_type: gp2
          delete_on_termination: true
          kms_key_id: "{{ kms_key }}"
          encrypted: yes
    tags:
      cluster: cluster-int
      System: ecs
      Name: cluster-int:xvda
      ssh_access: ldap
      ssh_pub_key: ldap
  register: NEW_LAUNCH_TEMPLATE

ACTUAL RESULTS

Invalid type for parameter LaunchTemplateData.NetworkInterfaces[0].Description, value: None, type: <class 'NoneType'>, valid types: <class 'str'>
--
Invalid type for parameter LaunchTemplateData.NetworkInterfaces[0].Ipv6AddressCount, value: None, type: <class 'NoneType'>, valid types: <class 'int'>
Invalid type for parameter LaunchTemplateData.NetworkInterfaces[0].Ipv6Addresses, value: None, type: <class 'NoneType'>, valid types: <class 'list'>, <class 'tuple'>
Invalid type for parameter LaunchTemplateData.NetworkInterfaces[0].NetworkInterfaceId, value: None, type: <class 'NoneType'>, valid types: <class 'str'>
Invalid type for parameter LaunchTemplateData.NetworkInterfaces[0].PrivateIpAddress, value: None, type: <class 'NoneType'>, valid types: <class 'str'>
Invalid type for parameter LaunchTemplateData.NetworkInterfaces[0].SubnetId, value: None, type: <class 'NoneType'>, valid types: <class 'str'>
Invalid type for parameter LaunchTemplateData.BlockDeviceMappings[0].Ebs.Iops, value: None, type: <class 'NoneType'>, valid types: <class 'int'>
Invalid type for parameter LaunchTemplateData.BlockDeviceMappings[0].Ebs.SnapshotId, value: None, type: <class 'NoneType'>, valid types: <class 'str'>
Invalid type for parameter LaunchTemplateData.BlockDeviceMappings[0].NoDevice, value: None, type: <class 'NoneType'>, valid types: <class 'str'>
Invalid type for parameter LaunchTemplateData.BlockDeviceMappings[0].VirtualName, value: None, type: <class 'NoneType'>, valid types: <class 'str'>

[felixfontein]

What happens is that elements='dict' actually enables validation of options/suboptions, and that in turn ensures that default values are filled in - which happen to be None here. So there are three solutions, two of them that can be implemented right now:

1. Remove elements='dict', and thus disable validation of suboptions;
2. Add some filtering which removes keys with None values from the dictionaries in question;
3. Extend AnsibleModule to allow not putting None (or defaults in general) in.

No. 3 doesn't help for Ansible 2.9 and 2.10, so let's ignore that for now. No. 1 throws away some work, so I think the best solution is no. 2.

It would probably be best to have a helper function for that - which I guess would belong into amazon.aws's module_utils?

[wimnat]

I've added a helper function (see above) in amazon.aws to begin to address this.

Once that's merged i'll open a PR here to fix ec2_launch_template

[ansibullbot]

cc @jillr @tremble
click here for bot help

[markuman]

@wimnat Your helper function was merged.
Do you have enough time at the moment to fix this issue here with that function?

[kepstin]

Ah, I commented in #230 but I guess it actually belongs here:

The block_device_mappings parameter to the ec2_launch_template module actually takes a list of dicts, rather than a dict, and the current implementation of the scrub_none_parameters function doesn't doesn't recursively apply to lists of dicts.

adding another conditional branch to scrub_none_parameters like this:

    elif isinstance(v, list):
        clean_parameters[k] = [scrub_none_parameters(vv) if isinstance(vv, dict) else vv for vv in v]

would be necessary.

[wimnat]

@jillr @tremble

So I had a quick stab at this and something I thought was going to be easy is anything but. Perhaps you can provide some direction. I imported by helper function and then just tried to do this:

module.params = scrub_none_parameters(module.params)

I just end up with this

An exception occurred during task execution. To see the full traceback, use -vvv. The error was: AttributeError: can't set attribute
fatal: [127.0.0.1]: FAILED! => {"changed": false, "module_stderr": "Traceback (most recent call last):\n  File \"/root/.ansible/tmp/ansible-tmp-1612531498.6105824-2730-66623839143086/AnsiballZ_ec2_launch_template.py\", line 99, in <module>\n    _ansiballz_main()\n  File \"/root/.ansible/tmp/ansible-tmp-1612531498.6105824-2730-66623839143086/AnsiballZ_ec2_launch_template.py\", line 91, in _ansiballz_main\n    invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)\n  File \"/root/.ansible/tmp/ansible-tmp-1612531498.6105824-2730-66623839143086/AnsiballZ_ec2_launch_template.py\", line 40, in invoke_module\n    runpy.run_module(mod_name='ansible_collections.community.aws.plugins.modules.ec2_launch_template', init_globals=None, run_name='__main__', alter_sys=True)\n  File \"/usr/lib64/python3.6/runpy.py\", line 205, in run_module\n    return _run_module_code(code, init_globals, run_name, mod_spec)\n  File \"/usr/lib64/python3.6/runpy.py\", line 96, in _run_module_code\n    mod_name, mod_spec, pkg_name, script_name)\n  File \"/usr/lib64/python3.6/runpy.py\", line 85, in _run_code\n    exec(code, run_globals)\n  File \"/tmp/ansible_community.aws.ec2_launch_template_payload_oqw5mjnj/ansible_community.aws.ec2_launch_template_payload.zip/ansible_collections/community/aws/plugins/modules/ec2_launch_template.py\", line 736, in <module>\n  File \"/tmp/ansible_community.aws.ec2_launch_template_payload_oqw5mjnj/ansible_community.aws.ec2_launch_template_payload.zip/ansible_collections/community/aws/plugins/modules/ec2_launch_template.py\", line 715, in main\nAttributeError: can't set attribute\n", "module_stdout": "", "msg": "MODULE FAILURE\nSee stdout/stderr for the exact error", "rc": 1}

It seems changing module.params is difficult. Any thoughts on how to proceed?

[markuman]

When scrub_none_parameters expected a single dict, there are two opportunities imho.

1. patch it like @kepstin suggest
2. use scrub_none_parameters for each element of the params list (yes, a dump and inelegant loop)

It would be nice if we can fix the launch_template module with the next release.

[wimnat]

@markuman the patch is definitely required and I will raise a PR for that.

Using scrub_none_parameters every time you access a param though is inelegant and prone to failure. It just takes one missed call for everything to fall down. It will also make patching existing modules that much harder because there will be numerous changes required throughout each module.

[markuman]

#413

[stefanhorning]

Opened a PR with a fix here #438. As #413 didn't make a difference for me.

[markuman]

solved by #413