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 when Cluster Parameter Group is specified in rds_cluster.py #1417

Conversation

imesias
Copy link
Contributor

@imesias imesias commented Aug 19, 2022

SUMMARY

Fix KeyError when comparing state.
Fixes: #1409

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

rds_cluster.py

@imesias imesias changed the title [WIP] Fix DBClusterParameterGroupName Key Error when comparing state [WIP] Fix KeyError when Cluster Parameter Group is specified in rds_cluster.py Aug 19, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ ansible-galaxy-importer SUCCESS in 4m 27s
✔️ build-ansible-collection SUCCESS in 4m 57s
✔️ ansible-test-sanity-docker-devel SUCCESS in 10m 57s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 10m 11s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 10m 12s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 12m 58s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 6m 16s
✔️ ansible-test-units-community-aws-python39 SUCCESS in 6m 52s
✔️ ansible-test-splitter SUCCESS in 2m 54s
✔️ integration-community.aws-1 SUCCESS in 11m 24s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@ansibullbot
Copy link

@ansibullbot ansibullbot added WIP Work in progress bug This issue/PR relates to a bug has_issue module module needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) small_patch Hopefully easy to review labels Aug 19, 2022
@imesias imesias changed the title [WIP] Fix KeyError when Cluster Parameter Group is specified in rds_cluster.py Fix KeyError when Cluster Parameter Group is specified in rds_cluster.py Aug 19, 2022
@markuman
Copy link
Member

@imesias thank you for your PR.
Can you expand the integration test tests/integration/targets/rds_cluster with the case of your origin issue?
Another thing what every PR needs, is a changelog fragment

@ansibullbot ansibullbot added community_review and removed WIP Work in progress needs_triage labels Aug 19, 2022
@imesias
Copy link
Contributor Author

imesias commented Aug 19, 2022

@markuman I've added the changelog fragment and I guess the below should be a sufficient test, problem is there is no db cluster parameter group in the collection to create a cluster parameter group. I could use the cli as a workaround. Please let me know if that will work.

#... some task before to create the parameter group ...
  - name: Modify DB cluster parameter group
    rds_cluster:
      id: "{{ cluster_id }}"
      state: present
      db_cluster_parameter_group_name: "{{ new_db_parameter_group_name }}"
    register: _result_modify_db_parameter_group_name

  - assert:
      that:
        - _result_modify_db_parameter_group_name.changed
        - "'allocated_storage' in _result_modify_db_parameter_group_name"
        - _result_modify_db_parameter_group_name.allocated_storage == 1
        - "'cluster_create_time' in _result_modify_db_parameter_group_name"
        - _result_modify_db_parameter_group_name.copy_tags_to_snapshot == false
        - "'db_cluster_arn' in _result_modify_db_parameter_group_name"
        - "_result_modify_db_parameter_group_name.db_cluster_identifier == '{{ cluster_id }}'"
        - "'db_cluster_parameter_group' in _result_modify_db_parameter_group_name"
        - "'db_cluster_resource_id' in _result_modify_db_parameter_group_name"
        - "'endpoint' in _result_modify_db_parameter_group_name"
        - "'engine' in _result_modify_db_parameter_group_name"
        - _result_modify_db_parameter_group_name.engine == "{{ engine }}"
        - "'engine_mode' in _result_modify_db_parameter_group_name"
        - _result_modify_db_parameter_group_name.engine_mode == "provisioned"
        - "'engine_version' in _result_modify_db_parameter_group_name"
        - "'master_username' in _result_modify_db_parameter_group_name"
        - _result_modify_db_parameter_group_name.master_username == "{{ username }}"
        - "'port' in _result_modify_db_parameter_group_name"
        - _result_modify_db_parameter_group_name.db_cluster_parameter_group == "{{ new_db_parameter_group_name }}"
        - "'status' in _result_modify_db_parameter_group_name"
        - _result_modify_db_parameter_group_name.status == "available"
        - "'tags' in _result_modify_db_parameter_group_name"
        - "'vpc_security_groups' in _result_modify_db_parameter_group_name"

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ ansible-galaxy-importer SUCCESS in 3m 34s
✔️ build-ansible-collection SUCCESS in 5m 19s
✔️ ansible-test-sanity-docker-devel SUCCESS in 12m 44s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 9m 59s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 10m 33s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 8m 59s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 6m 19s
✔️ ansible-test-units-community-aws-python39 SUCCESS in 6m 07s
✔️ ansible-test-splitter SUCCESS in 2m 27s
✔️ integration-community.aws-1 SUCCESS in 11m 15s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@markuman
Copy link
Member

@imesias hm, thereretically yes. like https://github.com/ansible-collections/amazon.aws/blob/ad7a0eb22a3e108451959eceff4f1bd78ea8be7c/tests/integration/targets/ec2_instance/roles/ec2_instance/tasks/termination_protection.yml#L47-L54
But it's a changing aws call in this case, and we must take care that the ressources are also removed when the test is finished (succeeded or failed).
What do you think @alinabuzachis?

@imesias
Copy link
Contributor Author

imesias commented Aug 19, 2022

@markuman @alinabuzachis Expanded integration tests for parameter group test case. Uses cli to create the parameter group and a cleanup task added as well.

@alinabuzachis
Copy link
Contributor

@imesias hm, thereretically yes. like https://github.com/ansible-collections/amazon.aws/blob/ad7a0eb22a3e108451959eceff4f1bd78ea8be7c/tests/integration/targets/ec2_instance/roles/ec2_instance/tasks/termination_protection.yml#L47-L54
But it's a changing aws call in this case, and we must take care that the ressources are also removed when the test is finished (succeeded or failed).
What do you think @alinabuzachis?

Agree. If I find some time I’ll work on adding this new module.

Copy link
Contributor

@alinabuzachis alinabuzachis left a comment

Choose a reason for hiding this comment

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

@imesias thank you for working on this. LGTM!

@ansibullbot ansibullbot added integration tests/integration tests tests and removed small_patch Hopefully easy to review labels Aug 19, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

✔️ ansible-galaxy-importer SUCCESS in 4m 23s
✔️ build-ansible-collection SUCCESS in 5m 06s
✔️ ansible-test-sanity-docker-devel SUCCESS in 10m 28s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 10m 04s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 12m 01s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 10m 02s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 6m 15s
✔️ ansible-test-units-community-aws-python39 SUCCESS in 5m 55s
✔️ ansible-test-splitter SUCCESS in 2m 35s
integration-community.aws-1 FAILURE in 17m 56s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@imesias
Copy link
Contributor Author

imesias commented Aug 19, 2022

@markuman @alinabuzachis hmmm, Seems there are 3 issues.

  1. It looks like I'll need to add engine to the playbook module args for that modify task.
TASK [rds_cluster : Modify DB cluster parameter group] *************************
task path: /home/zuul/.ansible/collections/ansible_collections/community/aws/tests/integration/targets/rds_cluster/roles/rds_cluster/tasks/test_modify.yml:179
The full traceback is:
Traceback (most recent call last):
  File "/tmp/ansible_rds_cluster_payload_f_en1hek/ansible_rds_cluster_payload.zip/ansible_collections/amazon/aws/plugins/module_utils/rds.py", line 206, in call_method
    result = AWSRetry.jittered_backoff(catch_extra_error_codes=retry_codes)(method)(**parameters)
  File "/tmp/ansible_rds_cluster_payload_f_en1hek/ansible_rds_cluster_payload.zip/ansible_collections/amazon/aws/plugins/module_utils/cloud.py", line 119, in _retry_wrapper
    return _retry_func(
  File "/tmp/ansible_rds_cluster_payload_f_en1hek/ansible_rds_cluster_payload.zip/ansible_collections/amazon/aws/plugins/module_utils/cloud.py", line 69, in _retry_func
    return func()
  File "/tmp/ansible_rds_cluster_payload_f_en1hek/ansible_rds_cluster_payload.zip/ansible_collections/amazon/aws/plugins/module_utils/modules.py", line 329, in deciding_wrapper
    return unwrapped(*args, **kwargs)
  File "/home/zuul/venv/lib/python3.9/site-packages/botocore/client.py", line 386, in _api_call
    return self._make_api_call(operation_name, kwargs)
  File "/home/zuul/venv/lib/python3.9/site-packages/botocore/client.py", line 677, in _make_api_call
    request_dict = self._convert_to_request_dict(
  File "/home/zuul/venv/lib/python3.9/site-packages/botocore/client.py", line 725, in _convert_to_request_dict
    request_dict = self._serializer.serialize_to_request(
  File "/home/zuul/venv/lib/python3.9/site-packages/botocore/validate.py", line 319, in serialize_to_request
    raise ParamValidationError(report=report.generate_report())
botocore.exceptions.ParamValidationError: Parameter validation failed:
Missing required parameter in input: "Engine"
  1. I will need to also look at why this conditional in the playbook is evaluating to false in the CI environment.
TASK [rds_cluster : Create DB cluster parameter group if not exists] ***********
task path: /home/zuul/.ansible/collections/ansible_collections/community/aws/tests/integration/targets/rds_cluster/roles/rds_cluster/tasks/test_modify.yml:168
skipping: [modify] => {
    "changed": false,
    "skip_reason": "Conditional result was False"
}
  1. It appears the attached role does not have permission to remove/cleanup a db cluster parameter group.
"stderr": "\nAn error occurred (AccessDenied) when calling the DeleteDBClusterParameterGroup operation: User: arn:aws:sts::XXXXXXXXXXX:assumed-role/ansible-core-ci-test-prod/prod=remote=zuul-cloud is not authorized to perform: rds:DeleteDBClusterParameterGroup on resource: arn:aws:rds:us-east-1:XXXXXXXXX:cluster-pg:test-db-parameter-group-2688144c4346-new because no identity-based policy allows the rds:DeleteDBClusterParameterGroup action"

I will have a look at resolving the first two, not sure who I can talk to about the number 3.

Thanks!

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

✔️ ansible-galaxy-importer SUCCESS in 4m 05s
✔️ build-ansible-collection SUCCESS in 4m 55s
✔️ ansible-test-sanity-docker-devel SUCCESS in 10m 24s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 10m 16s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 11m 42s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 10m 35s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 6m 09s
✔️ ansible-test-units-community-aws-python39 SUCCESS in 6m 10s
✔️ ansible-test-splitter SUCCESS in 2m 32s
integration-community.aws-1 FAILURE in 17m 01s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@imesias
Copy link
Contributor Author

imesias commented Aug 22, 2022

Hey @markuman @alinabuzachis, The CI AWS credentials require permission to create and remove a DBClusterParameterGroup can you please add permission rds:CreateDBClusterParameterGroup and rds:DeleteDBClusterParameterGroup to the appropriate role to allow the expanded integration test to succeed.

Kind Regards,

@imesias imesias force-pushed the im-bugfix-rds-cluster-param-grp branch from fbc1118 to c5b0a70 Compare August 28, 2022 07:20
@ansibullbot ansibullbot added community_review and removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Aug 28, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

✔️ ansible-galaxy-importer SUCCESS in 3m 47s
✔️ build-ansible-collection SUCCESS in 5m 21s
✔️ ansible-test-sanity-docker-devel SUCCESS in 9m 17s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 9m 06s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 11m 22s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 9m 00s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 7m 08s
✔️ ansible-test-units-community-aws-python39 SUCCESS in 5m 06s
✔️ ansible-test-splitter SUCCESS in 2m 34s
integration-community.aws-1 FAILURE in 17m 22s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ ansible-galaxy-importer SUCCESS in 3m 56s
✔️ build-ansible-collection SUCCESS in 5m 39s
✔️ ansible-test-sanity-docker-devel SUCCESS in 12m 11s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 10m 17s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 10m 20s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 8m 58s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 6m 52s
✔️ ansible-test-units-community-aws-python39 SUCCESS in 5m 55s
✔️ ansible-test-splitter SUCCESS in 3m 02s
✔️ integration-community.aws-1 SUCCESS in 13m 11s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@imesias
Copy link
Contributor Author

imesias commented Sep 5, 2022

@gravesm Thanks for the assist and update. Much appreciated. @alinabuzachis @markuman integ tests are now passing.

@markuman markuman added backport-3 PR should be backported to the stable-3 branch backport-4 PR should be backported to the stable-4 branch labels Sep 5, 2022
@markuman markuman added the mergeit Merge the PR (SoftwareFactory) label Sep 5, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

✔️ ansible-galaxy-importer SUCCESS in 4m 03s
✔️ build-ansible-collection SUCCESS in 6m 25s
✔️ ansible-test-sanity-docker-devel SUCCESS in 9m 05s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 9m 00s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 11m 35s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 10m 07s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 6m 52s
✔️ ansible-test-units-community-aws-python39 SUCCESS in 7m 00s
✔️ ansible-test-splitter SUCCESS in 2m 32s
✔️ integration-community.aws-1 SUCCESS in 14m 19s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit b3bc689 into ansible-collections:main Sep 5, 2022
@patchback
Copy link

patchback bot commented Sep 5, 2022

Backport to stable-3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-3/b3bc68931de04fb2d8558b01c45988710d24cec3/pr-1417

Backported as #1432

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Sep 5, 2022
….py (#1417)

Fix KeyError when Cluster Parameter Group is specified in rds_cluster.py

SUMMARY
Fix KeyError when comparing state.
Fixes: #1409
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
rds_cluster.py

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Markus Bergholz <[email protected]>
(cherry picked from commit b3bc689)
@patchback
Copy link

patchback bot commented Sep 5, 2022

Backport to stable-4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4/b3bc68931de04fb2d8558b01c45988710d24cec3/pr-1417

Backported as #1433

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Sep 5, 2022
….py (#1417)

Fix KeyError when Cluster Parameter Group is specified in rds_cluster.py

SUMMARY
Fix KeyError when comparing state.
Fixes: #1409
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
rds_cluster.py

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Markus Bergholz <[email protected]>
(cherry picked from commit b3bc689)
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Sep 28, 2022
….py (#1417) (#1433)

[PR #1417/b3bc6893 backport][stable-4] Fix KeyError when Cluster Parameter Group is specified in rds_cluster.py

This is a backport of PR #1417 as merged into main (b3bc689).
SUMMARY
Fix KeyError when comparing state.
Fixes: #1409 
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
rds_cluster.py

Reviewed-by: Mark Chappell <None>
abikouo pushed a commit to abikouo/community.aws that referenced this pull request Oct 24, 2023
….py (ansible-collections#1417)

Fix KeyError when Cluster Parameter Group is specified in rds_cluster.py

SUMMARY
Fix KeyError when comparing state.
Fixes: ansible-collections#1409
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
rds_cluster.py

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Markus Bergholz <[email protected]>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections@b3bc689
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-3 PR should be backported to the stable-3 branch backport-4 PR should be backported to the stable-4 branch bug This issue/PR relates to a bug community_review has_issue integration tests/integration mergeit Merge the PR (SoftwareFactory) module module new_contributor Help guide this first time contributor plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

community.aws.rds_cluster module fails to manage cluster after creating cluster
5 participants