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

feat: support for StackPolicyDuringUpdateBody #155

Merged
merged 14 commits into from
Mar 11, 2021

Conversation

fvant
Copy link
Contributor

@fvant fvant commented Sep 2, 2020

SUMMARY

Cloudformation supports a policy that applies to a particular update only. This implement the functionality provided by the aws cloudformation update-stack command with the --stack-policy-during-update-body option to provide a modified policy..

AWS CloudFormation applies the override policy only during this update. The override policy doesn't permanently change the stack policy.

This is former PR [ansible/ansible] Support stack policy on update (#57300)

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

lib/ansible/modules/cloud/amazon/cloudformation.py or modules/cloudformation.py

ADDITIONAL INFORMATION

Boto3 documentation of the parameter used:
https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/cloudformation.html#CloudFormation.Client.update_stack


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.

Thanks for taking the time to submit this change.

A couple of tiny niggles, but mostly looking good. In addition to the inline comments, please also

  1. Add a (minor_changes) changelog fragment: https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs-how-to
  2. Please add some tests to the integration test suite to demonstrate that the change works https://github.com/ansible-collections/amazon.aws/blob/main/tests/integration/targets/cloudformation/tasks/main.yml

plugins/modules/cloudformation.py Outdated Show resolved Hide resolved
plugins/modules/cloudformation.py Outdated Show resolved Hide resolved
@ansibullbot
Copy link

@ansibullbot ansibullbot added affects_2.10 feature This issue/PR relates to a feature request module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) labels Sep 9, 2020
fvant and others added 3 commits September 12, 2020 18:08
Co-authored-by: Mark Chappell <mchappel@redhat.com>
Co-authored-by: Mark Chappell <mchappel@redhat.com>
@fvant
Copy link
Contributor Author

fvant commented Sep 13, 2020

Thanks for taking the time to submit this change.

A couple of tiny niggles, but mostly looking good. In addition to the inline comments, please also

  1. Add a (minor_changes) changelog fragment: https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs-how-to

Done

  1. Please add some tests to the integration test suite to demonstrate that the change works https://github.com/ansible-collections/amazon.aws/blob/main/tests/integration/targets/cloudformation/tasks/main.yml

For this test i need to create a stack with a policy in the first place, that test is not there atm. The stack policy support now only handles a URL, not a body. Not sure which url to use when pointing to GH ?
Alternatively, this test is postponed and i add a new attribute for stack_policy_body: in addition to the current stack_policy:

@tremble
Copy link
Contributor

tremble commented Sep 16, 2020

For this test i need to create a stack with a policy in the first place, that test is not there atm. The stack policy support now only handles a URL, not a body. Not sure which url to use when pointing to GH ?

Looking at the code (down around line 690) it looks like it's actually StackPolicyBody that's being set. The file that you pass is being read directly by Ansible rather than being passed as a URL to CloudWatch

    if module.params['stack_policy'] is not None and not module.check_mode and not module.params['create_changeset']:
        with open(module.params['stack_policy'], 'r') as stack_policy_fh:
            stack_params['StackPolicyBody'] = stack_policy_fh.read()

I'd suspect that this was to work around a quirk in the way Ansible deals with JSON in YAML files (things can get magically converted in ways that break JSON if you're not careful). I'm not a big fan of doing things this way as it means you need to do things like writing out temporary files.

So, a couple of additional suggestions:

  1. Update your new parameter to be of type json (this avoids the problems I hinted at)
  2. For testing this PR: You can add a file in files/ (you may need to copy this to a temporary location where you know the absolute path to get this to behave) and then pass the filename to the module. The docs imply stack_policy: files/<filename>, but I'm not 100% this is correct.
  3. A new stack_policy_body parameter (either in this PR or another) would be good, then we could look at deprecating template, stack_policy and template (direct use of file_handle.read() doesn't use the search paths that folks might expect from the file lookup).

@alinabuzachis
Copy link
Contributor

Hello @fvant, are you still working on this?

waiting_on_contributor

@goneri goneri added the waiting_on_contributor Needs help. Feel free to engage to get things unblocked label Jan 22, 2021
@ansibullbot ansibullbot added integration tests/integration tests tests labels Jan 30, 2021
@fvant fvant requested a review from tremble January 31, 2021 13:28
@fvant
Copy link
Contributor Author

fvant commented Jan 31, 2021

@tremble the tests i added failed as setting a stack policy is not allowed, removing the tests until further

{"code": "AccessDenied", "message": "User: arn:aws:sts::966509639900:assumed-role/ansible-core-ci-test-prod/prod=shippable=ansible-collections=amazon.aws=1313.24 is not authorized to perform: cloudformation:SetStackPolicy on resource: arn:aws:cloudformation:us-east-1:966509639900:stack/shippable-1313-24/7e1cbd30-63c2-11eb-996f-126643f85ad9"

@jillr
Copy link
Collaborator

jillr commented Feb 19, 2021

fvant added a commit to fvant/aws-terminator that referenced this pull request Feb 21, 2021
Trying to make this PR pass ansible-collections/amazon.aws#155 which gives this error {"code": "AccessDenied", "message": "User: arn:aws:sts::966509639900:assumed-role/ansible-core-ci-test-prod/prod=shippable=ansible-collections=amazon.aws=1313.24 is not authorized to perform: cloudformation:SetStackPolicy on resource: arn:aws:cloudformation:us-east-1:966509639900:stack/shippable-1313-24/7e1cbd30-63c2-11eb-996f-126643f85ad9"
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.

Minor tweak to the changelog and re-enable the tests

plugins/modules/cloudformation.py Outdated Show resolved Hide resolved
plugins/modules/cloudformation.py Outdated Show resolved Hide resolved
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.

Tests now pass as written by the author, and I've tweaked the changelog. I think we're ready to merge this.

@ansibullbot ansibullbot added community_review and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Mar 11, 2021
@tremble tremble merged commit 9907865 into ansible-collections:main Mar 11, 2021
@tremble
Copy link
Contributor

tremble commented Mar 11, 2021

@fvant : Many thanks for your work on this. I'm sorry it's taken a while to get this merged, but we got there in the end...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.10 community_review feature This issue/PR relates to a feature request integration tests/integration module module new_contributor Help guide this first time contributor plugins plugin (any type) tests tests waiting_on_contributor Needs help. Feel free to engage to get things unblocked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants