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

Fix KeyError: SecurityGroups in elasticache module. #410

Merged
merged 16 commits into from
Apr 20, 2021

Conversation

stefanhorning
Copy link
Contributor

@stefanhorning stefanhorning commented Feb 12, 2021

SUMMARY

Also improve docs a little.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

elasticache module

ADDITIONAL INFORMATION

Module would otherwise throw Fix KeyError: 'SecurityGroups' when updating an existing Cache Cluster with a security group.

Also it was confusing to me at first if the security group param should be provided when in a VPC, as the cache_security_groups flag can't be used then and I didn't read further down the docs (on the Docs website). Hence I added a hint for others. But ideally this should be handled by the module internally, as it can be detected if VPC or not, based on the cache_subnet_group param (so only one SG param needs to be exposed to user).

@ansibullbot
Copy link

@ansibullbot ansibullbot added bug This issue/PR relates to a bug community_review module module needs_triage plugins plugin (any type) small_patch Hopefully easy to review needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed community_review labels Feb 12, 2021
@ansibullbot ansibullbot added community_review and removed small_patch Hopefully easy to review needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 12, 2021
@ansibullbot ansibullbot added integration tests/integration needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR tests tests and removed community_review labels Feb 12, 2021
Copy link
Member

@markuman markuman left a comment

Choose a reason for hiding this comment

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

It would be nice if you can expand the integration test that it tests the SecurityGroup option of your PR.
Furthermore, you need a changelog fragment.

@stefanhorning
Copy link
Contributor Author

stefanhorning commented Feb 15, 2021

Sure, I was in the progress of looking into expanding the tests anyway.

@stefanhorning
Copy link
Contributor Author

stefanhorning commented Feb 15, 2021

Also I am getting this build error:

"/usr/local/lib/python2.7/dist-packages/OpenSSL/crypto.py:14: CryptographyDeprecationWarning: Python 2 is no longer supported by the Python core team. Support for it is now deprecated in cryptography, and will be removed in the next release.\n  from cryptography import utils, x509\nTraceback (most recent call last):\n  File \"/root/.ansible/tmp/ansible-tmp-1613384043.93-354-274813038104715/AnsiballZ_elasticache.py

Shouldn't we stop testing with Python 2.7, as it's super outdated an mist libraries start dropping support for it?

@tremble
Copy link
Contributor

tremble commented Feb 15, 2021

Shouldn't we stop testing with Python 2.7, as it's super outdated an mist libraries start dropping support for it.

We've mostly picked up our support 'requirements' from Ansible itself. Which will likely have some form of support for RHEL6 for another couple of years.

Because boto3 is dropping support for Python 2.7 I suspect we'll be releasing a 'major' version somewhere after July in which we drop official support for Python 2.7. ( https://boto3.amazonaws.com/v1/documentation/api/1.16.56/guide/migrationpy3.html )

@stefanhorning
Copy link
Contributor Author

stefanhorning commented Feb 16, 2021

Thanks for the tip @tremble, that seemed to have done the trick.

All code should be fixed now. Now it's just about fixing the CI setup to allow access for test user:

User: arn:aws:sts::966509639900:assumed-role/ansible-core-ci-test-prod/prod=shippable=ansible-collections=community.aws=1534.24 is not authorized to perform: elasticache:DescribeCacheClusters on resource: arn:aws:elasticache:us-east-1:966509639900:cluster:shippable-1534-24-elasticache-module-redis-test

@markuman
Copy link
Member

markuman commented Feb 16, 2021

@stefanhorning you need another PR in mattclay/aws-terminator
it's a service that deletes frequently all dangle services and resources.
I guess elasticache belongs to the data-services.yaml.
https://github.com/mattclay/aws-terminator/blob/master/aws/policy/data-services.yaml

Just reference in that PR to this PR.

@stefanhorning
Copy link
Contributor Author

I guess I addressed all issues now. So hopefully CI will turn green once mattclay/aws-terminator#126 is merged.

Copy link
Collaborator

@jillr jillr left a comment

Choose a reason for hiding this comment

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

New policies and terminator will be deployed shortly, couple of things here I found while testing those.

tests/integration/targets/elasticache/defaults/main.yml Outdated Show resolved Hide resolved
tests/integration/targets/elasticache/tasks/main.yml Outdated Show resolved Hide resolved
tests/integration/targets/elasticache/tasks/main.yml Outdated Show resolved Hide resolved
tests/integration/targets/elasticache/tasks/main.yml Outdated Show resolved Hide resolved
tests/integration/targets/elasticache/tasks/main.yml Outdated Show resolved Hide resolved
@stefanhorning
Copy link
Contributor Author

stefanhorning commented Feb 26, 2021

@jillr I think I addressed all your comments and a few more issues. I hope tests will run through now. Once green this PR can be merged.

@stefanhorning
Copy link
Contributor Author

Tests are running through now. Just get an error in one of the python versions (running in parallel to the others):

"code": "InsufficientCacheClusterCapacity", 
"message": "Cannot create a cache cluster because there is no availability zone corresponding to the subnets in the cache subnet group with sufficient capacity."

This is probably due to an AWS limit that get's hit if all the parallel tests try to create a cluster. @jillr Probably we need to increase the cache cluster limits for that AWS account.

Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

changelogs/fragments/410-elasticache-fixes.yml Outdated Show resolved Hide resolved
changelogs/fragments/410-elasticache-fixes.yml Outdated Show resolved Hide resolved
tests/integration/targets/elasticache/aliases Outdated Show resolved Hide resolved
@jillr
Copy link
Collaborator

jillr commented Apr 19, 2021

recheck

Copy link
Collaborator

@jillr jillr left a comment

Choose a reason for hiding this comment

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

Tests pass locally and LGTM once CI (excluding shippable) is green

@tremble
Copy link
Contributor

tremble commented Apr 20, 2021

recheck

@jillr jillr added the gate label Apr 20, 2021
@ansible-zuul ansible-zuul bot merged commit dbf3697 into ansible-collections:main Apr 20, 2021
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 15, 2021
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 16, 2021
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 17, 2021
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request May 25, 2022
…_ec2_vpc_nat_gateway

Migrate ec2_vpc_nat_gateway* modules and tests

Migrate ec2_vpc_nat_gateway* modules and tests

Reviewed-by: Jill R <None>
Reviewed-by: Alina Buzachis <None>
Reviewed-by: Gonéri Le Bouder <[email protected]>
Reviewed-by: Mark Chappell <None>
Reviewed-by: Mike Graves <[email protected]>
Reviewed-by: None <None>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug integration tests/integration module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants