Fix aws_ssm fails when gathering facts #583

richardsonky · 2021-05-26T19:49:43Z

Depends-On: #621

SUMMARY

Fixes #113

aws_ssm fails when gathering facts. This fix incorporates some code submitted by others, but also works on windows which others haven't done yet.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

aws_ssm

ADDITIONAL INFORMATION

Fixes aws_ssm connection plugin when gathering facts. Uses code a few others submitted but also fixes windows.
Changed logic in the exec_command, wrap_command, and post_process functions that help to unify the output from windows and linux to make it easier to locate return values of the commands.

We have been running this in our environment for over a month and it is working without issue against win2016, win2019, centos7, suse15, and rhel7 ec2 instances.

tremble · 2021-05-27T06:51:54Z

Thanks for taking the time to submit this PR.

I've not got the time to review it right now, however the sanity tests are throwing some errors that should be fixed:

https://dashboard.zuul.ansible.com/t/ansible/build/c060c97822fe445fabfd681f5dfd6ca3/console

ERROR: Found 2 pep8 issue(s) which need to be resolved:
ERROR: plugins/connection/aws_ssm.py:447:14: E121: continuation line under-indented for hanging indent
ERROR: plugins/connection/aws_ssm.py:457:70: E261: at least two spaces before inline comment

richardsonky · 2021-05-27T21:06:31Z

I had to revert the linux cmd f(printf ...) statements that were suggested by others, it wasn't going to pass the python2.6 checks. I went back to the old cmd method and modified it with ';'s so it works as expected and passes the python2.6 tests.

I also modified a few of the logging lines while i was in here to have it log in vvvv so that it doesn't show up in debug logs, only in connection. I've tested in each OS we support and it is working as expected.

KFoxder · 2021-06-08T20:41:29Z

@tremble Should this be assigned to someone for review?

goneri · 2021-06-29T20:24:23Z

I had to revert the linux cmd f(printf ...) statements that were suggested by others, it wasn't going to pass the python2.6 checks. I went back to the old cmd method and modified it with ';'s so it works as expected and passes the python2.6 tests.

I also modified a few of the logging lines while i was in here to have it log in vvvv so that it doesn't show up in debug logs, only in connection. I've tested in each OS we support and it is working as expected.

For the record, we now only support Py38+. It's a pretty recent change thus.

goneri · 2021-06-29T20:43:45Z

Can you quickly explain how you do you tests? Maybe you've got a test playbook?

richardsonky · 2021-06-29T20:53:53Z

Our tests are not really official but seem to work ok. I have a CF template to build an ec2 instance for each OS we support at our company. We have a system in place to kick off our playbooks using cloudwatch events, step functions, and fargate. It will run through our standard playbook to configure the ec2. We visually look at ansible logs to see if the playbooks ran correctly. I've found a few bugs using this test so it seems work, though not very automated and probably doesn't test every possibility.

wilinger · 2021-07-01T02:54:06Z

Did you also test on Ubuntu 20.04? Works on Amazon Linux but not Ubuntu for me

An exception occurred during task execution. To see the full traceback, use -vvv. The error was: ValueError: invalid literal for int() with base 10: "echo $'\\n'$?" fatal: [test_ubuntu1]: FAILED! => {"msg": "Unexpected failure during module execution.", "stdout": ""}

richardsonky · 2021-07-01T14:00:52Z

I did not test ubuntu, it's not one of the OSs we support, however i think i found the problem. There were two issues. The first is it did not detect the proper command output for ubuntu, so i modified the check to ensure that it finds the right output. Once that worked it then did not like the old method of wrapping the command, but it does work with the new printf statements. I put the printf statements back and ubuntu is working. I don't have an extended playbook for ubuntu, i pretty much just did a quick gather facts and it works. I also tested ubuntu 18.04 and it works now as well.

I redid my tests for each OS we support and they work still. So if the automated testing doesn't check old python versions anymore then it should pass those checks.

Let me know if that works ok for you now.

richardsonky · 2021-07-01T16:11:35Z

Looks like it's still failing on those older versions of python that do not support f strings. Do we need to find a solution that works with all those older versions still?

wilinger · 2021-07-01T17:36:31Z

I did not test ubuntu, it's not one of the OSs we support, however i think i found the problem. There were two issues. The first is it did not detect the proper command output for ubuntu, so i modified the check to ensure that it finds the right output. Once that worked it then did not like the old method of wrapping the command, but it does work with the new printf statements. I put the printf statements back and ubuntu is working. I don't have an extended playbook for ubuntu, i pretty much just did a quick gather facts and it works. I also tested ubuntu 18.04 and it works now as well.

I redid my tests for each OS we support and they work still. So if the automated testing doesn't check old python versions anymore then it should pass those checks.

Let me know if that works ok for you now.

The change seemed to have broke something because it's not working for both Ubuntu and Amazon Linux now. Counts down 'EXEC remaining' and loops. What seemed to work for me for both Amazon Linux and Ubuntu was the patched code posted on #113

<i-01a11b1adbdd11111> EXEC stdout line: 
<i-01a11b1adbdd11111> EXEC stdout line: Starting session with SessionId: botocore-session-11111111111-0f1db111f64ee111c
<i-01a11b1adbdd11111> EXEC remaining: 60
<i-01a11b1adbdd11111> EXEC remaining: 59
<i-01a11b1adbdd11111> EXEC stdout line: $ stty -echo
<i-01a11b1adbdd11111> EXEC stdout line: PS1=''
<i-01a11b1adbdd11111> EXEC stdout line: printf '%s\n' 'VKbBEZmnukenWpSHnhjpeLsytB';
<i-01a11b1adbdd11111> EXEC stdout line: echo | ( umask 77 && mkdir -p "` echo /tmp `"&& mkdir "` echo /tmp/ansible-tmp-1625158982.2448153-792605-17192929165503 `" && echo ansible-tmp-1625158982.2448153-792605-17192929165111="` echo /tmp/ansible-tmp-1625158982.2448153-792605-17192929165503 `" );
<i-01a11b1adbdd11111> EXEC stdout line: printf '\n%s\n%s\n' "$?" 'iWzKicCLGXsTVUgEgQFTjzkFtq';
<i-01a11b1adbdd11111> EXEC stdout line: $ VKbBEZmnukenWpSHnhjpeLsysa
<i-01a11b1adbdd11111> EXEC stdout line: ansible-tmp-1625158982.2448153-792605-17192929165111=/tmp/ansible-tmp-1625158982.2448153-792605-17192929165111
<i-01a11b1adbdd11111> EXEC stdout line: 
<i-01a11b1adbdd11111> EXEC stdout line: 0
<i-01a11b1adbdd11111> EXEC stdout line: iWzKicCLGXsTVUgEgQFTjzkFtq
<i-01a11b1adbdd11111> EXEC remaining: 58
<i-01a11b1adbdd11111> EXEC remaining: 57
<i-01a11b1adbdd11111> EXEC remaining: 56
<i-01a11b1adbdd11111> EXEC remaining: 55
<i-01a11b1adbdd11111> EXEC remaining: 54

richardsonky · 2021-07-02T12:38:11Z

Sorry about that, i had a typo when i transferred the edits to this repo. Please test again and let me know if that worked. I've tested on ansible linux and ubuntu and it appears to work ok for me.

wilinger · 2021-07-02T15:43:53Z

@richardsonky It's working now on both Ubuntu and Amazon Linux now. Thanks for the fix!

richardsonky · 2021-07-02T16:40:41Z

I had to revert the linux cmd f(printf ...) statements that were suggested by others, it wasn't going to pass the python2.6 checks. I went back to the old cmd method and modified it with ';'s so it works as expected and passes the python2.6 tests.
I also modified a few of the logging lines while i was in here to have it log in vvvv so that it doesn't show up in debug logs, only in connection. I've tested in each OS we support and it is working as expected.

For the record, we now only support Py38+. It's a pretty recent change thus.

@goneri
Looks like the checks for older python version is still a problem since they don't support f strings. Do we need to fix the code to work with those older versions?

goneri · 2021-07-02T20:58:16Z

