-
Notifications
You must be signed in to change notification settings - Fork 343
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
route53_ksk - new module #2412
base: main
Are you sure you want to change the base?
route53_ksk - new module #2412
Conversation
Signed-off-by: Alina Buzachis <[email protected]>
Signed-off-by: Alina Buzachis <[email protected]>
Signed-off-by: Alina Buzachis <[email protected]>
Signed-off-by: Alina Buzachis <[email protected]>
Signed-off-by: Alina Buzachis <[email protected]>
8a29f2c
to
bb0d46d
Compare
Docs Build 📝Thank you for contribution!✨ The docsite for this PR is available for download as an artifact from this run: You can compare to the docs for the File changes:
Click to see the diff comparison.NOTE: only file modifications are shown here. New and deleted files are excluded. diff --git a/home/runner/work/amazon.aws/amazon.aws/docsbuild/base/collections/amazon/aws/index.html b/home/runner/work/amazon.aws/amazon.aws/docsbuild/head/collections/amazon/aws/index.html
index 756215f..809427f 100644
--- a/home/runner/work/amazon.aws/amazon.aws/docsbuild/base/collections/amazon/aws/index.html
+++ b/home/runner/work/amazon.aws/amazon.aws/docsbuild/head/collections/amazon/aws/index.html
@@ -346,6 +346,7 @@
<li><p><a class="reference internal" href="route53_module.html#ansible-collections-amazon-aws-route53-module"><span class="std std-ref">route53 module</span></a> – add or delete entries in Amazons Route 53 DNS service</p></li>
<li><p><a class="reference internal" href="route53_health_check_module.html#ansible-collections-amazon-aws-route53-health-check-module"><span class="std std-ref">route53_health_check module</span></a> – Manage health checks in Amazons Route 53 DNS service</p></li>
<li><p><a class="reference internal" href="route53_info_module.html#ansible-collections-amazon-aws-route53-info-module"><span class="std std-ref">route53_info module</span></a> – Retrieves Route 53 details using AWS methods</p></li>
+<li><p><a class="reference internal" href="route53_ksk_module.html#ansible-collections-amazon-aws-route53-ksk-module"><span class="std std-ref">route53_ksk module</span></a> – Manages a key-signing key (KSK)</p></li>
<li><p><a class="reference internal" href="route53_zone_module.html#ansible-collections-amazon-aws-route53-zone-module"><span class="std std-ref">route53_zone module</span></a> – add or delete Route 53 zones</p></li>
<li><p><a class="reference internal" href="s3_bucket_module.html#ansible-collections-amazon-aws-s3-bucket-module"><span class="std std-ref">s3_bucket module</span></a> – Manage S3 buckets in AWS, DigitalOcean, Ceph, Walrus, FakeS3 and StorageGRID</p></li>
<li><p><a class="reference internal" href="s3_bucket_info_module.html#ansible-collections-amazon-aws-s3-bucket-info-module"><span class="std std-ref">s3_bucket_info module</span></a> – Lists S3 buckets in AWS</p></li>
diff --git a/home/runner/work/amazon.aws/amazon.aws/docsbuild/base/collections/amazon/aws/route53_info_module.html b/home/runner/work/amazon.aws/amazon.aws/docsbuild/head/collections/amazon/aws/route53_info_module.html
index 3866d22..2e6cc1b 100644
--- a/home/runner/work/amazon.aws/amazon.aws/docsbuild/base/collections/amazon/aws/route53_info_module.html
+++ b/home/runner/work/amazon.aws/amazon.aws/docsbuild/head/collections/amazon/aws/route53_info_module.html
@@ -22,7 +22,7 @@
<script src="../../../_static/sphinx_highlight.js?v=dc90522c"></script>
<script src="../../../_static/js/theme.js"></script>
<link rel="search" title="Search" href="../../../search.html" />
- <link rel="next" title="amazon.aws.route53_zone module – add or delete Route 53 zones" href="route53_zone_module.html" />
+ <link rel="next" title="amazon.aws.route53_ksk module – Manages a key-signing key (KSK)" href="route53_ksk_module.html" />
<link rel="prev" title="amazon.aws.route53_health_check module – Manage health checks in Amazons Route 53 DNS service" href="route53_health_check_module.html" /><!-- extra head elements for Ansible beyond RTD Sphinx Theme -->
@@ -1173,7 +1173,7 @@ see <a class="reference internal" href="#ansible-collections-amazon-aws-route53-
<footer><div class="rst-footer-buttons" role="navigation" aria-label="Footer">
<a href="route53_health_check_module.html" class="btn btn-neutral float-left" title="amazon.aws.route53_health_check module – Manage health checks in Amazons Route 53 DNS service" accesskey="p" rel="prev"><span class="fa fa-arrow-circle-left" aria-hidden="true"></span> Previous</a>
- <a href="route53_zone_module.html" class="btn btn-neutral float-right" title="amazon.aws.route53_zone module – add or delete Route 53 zones" accesskey="n" rel="next">Next <span class="fa fa-arrow-circle-right" aria-hidden="true"></span></a>
+ <a href="route53_ksk_module.html" class="btn btn-neutral float-right" title="amazon.aws.route53_ksk module – Manages a key-signing key (KSK)" accesskey="n" rel="next">Next <span class="fa fa-arrow-circle-right" aria-hidden="true"></span></a>
</div>
<hr/>
diff --git a/home/runner/work/amazon.aws/amazon.aws/docsbuild/base/collections/amazon/aws/route53_zone_module.html b/home/runner/work/amazon.aws/amazon.aws/docsbuild/head/collections/amazon/aws/route53_zone_module.html
index 571a00c..eab1cf2 100644
--- a/home/runner/work/amazon.aws/amazon.aws/docsbuild/base/collections/amazon/aws/route53_zone_module.html
+++ b/home/runner/work/amazon.aws/amazon.aws/docsbuild/head/collections/amazon/aws/route53_zone_module.html
@@ -23,7 +23,7 @@
<script src="../../../_static/js/theme.js"></script>
<link rel="search" title="Search" href="../../../search.html" />
<link rel="next" title="amazon.aws.s3_bucket module – Manage S3 buckets in AWS, DigitalOcean, Ceph, Walrus, FakeS3 and StorageGRID" href="s3_bucket_module.html" />
- <link rel="prev" title="amazon.aws.route53_info module – Retrieves Route 53 details using AWS methods" href="route53_info_module.html" /><!-- extra head elements for Ansible beyond RTD Sphinx Theme -->
+ <link rel="prev" title="amazon.aws.route53_ksk module – Manages a key-signing key (KSK)" href="route53_ksk_module.html" /><!-- extra head elements for Ansible beyond RTD Sphinx Theme -->
@@ -621,7 +621,7 @@ see <a class="reference internal" href="#ansible-collections-amazon-aws-route53-
<footer><div class="rst-footer-buttons" role="navigation" aria-label="Footer">
- <a href="route53_info_module.html" class="btn btn-neutral float-left" title="amazon.aws.route53_info module – Retrieves Route 53 details using AWS methods" accesskey="p" rel="prev"><span class="fa fa-arrow-circle-left" aria-hidden="true"></span> Previous</a>
+ <a href="route53_ksk_module.html" class="btn btn-neutral float-left" title="amazon.aws.route53_ksk module – Manages a key-signing key (KSK)" accesskey="p" rel="prev"><span class="fa fa-arrow-circle-left" aria-hidden="true"></span> Previous</a>
<a href="s3_bucket_module.html" class="btn btn-neutral float-right" title="amazon.aws.s3_bucket module – Manage S3 buckets in AWS, DigitalOcean, Ceph, Walrus, FakeS3 and StorageGRID" accesskey="n" rel="next">Next <span class="fa fa-arrow-circle-right" aria-hidden="true"></span></a>
</div>
diff --git a/home/runner/work/amazon.aws/amazon.aws/docsbuild/base/collections/index_module.html b/home/runner/work/amazon.aws/amazon.aws/docsbuild/head/collections/index_module.html
index be6d0c8..8b4c78a 100644
--- a/home/runner/work/amazon.aws/amazon.aws/docsbuild/base/collections/index_module.html
+++ b/home/runner/work/amazon.aws/amazon.aws/docsbuild/head/collections/index_module.html
@@ -254,6 +254,7 @@
<li><p><a class="reference internal" href="amazon/aws/route53_module.html#ansible-collections-amazon-aws-route53-module"><span class="std std-ref">amazon.aws.route53</span></a> – add or delete entries in Amazons Route 53 DNS service</p></li>
<li><p><a class="reference internal" href="amazon/aws/route53_health_check_module.html#ansible-collections-amazon-aws-route53-health-check-module"><span class="std std-ref">amazon.aws.route53_health_check</span></a> – Manage health checks in Amazons Route 53 DNS service</p></li>
<li><p><a class="reference internal" href="amazon/aws/route53_info_module.html#ansible-collections-amazon-aws-route53-info-module"><span class="std std-ref">amazon.aws.route53_info</span></a> – Retrieves Route 53 details using AWS methods</p></li>
+<li><p><a class="reference internal" href="amazon/aws/route53_ksk_module.html#ansible-collections-amazon-aws-route53-ksk-module"><span class="std std-ref">amazon.aws.route53_ksk</span></a> – Manages a key-signing key (KSK)</p></li>
<li><p><a class="reference internal" href="amazon/aws/route53_zone_module.html#ansible-collections-amazon-aws-route53-zone-module"><span class="std std-ref">amazon.aws.route53_zone</span></a> – add or delete Route 53 zones</p></li>
<li><p><a class="reference internal" href="amazon/aws/s3_bucket_module.html#ansible-collections-amazon-aws-s3-bucket-module"><span class="std std-ref">amazon.aws.s3_bucket</span></a> – Manage S3 buckets in AWS, DigitalOcean, Ceph, Walrus, FakeS3 and StorageGRID</p></li>
<li><p><a class="reference internal" href="amazon/aws/s3_bucket_info_module.html#ansible-collections-amazon-aws-s3-bucket-info-module"><span class="std std-ref">amazon.aws.s3_bucket_info</span></a> – Lists S3 buckets in AWS</p></li>
|
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 11m 19s |
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 4m 23s |
Signed-off-by: Alina Buzachis <[email protected]>
1ffb4d1
to
01aaf1e
Compare
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 4m 31s |
a94bf6d
to
d4acf2d
Compare
Build failed.
|
d4acf2d
to
9cdbb0a
Compare
Signed-off-by: Alina Buzachis <[email protected]>
9cdbb0a
to
f0289dc
Compare
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 3m 22s |
recheck |
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 5m 10s |
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 4m 24s |
c5a5b30
to
3385733
Compare
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 4m 19s |
Signed-off-by: Alina Buzachis <[email protected]>
3385733
to
7af2ab3
Compare
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 5m 19s |
I would recommend creating a separate PR to add the new module for better clarity and easier review, rather than combining it with the addition of DNSSEC signing functionality. |
Signed-off-by: Alina Buzachis <[email protected]>
011154e
to
4201cbd
Compare
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 37s |
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 5m 20s |
plugins/modules/route53_ksk.py
Outdated
amazon.aws.route53_ksk: | ||
name: "{{ resource_prefix }}-ksk" | ||
hosted_zone_id: "{{ _hosted_zone.zone_id }}" | ||
status: "INACTIVE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is status needed when we delete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it is not. However the module was saying that the user need to deactivate the KSK before deleting. I changed the behaviour and not it deactivates by default when the KSK need to be deleted.
if module.check_mode: | ||
module.exit_json( | ||
changed=changed, | ||
msg=f"Would have updated the Key Signing Key status to {status} if not in check_mode.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we return the changed dict (would have changed if not check_mode = true) as well?
else: | ||
changed = True | ||
if module.check_mode: | ||
module.exit_json(changed=changed, msg="Would have created the Key Signing Key if not in check_mode.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we return the changed dict (would have changed if not check_mode = true) as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry but I don't understand what you mean. Can you please elaborate? changed is a boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the confusion in my previous statement. What I meant was: in addition to the 'changed' status, do you think it's possible to also return the "response/return dictionary," even if no changes are made to the resource? By definition, any module designed to support 'check mode' reports what changes would have been made without actually applying them. However, this approach is not followed by our cloud modules. I was wondering if we could start implementing this behavior in new modules and then work on modifying the existing ones later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, understood, done!
plugins/modules/route53_ksk.py
Outdated
if module.check_mode: | ||
module.exit_json(changed=changed, msg="Would have deleted the Key Signing Key if not in check_mode.") | ||
|
||
if module.params.get("status") == "INACTIVE": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for a condition to have state = "delete" and status = "ACTIVE"? I'm not sure if such a scenario can exist. Do we need this condition? We can ignore status for deletion. I think we should deactivate the key signing key before deleting in any case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This situation was already handled because there was a note in the description asking the user to deactivate the key before deleting it. However, since it is not possible to delete a key without first deactivating it, eliminating an additional step or the need to specify the status: INACTIVE, I handled deactivation by default when the user wants to delete it.
amazon.aws.route53_ksk: | ||
name: "{{ resource_prefix }}-ksk" | ||
hosted_zone_id: "{{ _hosted_zone.zone_id }}" | ||
status: "INACTIVE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about "status: INACTIVE" for deletion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
status: "INACTIVE" is not for deletion. state: absent
is for deletion. status: "INACTIVE" deactivate the ksk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the "status" parameter when the state is set to "absent/delete"? I'm trying to understand—if the goal is to delete the key, does its current status matter? Can we delete an ACTIVE key? If not, will it throw an error?
Are we going to delete the Key irrespective of its status?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we do not need it to delete a KSK. I updated the examples.
Signed-off-by: Alina Buzachis <[email protected]>
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 6m 33s |
Signed-off-by: Alina Buzachis <[email protected]>
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 52s |
Signed-off-by: Alina Buzachis <[email protected]>
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 5m 08s |
if ksk["Status"] != status: | ||
changed = True | ||
|
||
if module.check_mode: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ksk["Status"] = status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the same reason as the delete operation, when the resource is actually updated, the value of ksk["Status"]
will be modified or set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, done although as mentioned below there is the informational message saying that the status would have been updated to ACTIVE/INACTIVE if it was not in control mode.
In general, I don't really know if we tweak the retuned result of the resource in check mode to reflect the desired updates though. If we do so, the msg probably becomes useless... but I don't know.
plugins/modules/route53_ksk.py
Outdated
if module.check_mode: | ||
module.exit_json( | ||
changed=changed, | ||
key_signing_key=camel_dict_to_snake_dict(ksk), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key_signing_key=camel_dict_to_snake_dict(ksk), | |
key_signing_key={}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we could return the info about the resource that will be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand, check mode allows you to simulate the changes that would be made by the playbook or task. It shows you what would happen if the playbook were run in a live environment, but without making any actual changes to the actual resources. This is useful when you want to see the impact of the changes beforehand. Therefore, when a delete operation is performed, the return dictionary will be empty, which is the expected behavior here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is true, but since there is an informative message telling "Would have deleted if not in checked mode", I thought it will be useful if the actual resource is returned. However, I di not have a strong opinion about that so I updated as you suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! I wanted the cloud modules to adhere to the definition of check mode that is used consistently across Ansible.
Signed-off-by: Alina Buzachis <[email protected]>
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 3m 27s |
SUMMARY
Terminator PR mattclay/aws-terminator#312
Closes #1976
ISSUE TYPE
COMPONENT NAME
route53_ksk
ADDITIONAL INFORMATION