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

Initial implementation #1

Merged
merged 7 commits into from
Jul 24, 2018
Merged

Initial implementation #1

merged 7 commits into from
Jul 24, 2018

Conversation

aknysh
Copy link
Member

@aknysh aknysh commented Jul 23, 2018

what

  • Terraform module to provision an Elasticsearch cluster in a VPC with build-in Kibana and Logstash

why

  • Easily provision Elasticsearch cluster with the specified node count in the provided subnets in a VPC
  • Provision Elasticsearch domain policy that accepts a list of IAM role ARNs from which to permit management traffic to the cluster
  • Provision Security Group to control access to the Elasticsearch domain (inputs to the Security Group are other Security Groups or CIDRs blocks to be allowed to connect to the cluster)
  • Provision DNS hostname record for Elasticsearch cluster
  • DNS hostname record for Kibana

@aknysh aknysh self-assigned this Jul 23, 2018
@aknysh aknysh requested review from sarkis, osterman and goruha July 23, 2018 16:21
README.yaml Outdated
The subnets must be in different Availability Zones in the same region.
If you don't enable zone awareness, Amazon ES places an endpoint into only one subnet

Further reading:
Copy link
Member

Choose a reason for hiding this comment

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

Move this to references: section

main.tf Outdated
name = "${module.label.id}"
description = "Allow inbound traffic from Security Groups and CIDRs"

