Add ES cleanup module #1

joshmyers · 2018-12-14T13:04:42Z

what

This PR adds the main gubbins of the ES cleanup Lambda function and associated Terraform code

osterman · 2018-12-14T17:24:43Z

I think we should convey lambda somewhere in the repo name

osterman · 2018-12-14T17:29:17Z

README.yaml

+# Short description of this project
+description: |-
+ Terraform module to provision a scheduled Lambda function which will 
+ delete old Elasticsearch indexes using SigV4Auth authentication. The


Suggested change

delete old Elasticsearch indexes using SigV4Auth authentication. The

delete old Elasticsearch indexes using [SigV4Auth](https://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-authenticating-requests.html) authentication. The

not sure if that is the right link

It is the overarching sigV4 (needed when talking to ES clusters) documentation link - from there you can dig more into those docs.

osterman · 2018-12-14T17:30:25Z

README.yaml

+
+# Contributors to this project
+contributors:
+ - name: "Erik Osterman"


Remove non-contributors

osterman · 2018-12-14T17:31:03Z

example/main.tf

+ enabled = "true"
+ namespace = "example"
+ stage = "dev"
+ schedule = "rate(5 minutes)"


How would we limit this to specific indexes?

We could add an index whitelist should we wish. Currently the function accepts a specific index, or all.

osterman · 2018-12-14T17:31:29Z

lambda/es-cleanup.py

@@ -0,0 +1,210 @@
+#!/usr/bin/python


Add link of attribution to original lambda

Also, mention copyrights and license

Would we rather that in the function itself rather than https://github.com/cloudposse/terraform-aws-elasticsearch-cleanup/blob/0cb550ade3c70a52f7150ec4b7476ad2399772d4/README.yaml#L47-L48 ? NP if so

lambda/es-cleanup.py

osterman

See comments

osterman · 2018-12-14T17:36:09Z

variables.tf

+}
+
+variable "python_version" {
+ default = "2.7"


Missing description

comment addressed.

osterman · 2018-12-14T17:38:47Z

example/main.tf

+
+module "elasticsearch" {
+ source = "git::https://github.com/cloudposse/terraform-aws-elasticsearch.git?ref=tags/0.1.5"
+ namespace = "example"


We don't want to hardcode these in examples b/c it precludes us testing the module concurrently. Additionally, sometimes our community will deploy this version without changing the values, which means we cannot reuse them.

osterman · 2018-12-14T17:41:29Z

lambda/es-cleanup.py

+
+class ES_Cleanup(object):
+
+ name = "lambda_es_cleanup"


Just want to make sure this doesn't need to be unique to the AWS account

Nope, this is used to prefix outputted messaged when sending to SNS

main.tf

osterman · 2018-12-14T17:43:45Z

main.tf

+
+resource "aws_security_group" "default" {
+ count = "${local.enabled == true ? 1 : 0}"
+ name = "${module.label.id}-elasticsearch-cleanup"


We are not consistent about this, but technically should be using a new label with attributes = ["elasticsearch", "cleanup"]

Otherwise, we ignore the user's delimiter selection

osterman · 2018-12-14T17:44:40Z

main.tf

+
+resource "aws_iam_role_policy" "default" {
+ count = "${local.enabled == true ? 1 : 0}"
+ name = "${module.label.id}-elasticsearch-cleanup"


https://github.com/cloudposse/terraform-aws-elasticsearch-cleanup/pull/1/files#r241835709

osterman · 2018-12-14T17:45:33Z

variables.tf

+}
+
+variable "vpc_id" {
+ description = "VPC Id for the Lambda function - should be the same VPC as ES domain"


Suggested change

description = "VPC Id for the Lambda function - should be the same VPC as ES domain"

description = "VPC ID for the Lambda function. Should be the same VPC as the ES domain."

osterman · 2018-12-14T17:47:13Z

variables.tf

+}
+
+variable "index" {
+ description = "Index/indices to process comma separated, with all every index will be processed except '.kibana'"


Suggested change

description = "Index/indices to process comma separated, with all every index will be processed except '.kibana'"

description = "Index/indices to process. Use a comma-separated list. Specify`all` to match every index except for `.kibana`"

osterman · 2018-12-14T17:47:29Z

variables.tf

+}
+
+variable "delete_after" {
+ description = "Numbers of days to preserve"


Suggested change

description = "Numbers of days to preserve"

description = "Number of days to preserve"

osterman · 2018-12-14T17:47:58Z

variables.tf

+
+variable "enabled" {
+ type = "string"
+ description = "Enable or disable the ElasticSearch Cleanup function"


Suggested change

description = "Enable or disable the ElasticSearch Cleanup function"

description = "Enable or disable the Elasticsearch Cleanup function"

Seems like the canonical spelling is Elasticsearch per AWS documentation

osterman · 2018-12-14T17:48:36Z

variables.tf

+variable "tags" {
+ type = "map"
+ default = {}
+ description = "Additional tags (e.g. `map('BusinessUnit`,`XYZ`)"


Suggested change

description = "Additional tags (e.g. `map('BusinessUnit`,`XYZ`)"

description = "Additional tags (e.g. `map('BusinessUnit','XYZ')`"

That said, the origin of this map(..) business is from OLD terraforms which did not support { ... } notation. At some point we need to bite the bullet.

aknysh · 2018-12-14T20:32:25Z

.gitignore

@@ -1,7 +1,6 @@
 # Compiled files


use our standard .gitignore for Terraform repos:

# Local .terraform directories **/.terraform/* # .tfstate files *.tfstate *.tfstate.* # .tfvars files *.tfvars **/.idea **/*.iml **/.build-harness **/build-harness

aknysh · 2018-12-14T20:34:44Z

README.yaml

+ description: "Terraform module for dynamic subnets provisioning."
+ url: "https://github.com/cloudposse/terraform-aws-dynamic-subnets"
+ - name: "terraform-aws-elasticsearch"
+ description: "Terraform module for AWS ElasticSearch provisioning."


use canonical Elasticsearch

aknysh · 2018-12-14T20:36:56Z

README.yaml

+# How to use this project
+usage: |-
+ ```hcl
+ module "elasticsearch-cleanup" {


Suggested change

module "elasticsearch-cleanup" {

module "elasticsearch_cleanup" {

use snake case to name resources

aknysh · 2018-12-14T20:37:56Z

README.yaml

+ enabled = "true"
+ namespace = "example"
+ stage = "dev"
+ schedule = "rate(5 minutes)"


is this just a free text for the schedule, not regex?

https://docs.aws.amazon.com/AmazonCloudWatch/latest/events/ScheduledEvents.html this can be in cron or rate format

aknysh · 2018-12-14T20:39:17Z

example/main.tf

+}
+
+module "elasticsearch" {
+ source = "git::https://github.com/cloudposse/terraform-aws-elasticsearch.git?ref=tags/0.1.5"


Suggested change

source = "git::https://github.com/cloudposse/terraform-aws-elasticsearch.git?ref=tags/0.1.5"

source = "git::https://github.com/cloudposse/terraform-aws-elasticsearch.git?ref=master"

use master for consistency

Sure, I was going for a known working release

it's kind of a chicken and the egg - also easily forgotten to update. We tend to pin our README.md examples to master so users who copy and paste get something that probably works.

aknysh · 2018-12-14T20:39:56Z

example/main.tf

+ vpc_id = "${module.vpc.vpc_id}"
+ subnet_ids = ["${module.subnets.public_subnet_ids}"]
+ zone_awareness_enabled = "true"
+ elasticsearch_version = "6.2"


Suggested change

elasticsearch_version = "6.2"

elasticsearch_version = "6.3"

use the latest supported version

aknysh · 2018-12-17T14:58:56Z

main.tf

+resource "aws_cloudwatch_event_target" "default" {
+ count = "${local.enabled == true ? 1 : 0}"
+ target_id = "${local.function_name}"
+ rule = "${aws_cloudwatch_event_rule.default.name}"


Suggested change

rule = "${aws_cloudwatch_event_rule.default.name}"

rule = "${join("", aws_cloudwatch_event_rule.default.*.name})"

aknysh · 2018-12-17T15:00:08Z

variables.tf

+ description = "Subnet ids"
+}
+
+variable "sns_alert" {


Suggested change

variable "sns_alert" {

variable "sns_arn" {

aknysh · 2018-12-17T15:00:35Z

variables.tf

+variable "sns_alert" {
+ type = "string"
+ default = ""
+ description = "SNS ARN to pusblish any alert"


Suggested change

description = "SNS ARN to pusblish any alert"

description = "SNS ARN to publish alerts"

aknysh · 2018-12-17T15:01:05Z

variables.tf

+variable "index" {
+ type = "string"
+ default = "all"
+ description = "Index/indices to process. Use a comma-separated list. Specify`all` to match every index except for `.kibana`"


Suggested change

description = "Index/indices to process. Use a comma-separated list. Specify`all` to match every index except for `.kibana`"

description = "Index/indices to process. Use a comma-separated list. Specify `all` to match every index except for `.kibana`"

aknysh · 2018-12-17T15:02:10Z

variables.tf

+ description = "Stage, e.g. 'prod', 'staging', 'dev', or 'test'"
+}
+
+variable "environment" {


this var not needed

aknysh · 2018-12-17T15:02:53Z

variables.tf

+
+variable "index_format" {
+ default = "%Y.%m.%d"
+ description = "Combined with 'index' varible is used to evaluate the index age"


Suggested change

description = "Combined with 'index' varible is used to evaluate the index age"

description = "Combined with 'index' variable and is used to evaluate the index age"

variables.tf

main.tf

variables.tf

main.tf

aknysh · 2018-12-17T15:17:43Z

main.tf

+
+data "aws_iam_policy_document" "default" {
+ statement {
+ actions = [


do we need to give these permissions to the module?

Yeah these are needed to the Lambda function can a) write log files to CloudWatch logs b) post a v4 signed payload to ES

main.tf

aknysh

see inline comments

joshmyers · 2018-12-17T18:10:10Z

@aknysh Ready for another review

README.yaml

docs/terraform.md

aknysh

see comments

This creates all the base resources necessary to test the elasticsearch-cleanup module. Unfortunately this requires a two stage apply as the `aws_route53_zone` resource fails to compute ID when passed into other modules. See: cloudposse/terraform-aws-elasticsearch#13

Pull the deployment artifact from S3 using the new terraform-external-module-artifact module. This approach has the benefit of not checking in zip files into git, but the downside of more preamble when wanting to test this module as per the example docs. You need to know that the function should already be in S3 at a pre determined path. We need to think of proper deployment and promotion of these artifacts through different S3 buckets e.g. artifacts.testing.cloudposse.org -> artifacts.prod.cloudposse.org

@osterman

@osterman already commented and handed over to @aknysh

joshmyers force-pushed the add_es_cleanup_module branch 2 times, most recently from 3bd9ac6 to 5512c28 Compare December 14, 2018 16:59

osterman reviewed Dec 14, 2018

View reviewed changes

README.yaml Outdated

# Contributors to this project

contributors:

- name: "Erik Osterman"

Copy link

Member

osterman Dec 14, 2018

Choose a reason for hiding this comment

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

Remove non-contributors

osterman reviewed Dec 14, 2018

View reviewed changes

lambda/es-cleanup.py Show resolved Hide resolved

joshmyers force-pushed the add_es_cleanup_module branch from 5512c28 to bfd7c0f Compare December 14, 2018 17:33

osterman previously requested changes Dec 14, 2018

View reviewed changes

osterman requested a review from aknysh December 14, 2018 17:36

osterman reviewed Dec 14, 2018

View reviewed changes

main.tf Show resolved Hide resolved

osterman reviewed Dec 14, 2018

View reviewed changes

joshmyers force-pushed the add_es_cleanup_module branch 3 times, most recently from 4ef843b to fbdd26d Compare December 14, 2018 18:27

aknysh reviewed Dec 14, 2018

View reviewed changes