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 support for EC2 IMDSv2 (Instance Metadata Service) #43

Merged

Conversation

roadmapper
Copy link
Contributor

@roadmapper roadmapper commented May 3, 2020

Porting the PR from the main Ansible repo: ansible/ansible#68098

SUMMARY

In November 2019, AWS announced a new version of the instance metadata service (IMDSv2) that supports session authentication. Currently, if the ec2_metadata_facts module is used on an EC2 instance that only has IMDSv2 enabled the module will fail. This change adds support for getting the session token for IMDSv2.

Fixes ansible/ansible#67981

When the Instance Metadata Service HTTP endpoint is disabled, the module will fail to get facts:

$ aws ec2 modify-instance-metadata-options --instance-id <instance_id> --http-endpoint disabled
$ ansible all -i 'localhost,' -m ec2_metadata_facts -c local
localhost | FAILED! => {
    "ansible_facts": {
        "discovered_interpreter_python": "/usr/bin/python"
    },
    "changed": false,
    "msg": "Failed to retrieve metadata token from AWS: HTTP Error 403: Forbidden",
    "response": {
        "body": "",
        "connection": "close",
        "content-length": "0",
        "content-type": "text/plain",
        "date": "Sun, 22 Mar 2020 19:58:36 GMT",
        "msg": "HTTP Error 403: Forbidden",
        "server": "EC2ws",
        "status": 403,
        "url": "http://169.254.169.254/latest/api/token"
    }
}
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

ec2_metadata_facts

@jillr jillr changed the base branch from master to main July 2, 2020 19:22
@roadmapper
Copy link
Contributor Author

@jillr, do you know who I could reach out to to get this PR reviewed? Would love to be able to help get this change as it has been 8 months since Amazon introduced the newer version of the instance metadata service.

@ansibullbot
Copy link

@roadmapper this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibullbot ansibullbot added affects_2.10 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_triage new_contributor Help guide this first time contributor owner_pr PR created by owner/maintainer stale_ci CI is older than 7 days, rerun before merging labels Aug 19, 2020
@jillr jillr removed stale_ci CI is older than 7 days, rerun before merging needs_triage labels Aug 28, 2020
@Lauren-Buchholz-Moxe
Copy link

We are having the same issue, would love to get this fix merged and ready

@jgournet
Copy link

Is there any way to make this PR progress please ?
CC @jillr

@tremble
Copy link
Contributor

tremble commented Nov 23, 2020

The biggest thing to help get the PR merged would be to add some integration or unit tests. While there's some of the initial framework for an ec2_metadata_facts integration test, I don't see any real tests built out. Because of the way the CI infrastructure is set up unit tests may be simpler to build.

However, I'm also concerned that this looks like a backwards incompatible change. While many of the modules are initially designed and tested against AWS, there are a significant number of IaaS services which support things like the metadata interface but aren't AWS so might not support the IMDSv2 interfaces. (For example OpenStack: https://docs.openstack.org/nova/latest/admin/metadata-service.html) While these aren't the primary use case for this collection there are people who use the modules this way and it's generally preferable to avoid breaking things which did work. Personally I'd prefer if this behaviour was either opt-in, or defaulted to falling back to the v1 service.

@tremble
Copy link
Contributor

tremble commented Dec 3, 2020

@jillr is working on integration tests with #212

@roadmapper
Copy link
Contributor Author

However, I'm also concerned that this looks like a backwards incompatible change. While many of the modules are initially designed and tested against AWS, there are a significant number of IaaS services which support things like the metadata interface but aren't AWS so might not support the IMDSv2 interfaces. (For example OpenStack: https://docs.openstack.org/nova/latest/admin/metadata-service.html) While these aren't the primary use case for this collection there are people who use the modules this way and it's generally preferable to avoid breaking things which did work. Personally I'd prefer if this behaviour was either opt-in, or defaulted to falling back to the v1 service.

It's been a little while since I've tested this change, but from what I recall: if IMDSv2 is not enabled, requesting the token doesn't do anything (accepted by the metadata service and nothing is returned).

However, that being said, I'd be happy to change the module to accept a flag to opt-in to requesting a token for the IMDS. The point about having AWS-like environments using this module was not something I was aware of and is a good reason to make the feature opt-in.

If the feature is opt-in, should I change to documentation to point out that the use of this module without a flag will mean that if it is used on a IMDSv2 enabled EC2 instance, the module will fail?

@ansibullbot ansibullbot added has_issue needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jan 13, 2021
@tremble tremble force-pushed the add-support-for-ec2-imdsv2 branch from 051bd73 to b25a630 Compare March 13, 2021 12:25
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.

I've updated the code to use (200, 404) for this case I was worried about, and added a changelog. If the tests pass I think we're good to merge this PR.

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

@ansibullbot ansibullbot removed the merge_commit This PR contains at least one merge commit. Please resolve! label Mar 13, 2021
@ansibullbot ansibullbot added plugins plugin (any type) and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Mar 13, 2021
@tremble
Copy link
Contributor

tremble commented Mar 13, 2021

I've also tested this PR against OpenStack and it fails over cleanly.

@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 13, 2021
@tremble
Copy link
Contributor

tremble commented Mar 13, 2021

Tests have all passed. I've also manually tested this against OpenStack (which doesn't support IMDSv2) and it failed over cleanly.

Thanks for your submission, and I apologise for how long it took to get this PR merged.

@tremble tremble merged commit 359366a into ansible-collections:main Mar 13, 2021
@roadmapper
Copy link
Contributor Author

Thanks for helping to get this merged in! Appreciate the edits @tremble 👏

@r3r00t3d r3r00t3d mentioned this pull request Mar 26, 2021
@ado120
Copy link

ado120 commented May 18, 2021

What version of ansible has these changes?

@tremble
Copy link
Contributor

tremble commented Jul 8, 2021

@ado120 This feature should be available in the 1.5.0 release of this collection. I believe this is available with the Ansible 3.4.0 release: https://groups.google.com/g/ansible-devel/c/1buCqmI253g?pli=1

@roadmapper roadmapper deleted the add-support-for-ec2-imdsv2 branch September 12, 2021 18:28
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
…eds (ansible-collections#43)

* Fix policy arg to actually work with JSON strings that is needs. Also update docs.

* Fix typo in docs

* Fix long line in example

* Update type in docs too

Co-Authored-By: Mark Chappell <[email protected]>

* Remove unecessary documentation for aws_kms policy param

Co-Authored-By: Mark Chappell <[email protected]>

Co-authored-by: Mark Chappell <[email protected]>
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
…eds (ansible-collections#43)

* Fix policy arg to actually work with JSON strings that is needs. Also update docs.

* Fix typo in docs

* Fix long line in example

* Update type in docs too

Co-Authored-By: Mark Chappell <[email protected]>

* Remove unecessary documentation for aws_kms policy param

Co-Authored-By: Mark Chappell <[email protected]>

Co-authored-by: Mark Chappell <[email protected]>
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Oct 24, 2023
…eds (ansible-collections#43)

* Fix policy arg to actually work with JSON strings that is needs. Also update docs.

* Fix typo in docs

* Fix long line in example

* Update type in docs too

Co-Authored-By: Mark Chappell <[email protected]>

* Remove unecessary documentation for aws_kms policy param

Co-Authored-By: Mark Chappell <[email protected]>

Co-authored-by: Mark Chappell <[email protected]>
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 has_issue module module new_contributor Help guide this first time contributor owner_pr PR created by owner/maintainer plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Anasible EC2 Metadata Facts module not using new Metadata Service V2
7 participants