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

[ec2_vpc_net_facts] Fix UnsupportedOperation for regions other than u… #35302

Merged
merged 3 commits into from Feb 21, 2018

Conversation

s-hertel
Copy link
Contributor

@s-hertel s-hertel commented Jan 24, 2018

…s-east-1

SUMMARY

Fixes

  tasks:
    - name: test Frankfurt VPC
      ec2_vpc_net_facts:
        profile: shertel
        region: eu-central-1
        filters:
          "tag:Name": "AnsibleTest"
Traceback (most recent call last):
  File "/var/folders/by/k8_fbl593dlctgqmwq5wzl2c0000gn/T/ansible_2ycJ8S/ansible_module_ec2_vpc_net_facts.py", line 196, in describe_vpcs
    cl_enabled = connection.describe_vpc_classic_link(VpcIds=vpc_list)
  File "/Users/shertel/Workspace/ansible/venv/python2.7/lib/python2.7/site-packages/botocore/client.py", line 317, in _api_call
    return self._make_api_call(operation_name, kwargs)
  File "/Users/shertel/Workspace/ansible/venv/python2.7/lib/python2.7/site-packages/botocore/client.py", line 615, in _make_api_call
    raise error_class(parsed_response, operation_name)
ClientError: An error occurred (UnsupportedOperation) when calling the DescribeVpcClassicLink operation: The functionality you requested is not available in this region.

After fix:

ok: [localhost] => {
    "attempts": 1,
    "changed": false,
    "failed": false,
    "invocation": {
        "module_args": {
            "aws_access_key": null,
            "aws_secret_key": null,
            "ec2_url": null,
            "filters": {
                "tag:Name": "AnsibleTest"
            },
            "profile": "shertel",
            "region": "eu-central-1",
            "security_token": null,
            "validate_certs": true,
            "vpc_ids": []
        }
    },
    "vpcs": [
        {
            "cidr_block": "10.0.0.0/16",
            "cidr_block_association_set": [
                {
                    "association_id": "vpc-cidr-assoc-40ca1928",
                    "cidr_block": "10.0.0.0/16",
                    "cidr_block_state": {
                        "state": "associated"
                    }
                }
            ],
            "classic_link_dns_supported": false,
            "classic_link_enabled": false,
            "dhcp_options_id": "dopt-d034d7b8",
            "enable_dns_hostnames": true,
            "enable_dns_support": true,
            "id": "vpc-7afb2811",
            "instance_tenancy": "default",
            "is_default": false,
            "state": "available",
            "tags": {
                "Name": "AnsibleTest"
            },
            "vpc_id": "vpc-7afb2811"
        }
    ]
}
META: ran handlers
META: ran handlers

PLAY RECAP ***********************************************************************************************************************************
localhost                  : ok=1    changed=0    unreachable=0    failed=0
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/modules/cloud/amazon/ec2_vpc_net_facts.py

ANSIBLE VERSION
2.5.0

@ansibot
Copy link
Contributor

ansibot commented Jan 24, 2018

@ansibot ansibot added aws bugfix_pull_request cloud core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jan 24, 2018
@@ -193,12 +193,18 @@ def describe_vpcs(connection, module):

# We can get these results in bulk but still needs two separate calls to the API
try:
cl_enabled = connection.describe_vpc_classic_link(VpcIds=vpc_list)
if connection._client_config.region_name != 'us-east-1':
Copy link
Contributor

Choose a reason for hiding this comment

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

just catch the unsupported operation exception and ignore it. No need to do region checking.

except botocore.exceptions.ClientError as e:
module.fail_json(msg=e.message, exception=traceback.format_exc(), **camel_dict_to_snake_dict(e.response))

try:
cl_dns_support = connection.describe_vpc_classic_link_dns_support(VpcIds=vpc_list)
if connection._client_config.region_name != 'us-east-1':
Copy link
Contributor

Choose a reason for hiding this comment

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

As above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! Also updated the exception handling for the rest of the module so it is Python 3 compatible and handles BotoCoreError.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. needs_triage Needs a first human triage before being processed. labels Jan 25, 2018
@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Feb 6, 2018
@willthames
Copy link
Contributor

The exception handling for BotoCoreError and ClientError just looks like a good time to move to AnsibleAWSModule and fail_json_aws and let that deal with the differences.

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Feb 16, 2018
@ryansb
Copy link
Contributor

ryansb commented Feb 21, 2018

I'm also eyeing this for a backport, so I'd rather not introduce a migration too since AnsibleAWSModule doesn't inherit from AnsibleModule & lacks some of the API. Instead of a migration, I like the slightly more verbose but simpler new exception handling.

@ryansb
Copy link
Contributor

ryansb commented Feb 21, 2018

rebuild_merge

@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 21, 2018
@ansibot ansibot merged commit 710db04 into ansible:devel Feb 21, 2018
s-hertel added a commit to s-hertel/ansible that referenced this pull request Feb 21, 2018
ansible#35302)

* [ec2_vpc_net_facts] Fix UnsupportedOperation for regions other than us-east-1

* Make fix more Pythonic

* Fix the exception handling for the module
ryansb pushed a commit that referenced this pull request Feb 21, 2018
#35302) (#36512)

* [ec2_vpc_net_facts] Fix UnsupportedOperation for regions other than us-east-1

* Make fix more Pythonic

* Fix the exception handling for the module
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 6, 2018
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
aws bug This issue/PR relates to a bug. cloud core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants