Fix idempotency and logic to update existing aggregator #645

ichekaldin · 2021-07-16T19:39:07Z

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

ansibullbot · 2021-07-16T19:44:20Z

cc @jillr @markuman @s-hertel @slapula @tremble @wimnat
click here for bot help

ichekaldin · 2021-07-19T22:27:27Z

I see that ansible/check failed two tests:

ansible-galaxy-importer - I can't clearly see what the error is.
ansible-test-cloud-integration-aws-py36_2 - the error is:

ERROR: The 1 integration test(s) listed below (out of 20) failed. See error output above for details:
ec2_vpc_endpoint_service_info

I haven't modified any files related to ec2_vpc_endpoint_service_info as part of this PR.

Am I missing anything obvious?

alinabuzachis · 2021-07-20T15:43:26Z

recheck

ichekaldin · 2021-08-10T13:19:41Z

@alinabuzachis It looks like the tests fail with the same errors.

tremble · 2021-08-10T13:22:45Z

@ichekaldin Sorry about that, the move to Zuul caused all sorts of fun.

tremble · 2021-08-10T13:36:55Z

@ichekaldin While they're not run in CI due to constraints with AWS Config (eg. you can't have multiple aggregators) There are some basic integration tests for aws_config (including the aggregator)

https://github.com/ansible-collections/community.aws/blob/main/tests/integration/targets/aws_config/tasks/main.yaml

Please could you update the tests to include idempotency tests and some regression tests for the issues you hit.

ichekaldin · 2021-08-10T18:46:47Z

@tremble I added a test to validate that specifying empty list of accounts and the organization source works:

    - name: test resource_type configuration_aggregator with organization_source parameter
      aws_config_aggregator:
        name: random_name
        state: present
        account_sources: []
        organization_source:
          all_aws_regions: true
          role_arn: "{{ config_iam_role.arn }}"
      register: output
      ignore_errors: true

Would that be sufficient?

ichekaldin · 2021-08-17T15:47:09Z

@tremble Sorry to keep bumping this - I'd love for this fix to get merged before the next release. What do you think about the test I added?

tremble

Thanks for your submission.

account_sources and organization_source are both currently required parameters (line 167/168) so on its own this change doesn't make sense.

Additionally, the integration test resulted in exceptions when I tried to run it and I'm not sure it's the right test.

tremble · 2021-10-22T15:21:24Z

plugins/modules/aws_config_aggregator.py

@@ -14,6 +14,7 @@
 short_description: Manage AWS Config aggregations across multiple accounts
 description:
    - Module manages AWS Config resources
+requirements: [ 'botocore', 'boto3' ]


Suggested change

requirements: [ 'botocore', 'boto3' ]

The botocore/boto3 requirements are automatically added.

tremble · 2021-10-22T15:46:33Z

tests/integration/targets/aws_config/tasks/main.yaml

+      assert:
+        that:
+           - output.failed
+           - 'output.msg.startswith("missing required arguments: organization_source")'


This test looks wrong...

