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

[WIP] r/aws_elasticsearchdomain: add support for cognito_options #5346

Merged
merged 14 commits into from
Aug 2, 2018
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions aws/resource_aws_elasticsearch_domain.go
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,33 @@ func resourceAwsElasticSearchDomain() *schema.Resource {
Default: "1.5",
ForceNew: true,
},
"cognito_options": {
Type: schema.TypeList,
Optional: true,
ForceNew: false,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"enabled": {
Type: schema.TypeBool,
Optional: true,
Default: true,
},
"user_pool_id": {
Type: schema.TypeString,
Required: true,
},
"identity_pool_id": {
Type: schema.TypeString,
Required: true,
},
"role_arn": {
Type: schema.TypeString,
Required: true,
},
},
},
},

"tags": tagsSchema(),
},
Expand Down Expand Up @@ -384,6 +411,21 @@ func resourceAwsElasticSearchDomainCreate(d *schema.ResourceData, meta interface
}
}

if v, ok := d.GetOk("cognito_options"); ok {

options := v.([]interface{})
if len(options) > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: This error checking is already performed by the attribute schema so it can be removed 👍

  • MaxItems: 1 handles len(options) > 1
  • Required: true on nested arguments handles options[0] == nil

I'd recommend going with the below to simplify this:

if v, ok := d.GetOk("cognito_options"); ok && len(v.([]interface{})) > 0 {
  m := v.([]interface{})[0].(map[string]interface{})
  input.CognitoOptions = expandESCognitoOptions(m)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh cool! That's much cleaner!

return fmt.Errorf("Only a single cognito_options block is expected")
} else if len(options) == 1 {
if options[0] == nil {
return fmt.Errorf("At least one field is expected inside cognito_options")
}

s := options[0].(map[string]interface{})
input.CognitoOptions = expandESCognitoOptions(s)
}
}

log.Printf("[DEBUG] Creating ElasticSearch domain: %s", input)

