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

Fix/ssm bucket auth #854

Closed
wants to merge 7 commits into from
Closed

Conversation

gillg
Copy link

@gillg gillg commented Jan 8, 2022

SUMMARY

Duplicates stale #603 (I can't contribute directly on it)

When using ssm to connect to systems in aws it is required that we utilize an s3 bucket to transfer files from the source to the destination server. When the bucket resides in a different region than the destination server the wrong s3 endpoint is being selected. This adds a new configuration value, ansible_aws_ssm_bucket_region, to allow the s3 buckets region to be set directly allowing the transfer to occur as would be expected.
Prevent from bugs like

<Code>AuthorizationQueryParametersError</Code>
<Message>Error parsing the X-Amz-Credential parameter; the region 'us-west-2' is wrong; expecting 'us-east-1'</Message>

if you use an inventory like below where the region of the EC2 is dynamic inside the account.

plugin: amazon.aws.aws_ec2
[...]
compose:
  ansible_aws_ssm_region: placement.region
ISSUE TYPE
 * Bugfix Pull Request
COMPONENT NAME

plugins/connection/aws_ssm.py

Next improvement

Add 4 new vars ansible_aws_ssm_bucket_[profile|access_key_id|secret_access_key|session_token] to allow a uniq bucket shared accross multiple account or located in a central place.

@gillg
Copy link
Author

gillg commented Jan 8, 2022

@markuman I duplicated the PR #603 to try finish it

@@ -21,6 +21,7 @@ aws_ssm_linux
[aws_ssm:vars]
ansible_connection=community.aws.aws_ssm
ansible_aws_ssm_bucket_name={{s3_bucket_name}}
ansible_aws_ssm_bucket_region={{s3_bucket_region}}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most important part for integration test

@gillg
Copy link
Author

gillg commented Jan 9, 2022

recheck

@@ -7,4 +7,5 @@ windows_ami_name: Windows_Server-2019-English-Full-Base-*
# see:
# - https://github.com/mattclay/aws-terminator/pull/181
# - https://github.com/ansible-collections/community.aws/pull/763
s3_bucket_name: ssm-encrypted-test-bucket
s3_bucket_name: "{{ tiny_prefix }}-ssm-encrypted-test-bucket"
Copy link
Contributor

@alinabuzachis alinabuzachis Jan 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gillg Thank you for working on this. ssm-encrypted-test-bucket has been added in this PR #763 (comment) since It is an encrypted bucket requiring up to ~24hour to be created (we have to use a permanent one). At the moment, the integration tests of the aws_ssm connection plugin have been also disabled because of other issues. Please have a look at this PR #763 for a history.
@jillr can you confirm please?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

24h to create a bucket 😨 !? I'm surprised but why not.
Don't you think it could be more revelent to create an unencrypted bucket by default, but add a resource policy to ensure "PUT" commands forces encryption ? Because if you enable encryption in bucket properties that means you object will be encrypted by default event if you don't ask for it. But it seems to don't be the use case of SSM plugin where we ask explicitely to encrypt new objects.

If there is no workaround what would be the next steps ? Do we assume to skip integration tests for this plugin for now ?
Just in case... I tested it localy in my context 😅

In the meantime I try to work on other PRs because I use this plugin for the first time and I prefer make it stable and close all pending PRs since almost 1yr.

@Hokwang
Copy link

Hokwang commented Mar 7, 2022

please take care this PR. I think #705 should not close yet.

@ansibullbot ansibullbot added bug This issue/PR relates to a bug community_review connection connection plugin has_issue integration tests/integration new_contributor Help guide this first time contributor plugins plugin (any type) tests tests labels Mar 7, 2022
@Hokwang
Copy link

Hokwang commented Mar 7, 2022

I tested with your PR's aws_ssm.py, but it has error, too.
When I edit like below, it work like charm.
config=Config(signature_version="s3v4", s3={'addressing_style': 'path'})

so you need to think #786 and I think you can merge that to this PR.

@gillg
Copy link
Author

gillg commented Mar 7, 2022

Both PRs was independant and for 2 different things. I can merge them together but I was not sure about maintainers opinion.
Any opinion @alinabuzachis ?

@Hokwang
Copy link

Hokwang commented Mar 17, 2022

@alinabuzachis waiting your comment.

@jatorcasso
Copy link
Contributor

I tested with your PR's aws_ssm.py, but it has error, too. When I edit like below, it work like charm. config=Config(signature_version="s3v4", s3={'addressing_style': 'path'})

so you need to think #786 and I think you can merge that to this PR.

Hi @Hokwang @gillg - if this PR requires the changes from #786, then we can have this PR depend on it prior to merging (see #1078 (comment) for correct formatting)

@jatorcasso jatorcasso closed this May 9, 2022
@jatorcasso jatorcasso reopened this May 9, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

ansible-galaxy-importer FAILURE in 5m 05s (non-voting)
✔️ build-ansible-collection SUCCESS in 5m 37s
✔️ ansible-test-sanity-docker-devel SUCCESS in 15m 58s (non-voting)
ansible-test-sanity-docker-milestone FAILURE in 10m 32s
✔️ ansible-test-sanity-docker-stable-2.9 SUCCESS in 17m 04s
✔️ ansible-test-sanity-docker-stable-2.11 SUCCESS in 13m 18s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 11m 41s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 8m 22s
✔️ ansible-test-splitter SUCCESS in 3m 43s
✔️ integration-community.aws-1 SUCCESS in 5m 17s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@alinabuzachis
Copy link
Contributor

Both PRs was independant and for 2 different things. I can merge them together but I was not sure about maintainers opinion.
Any opinion @alinabuzachis ?

@gillg @Hokwang I apologise for the delay, but I completely missed these mentions. I guess we can keep the PR separate. BTW, we need to test them locally since aws_ssm integration tests are disabled for the moment.

@ds-rlou
Copy link

ds-rlou commented Jun 14, 2022

Hi guys, any idea when we can have this fix merged? This issue really prevent us from using the SSM connection plugin across multiple region.

@Hokwang
Copy link

Hokwang commented Aug 16, 2022

@alinabuzachis Please take care all issues regarding aws_ssm.

@ansibullbot 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 and removed community_review labels Aug 16, 2022
@tremble
Copy link
Contributor

tremble commented Jan 20, 2023

Thanks for taking this time to open this PR, and I'm sorry it's taken so long for us to deal with this.

With #1428 the plugin now explicitly asks S3 which region a bucket's in and uses that. This should solve the issues you've been seeing. In some cases it may also be necessary to use #1633 to set the addressing style to 'virtual'.

@tremble tremble closed this Jan 20, 2023
abikouo pushed a commit to abikouo/community.aws that referenced this pull request Oct 24, 2023
…tions#854)

ec2_instance: Add example to spin up instance with a attached volume from a snapshot

SUMMARY

Fixes ansible-collections#803
Add example to spin up new instance with an attached volume from a snapshot

ISSUE TYPE


Docs Pull Request

COMPONENT NAME

ec2_instance

Reviewed-by: Jill R <None>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug connection connection plugin has_issue integration tests/integration 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 needs_triage 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.

8 participants