Repository: ansible/ansible
Pull request: add boto3_tag_list_to_ansible_dict to ec2_vpc_peering_facts.py, and parameter checking to ec2_vpc_peer.py
##### SUMMARY
1) Alter ec2_vpc_peering_facts.py to use boto3_tag_list_to_ansible_dict so tag names don't get altered unexpectedly from the camel_dict_to_snake_dict function that gets used.

I just took code from modules/cloud/amazon/ec2_instance_facts.py and put it in ec2_vpc_peering_facts.py.

Fixes #52305 

2) ec2_vpc_peer.py has unhelpful error messages when you are missing a parameter. This is easily fixable by doing some parameter checking in main(). I copied a similar format from ec2.py to do this error checking.

Fixes #52333
##### ISSUE TYPE
- Feature Pull Request

##### COMPONENT NAME
modules/cloud/amazon/ec2_vpc_peering_facts.py
modules/cloud/amazon/ec2_vpc_peer_facts.py

##### ADDITIONAL INFORMATION
AWS tag names and values would get altered from the camel_dict_to_snake_dict function in main(). This makes querying for information incorrect. In ec2_instance_facts.py, boto3_tag_list_to_ansible_dict is used to solve this problem. Adding it to ec2_vpc_peering_facts.py would solve the problem for this file as well.


Problem statement:
Alter ec2_vpc_peering_facts.py to use boto3_tag_list_to_ansible_dict
##### SUMMARY
Alter ec2_vpc_peering_facts.py to use boto3_tag_list_to_ansible_dict so tag names don't get altered unexpectedly from the camel_dict_to_snake_dict function that gets used.

##### ISSUE TYPE
- Feature Idea

##### COMPONENT NAME
modules/cloud/amazon/ec2_vpc_peering_facts.py

##### ADDITIONAL INFORMATION
AWS tag names and values would get altered from the camel_dict_to_snake_dict function in main(). This makes querying for information incorrect. In ec2_instance_facts.py,  boto3_tag_list_to_ansible_dict is used to solve this problem. Adding it to ec2_vpc_peering_facts.py would solve the problem for this file as well.

ec2_vpc_peer.py has unhelpful error message
##### SUMMARY
ec2_vpc_peer.py doesn't have parameter checking for its various uses, so error messages are relatively unhelpful. There should be parameter checking in main() before it calls its functions to accommodate this.

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
modules/cloud/amazon/ec2_vpc_peer.py

##### ANSIBLE VERSION
```
irrelevant
```

##### CONFIGURATION
```
irrelevant
```

##### OS / ENVIRONMENT
Linux (but irrelevant)

##### STEPS TO REPRODUCE
- name: Missing vpc_id
   aws_access_key: xxxxxxxxx
   aws_secret_key: xxxxxxxxx
   peer_vpc_id: xxxxxxx
   state: create
   
   

<!--- Paste example playbooks or commands between quotes below -->
```
- name: Missing vpc_id
   aws_access_key: xxxxxxxxx
   aws_secret_key: xxxxxxxxx
   peer_vpc_id: xxxxxxx
   state: create
```

##### EXPECTED RESULTS
fatal: [localhost]: FAILED! => {"changed" false, "failed": true, "msg": "vpc_id and peering_id are required to create peering connection"}


##### ACTUAL RESULTS
```
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: Invalid type for parameter Filters[1].Values[0], value: None, type: <type 'NoneType'>, valid types: <type 'basestring'>
fatal: [localhost]: FAILED! => {"changed": false, "failed": true, "module_stderr": "Traceback (most recent call last):\n  File \"/tmp/ansible_8K4qEm/ansible_module_ec2_vpc_peer.py\", line 398, in <module>\n    main()\n  File \"/tmp/ansible_8K4qEm/ansible_module_ec2_vpc_peer.py\", line 388, in main\n    (changed, results) = create_peer_connection(client, module)\n  File \"/tmp/ansible_8K4qEm/ansible_module_ec2_vpc_peer.py\", line 262, in create_peer_connection\n    peering_conns = describe_peering_connections(params, client)\n  File \"/tmp/ansible_8K4qEm/ansible_module_ec2_vpc_peer.py\", line 233, in describe_peering_connections\n    {'Name': 'accepter-vpc-info.vpc-id', 'Values': [params['PeerVpcId']]}\n  File \"/usr/lib/python2.6/site-packages/botocore/client.py\", line 314, in _api_call\n    return self._make_api_call(operation_name, kwargs)\n  File \"/usr/lib/python2.6/site-packages/botocore/client.py\", line 586, in _make_api_call\n    api_params, operation_model, context=request_context)\n  File \"/usr/lib/python2.6/site-packages/botocore/client.py\", line 621, in _convert_to_request_dict\n    api_params, operation_model)\n  File \"/usr/lib/python2.6/site-packages/botocore/validate.py\", line 291, in serialize_to_request\n    raise ParamValidationError(report=report.generate_report())\nbotocore.exceptions.ParamValidationError: Parameter validation failed:\nInvalid type for parameter Filters[0].Values[0], value: None, type: <type 'NoneType'>, valid types: <type 'basestring'>\nInvalid type for parameter Filters[1].Values[0], value: None, type: <type 'NoneType'>, valid types: <type 'basestring'>\n", "module_stdout": "", "msg": "MODULE FAILURE", "rc": 1}
```