// IAM Roles can take some time to propagate if set in AccessPolicies and created in the same terraform
Expand All @@ -402,6 +444,9 @@ func resourceAwsElasticSearchDomainCreate(d *schema.ResourceData, meta interface
if isAWSErr(err, "ValidationException", "Domain is still being deleted") {
return resource.RetryableError(err)
}
if isAWSErr(err, "ValidationException", "Amazon Elasticsearch must be allowed to use the passed role") {
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

return resource.RetryableError(err)
}

return resource.NonRetryableError(err)
}
Expand Down Expand Up @@ -504,6 +549,10 @@ func resourceAwsElasticSearchDomainRead(d *schema.ResourceData, meta interface{}
if err != nil {
return err
}
err = d.Set("cognito_options", flattenESCognitoOptions(ds.CognitoOptions))
if err != nil {
return err
}
if ds.SnapshotOptions != nil {
d.Set("snapshot_options", map[string]interface{}{
"automated_snapshot_start_hour": *ds.SnapshotOptions.AutomatedSnapshotStartHour,
Expand Down Expand Up @@ -634,6 +683,12 @@ func resourceAwsElasticSearchDomainUpdate(d *schema.ResourceData, meta interface
input.VPCOptions = expandESVPCOptions(s)
}

if d.HasChange("cognito_options") {
options := d.Get("cognito_options").([]interface{})
s := options[0].(map[string]interface{})
Copy link
Contributor

Choose a reason for hiding this comment

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

This line looks like it can cause a panic if cognito_options is removed from the configuration since options will have 0 elements. We should perform a length check and set the parameter to nil/empty/disabled as required, e.g.

if d.HasChange("cognito_options") {
  // default to disabling (change as necessary! this could also be handled in expandESCognitoOptions)
  input.CognitoOptions = &elasticsearch.CognitoOptions{
    Enabled: aws.Bool(false),
  }
  // only enable if provided
  if v, ok := d.GetOk("cognito_options"); ok && len(v.([]interface{})) > 0 {
    m := v.([]interface{})[0].(map[string]interface{})
    input.CognitoOptions = expandESCognitoOptions(m)
  }
}

I'd recommend adding a second TestStep to the acceptance test that covers trying to remove the cognito_options to ensure we're okay here. 👍

input.CognitoOptions = expandESCognitoOptions(s)
}

if d.HasChange("log_publishing_options") {
input.LogPublishingOptions = make(map[string]*elasticsearch.LogPublishingOption)
options := d.Get("log_publishing_options").(*schema.Set).List()
Expand Down
95 changes: 95 additions & 0 deletions aws/resource_aws_elasticsearch_domain_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,26 @@ func TestAccAWSElasticSearchDomain_LogPublishingOptions(t *testing.T) {
})
}

func TestAccAWSElasticSearchDomain_CognitoOptions(t *testing.T) {
var domain elasticsearch.ElasticsearchDomainStatus
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckESDomainDestroy,
Steps: []resource.TestStep{
{
Config: testAccESDomainConfig_CognitoOptions(acctest.RandInt()),
Check: resource.ComposeTestCheckFunc(
testAccCheckESDomainExists("aws_elasticsearch_domain.example", &domain),
resource.TestCheckResourceAttr(
"aws_elasticsearch_domain.example", "elasticsearch_version", "6.0"),
testAccCheckESCognitoOptions(true, &domain),
),
},
},
})
}

func testAccCheckESNumberOfSecurityGroups(numberOfSecurityGroups int, status *elasticsearch.ElasticsearchDomainStatus) resource.TestCheckFunc {
return func(s *terraform.State) error {
count := len(status.VPCOptions.SecurityGroupIds)
Expand Down Expand Up @@ -501,6 +521,16 @@ func testAccCheckESEncrypted(encrypted bool, status *elasticsearch.Elasticsearch
}
}

func testAccCheckESCognitoOptions(enabled bool, status *elasticsearch.ElasticsearchDomainStatus) resource.TestCheckFunc {
return func(s *terraform.State) error {
conf := status.CognitoOptions
if *conf.Enabled != enabled {
return fmt.Errorf("CognitoOptions not set properly. Given: %t, Expected: %t", *conf.Enabled, enabled)
}
return nil
}
}

func testAccLoadESTags(conf *elasticsearch.ElasticsearchDomainStatus, td *elasticsearch.ListTagsOutput) resource.TestCheckFunc {
return func(s *terraform.State) error {
conn := testAccProvider.Meta().(*AWSClient).esconn
Expand Down Expand Up @@ -1064,3 +1094,68 @@ resource "aws_elasticsearch_domain" "example" {
}
`, randInt, randInt, randInt)
}

func testAccESDomainConfig_CognitoOptions(randInt int) string {
return fmt.Sprintf(`
resource "aws_cognito_user_pool" "example" {
name = "tf-test-%d"
}

resource "aws_cognito_user_pool_domain" "example" {
domain = "tf-test-%d"
user_pool_id = "${aws_cognito_user_pool.example.id}"
}

resource "aws_cognito_identity_pool" "example" {
identity_pool_name = "tf_test_%d"
allow_unauthenticated_identities = false
}

resource "aws_iam_role" "example" {
name = "tf-test-%d"
path = "/service-role/"
assume_role_policy = "${data.aws_iam_policy_document.assume-role-policy.json}"
}

data "aws_iam_policy_document" "assume-role-policy" {
statement {
sid = ""
actions = ["sts:AssumeRole"]
effect = "Allow"

principals {
type = "Service"
identifiers = ["es.amazonaws.com"]
}
}
}

resource "aws_iam_role_policy_attachment" "example" {
role = "${aws_iam_role.example.name}"
policy_arn = "arn:aws:iam::aws:policy/AmazonESCognitoAccess"
}

resource "aws_elasticsearch_domain" "example" {
domain_name = "tf-test-%d"

elasticsearch_version = "6.0"

cognito_options {
enabled = true
user_pool_id = "${aws_cognito_user_pool.example.id}"
identity_pool_id = "${aws_cognito_identity_pool.example.id}"
role_arn = "${aws_iam_role.example.arn}"
}

ebs_options {
ebs_enabled = true
volume_size = 10
}

depends_on = [
"aws_iam_role.example",
"aws_iam_role_policy_attachment.example"
]
}
`, randInt, randInt, randInt, randInt, randInt)
}
45 changes: 45 additions & 0 deletions aws/structure.go
Original file line number Diff line number Diff line change
Expand Up @@ -1088,6 +1088,51 @@ func flattenESClusterConfig(c *elasticsearch.ElasticsearchClusterConfig) []map[s
return []map[string]interface{}{m}
}

func expandESCognitoOptions(m map[string]interface{}) *elasticsearch.CognitoOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like its possible to simply some of the resource logic by accepting the []interface{} from the schema here instead of the map[string]interface{}. Then we can handle the length/nil checks in one place.

e.g.

if v, ok := d.GetOk("cognito_options"); ok {
  input.CognitoOptions = expandESCognitoOptions(v.([]interface{}))
}

options := elasticsearch.CognitoOptions{}

if cognitoEnabled, ok := m["enabled"]; ok {
options.Enabled = aws.Bool(cognitoEnabled.(bool))

if cognitoEnabled.(bool) {

if v, ok := m["user_pool_id"]; ok && v.(string) != "" {
options.UserPoolId = aws.String(v.(string))
}
if v, ok := m["identity_pool_id"]; ok && v.(string) != "" {
options.IdentityPoolId = aws.String(v.(string))
}
if v, ok := m["role_arn"]; ok && v.(string) != "" {
options.RoleArn = aws.String(v.(string))
}
}
}

return &options
}

func flattenESCognitoOptions(c *elasticsearch.CognitoOptions) []map[string]interface{} {
m := map[string]interface{}{}

if c.Enabled != nil {
m["enabled"] = *c.Enabled
}

if aws.BoolValue(c.Enabled) {
if c.UserPoolId != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: We can remove the nil checks and still prevent potential panics at the same time by using the SDK provided functions like aws.StringValue() -- since the schema will default to "" for schema.TypeString and aws.StringValue() returns "" for nil.

m["identity_pool_id"] = aws.StringValue(c.IdentityPoolId)
m["user_pool_id"] = aws.StringValue(c.UserPoolId)
m["role_arn"] = aws.StringValue(c.RoleArn)

m["user_pool_id"] = *c.UserPoolId
}
if c.IdentityPoolId != nil {
m["identity_pool_id"] = *c.IdentityPoolId
}
if c.RoleArn != nil {
m["role_arn"] = *c.RoleArn
}
}

return []map[string]interface{}{m}
}

func flattenESEBSOptions(o *elasticsearch.EBSOptions) []map[string]interface{} {
m := map[string]interface{}{}

Expand Down
8 changes: 8 additions & 0 deletions website/docs/r/elasticsearch_domain.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,14 @@ Security Groups and Subnets referenced in these attributes must all be within th
* `cloudwatch_log_group_arn` - (Required) ARN of the Cloudwatch log group to which log needs to be published.
* `enabled` - (Optional, Default: true) Specifies whether given log publishing option is enabled or not.

**cognito_options** supports the following attribute:

AWS documentation: [Amazon Cognito Authentication for Kibana](https://docs.aws.amazon.com/elasticsearch-service/latest/developerguide/es-cognito-auth.html)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing the enabled attribute documentation 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️

* `user_pool_id` - (Required) ID of the Cognito User Pool to use
* `identity_pool_id` - (Required) ID of the Cognito Identity Pool to use
* `role_arn` - (Required) ARN of the IAM role that has the AmazonESCognitoAccess policy attached

## Attributes Reference

In addition to all arguments above, the following attributes are exported:
Expand Down