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

add KmsMasterKeyId as attribute option for boto3 call #762

Merged
merged 8 commits into from Oct 27, 2021
Merged
Show file tree
Hide file tree
Changes from 7 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
2 changes: 2 additions & 0 deletions changelogs/fragments/762-fix-kmsmasterkeyid-attr.yaml
@@ -0,0 +1,2 @@
minor_changes:
- sqs_queue - Providing a kms_master_key_id will now enable SSE properly (https://github.com/ansible-collections/community.aws/pull/762)
13 changes: 10 additions & 3 deletions plugins/modules/sqs_queue.py
Expand Up @@ -290,18 +290,25 @@ def describe_queue(client, queue_url):

def create_or_update_sqs_queue(client, module):
is_fifo = (module.params.get('queue_type') == 'fifo')
kms_master_key_id = module.params.get('kms_master_key_id')
queue_name = get_queue_name(module, is_fifo)
result = dict(
name=queue_name,
region=module.params.get('region'),
changed=False,
changed=False
tremble marked this conversation as resolved.
Show resolved Hide resolved
)

queue_url = get_queue_url(client, queue_name)
result['queue_url'] = queue_url

# Create a dict() to hold attributes that will be passed to boto3
create_attributes = {}

if not queue_url:
create_attributes = {'FifoQueue': 'true'} if is_fifo else {}
if is_fifo:
create_attributes['FifoQueue'] = "True"
if kms_master_key_id:
create_attributes['KmsMasterKeyId'] = kms_master_key_id
result['changed'] = True
if module.check_mode:
return result
Expand Down Expand Up @@ -385,7 +392,7 @@ def update_sqs_queue(module, client, queue_url):
if changed and not check_mode:
client.set_queue_attributes(QueueUrl=queue_url, Attributes=attributes_to_set, aws_retry=True)

return changed, existing_attributes.get('queue_arn'),
return changed, existing_attributes.get('queue_arn')


def delete_sqs_queue(client, module):
Expand Down
23 changes: 23 additions & 0 deletions tests/integration/targets/sqs_queue/tasks/main.yml
@@ -1,22 +1,26 @@
---
- name: Main test block

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:
- block:
- name: Test creating SQS queue
sqs_queue:
name: "{{ resource_prefix }}{{ 1000 | random }}"
register: create_result

- name: Assert SQS queue created
assert:
that:
- create_result.changed
- create_result.region == "{{ aws_region }}"

jillr marked this conversation as resolved.
Show resolved Hide resolved
always:
- name: Test deleting SQS queue
sqs_queue:
Expand All @@ -26,26 +30,32 @@
retries: 3
delay: 3
until: delete_result.changed

- name: Assert SQS queue deleted
assert:
that:
- delete_result.changed

- name: Test delete SQS queue that doesn't exist
sqs_queue:
name: "{{ resource_prefix }}{{ 1000 | random }}"
state: absent
register: delete_result

- name: Assert delete non-existant queue returns cleanly
assert:
that:
- delete_result.changed == False

- name: Test queue features
block:
- name: Test create queue with attributes
sqs_queue:
name: "{{ resource_prefix }}{{ 1000 | random }}"
default_visibility_timeout: 900
delivery_delay: 900
# Test SSE encryption
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Test SSE encryption

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback @tremble. I just noticed that the comment should be Test SSE, but you don't want any comments around test tasks?

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally the test description like this would be in the name block up top. having it listed here doesn't bring much value, we already know what that parameter is supposed to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we already know what that parameter is supposed to do.

Although kms_master_key_id involves encryption, I didn't know specifying kms_master_key_id automatically turned it on. I was thinking there was a separate encryption key that corresponded with a boolean value. The boto3 docs don't really jump out and say it either (https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/sqs.html#SQS.Client.create_queue), unless I missed something. If you don't want a mention in the tests, do you think adding something in the module docs would help clarify?

Copy link
Contributor

Choose a reason for hiding this comment

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

As someone using this feature, if you think the docs need a tweak to help folks understand what's going on then most certainly update them.

Only the docs at the top of the Python file need to be updated. The RST is auto-generated from them.

kms_master_key_id: alias/aws/sqs
maximum_message_size: 9009
message_retention_period: 900
receive_message_wait_time: 10
Expand All @@ -55,18 +65,21 @@
Effect: Allow
Action: "*"
register: create_result

- name: Assert queue created with configuration
assert:
that:
- create_result.changed
- create_result.default_visibility_timeout == 900
- create_result.delivery_delay == 900
- create_result.kms_master_key_id == "alias/aws/sqs"
- create_result.maximum_message_size == 9009
- create_result.message_retention_period == 900
- create_result.receive_message_wait_time == 10
- create_result.policy.Version == "2012-10-17"
- create_result.policy.Statement[0].Effect == "Allow"
- create_result.policy.Statement[0].Action == "*"

- name: Test idempotentcy
sqs_queue:
name: "{{ create_result.name }}"
Expand All @@ -81,10 +94,12 @@
Effect: Allow
Action: "*"
falcon78921 marked this conversation as resolved.
Show resolved Hide resolved
register: create_result

- name: Assert nothing changed
assert:
that:
- not create_result.changed

- name: Test update
sqs_queue:
name: "{{ create_result.name }}"
Expand All @@ -99,12 +114,14 @@
Effect: Allow
Action: "*"
register: create_result

tremble marked this conversation as resolved.
Show resolved Hide resolved
- name: Assert queue updated with configuration
assert:
that:
- create_result.changed
- create_result.default_visibility_timeout == 899
- create_result.delivery_delay == 899
- create_result.kms_master_key_id == "alias/aws/sqs"
- create_result.maximum_message_size == 9008
- create_result.message_retention_period == 899
- create_result.receive_message_wait_time == 9
Expand All @@ -120,19 +137,22 @@
retries: 3
delay: 3
until: delete_result.changed

- name: Test queue with redrive
block:
- name: Creating dead letter queue
sqs_queue:
name: "{{ resource_prefix }}{{ 1000 | random }}"
register: dead_letter_queue

- name: Test create queue with redrive_policy
sqs_queue:
name: "{{ resource_prefix }}{{ 1000 | random }}"
redrive_policy:
maxReceiveCount: 5
deadLetterTargetArn: "{{ dead_letter_queue.queue_arn }}"
register: create_result

- name: Assert queue created with configuration
assert:
that:
Expand All @@ -148,6 +168,7 @@
with_items:
- { name: "{{ create_result.name }}" }
- { name: "{{ dead_letter_queue.name }}" }

- name: Test FIFO queue
block:
- name: Creating FIFO queue
Expand All @@ -156,10 +177,12 @@
queue_type: fifo
content_based_deduplication: yes
register: create_result

- name: Assert queue created with configuration
assert:
that:
- create_result.changed

always:
- name: Cleaning up queue
sqs_queue:
Expand Down