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

rds_subnet_group : Sanity Check fixes (docs) and Integration tests #63214

Merged
merged 3 commits into from Oct 23, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions hacking/aws_config/testing_policies/database-policy.json
Expand Up @@ -88,6 +88,17 @@
"Effect": "Allow",
"Resource": "*"
},
{
"Sid": "AllowRDSSubnetGroups",
"Effect": "Allow",
"Action": [
"rds:CreateDBSubnetGroup",
"rds:DeleteDBSubnetGroup",
"rds:DescribeDBSubnetGroups",
"rds:ModifyDBSubnetGroup"
],
"Resource": ["*"]
},
{
"Sid": "DMSEndpoints",
"Effect": "Allow",
Expand Down
11 changes: 8 additions & 3 deletions lib/ansible/modules/cloud/amazon/rds_subnet_group.py
Expand Up @@ -23,18 +23,23 @@
description:
- Specifies whether the subnet should be present or absent.
required: true
default: present
choices: [ 'present' , 'absent' ]
type: str
name:
description:
- Database subnet group identifier.
required: true
type: str
description:
description:
- Database subnet group description. Only set when a new group is added.
- Database subnet group description.
- Required when I(state=present).
type: str
subnets:
description:
- List of subnet IDs that make up the database subnet group.
- Required when I(state=present).
type: list
author: "Scott Anderson (@tastychutney)"
extends_documentation_fragment:
- aws
Expand Down Expand Up @@ -138,7 +143,7 @@ def main():
group_subnets = module.params.get('subnets') or {}

if state == 'present':
for required in ['name', 'description', 'subnets']:
for required in ['description', 'subnets']:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine as it is, but we also have a conventional pattern for this for AnsibleModule/AnsibleAWSModule.

required_if = [('state', 'present', ['description', 'subnets'])]
module = AnsibleModule(argument_spec=argument_spec, required_if=required_if)

I agree that the else below this is a bad pattern. Normally we ignore extra options so someone can just easily edit state: present to state: absent rather than have to cull superfluous options from the task.

if not module.params.get(required):
module.fail_json(msg=str("Parameter %s required for state='present'" % required))
else:
Expand Down
2 changes: 2 additions & 0 deletions test/integration/targets/rds_subnet_group/aliases
@@ -0,0 +1,2 @@
cloud/aws
shippable/aws/group2
8 changes: 8 additions & 0 deletions test/integration/targets/rds_subnet_group/defaults/main.yml
@@ -0,0 +1,8 @@
vpc_cidr: '10.{{ 256 | random(seed=resource_prefix) }}.0.0/16'
subnet_a: '10.{{ 256 | random(seed=resource_prefix) }}.10.0/24'
subnet_b: '10.{{ 256 | random(seed=resource_prefix) }}.11.0/24'
subnet_c: '10.{{ 256 | random(seed=resource_prefix) }}.12.0/24'
subnet_d: '10.{{ 256 | random(seed=resource_prefix) }}.13.0/24'

group_description: 'Created by integration test : {{ resource_prefix }}'
group_description_changed: 'Created by integration test : {{ resource_prefix }} - changed'
3 changes: 3 additions & 0 deletions test/integration/targets/rds_subnet_group/meta/main.yml
@@ -0,0 +1,3 @@
dependencies:
- prepare_tests
- setup_ec2
113 changes: 113 additions & 0 deletions test/integration/targets/rds_subnet_group/tasks/main.yml
@@ -0,0 +1,113 @@
---
# Tests for rds_subnet_group
#
# Note: (From Amazon's documentation)
# https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/rds.html#RDS.Client.modify_db_subnet_group
# DB subnet groups must contain at least one subnet in at least two AZs in the
# AWS Region.

- module_defaults:
group/aws:
aws_access_key: '{{ aws_access_key }}'
aws_secret_key: '{{ aws_secret_key }}'
security_token: '{{ security_token | default(omit) }}'
region: '{{ aws_region }}'
block:

# ============================================================

- name: 'Fetch AZ availability'
aws_az_info:
register: az_info

- name: 'Assert that we have multiple AZs available to us'
assert:
that: az_info.availability_zones | length >= 2

- name: 'Pick AZs'
set_fact:
az_one: '{{ az_info.availability_zones[0].zone_name }}'
az_two: '{{ az_info.availability_zones[1].zone_name }}'

# ============================================================

- name: 'Create a VPC'
ec2_vpc_net:
state: present
cidr_block: '{{ vpc_cidr }}'
name: '{{ resource_prefix }}'
register: vpc

- name: 'Create subnets'
ec2_vpc_subnet:
state: present
cidr: '{{ item.cidr }}'
az: '{{ item.az }}'
vpc_id: '{{ vpc.vpc.id }}'
tags:
Name: '{{ item.name }}'
with_items:
- cidr: '{{ subnet_a }}'
az: '{{ az_one }}'
name: '{{ resource_prefix }}-subnet-a'
- cidr: '{{ subnet_b }}'
az: '{{ az_two }}'
name: '{{ resource_prefix }}-subnet-b'
- cidr: '{{ subnet_c }}'
az: '{{ az_one }}'
name: '{{ resource_prefix }}-subnet-c'
- cidr: '{{ subnet_d }}'
az: '{{ az_two }}'
name: '{{ resource_prefix }}-subnet-d'
register: subnets

- set_fact:
subnet_ids: '{{ subnets | json_query("results[].subnet.id") | list }}'

# ============================================================

- include_tasks: 'params.yml'

- include_tasks: 'tests.yml'

# ============================================================

always:
- name: 'Remove subnet group'
rds_subnet_group:
state: absent
name: '{{ resource_prefix }}'
ignore_errors: yes

- name: 'Remove subnets'
ec2_vpc_subnet:
state: absent
cidr: '{{ item.cidr }}'
vpc_id: '{{ vpc.vpc.id }}'
with_items:
- cidr: '{{ subnet_a }}'
name: '{{ resource_prefix }}-subnet-a'
- cidr: '{{ subnet_b }}'
name: '{{ resource_prefix }}-subnet-b'
- cidr: '{{ subnet_c }}'
name: '{{ resource_prefix }}-subnet-c'
- cidr: '{{ subnet_d }}'
name: '{{ resource_prefix }}-subnet-d'
ignore_errors: yes
register: removed_subnets
until: removed_subnets is succeeded
retries: 5
delay: 5

- name: 'Remove the VPC'
ec2_vpc_net:
state: absent
cidr_block: '{{ vpc_cidr }}'
name: '{{ resource_prefix }}'
ignore_errors: yes
register: removed_vpc
until: removed_vpc is success
retries: 5
delay: 5

# ============================================================
62 changes: 62 additions & 0 deletions test/integration/targets/rds_subnet_group/tasks/params.yml
@@ -0,0 +1,62 @@
---
# Try creating without a description
- name: 'Create a subnet group (no description)'
rds_subnet_group:
state: present
name: '{{ resource_prefix }}'
subnets:
- '{{ subnet_ids[0] }}'
- '{{ subnet_ids[1] }}'
ignore_errors: yes
register: create_missing_param
- assert:
that:
- create_missing_param is failed
- "'description' in create_missing_param.msg"
- "\"required for state='present'\" in create_missing_param.msg"

# Try creating without subnets
- name: 'Create a subnet group (no subnets)'
rds_subnet_group:
state: present
name: '{{ resource_prefix }}'
description: '{{ group_description }}'
ignore_errors: yes
register: create_missing_param
- assert:
that:
- create_missing_param is failed
- "'subnets' in create_missing_param.msg"
- "\"required for state='present'\" in create_missing_param.msg"

# XXX This feels like a bad pattern
Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity, I believe you mean the module behaviour feels like a bad pattern, or is it something in the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The module behaviour: It feels wrong to error during removal because extra options were passed.

# Try deleting with subnets
- name: 'Delete a subnet group (with subnets)'
rds_subnet_group:
state: absent
name: '{{ resource_prefix }}'
subnets:
- '{{ subnet_ids[0] }}'
- '{{ subnet_ids[1] }}'
ignore_errors: yes
register: delete_extra_param
- assert:
that:
- delete_extra_param is failed
- "'subnets' in delete_extra_param.msg"
- "\"not allowed for state='absent'\" in delete_extra_param.msg"

# XXX This feels like a bad pattern
# Try deleting with a description
- name: 'Create a subnet group (with description)'
rds_subnet_group:
state: absent
name: '{{ resource_prefix }}'
description: '{{ group_description }}'
ignore_errors: yes
register: delete_extra_param
- assert:
that:
- delete_extra_param is failed
- "'description' in delete_extra_param.msg"
- "\"not allowed for state='absent'\" in delete_extra_param.msg"