Hints from issue discussion:
Files identified in the description:
* [lib/ansible/modules/cloud/amazon/ec2_vpc_peering_facts.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/amazon/ec2_vpc_peering_facts.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @Etherdaemon @willthames
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
@bbui1997, just so you are aware we have a dedicated Working Group for aws.
You can find other people interested in this in `#ansible-aws` on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: community_workgroups --->

Commit message: Add parameter checking to ec2_vpc_peer and give helpful error message

Patch to review:
diff --git a/lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py b/lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py
index 58fabe6c3a0f81..c8dc1d02dc8bc6 100644
--- a/lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py
+++ b/lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py
@@ -408,6 +408,9 @@ def main():
     if not HAS_BOTO3:
         module.fail_json(msg='json, botocore and boto3 are required.')
     state = module.params.get('state')
+    peering_id = modules.params.get('peering_id')
+    vpc_id = modules.params.get('vpc_id')
+    peer_vpc_id = module.params.get('peer_vpc_id')
     try:
         region, ec2_url, aws_connect_kwargs = get_aws_connection_info(module, boto3=True)
         client = boto3_conn(module, conn_type='client', resource='ec2',
@@ -416,11 +419,20 @@ def main():
         module.fail_json(msg="Can't authorize connection - " + str(e))
 
     if state == 'present':
+        if not vpc_id and not peer_vpc_id:
+            module.fail_json(msg='vpc_id and peer_vpc_id parameters are required for creating peering connection')
+
         (changed, results) = create_peer_connection(client, module)
         module.exit_json(changed=changed, peering_id=results)
     elif state == 'absent':
+        if not peering_id or (not vpc_id and not peer_vpc_id):
+            module.fail_json(msg='peering_id or vpc_id and peer_vpc_id parameters are required for removing peering connection')
+
         remove_peer_connection(client, module)
     else:
+        if not peering_id:
+            module.fail_json(msg='peering_id parameter is required to accept or reject peering connection')
+
         (changed, results) = accept_reject(state, client, module)
         module.exit_json(changed=changed, peering_id=results)
 
diff --git a/lib/ansible/modules/cloud/amazon/ec2_vpc_peering_facts.py b/lib/ansible/modules/cloud/amazon/ec2_vpc_peering_facts.py
index 983f1afa553813..4242ca8378fdb4 100644
--- a/lib/ansible/modules/cloud/amazon/ec2_vpc_peering_facts.py
+++ b/lib/ansible/modules/cloud/amazon/ec2_vpc_peering_facts.py
@@ -74,7 +74,8 @@
     pass  # will be picked up by imported HAS_BOTO3
 
 from ansible.module_utils.basic import AnsibleModule
-from ansible.module_utils.ec2 import (ec2_argument_spec, boto3_conn, get_aws_connection_info,
+from ansible.module_utils.ec2 import (boto3_tag_list_to_ansible_dict,
+                                      ec2_argument_spec, boto3_conn, get_aws_connection_info,
                                       ansible_dict_to_boto3_filter_list, HAS_BOTO3, camel_dict_to_snake_dict)
 
 
@@ -128,8 +129,13 @@ def main():
     except botocore.exceptions.NoCredentialsError as e:
         module.fail_json(msg=str(e))
 
+    # Turn the boto3 result in to ansible friendly_snaked_names
     results = [camel_dict_to_snake_dict(peer) for peer in get_vpc_peers(ec2, module)]
 
+    # Turn the boto3 result in to ansible friendly tag dictionary
+    for peer in results:
+        peer['tags'] = boto3_tag_list_to_ansible_dict(peer.get('tags', []))
+
     module.exit_json(result=results)
 
 


Code review comments:
diff_hunk: @@ -416,11 +419,20 @@ def main():
         module.fail_json(msg="Can't authorize connection - " + str(e))
 
     if state == 'present':
+        if not vpc_id and not peer_vpc_id:
path: lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py
text: @user1:
you can use 

```
required_if([
    ('state', 'present', ['vpc_id', 'peer_vpc_id']),
    ('state', 'accept', ['peering_id']),
    ('state', 'reject', ['peering_id']),
 ])
```
Unfortunately the absent state check is too complicated for required_if

See https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/amazon/aws_direct_connect_gateway.py#L339-L340 for an example of using required_if

@author:
Cool. Didn't know that existed. Also changed the absent check msg to be similar to what gets returned from required_if.