azurerm_service_fabric_cluster support for azure_active_directory

[DenheenJ]

Community Note

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
• If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

Add support for Azure Active Directory Security to Azure Service Fabric Clusters

New or Affected Resource(s)

azurerm_service_fabric_cluster
feature
azure_active_directory

Potential Terraform Configuration

	azure_active_directory {
	  tenant_id           = "00000000-0000-0000-0000-000000000000"
	  cluster_application = "00000000-0000-0000-0000-000000000000"
	  client_application  = "00000000-0000-0000-0000-000000000000"
	}

Potential Fix

https://github.com/DenheenJ/terraform-provider-azurerm
affected files:
azurerm/resource_arm_service_fabric_cluster.go
azurerm/resource_arm_service_fabric_cluster_test.go
website/docs/r/service_fabric_cluster.html.markdown

• #0000

[tombuildsstuff]

hey @DenheenJ

Thanks for opening this issue :)

Taking a quick look into your branch this appears to be off to a good start - I'd suggest in Terraform that these fields may make more sense as cluster_application_id and client_application_id, to indicate that these values are ID's - what do you think?

Thanks!

[DenheenJ]

@tombuildsstuff i think you are right, i've amended the fields and updated master on the fork.

My only issue with the implementation is that it must create a new resource if the azure_active_directory sub resource is used, as the azure sdk similar to the diagnostics sub resource doesn't support updates at present only create.

[tombuildsstuff]

@DenheenJ

@tombuildsstuff i think you are right, i've amended the fields and updated master on the fork.

👍 sounds good - if you're planning on opening a PR for this would you mind doing so from a branch other than master? Occasionally it's helpful to push some minor changes prior to merging (e.g. rebasing to fix any merge conflicts), but the automatic branch protection for the master branch can mean it's not possible to push those changes, unfortunately

My only issue with the implementation is that it must create a new resource if the azure_active_directory sub resource is used, as the azure sdk similar to the diagnostics sub resource doesn't support updates at present only create.

That's a wider issue across multiple resources in the Azure API - as such I'm not sure there's much else we can do to work around that, unfortunately - so this seems like a sensible approach for the moment 👍

Thanks!

[DenheenJ]

PR created
#2553

Thank you for the input @tombuildsstuff 👍

[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!