Backup plan params bugfix (#1611) (#1620)

[PR #1611/ad953e83 backport][stable-6] Backup plan params bugfix

This is a backport of PR #1611 as merged into main (ad953e8).
SUMMARY
This updates the backup_plan module to remove all None values from nested dicts in supplied params. Previously nested None values were retained and caused errors when sent through to the boto3 client calls.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
backup_plan
ADDITIONAL INFORMATION
Creating or updating a plan without providing an optional value in a nested dict would result in that value being set to None, and it wasn't being removed. For example:
    - name: Update Backup plan
      amazon.aws.backup_plan:
        backup_plan_name: my-backup-plan
        rules:
          - rule_name: my-rule
            target_backup_vault_name: my-vault
            schedule_expression: "cron(0 * ? * * *)"
            lifecycle:
              move_to_cold_storage_after_days: 30

The optional lifecycle.delete_after_days option in the above example would be set to None by default, and the removal of None values wasn't recursively going through the entire dict. This is now fixed and the above example will work.

Reviewed-by: Alina Buzachis

changelogs/fragments/202306012-backup_plan-remove-none-from-nested-params.yml

@@ -0,0 +1,3 @@
+---
+bugfixes:
+  - backup_plan - Use existing `scrub_none_values` function from module_utils to remove None values from nested dicts in supplied params. Nested None values were being retained and causing an error when sent through to the boto3 client operation (https://github.com/ansible-collections/amazon.aws/pull/1611).

plugins/modules/backup_plan.py

@@ -257,9 +257,10 @@
 
 from ansible.module_utils.common.dict_transformations import camel_dict_to_snake_dict
 from ansible.module_utils.common.dict_transformations import snake_dict_to_camel_dict
-from ansible_collections.amazon.aws.plugins.module_utils.tagging import compare_aws_tags
 from ansible_collections.amazon.aws.plugins.module_utils.backup import get_plan_details
 from ansible_collections.amazon.aws.plugins.module_utils.modules import AnsibleAWSModule
+from ansible_collections.amazon.aws.plugins.module_utils.tagging import compare_aws_tags
+from ansible_collections.amazon.aws.plugins.module_utils.transformation import scrub_none_parameters
 
 try:
     from botocore.exceptions import ClientError, BotoCoreError
@@ -541,10 +542,9 @@ def main():
     plan_name = module.params["backup_plan_name"]
     plan = {
         "backup_plan_name": module.params["backup_plan_name"],
-        "rules": [{k: v for k, v in rule.items() if v is not None} for rule in module.params["rules"] or []],
+        "rules": [scrub_none_parameters(rule) for rule in module.params["rules"] or []],
         "advanced_backup_settings": [
-            {k: v for k, v in setting.items() if v is not None}
-            for setting in module.params["advanced_backup_settings"] or []
+            scrub_none_parameters(setting) for setting in module.params["advanced_backup_settings"] or []
         ],
     }
     tags = module.params["tags"]

tests/integration/targets/backup_plan/tasks/main.yml

@@ -119,8 +119,8 @@
             copy_actions:
               - destination_backup_vault_arn: "{{ backup_vault_create_result.vault.backup_vault_arn }}"
                 lifecycle:
-                  move_to_cold_storage_after_days: 90
                   delete_after_days: 300
+                  move_to_cold_storage_after_days: 90
         tags:
           status: archive
       register: backup_plan_update_result
@@ -137,6 +137,133 @@
           - backup_plan_update_result.backup_plan.rules != backup_plan_info.backup_plans[0].backup_plan.rules
           - backup_plan_update_result.tags != backup_plan_info.backup_plans[0].tags
 
+    - name: Get updated backup plan details
+      amazon.aws.backup_plan_info:
+        backup_plan_names:
+          - "{{ backup_plan_name }}"
+      register: backup_plan_info
+
+    - name: Update backup plan without nested optional values in check mode
+      amazon.aws.backup_plan:
+        backup_plan_name: "{{ backup_plan_name }}"
+        rules:
+          - rule_name: hourly
+            target_backup_vault_name: "{{ backup_vault_name }}"
+            schedule_expression: "cron(0 * ? * * *)"
+            start_window_minutes: 60
+            completion_window_minutes: 150
+            lifecycle:
+              delete_after_days: 120
+            recovery_point_tags:
+              type: hourly_backup
+            copy_actions:
+              - destination_backup_vault_arn: "{{ backup_vault_create_result.vault.backup_vault_arn }}"
+                lifecycle:
+                  move_to_cold_storage_after_days: 90
+        tags:
+          status: archive
+      check_mode: true
+      register: check_mode_update_without_nested_optional_values_result
+
+    - name: Verify backup plan update without nested optional values in check mode result
+      ansible.builtin.assert:
+        that:
+          - check_mode_update_without_nested_optional_values_result.exists is true
+          - check_mode_update_without_nested_optional_values_result.changed is true
+          - check_mode_update_without_nested_optional_values_result.backup_plan.rules != backup_plan_info.backup_plans[0].backup_plan.rules
+
+    - name: Get backup plan details after update in check mode
+      amazon.aws.backup_plan_info:
+        backup_plan_names:
+          - "{{ backup_plan_name }}"
+      register: backup_plan_info_after_check_mode_update
+
+    - name: Verify backup plan was not actually updated in check mode
+      ansible.builtin.assert:
+        that:
+          - backup_plan_info_after_check_mode_update.backup_plans[0] == backup_plan_info.backup_plans[0]
+
+    - name: Update backup plan without nested optional values
+      amazon.aws.backup_plan:
+        backup_plan_name: "{{ backup_plan_name }}"
+        rules:
+          - rule_name: hourly
+            target_backup_vault_name: "{{ backup_vault_name }}"
+            schedule_expression: "cron(0 * ? * * *)"
+            start_window_minutes: 60
+            completion_window_minutes: 150
+            lifecycle:
+              delete_after_days: 120
+            recovery_point_tags:
+              type: hourly_backup
+            copy_actions:
+              - destination_backup_vault_arn: "{{ backup_vault_create_result.vault.backup_vault_arn }}"
+                lifecycle:
+                  move_to_cold_storage_after_days: 90
+        tags:
+          status: archive
+      register: update_without_nested_optional_values_result
+
+    - name: Verify backup plan update without nested optional values result
+      ansible.builtin.assert:
+        that:
+          - update_without_nested_optional_values_result.exists is true
+          - update_without_nested_optional_values_result.changed is true
+          - update_without_nested_optional_values_result.backup_plan_id == backup_plan_info.backup_plans[0].backup_plan_id
+          - update_without_nested_optional_values_result.backup_plan_arn == backup_plan_info.backup_plans[0].backup_plan_arn
+          - update_without_nested_optional_values_result.creation_date != backup_plan_info.backup_plans[0].creation_date
+          - update_without_nested_optional_values_result.version_id != backup_plan_info.backup_plans[0].version_id
+          - update_without_nested_optional_values_result.backup_plan.rules != backup_plan_info.backup_plans[0].backup_plan.rules
+          - update_without_nested_optional_values_result.tags == backup_plan_info.backup_plans[0].tags
+
+    - name: Get updated backup plan details
+      amazon.aws.backup_plan_info:
+        backup_plan_names:
+          - "{{ backup_plan_name }}"
+      register: updated_backup_plan_info
+
+    - name: Verify backup plan was actually updated
+      ansible.builtin.assert:
+        that:
+          - updated_backup_plan_info.backup_plans[0].backup_plan_name == backup_plan_info.backup_plans[0].backup_plan_name
+          - updated_backup_plan_info.backup_plans[0].backup_plan_arn == backup_plan_info.backup_plans[0].backup_plan_arn
+          - updated_backup_plan_info.backup_plans[0].version_id != backup_plan_info.backup_plans[0].version_id
+          - updated_backup_plan_info.backup_plans[0].backup_plan.rules != backup_plan_info.backup_plans[0].backup_plan.rules
+          - updated_backup_plan_info.backup_plans[0].tags == backup_plan_info.backup_plans[0].tags
+
+    - name: Update backup plan without nested optional values - idempotency
+      amazon.aws.backup_plan:
+        backup_plan_name: "{{ backup_plan_name }}"
+        rules:
+          - rule_name: hourly
+            target_backup_vault_name: "{{ backup_vault_name }}"
+            schedule_expression: "cron(0 * ? * * *)"
+            start_window_minutes: 60
+            completion_window_minutes: 150
+            lifecycle:
+              delete_after_days: 120
+            recovery_point_tags:
+              type: hourly_backup
+            copy_actions:
+              - destination_backup_vault_arn: "{{ backup_vault_create_result.vault.backup_vault_arn }}"
+                lifecycle:
+                  move_to_cold_storage_after_days: 90
+        tags:
+          status: archive
+      register: update_without_nested_optional_values_idempotency_result
+
+    - name: Verify backup plan update without nested optional values idempotency result
+      ansible.builtin.assert:
+        that:
+          - update_without_nested_optional_values_idempotency_result.exists is true
+          - update_without_nested_optional_values_idempotency_result.changed is false
+          - update_without_nested_optional_values_idempotency_result.backup_plan_id == updated_backup_plan_info.backup_plans[0].backup_plan_id
+          - update_without_nested_optional_values_idempotency_result.backup_plan_arn == updated_backup_plan_info.backup_plans[0].backup_plan_arn
+          - update_without_nested_optional_values_idempotency_result.creation_date == updated_backup_plan_info.backup_plans[0].creation_date
+          - update_without_nested_optional_values_idempotency_result.version_id == updated_backup_plan_info.backup_plans[0].version_id
+          - update_without_nested_optional_values_idempotency_result.backup_plan.rules == updated_backup_plan_info.backup_plans[0].backup_plan.rules
+          - update_without_nested_optional_values_idempotency_result.tags == updated_backup_plan_info.backup_plans[0].tags
+
     - name: Delete backup plan in check mode
       amazon.aws.backup_plan:
         backup_plan_name: "{{ backup_plan_name }}"