From 77d5813ace46ccfe54ea7cebd0d4ab7d2b487538 Mon Sep 17 00:00:00 2001
From: Ivan Chekaldin <39010411+ichekaldin@users.noreply.github.com>
Date: Sat, 2 Jul 2022 04:00:07 -0400
Subject: [PATCH] Fix idempotency and logic to update existing aggregator
 (#645)

Fix idempotency and logic to update existing aggregator

SUMMARY

describe_configuration_aggregators method returns output similar
to the following:
{
    "ConfigurationAggregators": [
        {
            "ConfigurationAggregatorName": "test-name",
            "ConfigurationAggregatorArn": "arn:aws:config:us-east-1:123412341234:config-aggregator/config-aggregator-xbxbvyq5",
            "OrganizationAggregationSource": {
                "RoleArn": "arn:aws:iam::123412341234:role/my-role",
                "AllAwsRegions": true
            },
            "CreationTime": 1619030767.047,
            "LastUpdatedTime": 1626463216.998
        }
    ]
}

As a result, lines 134-136 fail:
    del current_params['ConfigurationAggregatorArn']
    del current_params['CreationTime']
    del current_params['LastUpdatedTime']

as they try to delete attributes from the current_params as opposed
to current_params['ConfigurationAggregators'][0].
The error message is:
KeyError: 'ConfigurationAggregators'

Additionally, if no account_sources attribute is specified, the module
fails idempotency check, because in that case AccountAggregationSources
attribute is present in params, but not in current_params.

ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

aws_config_aggregator
ADDITIONAL INFORMATION

Reviewed-by: Mark Chappell <None>
---
 ..._aggregator-fix-update-and-idempotency.yml |  3 +
 plugins/modules/aws_config_aggregator.py      | 15 ++--
 .../targets/aws_config/tasks/main.yaml        | 72 +++++++++++++++++++
 3 files changed, 84 insertions(+), 6 deletions(-)
 create mode 100644 changelogs/fragments/645-aws_config_aggregator-fix-update-and-idempotency.yml

diff --git a/changelogs/fragments/645-aws_config_aggregator-fix-update-and-idempotency.yml b/changelogs/fragments/645-aws_config_aggregator-fix-update-and-idempotency.yml
new file mode 100644
index 00000000000..ed091a75976
--- /dev/null
+++ b/changelogs/fragments/645-aws_config_aggregator-fix-update-and-idempotency.yml
@@ -0,0 +1,3 @@
+bugfixes:
+  - aws_config_aggregator - Fix idempotency when ``account_sources`` parameter is not specified (https://github.com/ansible-collections/community.aws/pull/645).
+  - aws_config_aggregator - Fix `KeyError` when updating existing aggregator (https://github.com/ansible-collections/community.aws/pull/645).
diff --git a/plugins/modules/aws_config_aggregator.py b/plugins/modules/aws_config_aggregator.py
index 393413c07b9..f46f11fcafa 100644
--- a/plugins/modules/aws_config_aggregator.py
+++ b/plugins/modules/aws_config_aggregator.py
@@ -126,22 +126,25 @@ def create_resource(client, module, params, result):
 
 
 def update_resource(client, module, params, result):
+    result['changed'] = False
+
     current_params = client.describe_configuration_aggregators(
         ConfigurationAggregatorNames=[params['ConfigurationAggregatorName']]
-    )
+    )['ConfigurationAggregators'][0]
 
-    del current_params['ConfigurationAggregatorArn']
-    del current_params['CreationTime']
-    del current_params['LastUpdatedTime']
+    if params['AccountAggregationSources'] != current_params.get('AccountAggregationSources', []):
+        result['changed'] = True
+
+    if params['OrganizationAggregationSource'] != current_params.get('OrganizationAggregationSource', {}):
+        result['changed'] = True
 
-    if params != current_params['ConfigurationAggregators'][0]:
+    if result['changed']:
         try:
             client.put_configuration_aggregator(
                 ConfigurationAggregatorName=params['ConfigurationAggregatorName'],
                 AccountAggregationSources=params['AccountAggregationSources'],
                 OrganizationAggregationSource=params['OrganizationAggregationSource']
             )
-            result['changed'] = True
             result['aggregator'] = camel_dict_to_snake_dict(resource_exists(client, module, params))
             return result
         except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:
diff --git a/tests/integration/targets/aws_config/tasks/main.yaml b/tests/integration/targets/aws_config/tasks/main.yaml
index 686b4dc91aa..b4c1bf4ab3e 100644
--- a/tests/integration/targets/aws_config/tasks/main.yaml
+++ b/tests/integration/targets/aws_config/tasks/main.yaml
@@ -201,6 +201,36 @@
         that:
           - output.changed
 
+    - name: Create aws_config_aggregator
+      aws_config_aggregator:
+        name: random_name
+        state: present
+        account_sources: []
+        organization_source:
+          all_aws_regions: true
+          role_arn: "{{ config_iam_role.arn }}"
+      register: output
+
+    - name: assert success
+      assert:
+        that:
+           - output is changed
+
+    - name: Create aws_config_aggregator - idempotency
+      aws_config_aggregator:
+        name: random_name
+        state: present
+        account_sources: []
+        organization_source:
+          all_aws_regions: true
+          role_arn: "{{ config_iam_role.arn }}"
+      register: output
+
+    - name: assert not changed
+      assert:
+        that:
+           - output is not changed
+
     # ============================================================
     # Update testing
     # ============================================================
@@ -250,6 +280,40 @@
         that:
           - output.changed
 
+    - name: Update aws_config_aggregator
+      aws_config_aggregator:
+        name: random_name
+        state: present
+        account_sources: []
+        organization_source:
+          all_aws_regions: false
+          aws_regions:
+            - '{{ aws_region }}'
+          role_arn: "{{ config_iam_role.arn }}"
+      register: output
+
+    - name: assert success
+      assert:
+        that:
+           - output is changed
+
+    - name: Update aws_config_aggregator - idempotency
+      aws_config_aggregator:
+        name: random_name
+        state: present
+        account_sources: []
+        organization_source:
+          all_aws_regions: false
+          aws_regions:
+            - '{{ aws_region }}'
+          role_arn: "{{ config_iam_role.arn }}"
+      register: output
+
+    - name: assert success
+      assert:
+        that:
+           - output is not changed
+
     # ============================================================
     # Read testing
     # ============================================================
@@ -300,6 +364,14 @@
           - not output.changed
 
   always:
+
+    - name: delete aws_config_aggregator
+      aws_config_aggregator:
+        name: random_name
+        state: absent
+      register: output
+      ignore_errors: true
+
     # ============================================================
     # Destroy testing
     # ============================================================