Add support for tagging certificates. Fix deprecated tasks in aws_acm integration tests #870
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Build failed.
|
|
Build succeeded.
|
|
Build failed.
|
|
Build succeeded.
|
sebastien-rosset
changed the title
|
Build succeeded.
|
markuman
reviewed
Jan 17, 2022
markuman
reviewed
Jan 17, 2022
markuman
requested changes
Jan 17, 2022
|
Build succeeded.
|
|
Build succeeded.
|
|
There is an intermittent timing bug in plugins/module_utils/acm.py:get_certificates of the A
I'm still trying to understand the behavior of the @AWSRetry.backoff(tries=5, delay=5, backoff=2.0, catch_extra_error_codes=['ResourceNotFoundException', 'RequestInProgressException'])To reproduce, the following tasks can be executed multiple times until it fails: - name: delete cert
aws_acm:
state: absent
domain_name: 'mydoamain.com'
- name: check cert was deleted
aws_acm_info:
tags:
Name: 'my_certificate_name'This happens in the integration tests here: https://github.com/ansible-collections/community.aws/blob/main/tests/integration/targets/aws_acm/tasks/main.yml#L262-L267 Here is a stack trace when the exception occurs: |
|
Build succeeded.
|
| @@ -1,5 +1,5 @@ | |||
| # https://github.com/ansible/ansible/issues/67788 | |||
| unstable | |||
| # unstable | |||
There was a problem hiding this comment.
@markuman I'm unclear what to do about this label.
It looks like when unstable is set, the tests do not execute. But isn't expected that test must pass as a gating criteria for the PR?
On the other hand, if the module is known to have issue such as the referenced issue, how can we test?
I've also discovered a new intermittent issue: ansible-collections/amazon.aws#622. I think as a workaround, I could add a retry loop in the ansible task.
What about the new IAM permissions in mattclay/aws-terminator#188?
There was a problem hiding this comment.
ping @markuman.
There was a problem hiding this comment.
What should I do next? Should I disable these integration tests because there is a bug in amazon.aws?
Here is a fix for amazon.aws: ansible-collections/amazon.aws#646, but now I'm concerned I'm going to have to wait until there is a new release of amazon.aws that includes a fix?
There was a problem hiding this comment.
AFAIK the next amazon.aws release is scheduled one week before community.aws will release its next version.
imo, I would try to wait this two weeks if that solves the Issue with the CI.
There was a problem hiding this comment.
Ouch. This is not encouraging. Given you are planning to release community.aws on Feb 14th and you are suggesting to wait 2 weeks, does that mean this PR wouldn't make it for 3.1?
There was a problem hiding this comment.
No. afaik amazon.aws is planed to release one week before community.aws.
Means Feb 7th.
But since I'm not a maintainer of amazon.aws, I cannot guarantee that. That's just something I've noticed on IRC/Matrix.
Missing the release train happens more often than one would like. There will be 3.2.0 soon I think.
|
Build failed.
|
|
Build failed.
|
|
Build failed.
|
|
Build failed.
|
|
Build failed.
|
This was referenced Jan 24, 2022
|
@sebastien-rosset this PR contains the following merge commits: Please rebase your branch to remove these commits. |
sebastien-rosset
force-pushed
the
acm-tags
branch
from
25c1f87
to
29292ce
Compare
|
Build succeeded (third-party-check pipeline).
|
|
Build succeeded.
|
marknet15
reviewed
Feb 7, 2022
| # The module was originally implemented to filter certificates based on the 'Name' tag. | ||
| # Other tags are not used to filter certificates. | ||
| # It would make sense to replace the existing name_tag, domain, certificate_arn attributes | ||
| # with a 'filter' attribute, but that would break backwards-compatibility. |
There was a problem hiding this comment.
@markuman @alinabuzachis what do you think about adding a deprecation notice for the attributes mentioned here ? Maybe one for a separate PR adding also the mentioned filter attribute.
There was a problem hiding this comment.
Can this be handled separately?
There was a problem hiding this comment.
@marknet15 I guess we should add a deprecation notice. I am still confused because signature_algorithms doesn't seem to be documented. So, I guess we need to update the documentation with this param and the new one that replaces it and add a deprecation notice for signature_algorithms. But probably it would make sense to have that in separate PRs. Not sure if this makes sense @markuman.
There was a problem hiding this comment.
What else do I need to do? This is stalling.
There was a problem hiding this comment.
@markuman how would you suggest to proceed here?
There was a problem hiding this comment.
Oh sorry, this runs out of focus here ...
The signature_algorithms might be some deprecation thing maybe
This module
community.crypto.openssl_certificatehas been removed in version 2.0.0 of community.crypto. The ‘community.crypto.openssl_certificate’ module has been renamed to ‘community.crypto.x509_certificate’
It does not exists in the example section of the main branch anymore, only in the stable-1 branch.
# It would make sense to replace the existing name_tag, domain, certificate_arn attributes # with a 'filter' attribute, but that would break backwards-compatibility.
I agree, we can do this in a separately PR. Ideally the new filter functionality is added first and works mutually exclusive with the former parameters.
I would suggest that it is added in 3.x.x. and the deprecation warnings will be introduced in 4.x.x
There was a problem hiding this comment.
Can this conversation be resolved? I don't see any change being requested for this PR.
softwarefactory-project-zuul bot
pushed a commit
to ansible-collections/amazon.aws
that referenced
this pull request
Feb 9, 2022
Handle ResourceNotFoundException while iterating certificates SUMMARY The module/utils/acm.py was not correctly handling deletion of certificates. While iterating over a list of certificates, the get_certificate function was making API calls to obtain more information about the certificates, but some certificates may be deleted while iterating. Fixes #622 ISSUE TYPE Bugfix Pull Request COMPONENT NAME acm.py ADDITIONAL INFORMATION Wow, it seems many tests are very flaky. I'm attempting to fix an issue in ACM, but problems occur elsewhere. Not to mention I raised this PR to fix #622, which was discovered while working on ansible-collections/community.aws#870. And I discovered other issues as well, and so it looks like it's not possible to make any progress without going down a tree of bug fixes. TypeError: 'NoneType' object is not subscriptable fatal: [testhost]: FAILED! => { "changed": false, "module_stderr": "Traceback (most recent call last):\n File \"<stdin>\", line 121, in <module>\n File \"<stdin>\", line 113, in _ansiballz_main\n File \"<stdin>\", line 61, in invoke_module\n File \"/usr/lib64/python3.8/runpy.py\", line 207, in run_module\n return _run_module_code(code, init_globals, run_name, mod_spec)\n File \"/usr/lib64/python3.8/runpy.py\", line 97, in _run_module_code\n _run_code(code, mod_globals, init_globals,\n File \"/usr/lib64/python3.8/runpy.py\", line 87, in _run_code\n exec(code, run_globals)\n File \"/tmp/ansible_ec2_vpc_igw_payload_g4o1kl_r/ansible_ec2_vpc_igw_payload.zip/ansible_collections/amazon/aws/plugins/modules/ec2_vpc_igw.py\", line 248, in <module>\n File \"/tmp/ansible_ec2_vpc_igw_payload_g4o1kl_r/ansible_ec2_vpc_igw_payload.zip/ansible_collections/amazon/aws/plugins/modules/ec2_vpc_igw.py\", line 242, in main\n File \"/tmp/ansible_ec2_vpc_igw_payload_g4o1kl_r/ansible_ec2_vpc_igw_payload.zip/ansible_collections/amazon/aws/plugins/modules/ec2_vpc_igw.py\", line 132, in process\n File \"/tmp/ansible_ec2_vpc_igw_payload_g4o1kl_r/ansible_ec2_vpc_igw_payload.zip/ansible_collections/amazon/aws/plugins/modules/ec2_vpc_igw.py\", line 220, in ensure_igw_present\n File \"/tmp/ansible_ec2_vpc_igw_payload_g4o1kl_r/ansible_ec2_vpc_igw_payload.zip/ansible_collections/amazon/aws/plugins/modules/ec2_vpc_igw.py\", line 158, in get_igw_info\nTypeError: 'NoneType' object is not subscriptable\n", "module_stdout": "", "msg": "MODULE FAILURE\nSee stdout/stderr for the exact error", "rc": 1 } Reviewed-by: Alina Buzachis <None> Reviewed-by: Jill R <None>
patchback bot
pushed a commit
to ansible-collections/amazon.aws
that referenced
this pull request
Feb 9, 2022
Handle ResourceNotFoundException while iterating certificates SUMMARY The module/utils/acm.py was not correctly handling deletion of certificates. While iterating over a list of certificates, the get_certificate function was making API calls to obtain more information about the certificates, but some certificates may be deleted while iterating. Fixes #622 ISSUE TYPE Bugfix Pull Request COMPONENT NAME acm.py ADDITIONAL INFORMATION Wow, it seems many tests are very flaky. I'm attempting to fix an issue in ACM, but problems occur elsewhere. Not to mention I raised this PR to fix #622, which was discovered while working on ansible-collections/community.aws#870. And I discovered other issues as well, and so it looks like it's not possible to make any progress without going down a tree of bug fixes. TypeError: 'NoneType' object is not subscriptable fatal: [testhost]: FAILED! => { "changed": false, "module_stderr": "Traceback (most recent call last):\n File \"<stdin>\", line 121, in <module>\n File \"<stdin>\", line 113, in _ansiballz_main\n File \"<stdin>\", line 61, in invoke_module\n File \"/usr/lib64/python3.8/runpy.py\", line 207, in run_module\n return _run_module_code(code, init_globals, run_name, mod_spec)\n File \"/usr/lib64/python3.8/runpy.py\", line 97, in _run_module_code\n _run_code(code, mod_globals, init_globals,\n File \"/usr/lib64/python3.8/runpy.py\", line 87, in _run_code\n exec(code, run_globals)\n File \"/tmp/ansible_ec2_vpc_igw_payload_g4o1kl_r/ansible_ec2_vpc_igw_payload.zip/ansible_collections/amazon/aws/plugins/modules/ec2_vpc_igw.py\", line 248, in <module>\n File \"/tmp/ansible_ec2_vpc_igw_payload_g4o1kl_r/ansible_ec2_vpc_igw_payload.zip/ansible_collections/amazon/aws/plugins/modules/ec2_vpc_igw.py\", line 242, in main\n File \"/tmp/ansible_ec2_vpc_igw_payload_g4o1kl_r/ansible_ec2_vpc_igw_payload.zip/ansible_collections/amazon/aws/plugins/modules/ec2_vpc_igw.py\", line 132, in process\n File \"/tmp/ansible_ec2_vpc_igw_payload_g4o1kl_r/ansible_ec2_vpc_igw_payload.zip/ansible_collections/amazon/aws/plugins/modules/ec2_vpc_igw.py\", line 220, in ensure_igw_present\n File \"/tmp/ansible_ec2_vpc_igw_payload_g4o1kl_r/ansible_ec2_vpc_igw_payload.zip/ansible_collections/amazon/aws/plugins/modules/ec2_vpc_igw.py\", line 158, in get_igw_info\nTypeError: 'NoneType' object is not subscriptable\n", "module_stdout": "", "msg": "MODULE FAILURE\nSee stdout/stderr for the exact error", "rc": 1 } Reviewed-by: Alina Buzachis <None> Reviewed-by: Jill R <None> (cherry picked from commit 4b12454)
softwarefactory-project-zuul bot
pushed a commit
to ansible-collections/amazon.aws
that referenced
this pull request
Feb 9, 2022
…657) [PR #646/4b124547 backport][stable-3] Handle ResourceNotFoundException while iterating certificates This is a backport of PR #646 as merged into main (4b12454). SUMMARY The module/utils/acm.py was not correctly handling deletion of certificates. While iterating over a list of certificates, the get_certificate function was making API calls to obtain more information about the certificates, but some certificates may be deleted while iterating. Fixes #622 ISSUE TYPE Bugfix Pull Request COMPONENT NAME acm.py ADDITIONAL INFORMATION Wow, it seems many tests are very flaky. I'm attempting to fix an issue in ACM, but problems occur elsewhere. Not to mention I raised this PR to fix #622, which was discovered while working on ansible-collections/community.aws#870. And I discovered other issues as well, and so it looks like it's not possible to make any progress without going down a tree of bug fixes. TypeError: 'NoneType' object is not subscriptable fatal: [testhost]: FAILED! => { "changed": false, "module_stderr": "Traceback (most recent call last):\n File \"<stdin>\", line 121, in <module>\n File \"<stdin>\", line 113, in _ansiballz_main\n File \"<stdin>\", line 61, in invoke_module\n File \"/usr/lib64/python3.8/runpy.py\", line 207, in run_module\n return _run_module_code(code, init_globals, run_name, mod_spec)\n File \"/usr/lib64/python3.8/runpy.py\", line 97, in _run_module_code\n _run_code(code, mod_globals, init_globals,\n File \"/usr/lib64/python3.8/runpy.py\", line 87, in _run_code\n exec(code, run_globals)\n File \"/tmp/ansible_ec2_vpc_igw_payload_g4o1kl_r/ansible_ec2_vpc_igw_payload.zip/ansible_collections/amazon/aws/plugins/modules/ec2_vpc_igw.py\", line 248, in <module>\n File \"/tmp/ansible_ec2_vpc_igw_payload_g4o1kl_r/ansible_ec2_vpc_igw_payload.zip/ansible_collections/amazon/aws/plugins/modules/ec2_vpc_igw.py\", line 242, in main\n File \"/tmp/ansible_ec2_vpc_igw_payload_g4o1kl_r/ansible_ec2_vpc_igw_payload.zip/ansible_collections/amazon/aws/plugins/modules/ec2_vpc_igw.py\", line 132, in process\n File \"/tmp/ansible_ec2_vpc_igw_payload_g4o1kl_r/ansible_ec2_vpc_igw_payload.zip/ansible_collections/amazon/aws/plugins/modules/ec2_vpc_igw.py\", line 220, in ensure_igw_present\n File \"/tmp/ansible_ec2_vpc_igw_payload_g4o1kl_r/ansible_ec2_vpc_igw_payload.zip/ansible_collections/amazon/aws/plugins/modules/ec2_vpc_igw.py\", line 158, in get_igw_info\nTypeError: 'NoneType' object is not subscriptable\n", "module_stdout": "", "msg": "MODULE FAILURE\nSee stdout/stderr for the exact error", "rc": 1 }
markuman
approved these changes
Mar 29, 2022
markuman
reviewed
Mar 29, 2022
markuman
reviewed
Mar 29, 2022
Co-authored-by: Markus Bergholz <[email protected]>
Co-authored-by: Markus Bergholz <[email protected]>
alinabuzachis
approved these changes
Mar 31, 2022
|
community.aws 3.2.0 waits for amazon.aws 3.2.0. So I think it's possible to bring it into stable-3 until this evening.....hopefully :) |
Backport to stable-3: 💚 backport PR created✅ Backport PR branch: Backported as #1044 🤖 @patchback |
patchback bot
pushed a commit
that referenced
this pull request
Mar 31, 2022
… integration tests (#870) Add support for tagging certificates. Fix deprecated tasks in aws_acm integration tests SUMMARY This PR adds support for configuring arbitrary tags when importing a certificate using the aws_acm module. Previously, it was only possible to set the 'Name' tag. Additionally, this PR fixes issues with the aws_acm integration tests. The integration tests were using deprecated tasks or attributes, such as openssl_certificate. ISSUE TYPE Bugfix Pull Request COMPONENT NAME aws_acm ADDITIONAL INFORMATION Changes to the aws_acm.py module: Add new tags and purge_tags attributes. The certificate_arn attribute is now allowed when state='present'. A playbook should be allowed to modify an existing certificate entry by providing the ARN. For example, a play may want to add, modify, remove tags on an existing certificate. The aws_acm module returns the updated tags. See example below. Refactor aws_acm.py to improve code reuse and make it possible to set arbitrary tags. This should also help to 1) improve readability. 2) prepare for #869 which I am planning to work on next. Backwards-compatibility is retained, even though it might make sense to normalize some of the attributes. Example return value: "certificate": { "arn": "arn:aws:acm:us-west-1:account:certificate/f85abf9d-4bda-4dcc-98c3-770664a68243", "domain_name": "acm1.949058644.ansible.com", "tags": { "Application": "search", "Environment": "development", "Name": "ansible-test-78006277-398b5796f999_949058644_1" } } Integration tests: The openssl_certificate task is deprecated. Migrate to x509_certificate. The signature_algorithms attribute is no longer supported by the new x509_certificate task. Using selfsigned_digest instead. The integration tests for the aws_acm module pass locally. I see ansible/ansible#67788 has been closed, but tests/integration/targets/aws_acm/aliases still has unstable. I am not sure what to do about it. I was able to run the tests in my local workspace after making the above changes. Reviewed-by: Markus Bergholz <[email protected]> Reviewed-by: Sebastien Rosset <None> Reviewed-by: Mark Woolley <[email protected]> Reviewed-by: Alina Buzachis <None> (cherry picked from commit 29d37be)
softwarefactory-project-zuul bot
pushed a commit
that referenced
this pull request
Mar 31, 2022
… integration tests (#870) (#1044) [PR #870/29d37bed backport][stable-3] Add support for tagging certificates. Fix deprecated tasks in aws_acm integration tests This is a backport of PR #870 as merged into main (29d37be). SUMMARY This PR adds support for configuring arbitrary tags when importing a certificate using the aws_acm module. Previously, it was only possible to set the 'Name' tag. Additionally, this PR fixes issues with the aws_acm integration tests. The integration tests were using deprecated tasks or attributes, such as openssl_certificate. ISSUE TYPE Bugfix Pull Request COMPONENT NAME aws_acm ADDITIONAL INFORMATION Changes to the aws_acm.py module: Add new tags and purge_tags attributes. The certificate_arn attribute is now allowed when state='present'. A playbook should be allowed to modify an existing certificate entry by providing the ARN. For example, a play may want to add, modify, remove tags on an existing certificate. The aws_acm module returns the updated tags. See example below. Refactor aws_acm.py to improve code reuse and make it possible to set arbitrary tags. This should also help to 1) improve readability. 2) prepare for #869 which I am planning to work on next. Backwards-compatibility is retained, even though it might make sense to normalize some of the attributes. Example return value: "certificate": { "arn": "arn:aws:acm:us-west-1:account:certificate/f85abf9d-4bda-4dcc-98c3-770664a68243", "domain_name": "acm1.949058644.ansible.com", "tags": { "Application": "search", "Environment": "development", "Name": "ansible-test-78006277-398b5796f999_949058644_1" } } Integration tests: The openssl_certificate task is deprecated. Migrate to x509_certificate. The signature_algorithms attribute is no longer supported by the new x509_certificate task. Using selfsigned_digest instead. The integration tests for the aws_acm module pass locally. I see ansible/ansible#67788 has been closed, but tests/integration/targets/aws_acm/aliases still has unstable. I am not sure what to do about it. I was able to run the tests in my local workspace after making the above changes. Reviewed-by: Alina Buzachis <None>
1 task
abikouo
pushed a commit
to abikouo/community.aws
that referenced
this pull request
Oct 24, 2023
… api as expected by api call (ansible-collections#877) ec2_eni: change data type of `device_index` to str when passing it to api as expected by api call SUMMARY Currently the data type for parameter device_index here while being passed to api call is integer but one of the API calls later used here in the module (describe_network_interfaces) expects it to be string as per boto3 api documentation. Fixes ansible-collections#870 ISSUE TYPE Bugfix Pull Request COMPONENT NAME ec2_eni Reviewed-by: Mark Chappell <None> Reviewed-by: Jill R <None> Reviewed-by: Alina Buzachis <None>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
This PR adds support for configuring arbitrary tags when importing a certificate using the
aws_acmmodule. Previously, it was only possible to set the 'Name' tag.Additionally, this PR fixes issues with the
aws_acmintegration tests. The integration tests were using deprecated tasks or attributes, such asopenssl_certificate.ISSUE TYPE
COMPONENT NAME
aws_acm
ADDITIONAL INFORMATION
Changes to the
aws_acm.pymodule:tagsandpurge_tagsattributes.certificate_arnattribute is now allowed whenstate='present'. A playbook should be allowed to modify an existing certificate entry by providing the ARN. For example, a play may want to add, modify, remove tags on an existing certificate.aws_acmmodule returns the updated tags. See example below.aws_acm.pyto improve code reuse and make it possible to set arbitrary tags. This should also help to 1) improve readability. 2) prepare for Add support for requesting public and private ACM certificate #869 which I am planning to work on next.Backwards-compatibility is retained, even though it might make sense to normalize some of the attributes.
Example return value:
Integration tests:
openssl_certificatetask is deprecated. Migrate tox509_certificate.signature_algorithmsattribute is no longer supported by the newx509_certificatetask. Usingselfsigned_digestinstead.aws_acmmodule pass locally.unstable. I am not sure what to do about it. I was able to run the tests in my local workspace after making the above changes.