[WIP] r/aws_elasticsearchdomain: add support for cognito_options #5346
Conversation
Fix bug causing test to fail Add extra validation to assert CognitoOptions were set properly
bflad
added
enhancement
There was a problem hiding this comment.
HI @aoggz 👋 Thanks for submitting this, pretty exciting! Overall this is definitely on the right track, but I left some initial feedback. Please let us know if you have any questions or do not have time to implement these items.
As for the acceptance test refresh failure:
UPDATE: aws_cognito_identity_pool.example
cognito_identity_providers.#: "1" => "0"
cognito_identity_providers.188648750.client_id: "l5vruqq3skb07fcbq2dgfbfuo" => ""
cognito_identity_providers.188648750.provider_name: "cognito-idp.us-west-2.amazonaws.com/us-west-2_AhWuyNOVP" => ""
cognito_identity_providers.188648750.server_side_token_check: "true" => "false"
I'm personally okay if we double check if there's not already a filed service/cognito issue for this or create a new issue if there's not.
Once we have an issue number and since this refresh failure is outside the scope of the resource we are working on here, I'd recommend adding the following to the test configuration:
resource "aws_cognito_identity_pool" "example" {
# ... other configuration ...
# See also: https://github.com/terraform-providers/terraform-provider-aws/issues/XXXX
lifecycle {
ignore_changes = ["cognito_identity_providers"]
}
}I don't think we need to hold up this pull request to try and potentially adjust the other resource to better handle this situation.
| if v, ok := d.GetOk("cognito_options"); ok { | ||
|
|
||
| options := v.([]interface{}) | ||
| if len(options) > 1 { |
There was a problem hiding this comment.
Nitpick: This error checking is already performed by the attribute schema so it can be removed 👍
MaxItems: 1handleslen(options) > 1Required: trueon nested arguments handlesoptions[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)
}There was a problem hiding this comment.
Oh cool! That's much cleaner!
| @@ -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") { | |||
| @@ -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{}) | |||
There was a problem hiding this comment.
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. 👍
aws/structure.go
Outdated
| @@ -1088,6 +1088,51 @@ func flattenESClusterConfig(c *elasticsearch.ElasticsearchClusterConfig) []map[s | |||
| return []map[string]interface{}{m} | |||
| } | |||
|
|
|||
| func expandESCognitoOptions(m map[string]interface{}) *elasticsearch.CognitoOptions { | |||
There was a problem hiding this comment.
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{}))
}
aws/structure.go
Outdated
| } | ||
|
|
||
| if aws.BoolValue(c.Enabled) { | ||
| if c.UserPoolId != nil { |
There was a problem hiding this comment.
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)| **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) | ||
|
|
There was a problem hiding this comment.
This is missing the enabled attribute documentation 😄
bflad
added
the
waiting-response
|
@bflad Thanks for the feedback! I didn't find an issue already entered, but as I was about to enter one, I wondered if it makes sense to? It doesn't feel like an issue that needs fixed, since terraform is technically working as one would expect. The I'll work on implementing the changes you suggested (thanks again!) and will enter the issue if you feel like it's the way we should go. |
|
@aoggz you're probably right for now. 👍 |
|
@bflad I'm still working on incorporating your feedback, but I'm having some trouble with the tests. With the latest version, I'm seeing this issue when running my multi-step test. |
|
The schema can sometimes be a little awkward with "container" attributes like Here's code I've used to workaround this with some other resources: DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
if old == "1" && new == "0" {
return true
}
return false
},Hope this helps. |
Add func to suppress diff if cognito_options count changes from 1 to 0
|
@bflad, your suggestion worked! I added another test and simplified the test methods. |
bflad
removed
the
waiting-response
There was a problem hiding this comment.
Looks great, @aoggz! 🚀
17 tests passed (all tests)
=== RUN TestAccAWSElasticSearchDomain_duplicate
--- PASS: TestAccAWSElasticSearchDomain_duplicate (579.48s)
=== RUN TestAccAWSElasticSearchDomain_encrypt_at_rest_specify_key
--- PASS: TestAccAWSElasticSearchDomain_encrypt_at_rest_specify_key (755.62s)
=== RUN TestAccAWSElasticSearchDomain_v23
--- PASS: TestAccAWSElasticSearchDomain_v23 (768.86s)
=== RUN TestAccAWSElasticSearchDomain_tags
--- PASS: TestAccAWSElasticSearchDomain_tags (901.22s)
=== RUN TestAccAWSElasticSearchDomain_encrypt_at_rest_default_key
--- PASS: TestAccAWSElasticSearchDomain_encrypt_at_rest_default_key (948.02s)
=== RUN TestAccAWSElasticSearchDomain_LogPublishingOptions
--- PASS: TestAccAWSElasticSearchDomain_LogPublishingOptions (967.62s)
=== RUN TestAccAWSElasticSearchDomain_importBasic
--- PASS: TestAccAWSElasticSearchDomain_importBasic (967.74s)
=== RUN TestAccAWSElasticSearchDomain_complex
--- PASS: TestAccAWSElasticSearchDomain_complex (1122.03s)
=== RUN TestAccAWSElasticSearchDomain_basic
--- PASS: TestAccAWSElasticSearchDomain_basic (1276.59s)
=== RUN TestAccAWSElasticSearchDomain_vpc
--- PASS: TestAccAWSElasticSearchDomain_vpc (1425.47s)
=== RUN TestAccAWSElasticSearchDomain_CognitoOptionsCreateAndRemove
--- PASS: TestAccAWSElasticSearchDomain_CognitoOptionsCreateAndRemove (1513.36s)
=== RUN TestAccAWSElasticSearchDomain_policy
--- PASS: TestAccAWSElasticSearchDomain_policy (1824.14s)
=== RUN TestAccAWSElasticSearchDomain_update
--- PASS: TestAccAWSElasticSearchDomain_update (1876.49s)
=== RUN TestAccAWSElasticSearchDomain_internetToVpcEndpoint
--- PASS: TestAccAWSElasticSearchDomain_internetToVpcEndpoint (2006.04s)
=== RUN TestAccAWSElasticSearchDomain_CognitoOptionsUpdate
--- PASS: TestAccAWSElasticSearchDomain_CognitoOptionsUpdate (2049.42s)
=== RUN TestAccAWSElasticSearchDomain_vpc_update
--- PASS: TestAccAWSElasticSearchDomain_vpc_update (2119.34s)
=== RUN TestAccAWSElasticSearchDomain_update_volume_type
--- PASS: TestAccAWSElasticSearchDomain_update_volume_type (2769.39s)
|
This has been released in version 1.30.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
ghost
locked and limited conversation to collaborators
Fixes #4061
Changes proposed in this pull request:
Output from acceptance testing:
My test is currently failing. It looks like this is due to the 'behind-the-scenes' work that the Elasticsearch service is doing to configure Cognito to work with the cluster. It looks like the updates that the Elasticsearch service makes to the Cognito Identity Pool are being pulled in during the
refresh, which is causing the test to fail (since tf wasn't aware of the 'initial' state).I'm opening this PR before the this change is ready because I couldn't find an instance in the repo where a similar situation is occurring and would greatly appreciate feedback on how best to handle this!