Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cloudfront_info pagination bug #780

Merged

Conversation

marknet15
Copy link
Contributor

SUMMARY

Currently the cloudfront_info module is using a bespoke paginator function, which is causing some issues when over 100 distributions exist (see linked issue), when in fact it can be easily switched to a native boto3 paginator that would fix the bug reported in the linked issue.

Fixes #769

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

cloudfront_info

ADDITIONAL INFORMATION

Pagination docs: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/cloudfront.html#paginators

Tests following steps from linked issue:
ran a ansible task:

- hosts: localhost
  connection: local
  gather_facts: false
  tasks:
    - name: "Check if distribution already exists"
      cloudfront_info:
        distribution: true
        domain_name_alias: "XX"
      register: distribution_data

Results:

TASK [Check if distribution already exists] ************************************************************************************************************************************************************************************************************************************
task path: /root/test/test.yml:6
<127.0.0.1> ESTABLISH LOCAL CONNECTION FOR USER: root
<127.0.0.1> EXEC /bin/sh -c 'echo ~root && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /root/.ansible/tmp `"&& mkdir "` echo /root/.ansible/tmp/ansible-tmp-1635164453.7083595-389-139483031845166 `" && echo ansible-tmp-1635164453.7083595-389-139483031845166="` echo /root/.ansible/tmp/ansible-tmp-1635164453.7083595-389-139483031845166 `" ) && sleep 0'
Using module file /root/test/library/cloudfront_info.py
<127.0.0.1> PUT /root/.ansible/tmp/ansible-local-3861wn9tjv7/tmp6o0jv2s2 TO /root/.ansible/tmp/ansible-tmp-1635164453.7083595-389-139483031845166/AnsiballZ_cloudfront_info.py
<127.0.0.1> EXEC /bin/sh -c 'chmod u+x /root/.ansible/tmp/ansible-tmp-1635164453.7083595-389-139483031845166/ /root/.ansible/tmp/ansible-tmp-1635164453.7083595-389-139483031845166/AnsiballZ_cloudfront_info.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '/usr/bin/python3 /root/.ansible/tmp/ansible-tmp-1635164453.7083595-389-139483031845166/AnsiballZ_cloudfront_info.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c 'rm -f -r /root/.ansible/tmp/ansible-tmp-1635164453.7083595-389-139483031845166/ > /dev/null 2>&1 && sleep 0'
ok: [localhost] => {
    "changed": false,
    "cloudfront": {
        "XXXXXX": {
            "Distribution": {
                "ARN": "arn:aws:cloudfront::XXXXXXXX:distribution/XXXXXX",
                "ActiveTrustedKeyGroups": {
                    "Enabled": false,
                    "Quantity": 0
                },
                "ActiveTrustedSigners": {
                    "Enabled": false,
                    "Quantity": 0
                },
                "AliasICPRecordals": [
                    {
                        "CNAME": "XXXX",
                        "ICPRecordalStatus": "APPROVED"
                    }
                ],
...

@ansibullbot
Copy link

@ansibullbot ansibullbot added bug This issue/PR relates to a bug community_review module module needs_triage plugins plugin (any type) python3 labels Oct 25, 2021
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for digging into this.

I really like the use of boto3's pagination rather than something home-brew, however the decorator won't be able to do its thing without a minor tweak to the approach.

By catching ClientError and BotoCoreError within the methods the jittered backoff decorator never sees the rate limiting exceptions and thus can't retry the calls. To work around this I usually move just the pagination into a dedicated method, which can be wrapped, and the rest of the function (exception handling and all) can be done similar to the PR.

plugins/modules/cloudfront_info.py Show resolved Hide resolved
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor nit in the changelog, but otherwise LGTM

Co-authored-by: Mark Chappell <mchappel@redhat.com>
Copy link
Member

@markuman markuman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tremble tremble added the gate label Oct 25, 2021
Copy link
Contributor

@ansible-zuul ansible-zuul bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ansible-zuul ansible-zuul bot merged commit 498e0f0 into ansible-collections:main Oct 25, 2021
@marknet15 marknet15 deleted the fix-cloudfront_info-bug branch October 26, 2021 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug community_review module module plugins plugin (any type) python3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] self.paginated_response does not return the aggregated list of distributions (cloudfront_info)
4 participants