I had to revert the linux cmd f(printf ...) statements that were suggested by others, it wasn't going to pass the python2.6 checks. I went back to the old cmd method and modified it with ';'s so it works as expected and passes the python2.6 tests.
I also modified a few of the logging lines while i was in here to have it log in vvvv so that it doesn't show up in debug logs, only in connection. I've tested in each OS we support and it is working as expected.

For the record, we now only support Py38+. It's a pretty recent change thus.

@goneri
Looks like the checks for older python version is still a problem since they don't support f strings. Do we need to fix the code to work with those older versions?

I mean, we support py38+ on the controller side. We continue to support py2 on the remote nodes.

goneri · 2021-07-02T21:04:10Z

Ansible-test still checks with Python 2.6 to 3.7. To keep it quiet, we use ignore files to disable some checks: https://github.com/ansible-collections/community.aws/tree/main/tests/sanity

goneri · 2021-07-02T21:09:34Z

recheck

goneri · 2021-07-02T21:09:56Z

I've just pushed #621 to extend these filters.

jillr · 2021-07-07T22:09:59Z

For the record, we now only support Py38+. It's a pretty recent change thus.

We support 3.6+ for the AWS collections, but we definitely don't need to be testing on 2.x any longer.

damoun · 2021-09-30T16:45:41Z

Is there any news about this PR ? Looks like #621 is not needed anymore.

vranyes · 2021-12-13T22:48:25Z

This PR fixes this issue for me as well - any chance this can be merged?

kloveday2 · 2022-02-22T20:43:13Z

This PR also fixes the issue for me. What left to be done to get it merged?

softwarefactory-project-zuul · 2022-02-23T12:39:39Z

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

softwarefactory-project-zuul · 2022-02-23T12:41:16Z

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

softwarefactory-project-zuul · 2022-02-23T13:33:45Z

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

remove unnecessary changes add fragment fix changelog fix pep8 issues fix indent modification to pass python2.6 tests update to fix ubuntu. need to use new printf method fix typo in condition

ansibullbot · 2022-02-23T13:40:18Z

@richardsonky This PR was evaluated as a potentially problematic PR for the following reasons:

More than 50 changed files.

Such PR can only be merged by human. Contact a Core team member to review this PR on IRC: #ansible-devel on irc.freenode.net

click here for bot help

softwarefactory-project-zuul · 2022-02-23T13:40:19Z

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

richardsonky · 2022-02-23T13:44:47Z

not sure what i'm doing, so closing and will reopen a new one

Leon-Africa · 2022-08-29T12:31:59Z

Reference: #948 (comment)

aws_ec2 fix hostnames SUMMARY aws_ec2- fix hostnames bugs Should fix ansible-collections#582 and ansible-collections#583 Fix returned hosts when hostnames: - tag:Tag1,Tag2 Fix returned hosts when hostnames: - tag:Tag1 - tag:Tag2 Fix returned hosts when hostnames: - tag:Tag1=Test1,Tag2=Test2 Given and EC2 instance with the following tags tags: Tag1: Test1 Tag2: Test2 Instead of only returning "aws_ec2": { "hosts": [ "Test1" ] }, "tag_Name_instance_02": { "hosts": [ "Test1" ] }, "tag_Tag1_Test1": { "hosts": [ "Test1" ] }, "tag_Tag2_Test2": { "hosts": [ "Test1" ] } It returns now { "_meta": { "hostvars": { "Test1": { "ami_launch_index": 0, "ansible_host": "10.210.0.101", ...}, "Test2": { "ami_launch_index": 0, "ansible_host": "10.210.0.101", ...}, }, "all": { "children": [ "aws_ec2", "tag_Name_instance_02", "tag_Tag1_Test1", "tag_Tag2_Test2", "ungrouped" ] }, "aws_ec2": { "hosts": [ "Test1", "Test2" ] }, "tag_Name_instance_02": { "hosts": [ "Test1", "Test2" ] }, "tag_Tag1_Test1": { "hosts": [ "Test1", "Test2" ] }, "tag_Tag2_Test2": { "hosts": [ "Test1", "Test2" ] } } ISSUE TYPE Bugfix Pull Request COMPONENT NAME aws_ec2 ADDITIONAL INFORMATION Reviewed-by: Mark Chappell <None> Reviewed-by: Alina Buzachis <None> Reviewed-by: Markus Bergholz <[email protected]>