ingress {
Copy link
Member

Choose a reason for hiding this comment

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

Always use aws_security_group_rule for idempotency

README.yaml Outdated
description: |-
Terraform module to provision an [`Elasticsearch`](https://aws.amazon.com/elasticsearch-service/) cluster with built-in integrations with [Kibana](https://aws.amazon.com/elasticsearch-service/kibana/) and [Logstash](https://aws.amazon.com/elasticsearch-service/logstash/).

This module will create:
Copy link
Member

Choose a reason for hiding this comment

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

Move this to introduction

[![Cloud Posse](https://cloudposse.com/logo-300x69.svg)](https://cloudposse.com)

# terraform-aws-elasticsearch [![Build Status](https://travis-ci.org/cloudposse/terraform-aws-elasticsearch.svg?branch=master)](https://travis-ci.org/cloudposse/terraform-aws-elasticsearch) [![Latest Release](https://img.shields.io/github/release/cloudposse/terraform-aws-elasticsearch.svg)](https://github.com/cloudposse/terraform-aws-elasticsearch/releases/latest) [![Slack Community](https://slack.cloudposse.com/badge.svg)](https://slack.cloudposse.com)
Copy link
Member

Choose a reason for hiding this comment

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

Enable CI/CD

image

main.tf Outdated
enabled = "${var.enabled == "true" && length(var.dns_zone_id) > 0 ? "true" : "false"}"
namespace = "${var.namespace}"
stage = "${var.stage}"
name = "${format("kibana%s%s", var.delimiter, var.name)}"
Copy link
Member

Choose a reason for hiding this comment

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

let's make kibana a variable that defaults to kibana. e.g. it could be overridden to ui

Copy link
Member

Choose a reason for hiding this comment

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

Actually, let's make kibana%s%s a variable

goruha
goruha previously approved these changes Jul 23, 2018
Copy link
Member

@osterman osterman left a comment

Choose a reason for hiding this comment

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

See inline comments

README.yaml Outdated
- name: "es-createupdatedomains"
description: "Creating and Configuring Amazon Elasticsearch Service Domains"
url: "https://docs.aws.amazon.com/elasticsearch-service/latest/developerguide/es-createupdatedomains.html"
- name: "es-kibana"
Copy link
Member

Choose a reason for hiding this comment

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

Name doesn't need to be codified. It's the name (or title) of the link. We use "codified" names when referring to repositories because that's the easiest way to refer to them. In this case, I would make "Name" the title of the document and a description about why you think it's worth referencing.

README.yaml Outdated
- "docs/terraform.md"

references:
- name: "what-is-amazon-elasticsearch-service"
Copy link
Member

Choose a reason for hiding this comment

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

Don't codify

README.yaml Outdated
- name: "what-is-amazon-elasticsearch-service"
description: "What is Amazon Elasticsearch Service"
url: "https://docs.aws.amazon.com/elasticsearch-service/latest/developerguide/what-is-amazon-elasticsearch-service.html"
- name: "es-ac"
Copy link
Member

Choose a reason for hiding this comment

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

Don't codify.... etc...

tags = "${module.label.tags}"
}

resource "aws_security_group_rule" "ingress_security_groups" {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like we should distinguish between Elastic Search and Kibana.

Copy link
Member Author

Choose a reason for hiding this comment

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

good pint.
they are separate resources and having different roles to access them would be preferable.

Copy link
Member Author

Choose a reason for hiding this comment

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

actually, Kibana does not support IAM.
https://forums.aws.amazon.com/thread.jspa?threadID=217149
It's little bit complicated to set up access to Kibana.
The solution is to use a proxy for IP-based control, and Amazon Cognito for use-based control.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

let's move the decision of who can access what out of the module.
Top-level modules will provide:

variable "iam_role_arns" {
  type        = "list"
  default     = []
  description = "List of IAM role ARNs to permit access to the Elasticsearch domain"
}

variable "iam_actions" {
  type        = "list"
  default     = []
  description = "List of actions to allow for the IAM roles, _e.g._ `es:ESHttpGet`, `es:ESHttpPut`, `es:ESHttpPost`"
}

main.tf Outdated
tags = "${module.label.tags}"
}

resource "aws_elasticsearch_domain_policy" "es_vpc_management_access" {
Copy link
Member

Choose a reason for hiding this comment

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

from es_ prefix as we do not need the disambiguation.

Copy link
Member

@osterman osterman left a comment

Choose a reason for hiding this comment

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

See inline comments

README.md Outdated
- [Kibana and Logstash](https://docs.aws.amazon.com/elasticsearch-service/latest/developerguide/es-kibana.html) - Describes some considerations for using Kibana and Logstash with Amazon Elasticsearch Service
- [Amazon Cognito Authentication for Kibana](https://docs.aws.amazon.com/elasticsearch-service/latest/developerguide/es-cognito-auth.html) - Amazon Elasticsearch Service uses Amazon Cognito to offer user name and password protection for Kibana
- [elasticsearch_domain](https://www.terraform.io/docs/providers/aws/r/elasticsearch_domain.html) - Configuration for `elasticsearch_domain` Terraform resource
Copy link
Member

Choose a reason for hiding this comment

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

Terraform reference documentation for the elasticsearch_domain resource

README.md Outdated
- [Amazon Cognito Authentication for Kibana](https://docs.aws.amazon.com/elasticsearch-service/latest/developerguide/es-cognito-auth.html) - Amazon Elasticsearch Service uses Amazon Cognito to offer user name and password protection for Kibana
- [elasticsearch_domain](https://www.terraform.io/docs/providers/aws/r/elasticsearch_domain.html) - Configuration for `elasticsearch_domain` Terraform resource
- [elasticsearch_domain_policy](https://www.terraform.io/docs/providers/aws/r/elasticsearch_domain_policy.html) - Configuration for `elasticsearch_domain_policy` Terraform resource
Copy link
Member

Choose a reason for hiding this comment

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

Terraform reference documentation for the elasticsearch_domain_policy resource

@aknysh aknysh requested a review from osterman July 23, 2018 21:41
@osterman
Copy link
Member

Travis is failing

@aknysh aknysh merged commit 141a17b into master Jul 24, 2018
@aknysh aknysh deleted the init branch July 24, 2018 03:12
tuxtek referenced this pull request in tuxtek/terraform-aws-elasticsearch Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants