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

[WIP]: Add aws_secret module for managing secretsmanager on AWS #48486

Open
wants to merge 38 commits into
base: devel
from

Conversation

@rrey
Contributor

rrey commented Nov 10, 2018

SUMMARY

This is a rework of pull request #40093 as the author does not answer anymore on the PR thread.
I've rework the module but kept the lookup plugin and test unchanged (I've applied @s-hertel comments though).

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

modules/cloud/amazon/aws_secret.py

ANSIBLE VERSION
ansible 2.6.0
  config file = None
  configured module search path = [u'/Users/remirey/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/remirey/.venv/grafana/lib/python2.7/site-packages/ansible
  executable location = /Users/remirey/.venv/grafana/bin/ansible
  python version = 2.7.13 (default, Apr  4 2017, 08:47:57) [GCC 4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.38)]
@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 10, 2018

@rrey, just so you are aware we have a dedicated Working Group for aws.
You can find other people interested in this in #ansible-aws on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 10, 2018

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/modules/cloud/amazon/aws_secret.py:110:4: dangerous-default-value Dangerous default value {} as argument
lib/ansible/modules/cloud/amazon/aws_secret.py:261:12: unreachable Unreachable code

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_secret.py:20:0: SyntaxError: Non-ASCII character '\xc3' in file /root/ansible/lib/ansible/modules/cloud/amazon/aws_secret.py on line 21, but no encoding declared; see http://www.python.org/peps/pep-0263.html for details

The test ansible-test sanity --test import --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_secret.py:20:0: SyntaxError: Non-ASCII character '\xc3' in file /root/ansible/lib/ansible/modules/cloud/amazon/aws_secret.py on line 21, but no encoding declared; see http://python.org/dev/peps/pep-0263/ for details

The test ansible-test sanity --test import --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_secret.py:100:0: ImportError: No module named 'boto3'

The test ansible-test sanity --test import --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_secret.py:100:0: ModuleNotFoundError: No module named 'boto3'

The test ansible-test sanity --test import --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_secret.py:100:0: ModuleNotFoundError: No module named 'boto3'

The test ansible-test sanity --test integration-aliases [explain] failed with 1 error:

test/integration/targets/aws_secret/aliases:0:0: missing alias `shippable/aws/group[1-2]` or `unsupported`

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Nov 10, 2018

@willthames

This comment has been minimized.

Contributor

willthames commented Nov 11, 2018

My usual approach with IAM roles for Ansible is to use a name that is consistent between test runs - that way we can ensure that the role exists once and we don't need to give CI the CreateRole permission.

Rather than test-secrets-manager-role-{{ resource_prefix }}, just use test-secrets-manager-role

@gundalow

This comment has been minimized.

Contributor

gundalow commented Nov 12, 2018

Looks like permissions will need to be given so this test can run:
I'll ask @s-hertel to take a look

                          Traceback (most recent call last):
  File "/tmp/ansible_iam_role_payload_27dTZh/__main__.py", line 249, in create_or_update_role
    role = connection.create_role(**params)
  File "/usr/local/lib/python2.7/dist-packages/botocore/client.py", line 320, in _api_call
    return self._make_api_call(operation_name, kwargs)
  File "/usr/local/lib/python2.7/dist-packages/botocore/client.py", line 623, in _make_api_call
    raise error_class(parsed_response, operation_name)
ClientError: An error occurred (AccessDenied) when calling the CreateRole operation: User: arn:aws:sts::966509639900:assumed-role/ansible-core-ci-test-prod/prod=shippable=ansible=ansible=92943.67 is not authorized to perform: iam:CreateRole on resource: arn:aws:iam::966509639900:role/test-secrets-manager-role-shippable-92943-67
@s-hertel

Thanks for picking this up to help get it over the line @rrey!

pause:
seconds: 30
- name: no changes to secret

This comment has been minimized.

@s-hertel

s-hertel Nov 13, 2018

Contributor

Maybe a module bug - this task is making changes so the assert below fails.

This comment has been minimized.

@rrey

rrey Nov 20, 2018

Contributor

Noob question:
What is the ansible-test command that allow me to run the integration test just for aws_secret ?

I'm a bit cheating locally and I guess it will be a source of differences in the result...

This comment has been minimized.

@mattclay

mattclay Nov 26, 2018

Member

ansible-test integration aws_secret -v

assert:
that:
- result.failed
- 'result.msg.startswith("missing required arguments:")'

This comment has been minimized.

@s-hertel

s-hertel Nov 13, 2018

Contributor

The above task fails with "The aws_secret module requires a region and none was found in configuration, environment variables or module parameters". If you add region: "{{ aws_region }}" it will start failing with "Failed to describe secret: Unable to locate credentials". This is the same for the next two tasks too.

This comment has been minimized.

@rrey

rrey Nov 20, 2018

Contributor

that' clearly one of the diff from my env. I'll reproduce this and check how to fix with the ansible-test command you'll provide :)

Show resolved Hide resolved test/integration/targets/aws_secret/tasks/main.yaml Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/amazon/aws_secret.py Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/amazon/aws_secret.py
except (botocore.exceptions.ProfileNotFound, botocore.exceptions.PartialCredentialsError) as e:
if boto_profile:
try:
connection = boto3.session.Session(profile_name=boto_profile).client('secretsmanager', region)

This comment has been minimized.

@s-hertel

s-hertel Nov 13, 2018

Contributor

If this fails, it would be good to be able to fall back to instance role credentials if they exist. Here's a commit fixing that for another plugin for reference if it helps d5a5e37#diff-fde53b7bca9c303ec46c0a71219bab47

This comment has been minimized.

@rrey

rrey Nov 20, 2018

Contributor

ok, I pushed a fix for your comments except this one.

This part of the code looks to be a copy of aws_ssm plugin. shouldn't the credentials retrieval be factorized somewhere in aws common code ?

short_description: Manage secrets stored in AWS Secrets Manager.
description:
- Create, update, and delete secrets stored in AWS Secrets Manager.
author: "REY Remi (@rrey)"

This comment has been minimized.

@s-hertel

s-hertel Nov 13, 2018

Contributor

Do you mind making this a list and adding @slapula (Aaron Smith <ajsmith10381@gmail.com>) as an additional author to this module and the plugin since this is based on his work? If he resurfaces he'll be able to get pinged on things to help provide shipits and maintain the module and plugin.

This comment has been minimized.

@rrey

rrey Nov 20, 2018

Contributor

No problem, I changed it initially because I modified most of the code from the module.

Show resolved Hide resolved lib/ansible/modules/cloud/amazon/aws_secret.py

@samdoran samdoran changed the title from wip: Add aws_secret module for managing secretsmanager on AWS to [WIP]: Add aws_secret module for managing secretsmanager on AWS Nov 13, 2018

@bcoca bcoca removed the needs_triage label Nov 13, 2018

@ansibot ansibot removed the stale_ci label Nov 20, 2018

@rrey

This comment has been minimized.

Contributor

rrey commented Nov 20, 2018

Thanks for the review @s-hertel !

@slapula

This comment has been minimized.

Contributor

slapula commented Nov 26, 2018

@rrey Sorry I've been out of pocket these past couple months. I have cycles now to look at my Ansible PRs (including the secrets_manager module but I'll defer to this one since this work is more recent). If you need any assistance with this please let me know.

@rrey

This comment has been minimized.

Contributor

rrey commented Nov 30, 2018

@slapula : Thanks !
@s-hertel : Can you fix the issue with the role creation ?

 ClientError: An error occurred (AccessDenied) when calling the CreateRole operation: User: arn:aws:sts::966509639900:assumed-role/ansible-core-ci-test-prod/prod=shippable=ansible=ansible=94536.67 is not authorized to perform: iam:CreateRole on resource: arn:aws:iam::966509639900:role/test-secrets-manager-role

Should I drop the deletion of the role at the end of the tests to allow you to create it once and for all as suggested by @willthames ?

@ansibot ansibot added the stale_ci label Nov 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment