azurerm_hdinsight_* - support HDInsight clusters with Data Lake Gen2 Filesystems

[dintskirveli]

Allows setting up HDInsight clusters using Data Lake Gen2 Filesystems for storage.

  storage_account_gen2 {
    storage_resource_id = azurerm_storage_account.landingarea.id
    filesystem_id  = azurerm_storage_data_lake_gen2_filesystem.landingarea.id
    managed_identity_resource_id = azurerm_user_assigned_identity.my-identity.id
    is_default           = true
  }

[dintskirveli]

@tombuildsstuff any chance we can get a review here?

[dintskirveli]

@katbyte @tombuildsstuff can we please get a review here?

[katbyte]

Hi @dintskirveli,

Thank you for this PR. Looking over the schema i am wondering it might not be better to create a new block storage_account_gen2 to simplify the logic, and then on expand/flatten we can just merge/split them by the type of storage account?

Also it looks like you have only updated the expand function. I think you need to update the flatten one too? as well as write some tests to ensure it works with each affected resource.

[dintskirveli]

Hi @dintskirveli,

Thank you for this PR. Looking over the schema i am wondering it might not be better to create a new block storage_account_gen2 to simplify the logic, and then on expand/flatten we can just merge/split them by the type of storage account?

Also it looks like you have only updated the expand function. I think you need to update the flatten one too? as well as write some tests to ensure it works with each affected resource.

@katbyte, thanks for looking!

I agree that storage_account_gen2 might be a better option.

As for tests... I can give them a try, but creating managed identities and assigning roles to them requires "Owner"-level privileges on the testing account. Are the test accounts expected to have those kinds of permissions?

[katbyte]

@dintskirveli,

For our CI they should have permission, and if not we can see about finagling something so we can run the tests 🙂

[dintskirveli]

Working on tests + updated documentation.

Based on this comment, there's no flatten to implement: https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/resource_arm_hdinsight_hadoop_cluster.go#L249

I don't think it matters since it is not possible update the storage account after creation of the cluster.

[dintskirveli]

Hi @katbyte!

This is ready for another pass. I have added tests for all relevant cluster types, split Gen2 config into a separate block, and added documentation. Note that I've omitted a few HDInsight cluster types because they do not support Gen2 storage:

• storm
• rserver
• ml_services

[katbyte]

Hi @dintskirveli,

I've run the tests and given this a quick review. In addition to a couple of comments i've left inline could we add for each resource a test that has both storage account times, with 1 default, and then another test that goes from gen1 -> both -> gen2?

Also the tests are failing with this:

Test ended in panic.

------- Stdout: -------
=== RUN   TestAccAzureRMHDInsightMLServicesCluster_virtualNetwork
=== PAUSE TestAccAzureRMHDInsightMLServicesCluster_virtualNetwork
=== CONT  TestAccAzureRMHDInsightMLServicesCluster_virtualNetwork

------- Stderr: -------
2019/11/14 23:30:33 [DEBUG] Registering Data Sources for "Compute"..
2019/11/14 23:30:33 [DEBUG] Registering Resources for "Compute"..
panic: interface conversion: interface {} is nil, not []interface {}

goroutine 420 [running]:
github.com/terraform-providers/terraform-provider-azurerm/azurerm.resourceArmHDInsightMLServicesClusterCreate(0xc000b973b0, 0x3b3a7a0, 0xc0016f2480, 0x0, 0x0)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/azurerm/resource_arm_hdinsight_ml_services_cluster.go:156 +0x1b29
github.com/hashicorp/terraform-plugin-sdk/helper/schema.(*Resource).Apply(0xc00060df80, 0xc000cc5d10, 0xc001681640, 0x3b3a7a0, 0xc0016f2480, 0xc000dadd01, 0xc000f77dd0, 0xc000dade40)
	/opt/teamcity-agent/work/458e5e4800bd94f6/pkg/mod/github.com/hashicorp/[email protected]/helper/schema/resource.go:305 +0x365
github.com/hashicorp/terraform-plugin-sdk/helper/schema.(*Provider).Apply(0xc0007f4900, 0xc000d91918, 0xc000cc5d10, 0xc001681640, 0xc000dadd80, 0xc000d8ab00, 0x3c8f760)
	/opt/teamcity-agent/work/458e5e4800bd94f6/pkg/mod/github.com/hashicorp/[email protected]/helper/schema/provider.go:294 +0x99
github.com/hashicorp/terraform-plugin-sdk/internal/helper/plugin.(*GRPCProviderServer).ApplyResourceChange(0xc0000b50c0, 0x4c6dc80, 0xc000f50f90, 0xc0012cce40, 0xc0000b50c0, 0xc000f50f90, 0xc000644a80)
	/opt/teamcity-agent/work/458e5e4800bd94f6/pkg/mod/github.com/hashicorp/[email protected]/internal/helper/plugin/grpc_provider.go:885 +0x882
github.com/hashicorp/terraform-plugin-sdk/internal/tfplugin5._Provider_ApplyResourceChange_Handler(0x403dae0, 0xc0000b50c0, 0x4c6dc80, 0xc000f50f90, 0xc0012ccde0, 0x0, 0x4c6dc80, 0xc000f50f90, 0xc000f5f500, 0x14c5)
	/opt/teamcity-agent/work/458e5e4800bd94f6/pkg/mod/github.com/hashicorp/[email protected]/internal/tfplugin5/tfplugin5.pb.go:3189 +0x217
google.golang.org/grpc.(*Server).processUnaryRPC(0xc000ef58c0, 0x4cc5d60, 0xc000cf0300, 0xc0008f8900, 0xc000d294a0, 0x77e90a0, 0x0, 0x0, 0x0)
	/opt/teamcity-agent/work/458e5e4800bd94f6/pkg/mod/google.golang.org/[email protected]/server.go:995 +0x460
google.golang.org/grpc.(*Server).handleStream(0xc000ef58c0, 0x4cc5d60, 0xc000cf0300, 0xc0008f8900, 0x0)
	/opt/teamcity-agent/work/458e5e4800bd94f6/pkg/mod/google.golang.org/[email protected]/server.go:1275 +0xd97
google.golang.org/grpc.(*Server).serveStreams.func1.1(0xc000db8320, 0xc000ef58c0, 0x4cc5d60, 0xc000cf0300, 0xc0008f8900)
	/opt/teamcity-agent/work/458e5e4800bd94f6/pkg/mod/google.golang.org/[email protected]/server.go:710 +0xbb
created by google.golang.org/grpc.(*Server).serveStreams.func1
	/opt/teamcity-agent/work/458e5e4800bd94f6/pkg/mod/google.golang.org/[email protected]/server.go:708 +0xa1

[katbyte]

could we move this to the bottom of the block docs (for all resource docs)

[dintskirveli]

Hmm, it looks to me like the convention is to have the NOTE follow the most relevant attribute. Let me know if you do still want this.

[dintskirveli]

@katbyte

Thanks for the review!

• I fixed the test failure, it was caused by mishandling of storage_account_gen2 in cluster types that don't support it
• Improved the validation of resource IDs per your suggestion
• Holding off on the documentation change, let me know if you still feel it's necessary.

[katbyte]

Thanks for the updated @dintskirveli, LGTM now!

[ghost]

This has been released in version 1.37.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.37.0"
}
# ... other configuration ...

[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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!