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

Aws ssm version support #61045

Merged
merged 1 commit into from Sep 12, 2019

Conversation

@dangothemango
Copy link
Contributor

commented Aug 21, 2019

SUMMARY

AWS parameter store supports parameter versioning. The api allows for this with the syntax 'parameterName:versionNumber'. Currently, the aws_ssm module performs a string comparison on input parameters vs returned parameter names. When a version number is included, said string comparison fails as the name of the parameter returned by the api does not include the ':versionNumber'.

This is a simple change to remove the ':versionNumber' from the parameterName before the string comparison is performed.

I chose not to include version as an additional parameter both to capitalize on boto3's native functionality and to avoid weird collisions when using the bypath option of the module.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

aws_ssm

ADDITIONAL INFORMATION
{{ lookup('aws_ssm', '/os/test/uw-deploy/orgDataDir.db.pass:10', region='us-east-1') }}

Given this example, looking for version 10 of a parameter the boto3 response body looks like this

{
  "InvalidParameters": [],
  "Parameters": [
    {
      "Name": "/os/test/uw-deploy/orgDataDir.db.pass",
      "LastModifiedDate": "datetime.datetime(2019, 8, 20, 20, 20, 5, 666000, tzinfo=tzlocal())",
      "Value": "****",
      "Selector": ":1",
      "Version": 10,
      "Type": "SecureString",
      "ARN": "arn:aws:ssm:us-east-1:591355805062:parameter/os/test/uw-deploy/orgDataDir.db.pass"
    }
  ]
}

since "/os/test/uw-deploy/orgDataDir.db.pass" != '/os/test/uw-deploy/orgDataDir.db.pass:10' this falls into the else block and throws the AnsibleError on line 229.

When given a version number that doesnt exist, the behavior is as such:

{{ lookup('aws_ssm', '/os/test/uw-deploy/orgDataDir.db.pass:11', region='us-east-1') }}

{
  "InvalidParameters": ["/os/test/uw-deploy/orgDataDir.db.pass:11"],
  "Parameters": []
}

Because in this case the version number is included in the response, we do not want to remove it when performing the string comparison. If something that is not a number is passed after the colon, the behavior is the same.

Since aws does not allow colons in parameter names this should not cause any issues.

@dangothemango

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

ready_for_review

@damemi

This comment has been minimized.

Copy link

commented Aug 26, 2019

@dangothemango you should probably squash these 2 commits into 1

@dangothemango dangothemango force-pushed the dangothemango:aws_ssm_version_support branch from 68f165e to 95b5884 Aug 26, 2019

@dangothemango

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

@damemi I figured the Ansible team would squash my commits when merging so they had more control over the paperwork. Either way, the commits have been squashed

@dangothemango

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

The pipeline failed due to something not related to my changes

@gundalow

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

@dangothemango You are correct, no need to squash commits. We do that at merge time.

@damemi Have you tested this PR, does it work?

I've triggered a rerun of CI, hopefully that will clear the instability.

@gundalow gundalow added the pr_day label Sep 3, 2019

@gundalow gundalow self-assigned this Sep 3, 2019

@gundalow

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

CI is green @dangothemango @damemi is this good to merge now?

@damemi

This comment has been minimized.

Copy link

commented Sep 4, 2019

@gundalow for full disclosure I am not a contributor to this repo (as you might be able to tell by me incorrectly suggesting that @dangothemango should squash his commits), so reviewing/confirming this fix is beyond my ability. He originally pinged me about this PR, and in the spirit of good open-source I simply tried to bring it to the attention of more qualified maintainers such as yourself.

I would confirm with @dangothemango that he has tested this and merge at your discretion

@gundalow

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

@damemi Hi, thanks for the prompt and detailed reply.

@dangothemango

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

@gundalow I partially tested the change. Due to the simplicity of it and it being my first time committing to the project, rather than set up an entire development environment I simply extracted the changed part and verified that the loop behaved as expected with the responses I could see from boto3 in the current version of ansible

@gundalow

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

@dangothemango that's a perfectly sensible way of testing this. Thank you.

Given this I'll merge into devel for release in Ansible 2.10

@gundalow

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

rebuild_merge

@ansibot ansibot added shipit and removed community_review labels Sep 4, 2019

@gundalow gundalow merged commit 3dbf89e into ansible:devel Sep 12, 2019

1 check passed

Shippable Run 141834 status is SUCCESS.
Details
@gundalow

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

@dangothemango @damemi Thank you.

Merged into devel for release in Ansible 2.10. If you'd like this fix in Ansible 2.9 please raise a backport PR (including a changelog/fragment) https://docs.ansible.com/ansible/latest/community/development_process.html#making-your-pr-merge-worthy

@sivel sivel removed the needs_triage label Sep 12, 2019

vasilyprokopov added a commit to vasilyprokopov/ansible that referenced this pull request Sep 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.