ansibullbot added bug This issue/PR relates to a bug community_review connection connection plugin needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) labels May 26, 2021

gravesm removed the needs_triage label Jun 29, 2021

gravesm requested review from jillr and goneri June 29, 2021 20:04

wilinger mentioned this pull request Aug 21, 2021

ssm connection plugin fails at gathering facts #113

Closed

ansibullbot added the has_issue label Sep 30, 2021

changes to fix ssm for linux and windows

1677f98

remove unnecessary changes add fragment fix changelog fix pep8 issues fix indent modification to pass python2.6 tests update to fix ubuntu. need to use new printf method fix typo in condition

ansibullbot added 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 labels Feb 23, 2022

richardsonky closed this Feb 23, 2022

richardsonky mentioned this pull request Feb 23, 2022

fix for ssm gather fact failure #948

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix aws_ssm fails when gathering facts #583

Fix aws_ssm fails when gathering facts #583

richardsonky commented May 26, 2021 •

edited by goneri

Loading

tremble commented May 27, 2021 •

edited

Loading

richardsonky commented May 27, 2021

KFoxder commented Jun 8, 2021

goneri commented Jun 29, 2021

goneri commented Jun 29, 2021

richardsonky commented Jun 29, 2021 •

edited

Loading

wilinger commented Jul 1, 2021

richardsonky commented Jul 1, 2021

richardsonky commented Jul 1, 2021

wilinger commented Jul 1, 2021 •

edited

Loading

richardsonky commented Jul 2, 2021

wilinger commented Jul 2, 2021

richardsonky commented Jul 2, 2021

goneri commented Jul 2, 2021

goneri commented Jul 2, 2021

goneri commented Jul 2, 2021

goneri commented Jul 2, 2021

jillr commented Jul 7, 2021

damoun commented Sep 30, 2021

vranyes commented Dec 13, 2021

kloveday2 commented Feb 22, 2022

softwarefactory-project-zuul bot commented Feb 23, 2022

softwarefactory-project-zuul bot commented Feb 23, 2022

softwarefactory-project-zuul bot commented Feb 23, 2022

ansibullbot commented Feb 23, 2022

softwarefactory-project-zuul bot commented Feb 23, 2022

richardsonky commented Feb 23, 2022

Leon-Africa commented Aug 29, 2022

Fix aws_ssm fails when gathering facts #583

Fix aws_ssm fails when gathering facts #583

Conversation

richardsonky commented May 26, 2021 • edited by goneri Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

tremble commented May 27, 2021 • edited Loading

richardsonky commented May 27, 2021

KFoxder commented Jun 8, 2021

goneri commented Jun 29, 2021

goneri commented Jun 29, 2021

richardsonky commented Jun 29, 2021 • edited Loading

wilinger commented Jul 1, 2021

richardsonky commented Jul 1, 2021

richardsonky commented Jul 1, 2021

wilinger commented Jul 1, 2021 • edited Loading

richardsonky commented Jul 2, 2021

wilinger commented Jul 2, 2021

richardsonky commented Jul 2, 2021

goneri commented Jul 2, 2021

goneri commented Jul 2, 2021

goneri commented Jul 2, 2021

goneri commented Jul 2, 2021

jillr commented Jul 7, 2021

damoun commented Sep 30, 2021

vranyes commented Dec 13, 2021

kloveday2 commented Feb 22, 2022

softwarefactory-project-zuul bot commented Feb 23, 2022

softwarefactory-project-zuul bot commented Feb 23, 2022

softwarefactory-project-zuul bot commented Feb 23, 2022

ansibullbot commented Feb 23, 2022

softwarefactory-project-zuul bot commented Feb 23, 2022

richardsonky commented Feb 23, 2022

Leon-Africa commented Aug 29, 2022

richardsonky commented May 26, 2021 •

edited by goneri

Loading

tremble commented May 27, 2021 •

edited

Loading

richardsonky commented Jun 29, 2021 •

edited

Loading

wilinger commented Jul 1, 2021 •

edited

Loading