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

add metadata options to ec2 template #322

Merged
merged 2 commits into from
Mar 14, 2021
Merged

add metadata options to ec2 template #322

merged 2 commits into from
Mar 14, 2021

Conversation

danquack
Copy link
Contributor

@danquack danquack commented Dec 7, 2020

SUMMARY

Looking to add the ability to manage launch templates metadata options

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

ec2_launch_template

ADDITIONAL INFORMATION

https://aws.amazon.com/blogs/security/defense-in-depth-open-firewalls-reverse-proxies-ssrf-vulnerabilities-ec2-instance-metadata-service/

   - name: Configure launch template
      ec2_launch_template:
       ...
        metadata_options:
          http_tokens: required
          http_put_response_hop_limit: 1

@danquack
Copy link
Contributor Author

danquack commented Dec 9, 2020

Not sure if there's anything else to include, looking for direction on what this might take to get a review?
cc previous commenters to PRs @gravesm @jillr @s-hertel @tremble @wimnat

@tremble
Copy link
Contributor

tremble commented Dec 10, 2020

Hi @danquack

The main thing that needs updating is the integration tests. That they're not failing is a good start and implies you've not broken existing behaviour, however the new behaviour should be tested:

tests/integration/targets/ec2_launch_template/playbooks/roles/ec2_launch_template/tasks/main.yml

The integration tests are just Ansible playbooks.

After that we also need

  1. a changelog entry: https://docs.ansible.com/ansible/latest/reference_appendices/release_and_maintenance.html#changelogs
  2. version_added fields added to the module doc: https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html#documentation-fields

Unfortunately it looks like our CI environment currently doesn't have the policies in place to allow the tests to run. I'll try to look into getting that added, in the mean time @jillr put together a blog post which should help with getting the tests running under your own AWS Account: https://www.ansible.com/blog/getting-started-with-aws-ansible-module-development

@danquack
Copy link
Contributor Author

Thank you for the feedback. I'll go ahead and work toward this. It wasn't inherently clear to me, but with your feedback, I can most certainly work toward meeting all the specifications!

@danquack
Copy link
Contributor Author

danquack commented Dec 11, 2020

@tremble or @jillr so i was trying to test this locally via the instructions, but I am struggling to get the local environment up and running. I cloned the repo into $HOME/.ansible/collections/ansible_collections/community/aws/, and added cloud-config-aws.ini.template to the root of that directory, but the tests still dont want to seem to run. Is there any guidance you can provide? I did see you opened #326 for in CI though

✗ ansible-test integration --color -v --retry-on-error --allow-unsupported ec2_launch_template --docker          
Falling back to tests in "tests/integration/targets/" because "roles/test/" was not found.
WARNING: Excluding tests marked "cloud/aws" which require config (see "/private/tmp/ansible/test/lib/ansible_test/config/cloud-config-aws.ini.template"): ec2_launch_template
WARNING: All targets skipped.

@tremble
Copy link
Contributor

tremble commented Dec 14, 2020

The clue is here:

WARNING: Excluding tests marked "cloud/aws" which require config (see "/private/tmp/ansible/test/lib/ansible_test/config/cloud-config-aws.ini.template"): ec2_launch_template

See the "Testing Locally" section of @jillr's blog post https://www.ansible.com/blog/getting-started-with-aws-ansible-module-development
tests/integration/cloud-config-aws.ini needs to exist and contain credentials for an AWS account.

@danquack
Copy link
Contributor Author

danquack commented Dec 14, 2020

@tremble Ive been trying to follow that blog post for setting up, and i've got past the creds issue now that ive moved it to the right directory, but am now seeing an issue where inside docker it cant find the collection. Any insights into this error?

(aws) ➜  aws git:(main) pwd                                                             
$HOME/.ansible/collections/ansible_collections/amazon/aws
(aws) ➜  aws git:(main) ansible-test integration ec2_launch_template --allow-unsupported --docker
...
+ ansible-playbook -i ../../inventory -v playbooks/version_fail.yml -e @/root/ansible_collections/amazon/aws/tests/output/.tmp/integration/ec2_launch_template-ge0bqoh7-ÅÑŚÌβŁÈ/tests/integration/config-file-1ifzunh5.json
Using /root/ansible_collections/amazon/aws/tests/output/.tmp/integration/ec2_launch_template-ge0bqoh7-ÅÑŚÌβŁÈ/tests/integration/integration.cfg as config file
[WARNING]: running playbook inside collection amazon.aws
[WARNING]: Skipping callback plugin 'aws_resource_actions', unable to load

PLAY [localhost] ***************************************************************

TASK [Gathering Facts] *********************************************************
ok: [localhost]

TASK [Include vars file in roles/ec2_instance/defaults/main.yml] ***************
ok: [localhost] => {"ansible_facts": {"ec2_ami_image": {"ap-northeast-1": "ami-571e3c30", "ap-northeast-2": "ami-97cb19f9", "ap-south-1": "ami-11f0837e", "ap-southeast-1": "ami-30318f53", "ap-southeast-2": "ami-24959b47", "ca-central-1": "ami-daeb57be", "eu-central-1": "ami-7cbc6e13", "eu-west-1": "ami-0d063c6b", "eu-west-2": "ami-c22236a6", "sa-east-1": "ami-864f2dea", "us-east-1": "ami-ae7bfdb8", "us-east-2": "ami-9cbf9bf9", "us-west-1": "ami-7c280d1c", "us-west-2": "ami-0c2aba6c"}, "resource_prefix": "ansible-test-default-group"}, "ansible_included_var_files": ["/root/ansible_collections/amazon/aws/tests/output/.tmp/integration/ec2_launch_template-ge0bqoh7-ÅÑŚÌβŁÈ/tests/integration/targets/ec2_launch_template/playbooks/roles/ec2_launch_template/defaults/main.yml"], "changed": false}

TASK [create c4.large template (failure expected)] *****************************
fatal: [localhost]: FAILED! => {"msg": "Could not find imported module support code for ansible_collections.amazon.aws.plugins.modules.ec2_launch_template.  Looked for (['ansible_collections.amazon.aws.plugins.module_utils.core.AnsibleAWSModule', 'ansible_collections.amazon.aws.plugins.module_utils.core'])"}

TASK [check that graceful error message is returned when creation with cpu_options and old botocore] ***
fatal: [localhost]: FAILED! => {
    "assertion": "ec2_lt.msg == \"ec2_launch_template requires boto3 >= 1.6.0\"",
    "changed": false,
    "evaluated_to": false,
    "msg": "Assertion failed"
}

TASK [delete the c4.large template just in case it was created] ****************
fatal: [localhost]: FAILED! => {"msg": "Could not find imported module support code for ansible_collections.amazon.aws.plugins.modules.ec2_launch_template.  Looked for (['ansible_collections.amazon.aws.plugins.module_utils.core.AnsibleAWSModule', 'ansible_collections.amazon.aws.plugins.module_utils.core'])"}
...ignoring

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.

In general we only run tests with ignore_errors where we explicitly expect a failure (we're testing that it rejects behaviour, possibly with a nice error). If we can get these tests passing, then I think this change if good to merge.

@danquack
Copy link
Contributor Author

@tremble fixed the build so i think were good to go on this

@danquack danquack requested a review from tremble December 17, 2020 20:25
@ansibullbot
Copy link

@danquack this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibullbot ansibullbot added feature This issue/PR relates to a feature request merge_commit This PR contains at least one merge commit. Please resolve! module module needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor labels Jan 13, 2021
@danquack
Copy link
Contributor Author

@ansibullbot ready_for_review

@ansibullbot
Copy link

@danquack This PR contains @ mentions in at least one commit message. Those mentions can cause cascading notifications through GitHub and need to be removed. Please squash or amend your commits to remove the mentions.

click here for bot help

@ansibullbot ansibullbot added integration tests/integration plugins plugin (any type) tests tests 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 Jan 17, 2021
@danquack
Copy link
Contributor Author

danquack commented Jan 19, 2021

@ansibullbot ready_for_review fixed the commit message

@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 Jan 20, 2021
@danquack
Copy link
Contributor Author

@tremble is there anything mre this needs to get community review? Ive never commited to this repo so i dont know what the path is for getting community approval

@danquack
Copy link
Contributor Author

danquack commented Feb 2, 2021

@tremble bump

@danquack
Copy link
Contributor Author

danquack commented Mar 8, 2021

@tremble is this something you can approve now? It’s not clear what this PR is waiting on for merge

@tremble
Copy link
Contributor

tremble commented Mar 8, 2021

Apologies for the delay getting back to you.

Two things missing: changelog and version_added. Since you've been waiting so long on me I'll get those added and we can get this merged.

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.

Assuming the tests pass we should be good to merge this.

@danquack
Copy link
Contributor Author

danquack commented Mar 8, 2021

Thank you!

@tremble tremble merged commit 94af64c into ansible-collections:main Mar 14, 2021
tremble pushed a commit to pjrm/community.aws that referenced this pull request Mar 14, 2021
danquixote pushed a commit to danquixote/community.aws that referenced this pull request May 16, 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
danielcotton pushed a commit to danielcotton/community.aws that referenced this pull request Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants