Add module for interacting with AWS-hosted Elasticsearch #34277

steiza · 2017-12-28T19:31:11Z

SUMMARY

I've been using this to manage AWS-hosted Elasticsearch and thought I would post it to see if it would be useful to others.

I haven't submitted a PR for ansible before, but I did read over the Contributing pages - feedback welcome!

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

cloud/amazon/elasticsearch.py

ANSIBLE VERSION

[14:30:31] ~$ ansible --version
ansible 2.4.2.0
  config file = /Users/zachsteindler/.ansible.cfg
  configured module search path = [u'/Users/zachsteindler/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/ansible
  executable location = /Library/Frameworks/Python.framework/Versions/2.7/bin/ansible
  python version = 2.7.13 (v2.7.13:a06454b1afa1, Dec 17 2016, 12:39:47) [GCC 4.2.1 (Apple Inc. build 5666) (dot 3)]

ADDITIONAL INFORMATION

N/A

ansibot · 2017-12-28T19:34:30Z

cc @willthames
click here for bot help

ansibot · 2017-12-28T19:42:14Z

The test ansible-test sanity --test no-underscore-variable [?] failed with the following error:

Command "test/sanity/code-smell/no-underscore-variable.sh" returned exit status 2.
>>> Standard Output
== Underscore used as a variable ==
./lib/ansible/modules/cloud/amazon/elasticsearch.py:    region, _, aws_connect_kwargs = ec2.get_aws_connection_info(module)

The test ansible-test sanity --test pep8 [?] failed with the following errors:

lib/ansible/modules/cloud/amazon/elasticsearch.py:113:9: E261 at least two spaces before inline comment
lib/ansible/modules/cloud/amazon/elasticsearch.py:136:13: E123 closing bracket does not match indentation of opening bracket's line
lib/ansible/modules/cloud/amazon/elasticsearch.py:138:9: E123 closing bracket does not match indentation of opening bracket's line
lib/ansible/modules/cloud/amazon/elasticsearch.py:160:13: E123 closing bracket does not match indentation of opening bracket's line
lib/ansible/modules/cloud/amazon/elasticsearch.py:189:9: E123 closing bracket does not match indentation of opening bracket's line
lib/ansible/modules/cloud/amazon/elasticsearch.py:197:13: E123 closing bracket does not match indentation of opening bracket's line
lib/ansible/modules/cloud/amazon/elasticsearch.py:240:27: E251 unexpected spaces around keyword / parameter equals
lib/ansible/modules/cloud/amazon/elasticsearch.py:241:17: E131 continuation line unaligned for hanging indent
lib/ansible/modules/cloud/amazon/elasticsearch.py:242:28: E251 unexpected spaces around keyword / parameter equals
lib/ansible/modules/cloud/amazon/elasticsearch.py:244:38: E251 unexpected spaces around keyword / parameter equals
lib/ansible/modules/cloud/amazon/elasticsearch.py:249:29: E251 unexpected spaces around keyword / parameter equals
lib/ansible/modules/cloud/amazon/elasticsearch.py:255:13: E123 closing bracket does not match indentation of opening bracket's line
lib/ansible/modules/cloud/amazon/elasticsearch.py:256:9: E123 closing bracket does not match indentation of opening bracket's line
lib/ansible/modules/cloud/amazon/elasticsearch.py:260:9: E123 closing bracket does not match indentation of opening bracket's line

The test ansible-test sanity --test validate-modules [?] failed with the following error:

lib/ansible/modules/cloud/amazon/elasticsearch.py:0:0: E319 RETURN.domain.contains: required key not provided @ data['contains']. Got None

click here for bot help

ansibot · 2017-12-28T20:06:45Z

The test ansible-test sanity --test validate-modules [?] failed with the following error:

lib/ansible/modules/cloud/amazon/elasticsearch.py:0:0: E319 RETURN.domain.contains: required key not provided @ data['contains']. Got None

click here for bot help

ansibot · 2017-12-29T02:12:12Z

@Constantin007 @Constantin07 @Deepakkothandan @Etherdaemon @Java1Guy @Lujeni @MichaelBaydoun @Sodki @adq @akazakov @alachaum @amir343 @anryko @bekelchik @bpennypacker @brandond @carsongee @defunctio @dkhenry @fiunchinho @fivethreeo @garethr @gunzy83 @gurumaia @hyperized @iiibrad @infectsoldier @j-carl @jarv @Java1Guy @jimbydamonk @jmenga @joelthompson @jonhadfield @jsdalton @jsmartin @kaczynskid @leedm777 @linuxdynasty @loia @lwade @MichaelBaydoun @michaeljs1990 @minichate @mjschultz @mmochan @nadirollo @nand0p @naslanidis @NickBall @pjodouin @prasadkatti @psykotox @pwnall @raags @rickmendes @roadmapper @ryansydnor @scicoin-project @scottanderson42 @shepdelacreme @silviud @simplesteph @steynovich @tastychutney @tedder @tgerla @timmahoney @tombamford @whiter @wilvk @wimnat @zacblazic @zbal @zeekin @zimbatm

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

s-hertel · 2018-01-24T16:37:20Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+import time
+
+from ansible.module_utils.basic import AnsibleModule
+from ansible.module_utils import ec2


You don't need to import everything from ec2. Looks like you're using: ec2_argument_spec, HAS_BOTO3, get_aws_connection_info, boto3_conn

s-hertel · 2018-01-24T16:39:15Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+    sample: '{"Statement": [{"Action": "*", "Resource": "arn:aws:iam::0000000000000:user/you", "Effect": "Allow"}]}'
+"""
+
+import collections


You could just import defaultdict from collections

s-hertel · 2018-01-24T16:43:04Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+    argument_spec.update(
+        dict(
+            domain={'required': True},
+            version={'required': False, 'default': '5.1'},


'required': False is the default so you can omit that. Same for any of the following.

s-hertel · 2018-01-24T16:46:49Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+    return modifications_needed
+
+
+def main():


This module creates domains but doesn't have the ability to remove them. Is this incomplete? I would expect a state option that allows the choices 'present' or 'absent'.

s-hertel · 2018-01-24T16:48:02Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+
+    region, ec2_url, aws_connect_kwargs = ec2.get_aws_connection_info(module)
+
+    if region:


You can remove this check for region as it is now handled in boto3_conn.

s-hertel · 2018-01-24T17:06:04Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+        arn=domain_status['ARN'],
+        endpoint=domain_status.get('Endpoint'),
+        processing=domain_status['Processing'],
+        elasticsearch_version=domain_status['ElasticsearchVersion'],


I'm not sure, since it looks like you're picking info from two result dicts, but the camel_dict_to_snake_dict function might help you out for these lines - you could call that for both dicts and them combine them.

s-hertel · 2018-01-24T17:09:14Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+        elasticsearch_cluster_config['DedicatedMasterType'] = module.params['dedicated_master_type']
+        elasticsearch_cluster_config['DedicatedMasterCount'] = module.params['dedicated_master_count']
+
+    connection.create_elasticsearch_domain(


This needs to handle ClientError and BotoCoreError. See the guidelines if you haven't already: https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/amazon/GUIDELINES.md#exception-handling-for-boto3-and-botocore.

ClientError has a .response and BotoCoreError does not. So it could look something like:

try: connection.create_elasticsearch_domain(...) except ClientError as e: module.fail_json(msg="Unable to create elasticsearch domain: {0}".format(to_native(e)), exception=traceback.format_exc(), **camel_dict_to_snake_dict(e.response)) except BotoCoreError as e: module.fail_json(msg="Unable to create elasticsearch domain: {0}".format(to_native(e)), exception=traceback.format_exc())

That will require import traceback and from ansible.module_utils.ec2 import camel_dict_to_snake_dict and from ansible.module_utils._text import to_native

Or:

you can import AnsibleAWSModule from ansible.module_utils.aws.core to replace AnsibleModule, and then you can do:

try: connection.create_elasticsearch_domain(...) except (ClientError, BotoCoreError) as e: module.fail_json_aws(e, msg="Unable to create elasticsearch domain: {0}".format(to_native(e)))

(also needing from ansible.module_utils._text import to_native)

This would also let you remove the check for HAS_BOTO3 since AnsibleAWSModule handles that. :)

s-hertel · 2018-01-24T17:12:20Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+        if e.response['Error']['Code'] == 'ResourceNotFoundException':
+            return None
+        else:
+            raise


Either this should have exception handling that calls fail_json or fail_json_aws (which would need module passed to this function) or everywhere that calls is_present should have the exception handling.

s-hertel · 2018-01-24T17:16:04Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+
+                modification[key_path[-1]] = desired_value
+
+    except Exception as e:


This should catch more specific exceptions since as a rule of thumb we try to avoid except Exception.

s-hertel · 2018-01-24T17:21:13Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+            existing_config=existing_config,
+            modifications_needed=modifications_needed)
+
+    # Config is JSON, so compare JSON dictionaries instead of strings


You can compare two dicts with compare_policies() since it fixes comparisons with lists of length one vs a string and different dictionary order:
from ansible.module_utils.ec2 import compare_policies

if compare_policies(existing_access_json, supplied_access_json): modifications_needed['AccessPolicies'] = module.params['access_policies']

ansibot · 2018-02-05T20:36:54Z

The test ansible-test sanity --test import --python 2.6 [?] failed with the following error:

lib/ansible/modules/cloud/amazon/elasticsearch.py:173:0: ImportError: No module named botocore.exceptions

The test ansible-test sanity --test import --python 2.7 [?] failed with the following error:

lib/ansible/modules/cloud/amazon/elasticsearch.py:173:0: ImportError: No module named botocore.exceptions

The test ansible-test sanity --test import --python 3.5 [?] failed with the following error:

lib/ansible/modules/cloud/amazon/elasticsearch.py:173:0: ImportError: No module named 'botocore'

The test ansible-test sanity --test import --python 3.6 [?] failed with the following error:

lib/ansible/modules/cloud/amazon/elasticsearch.py:173:0: ModuleNotFoundError: No module named 'botocore'

The test ansible-test sanity --test import --python 3.7 [?] failed with the following error:

lib/ansible/modules/cloud/amazon/elasticsearch.py:173:0: ModuleNotFoundError: No module named 'botocore'

The test ansible-test sanity --test validate-modules [?] failed with the following error:

lib/ansible/modules/cloud/amazon/elasticsearch.py:0:0: E322 "wait" is listed in the argument_spec, but not documented in the module

click here for bot help

steiza · 2018-02-05T21:36:49Z

Okay! I think I've addressed all the feedback. Thanks for reviewing!

… bool

s-hertel

This looks great. It could also used some integration tets as per https://github.com/ansible/community/blob/master/group-aws/integration.md#policy

s-hertel · 2018-05-15T20:36:31Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+            # Reload after modifying
+            domain = is_present(client, module)
+
+        if module.params['tags']:


The provided tags should be compared with the current tags on the resource and only set tags if they are not a subset or equal to those that exist

s-hertel · 2018-05-15T20:38:02Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+    description:
+      - JSON string describing the access policy of the cluster.
+    required: true
+  wait:


Should this have a wait_timeout option to use in conjunction?

s-hertel · 2018-05-15T20:48:50Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+    zone_awareness: false
+    instance_type: "t2.medium.elasticsearch"
+    ebs_volume_size: 10
+    access_policies: '{"Statement": [{"Action": "*", "Principal": {"AWS": "arn:aws:iam::0000000000000:user/you"}, "Effect": "Allow"}]}'


Since the current policy will always be returned with a 'Version' and this is not specifying version explicitly this will always show changed=True. Can you add Version in this policy to show an example that will be idempotent?

s-hertel · 2018-05-15T20:51:28Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+            module.fail_json_aws(
+                e,
+                msg='Unable to create elasticsearch domain: {0}'.format(
+                    to_native(e))


e is added to the exception in fail_json_aws so doing it again here will result in a redundant error message. Same for the other exception handling. You could use .format(module.params['domain']) instead to let the user know which domain failed.

s-hertel · 2018-05-15T20:52:35Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+                e,
+                msg='Unexpected error {0}'.format(to_native(e))
+            )
+


BotoCoreError could be handled here. Since only ClientError has an e.response, you can do something like

except connection.exceptions.from_code('ResourceNotFoundException') as e: return None except (BotoCoreError, ClientError) as e: module.fail_json_aws(e)

Same for ensure_deleted()

s-hertel · 2018-05-15T20:56:05Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+        argument_spec=argument_spec,
+    )
+
+    region, ec2_url, aws_connect_kwargs = get_aws_connection_info(


You can replace this line through line 363 with simply: client = module.client('es')

wimnat · 2018-05-22T08:00:36Z

I know this has already name changed once but i think aws_elasticsearch maybe even aws_elasticsearch_domain is probably better because elasticsearch on its own is a product in its own right.

wimnat · 2018-05-22T08:01:53Z

I think you need to add a purge_tags parameter that defaults to yes to be more in line with other modules when it comes to handling pre-existing tags. If you're using the helper function for tags comparison then it's just an extra flag to pass in.

https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/amazon/GUIDELINES.md#dealing-with-tags

and
https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/amazon/GUIDELINES.md#compare_aws_tags

for reference

wimnat · 2018-05-22T08:18:45Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+    description:
+      - Dictionary of tags to ensure are present on resource. Will not remove other tags.
+    required: false
+


Sad to see some of the parameters originally supported disappear such as snapshot hour

wimnat · 2018-05-22T11:37:28Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+    argument_spec = ec2_argument_spec()
+    argument_spec.update(
+        dict(
+            domain={'required': True},


It is more common in ansible modules to use:

domain=dict(required=True)

wimnat · 2018-05-22T11:37:52Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+                'default': 'gp2', 'choices': ['standard', 'gp2', 'io1']
+            },
+            ebs_volume_size={'required': True, 'type': 'int'},
+            access_policies={'required': True},


This should have a type of 'json'

wimnat · 2018-05-22T12:07:41Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+    )
+
+    if module.params['state'] not in ['present', 'absent']:
+        module.fail_json(msg='"state" must be "present" or "absent"')


This statement is not required as it's handled by Ansible parameter checking... you've already specified the choices.

wimnat · 2018-05-22T12:58:49Z

lib/ansible/modules/cloud/amazon/elasticsearch.py

+
+        module.exit_json(
+            changed=changed,
+            arn=domain_status['ARN'],


It's more common to use the helper camel_dict_to_snake_dict here https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/amazon/GUIDELINES.md#camel_dict_to_snake_dict.

You can just do camel_dict_to_snake_dict(**domain_status)

alinalexandru · 2018-08-15T11:12:42Z

I am interested about this module. Is there anything left to do?

gundalow · 2019-02-07T08:27:43Z

@alinalexandru testing this module locally and reporting back if it works would be really helpful.

The common reason that PRs don't get merged is due to lack of feedback.

ansibot · 2019-02-07T08:50:28Z

The test ansible-test sanity --test validate-modules [explain] failed with 8 errors:

lib/ansible/modules/cloud/amazon/elasticsearch.py:0:0: E307 version_added should be 2.8. Currently 2.6
lib/ansible/modules/cloud/amazon/elasticsearch.py:0:0: E319 RETURN.access_policies.type: not a valid value for dictionary value @ data['access_policies']['type']. Got 'string'
lib/ansible/modules/cloud/amazon/elasticsearch.py:0:0: E319 RETURN.arn.type: not a valid value for dictionary value @ data['arn']['type']. Got 'string'
lib/ansible/modules/cloud/amazon/elasticsearch.py:0:0: E319 RETURN.dedicated_master_type.type: not a valid value for dictionary value @ data['dedicated_master_type']['type']. Got 'string'
lib/ansible/modules/cloud/amazon/elasticsearch.py:0:0: E319 RETURN.ebs_volume_type.type: not a valid value for dictionary value @ data['ebs_volume_type']['type']. Got 'string'
lib/ansible/modules/cloud/amazon/elasticsearch.py:0:0: E319 RETURN.elasticsearch_version.type: not a valid value for dictionary value @ data['elasticsearch_version']['type']. Got 'string'
lib/ansible/modules/cloud/amazon/elasticsearch.py:0:0: E319 RETURN.endpoint.type: not a valid value for dictionary value @ data['endpoint']['type']. Got 'string'
lib/ansible/modules/cloud/amazon/elasticsearch.py:0:0: E319 RETURN.instance_type.type: not a valid value for dictionary value @ data['instance_type']['type']. Got 'string'

click here for bot help

ansibot · 2020-01-31T23:08:36Z

cc @jillr @s-hertel @tremble
click here for bot help

Akasurde · 2020-08-19T10:04:25Z

Please submit a new module request to https://github.com/ansible-collections/community.aws. Thanks for your contribution.

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Dec 28, 2017

ansibot added ci_verified Changes made in this PR are causing tests to fail. and removed ci_verified Changes made in this PR are causing tests to fail. labels Dec 28, 2017

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Dec 29, 2017

s-hertel removed the needs_triage Needs a first human triage before being processed. label Dec 29, 2017

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 6, 2018

ansibot added the new_contributor This PR is the first contribution by a new community member. label Jan 22, 2018

s-hertel requested changes Jan 24, 2018

View reviewed changes

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jan 24, 2018

ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 5, 2018

ansibot added ci_verified Changes made in this PR are causing tests to fail. and removed ci_verified Changes made in this PR are causing tests to fail. labels Feb 5, 2018

steiza added 3 commits May 12, 2018 20:23

make state and instance_type required; add support for tags

750dca7

use items instead of iteritems; update documentation to include type:…

5162b46

… bool

remove choice from parameter definition

fe7f46e

steiza force-pushed the devel branch from b708d20 to fe7f46e Compare May 13, 2018 00:24

ansibot removed ci_verified Changes made in this PR are causing tests to fail. merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels May 13, 2018

s-hertel reviewed May 16, 2018

View reviewed changes

ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels May 22, 2018

wimnat reviewed May 22, 2018

View reviewed changes

wimnat suggested changes May 22, 2018

View reviewed changes

gundalow closed this Feb 7, 2019

gundalow reopened this Feb 7, 2019

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 15, 2019

ansibot added support:core This issue/PR relates to code supported by the Ansible Engineering Team. and removed support:community This issue/PR relates to code supported by the Ansible community. labels Apr 29, 2020

Akasurde closed this Aug 19, 2020

ansible locked and limited conversation to collaborators Sep 16, 2020


		region, ec2_url, aws_connect_kwargs = ec2.get_aws_connection_info(module)

		if region:


		modification[key_path[-1]] = desired_value

		except Exception as e:

Add module for interacting with AWS-hosted Elasticsearch #34277

Add module for interacting with AWS-hosted Elasticsearch #34277

Conversation

steiza commented Dec 28, 2017

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot commented Dec 28, 2017

ansibot commented Dec 28, 2017

ansibot commented Dec 28, 2017

ansibot commented Dec 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansibot commented Feb 5, 2018

steiza commented Feb 5, 2018

s-hertel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wimnat commented May 22, 2018 • edited

wimnat commented May 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alinalexandru commented Aug 15, 2018

gundalow commented Feb 7, 2019

ansibot commented Feb 7, 2019

ansibot commented Jan 31, 2020

Akasurde commented Aug 19, 2020

wimnat commented May 22, 2018 •

edited

wimnat commented May 22, 2018 •

edited