TASK [aws_config : test resource_type configuration_aggregator with organization_source parameter] **************************************************************************************************************** task path: /root/ansible_collections/community/aws/tests/output/.tmp/integration/aws_config-z_x25kln-ÅÑŚÌβŁÈ/tests/integration/targets/aws_config/tasks/main.yaml:156 Using module file /root/ansible_collections/community/aws/plugins/modules/aws_config_aggregator.py Pipelining is enabled. <testhost> ESTABLISH LOCAL CONNECTION FOR USER: root <testhost> EXEC /bin/sh -c 'ANSIBLE_DEBUG_BOTOCORE_LOGS=True /usr/bin/python3.10 && sleep 0' The full traceback is: Traceback (most recent call last): File "<stdin>", line 121, in <module> File "<stdin>", line 113, in _ansiballz_main File "<stdin>", line 61, in invoke_module File "/usr/lib/python3.10/runpy.py", line 209, in run_module return _run_module_code(code, init_globals, run_name, mod_spec) File "/usr/lib/python3.10/runpy.py", line 96, in _run_module_code _run_code(code, mod_globals, init_globals, File "/usr/lib/python3.10/runpy.py", line 86, in _run_code exec(code, run_globals) File "/tmp/ansible_aws_config_aggregator_payload_lfs1q8or/ansible_aws_config_aggregator_payload.zip/ansible_collections/community/aws/plugins/modules/aws_config_aggregator.py", line 228, in <module> File "/tmp/ansible_aws_config_aggregator_payload_lfs1q8or/ansible_aws_config_aggregator_payload.zip/ansible_collections/community/aws/plugins/modules/aws_config_aggregator.py", line 216, in main File "/tmp/ansible_aws_config_aggregator_payload_lfs1q8or/ansible_aws_config_aggregator_payload.zip/ansible_collections/community/aws/plugins/modules/aws_config_aggregator.py", line 119, in create_resource KeyError: 'AccountAggregationSources' fatal: [testhost]: FAILED! => { "changed": false, "module_stderr": "Traceback (most recent call last):\n File \"<stdin>\", line 121, in <module>\n File \"<stdin>\", line 113, in _ansiballz_main\n File \"<stdin>\", line 61, in invoke_module\n File \"/usr/lib/python3.10/runpy.py\", line 209, in run_module\n return _run_module_code(code, init_globals, run_name, mod_spec)\n File \"/usr/lib/python3.10/runpy.py\", line 96, in _run_module_code\n _run_code(code, mod_globals, init_globals,\n File \"/usr/lib/python3.10/runpy.py\", line 86, in _run_code\n exec(code, run_globals)\n File \"/tmp/ansible_aws_config_aggregator_payload_lfs1q8or/ansible_aws_config_aggregator_payload.zip/ansible_collections/community/aws/plugins/modules/aws_config_aggregator.py\", line 228, in <module>\n File \"/tmp/ansible_aws_config_aggregator_payload_lfs1q8or/ansible_aws_config_aggregator_payload.zip/ansible_collections/community/aws/plugins/modules/aws_config_aggregator.py\", line 216, in main\n File \"/tmp/ansible_aws_config_aggregator_payload_lfs1q8or/ansible_aws_config_aggregator_payload.zip/ansible_collections/community/aws/plugins/modules/aws_config_aggregator.py\", line 119, in create_resource\nKeyError: 'AccountAggregationSources'\n", "module_stdout": "", "msg": "MODULE FAILURE\nSee stdout/stderr for the exact error", "rc": 1 } ...ignoring TASK [aws_config : assert failure when called with organization_source parameter] ********************************************************************************************************************************* task path: /root/ansible_collections/community/aws/tests/output/.tmp/integration/aws_config-z_x25kln-ÅÑŚÌβŁÈ/tests/integration/targets/aws_config/tasks/main.yaml:167 fatal: [testhost]: FAILED! => { "assertion": "output.msg.startswith(\"missing required arguments: organization_source\")", "changed": false, "evaluated_to": false, "msg": "Assertion failed" }

ansibullbot · 2021-10-22T16:42:57Z

@ichekaldin this PR contains the following merge commits:

7064574

Please rebase your branch to remove these commits.

click here for bot help

tremble · 2021-10-22T16:44:25Z

tests/integration/targets/aws_config/tasks/main.yaml

           - output.failed
-           - 'output.msg.startswith("missing required arguments: organization_source")'
+           - output.changed


The aggregator doesn't exist, so it'll refuse to create due to account_sources being empty

ichekaldin · 2021-10-22T20:51:10Z

@tremble Thank you! I made some changes to new vs existing parameters are compared.

) [4.0.0] Bump minimum botocore version to 1.20.0 SUMMARY With the next major version we can bump botocore/boto3 again. Since 1.20.0 is now over a year old, we can bump the minimum version in preparation for 4.0.0. CI should still test backports against the relevant versions for the backported release. 1.20.0 was released 2021-02-02. 1.21.0 was released 2021-07-15, hopefully we'll release 4.0.0 before July. Should we release after mid-July we can always bump again. (Mentioned in ansible-collections#645) ISSUE TYPE Feature Pull Request COMPONENT NAME requirements.txt ADDITIONAL INFORMATION botocore] $ git show 1.20.0 tag 1.20.0 Tagger: aws-sdk-python-automation <[email protected]> Date: Tue Feb 2 19:11:44 2021 +0000 Tagging 1.20.0 release. commit b7d27dc39aea82e22e2c11443fbd02a4904367cd (tag: 1.20.0) Merge: cc497a593 27ebea65f Author: aws-sdk-python-automation <[email protected]> Date: Tue Feb 2 19:11:44 2021 +0000 Merge branch 'release-1.20.0' * release-1.20.0: Bumping version to 1.20.0 Update to latest models Add changelog for custom endpoints and ARN resources Allow custom endpoints when addressing ARN resources Add changes for Python 3.4/3.5 removal Fall back to Transfer-Encoding 'chunked' if AWSRequest body is not seekable stream boto3] $ git show 1.17.0 tag 1.17.0 Tagger: aws-sdk-python-automation <[email protected]> Date: Tue Feb 2 19:11:35 2021 +0000 Tagging 1.17.0 release. commit 1a35ed1ab41f967ea43420650075f2693cbbe08b (tag: 1.17.0) Merge: d77d24f9 35454bc5 Author: aws-sdk-python-automation <[email protected]> Date: Tue Feb 2 19:11:35 2021 +0000 Merge branch 'release-1.17.0' * release-1.17.0: Bumping version to 1.17.0 Add changelog entries from botocore Add S3 VPCE examples Add changes for Python 3.4/3.5 removal Reviewed-by: Alina Buzachis <None>

`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`.

…ters

tremble

Basic tests seem to show it's working

tremble · 2022-07-02T07:10:14Z

@ichekaldin Sorry it took so long to get back to this review.

softwarefactory-project-zuul · 2022-07-02T07:25:02Z

Build succeeded.

softwarefactory-project-zuul · 2022-07-02T08:00:06Z

Build succeeded (gate pipeline).

patchback · 2022-07-02T08:00:10Z

Backport to stable-3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-3/77d5813ace46ccfe54ea7cebd0d4ab7d2b487538/pr-645

Backported as #1296

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

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> (cherry picked from commit 77d5813)

patchback · 2022-07-02T08:00:19Z

Backport to stable-4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4/77d5813ace46ccfe54ea7cebd0d4ab7d2b487538/pr-645

Backported as #1297

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

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> (cherry picked from commit 77d5813)

[PR #645/77d5813a backport][stable-3] Fix idempotency and logic to update existing aggregator This is a backport of PR #645 as merged into main (77d5813). 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>

[PR #645/77d5813a backport][stable-4] Fix idempotency and logic to update existing aggregator This is a backport of PR #645 as merged into main (77d5813). 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>

ansibullbot added bug This issue/PR relates to a bug community_review module module needs_triage plugins plugin (any type) labels Jul 16, 2021

ansibullbot removed the needs_triage label Aug 10, 2021

ansibullbot added integration tests/integration tests tests labels Aug 10, 2021

tremble requested changes Oct 22, 2021

View reviewed changes

ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed community_review labels Oct 22, 2021

tremble reviewed Oct 22, 2021

View reviewed changes

ansibullbot added community_review and removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Oct 22, 2021

ichekaldin requested a review from tremble November 18, 2021 17:03

ichekaldin added 2 commits July 2, 2022 08:32

Create a test for config aggregator with organization source

7756f13

ichekaldin and others added 3 commits July 2, 2022 08:32

Explicitly compare organization_source and account_sources parame…

0f37c1d

…ters

Fix indentation

399ac82

Tweak integration tests

28da480

tremble approved these changes Jul 2, 2022

View reviewed changes

tremble added mergeit Merge the PR (SoftwareFactory) backport-4 PR should be backported to the stable-4 branch backport-3 PR should be backported to the stable-3 branch labels Jul 2, 2022

softwarefactory-project-zuul bot merged commit 77d5813 into ansible-collections:main Jul 2, 2022

patchback bot mentioned this pull request Jul 2, 2022

[PR #645/77d5813a backport][stable-3] Fix idempotency and logic to update existing aggregator #1296

Merged

patchback bot mentioned this pull request Jul 2, 2022

[PR #645/77d5813a backport][stable-4] Fix idempotency and logic to update existing aggregator #1297

Merged

ichekaldin deleted the aws_config_aggregator_idempotency branch July 2, 2022 14:58

renovate bot mentioned this pull request Aug 8, 2022

chore(deps): update dependency community.aws to v4.1.1 linxside/ansible-ee#44

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix idempotency and logic to update existing aggregator #645

Fix idempotency and logic to update existing aggregator #645

ichekaldin commented Jul 16, 2021

ansibullbot commented Jul 16, 2021

ichekaldin commented Jul 19, 2021

alinabuzachis commented Jul 20, 2021

ichekaldin commented Aug 10, 2021

tremble commented Aug 10, 2021

tremble commented Aug 10, 2021

ichekaldin commented Aug 10, 2021

ichekaldin commented Aug 17, 2021

tremble left a comment

tremble Oct 22, 2021

tremble Oct 22, 2021

tremble Oct 22, 2021

ansibullbot commented Oct 22, 2021

tremble Oct 22, 2021

ichekaldin commented Oct 22, 2021

tremble left a comment

tremble commented Jul 2, 2022

softwarefactory-project-zuul bot commented Jul 2, 2022

softwarefactory-project-zuul bot commented Jul 2, 2022

patchback bot commented Jul 2, 2022 •

edited

Loading

patchback bot commented Jul 2, 2022 •

edited

Loading

Fix idempotency and logic to update existing aggregator #645

Fix idempotency and logic to update existing aggregator #645

Conversation

ichekaldin commented Jul 16, 2021

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibullbot commented Jul 16, 2021

ichekaldin commented Jul 19, 2021

alinabuzachis commented Jul 20, 2021

ichekaldin commented Aug 10, 2021

tremble commented Aug 10, 2021

tremble commented Aug 10, 2021

ichekaldin commented Aug 10, 2021

ichekaldin commented Aug 17, 2021

tremble left a comment

Choose a reason for hiding this comment

tremble Oct 22, 2021

Choose a reason for hiding this comment

tremble Oct 22, 2021

Choose a reason for hiding this comment

tremble Oct 22, 2021

Choose a reason for hiding this comment

ansibullbot commented Oct 22, 2021

tremble Oct 22, 2021

Choose a reason for hiding this comment

ichekaldin commented Oct 22, 2021

tremble left a comment

Choose a reason for hiding this comment

tremble commented Jul 2, 2022

softwarefactory-project-zuul bot commented Jul 2, 2022

softwarefactory-project-zuul bot commented Jul 2, 2022

patchback bot commented Jul 2, 2022 • edited Loading

Backport to stable-3: 💚 backport PR created

patchback bot commented Jul 2, 2022 • edited Loading

Backport to stable-4: 💚 backport PR created

patchback bot commented Jul 2, 2022 •

edited

Loading

patchback bot commented Jul 2, 2022 •

edited

Loading