aws_emr_cluster importer

[stefansundin]

This is my first attempt at making an importer, but I tried this and it seems to work fine.

Not sure if aws_emr_instance_group would benefit from an importer as well? We don't use that resource so I don't have the ability to immediately test it.

Also fixed some typos in the docs for aws_emr_security_configuration.

Thanks!

[bflad]

This isn't as straightforward as it seems -- this resource is pretty old and does not properly set attributes in the Terraform state during the read function, which is now detected by the import test:

=== RUN   TestAccAWSEMRCluster_basic
--- FAIL: TestAccAWSEMRCluster_basic (606.94s)
    testing.go:518: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
        
        (map[string]string) {
        }
        
        
        (map[string]string) (len=6) {
         (string) (len=14) "configurations": (string) (len=37) "test-fixtures/emr_configurations.json",
         (string) (len=19) "core_instance_count": (string) (len=1) "1",
         (string) (len=18) "core_instance_type": (string) (len=8) "c4.large",
         (string) (len=33) "keep_job_flow_alive_when_no_steps": (string) (len=4) "true",
         (string) (len=20) "master_instance_type": (string) (len=8) "c4.large",
         (string) (len=22) "termination_protection": (string) (len=5) "false"
        }

In some of these cases, the fix is as simple as adding d.Set() from the API response in the read function. For others though its more complicated. Had I investigated #3652 more, definitely would have discovered these sooner. 😓

In the configurations attribute case, the resource handling of the attribute is completely incorrect to detect resource drift as its taking a file path/URL as input but the API is returning the actual JSON (of course it doesn't know or care about how the JSON content got there), yikes. You can see a related issues for this attribute in #543 and #1385. That attribute also has very weird behavior where it doesn't properly return errors as noted in #4247. Likely the only viable solution there is to create a new attribute (configurations_json?) that accepts a JSON string and deprecates the old attribute.

For now you can add this to the new import TestStep:

ImportStateVerifyIgnore: []string{"configurations"},

Sorry you found a can of worms! Are you able to see about fixing some of these easier ones (not configurations except with the ignore thing above)? If not, no big deal, we can add them to the ignore list for now and deal with bug reports later (which should have already been the case as they were missing drift detection). Some of these are thornier than others to properly get the information out of the API.

[stefansundin]

I'll give the easy ones a shot. Thanks for the write up.

[bflad]

Hey @stefansundin! 👋 Any luck so far? No worries if not, we're just trying to reduce the number of lingering pull requests and can make this work as-is. I didn't want to start doing anything here if you indeed had anything else to add to it. Thanks so much!

[stefansundin]

Hey @bflad. I gave it a try but I had some issues.

• I haven't found any way to get keep_job_flow_alive_when_no_steps by describing stuff.. so it seems that there is a legacy "job flow" API that terraform is using for some things, and it is hard to make sense of the documentation.. maybe this value is returned with another name in the new API.
• core_instance_type is already being set here: https://github.com/terraform-providers/terraform-provider-aws/blob/70417de771d89f3830277b907c0f0221600bfcec/aws/resource_aws_emr_cluster.go#L635
  • but for some reason the test still fails on it?
  • I was going to add d.Set("core_instance_count", coreGroup.RequestedInstanceCount) just below it.
• Think I can add d.Set("termination_protection", cluster.TerminationProtected) below this line: https://github.com/terraform-providers/terraform-provider-aws/blob/70417de771d89f3830277b907c0f0221600bfcec/aws/resource_aws_emr_cluster.go#L653
• I think I can add the following:

  masterGroup := findGroup(instanceGroups, "MASTER")
  if masterGroup != nil {
  	d.Set("master_instance_type", masterGroup.InstanceType)
  	d.Set("master_instance_count", masterGroup.RequestedInstanceCount)
  }
  

  right below this line: https://github.com/terraform-providers/terraform-provider-aws/blob/70417de771d89f3830277b907c0f0221600bfcec/aws/resource_aws_emr_cluster.go#L636
  For some reason there is no warning about master_instance_count, not sure why.

Anyway, the mixing of the legacy "job flow" API and the new cluster API made it a bit confusing to me. And I'm not an EMR expert by any means.

I have managed to run the tests now, but I haven't run it yet with the proposed changes above.

[stefansundin]

Hi @bflad. I rebased on master and made TestAccAWSEMRCluster_basic pass. I think it's merge-able now but please let me know if you can find anything else I should fix. Thanks!

[stefansundin]

@bflad I rebased again just to get Travis to rebuild and it passed now. I think it just encountered a random network error earlier, but I couldn't find a way to manually rebuild it.

[bflad]

Can you double check the other acceptance tests by adding the import TestStep? We need to verify that any configuration can successfully import without the core_instance ignores as well.

I think last time I ran the other acceptance tests with the additional TestStep I discovered that properly reading the attributes is actually more complicated since core_instance_count is actually 1 off from the actual value from the API (due to the master instance) and core_instance_type not being returned when there are no core instances created. I was hoping to provide some suggestions then got sidetracked and never commented here, sorry about that. 🙁

[stefansundin]

Thanks. I don't think I have enough area-expertise to solve this. If someone else wants to finish this, please feel free!

[bflad]

This has been released in version 1.56.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

[ghost]

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!