Update property name for kusto database principal

[neil-yechenwei]

fixes #7383

[jackofallops]

Hi @neil-yechenwei
Thanks for this PR.

It appears that when the resource was introduced the properties were incorrectly named for the structure they're being used to create. I think object_id is appropriate, but client_id is the misleading item in this case, as it's being used to specify the tenant_id portion of the AAD Principal.

I think we should add a schema entry for the principal string itself (with appropriate parse/validation) to deprecate and eventually replace type, client_id and object_id, rather than trying to unpick this by renaming attributes and adding to the confusion. The new attribute, and the attributes being deprecated will need to be set to Optional, and still supported/used if specified as they are now to prevent breaking change to existing usage.

[jrauschenbusch]

@neil-yechenwei @jackofallops Another proposal. We could add a whole new resource azurerm_database_principal_assignment and deprecate the current one.

Why? Because at the time of writing the azurerm_database_principal resource, there was no dedicated API endpoint for managing principal assignments. But as of 2020-02-15 API there are now additional endpoints to manage cluster and database principals.

API example: https://github.com/Azure/azure-rest-api-specs/blob/master/specification/azure-kusto/resource-manager/Microsoft.Kusto/stable/2020-02-15/examples/KustoDatabasePrincipalAssignmentsCreateOrUpdate.json

Go model: https://github.com/Azure/azure-sdk-for-go/blob/master/services/kusto/mgmt/2020-02-15/kusto/models.go#L1761

Proposal:

resource "azurerm_database_principal_assignment" "example" {
  "principal_id"   = "XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX" // user email, application ID, or security group name.
  "principal_type" = "App" // Possible values include: 'App', 'Group', 'User'
  "role"           = "Viewer" //  Possible values include: 'Admin', 'Ingestor', 'Monitor', 'User', 'UnrestrictedViewers', 'Viewer'
  "tenant_id"      = "XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX"
}

Why not replacing everything within the current implementation?

1. because it would be a breaking change and a lot of users would complain about it. (incompatible state, different attributes)
2. because there are dedicated endpoints and we now have the opportunity to use real IDs (non-artifical IDs) as we had to create before.

The same design could be applied to a new azurerm_cluster_principal_assignment resource.

[katbyte]

What is the reasoning behind changing the FQDN format here? as a client belongs in a tenant should it be tenant/client not client/tenant? this will change ther esource id and break existing users.

[katbyte]

i missed the above review from an old tab in changed - i like the proposal for a new resource and deprecating the old one entirely (and maybe adding notes in the docs & resource/property deprecation messages about the confusion

[jrauschenbusch]

I've now created a PR for my resource proposal:
#7484

Please take a look and let me know what you think about it.

Deprecation of the current resource is still open, but would be also part of the PR.

[jackofallops]

Thanks @jrauschenbusch - That feels like a more comprehensive way to handle the issue; as renaming of the schema here to correct an unfortunate earlier mis-naming feels unwieldy and likely to cause confusion. I'm going to close this in favour of your new resource on the newer API.

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