azurerm_kusto_cluster: fix perpetual diff on trusted_external_tenants - #32794
azurerm_kusto_cluster: fix perpetual diff on trusted_external_tenants#32794tpdownes wants to merge 1 commit into
Conversation
liuwuliuyun
left a comment
There was a problem hiding this comment.
Hi @tpdownes , thanks for the PR, I left some comments on the changed lines and I hope this PR could catch up to the 5.0 release in time.
| // set of tenant IDs, ignoring order and duplicates. It is used to suppress the perpetual diff caused | ||
| // by the Azure Data Explorer API returning the tenants in a different order than configured. | ||
| func trustedExternalTenantsEqual(a, b []interface{}) bool { | ||
| if len(a) != len(b) { |
There was a problem hiding this comment.
The comment says this compares sets while ignoring duplicates, but the length check and occurrence counts implement multiset equality. For example, ["tenant-a"] and ["tenant-a", "tenant-a"] return false.
Since this property is semantically set-valued and TypeSet deduplicates values in 5.0, please compare unique-value maps instead and add a duplicate-focused unit test. Alternatively, if duplicate counts are intentionally significant, please update the comment and naming accordingly.
| } | ||
|
|
||
| if features.FivePointOh() { | ||
| // `trusted_external_tenants` is an unordered set of tenant IDs. The Azure Data Explorer API |
There was a problem hiding this comment.
Changing trusted_external_tenants from TypeList to TypeSet is a breaking schema change: values become unordered and configurations can no longer reference them by numeric index.
The contributing guide requires breaking schema changes to be documented. Please add a bullet under azurerm_kusto_cluster in website/docs/5.0-upgrade-guide.html.markdown, such as:
The
trusted_external_tenantsproperty's type has changed from a List to a Set, meaning its values are unordered and can no longer be referenced by numeric index.
This follows the precedent established by #32460.
| Type: pluginsdk.TypeString, | ||
| ValidateFunc: validation.Any(validation.IsUUID, validation.StringIsEmpty, validation.StringInSlice([]string{"*"}, false)), | ||
| }, | ||
| } |
There was a problem hiding this comment.
The breaking-change guide recommends defining the target 5.0 schema normally and restoring the current behavior inside if !features.FivePointOh(). This keeps the eventual 5.0 cleanup deletion-only.
Please define trusted_external_tenants as the desired TypeSet in the main schema map, then replace it with the complete 4.x TypeList schema—including ConfigMode: pluginsdk.SchemaConfigModeAttr—inside the existing if !features.FivePointOh() block. The trailing if features.FivePointOh() block can then be removed.
liuwuliuyun
left a comment
There was a problem hiding this comment.
Left one comment, thanks~
3986508 to
52c8327
Compare
liuwuliuyun
left a comment
There was a problem hiding this comment.
The code changes look good to me now. One heads-up: since the main branch is on 5.0 now, you may need to follow the latest guideline for breaking changes once it’s published and adjust accordingly.
Are you telling me that I may need to update all the breaking features from 5.0 to 6.0? |
|
For the custom logic that compares two lists, I believe it will work in the 5.x release once this PR is merged, so it should not block any features. As for changing the property type from List to Set, I'm afraid that won't make it into 5.x because this PR was not merged before the 5.0 branch cut. In theory, that change would therefore be included in the 6.x release. Regarding your question, "Are you telling me that I may need to update all the breaking changes from 5.0 to 6.0?": Not necessarily. The custom list comparison logic should be available in 5.x, so that part does not require waiting for 6.x. The only change that would likely be deferred to 6.x is the List to Set property type change. And I respect members from Hashicorp's decision on this. |
The Azure Data Explorer API returns `trustedExternalTenants` in its own canonical order, which often differs from the configured order, producing a permanent no-op diff (hashicorp#32793). Add a `CustomizeDiff` that compares the old and new values as sets and, when they match, resets the planned value back to state. Genuine additions and removals change the set and still apply. The comparison is skipped unless the configured collection is wholly known, since `GetChange` can surface an unknown (known-after-apply) list as an empty slice, which would otherwise suppress a legitimate change.
52c8327 to
a7598d6
Compare
|
I've updated the PR for 5.0. I decided to remove all breaking changes and leave that as potential future work to plan and review. |
This comment has been minimized.
This comment has been minimized.
|
@tpdownes - One or more tests failed in this PR. Please review the failures. Total: 16 Test Details
|
Community Note
Description
azurerm_kusto_cluster.trusted_external_tenantsproduces a permanent, no-op plan diff. The Azure Data Explorer API returns the trusted tenants in its own canonical order, which frequently differs from the order submitted in configuration. Because the attribute is an order-significantTypeList, that reordering surfaces as a perpetual diff (typically a single-element rotation). Client-side normalization (sort()/reverse(sort())) in configuration cannot fix it, because the API's order is not derivable from the tenant values.Trust relationships are semantically an unordered set, so a
CustomizeDiffnow compares the old and new values as sets and, when they match, resets the planned value back to state. A genuine add/removal changes the set and still applies normally, so the attribute stays fully manageable.The comparison is skipped unless the configured collection is wholly known:
GetChangecan surface an unknown (known-after-apply) list as an empty slice, and if state is also empty the two would compare equal andSetNewwould incorrectly suppress a legitimate change.trustedExternalTenantsEqualcompares the two lists by their unique tenant IDs — set equality, ignoring order and duplicates.The attribute's schema type is unchanged. An earlier revision of this PR switched it to a
TypeSetunder the next major version flag; that has been dropped so this remains a non-breaking bug fix.PR Checklist
Changes to existing Resource / Data Source
go build/go vet/gofmtpass locally; the acceptance test requires Azure credentials and has not been run end-to-end — see Testing.)Testing
TestTrustedExternalTenantsEqualunit-tests the set-equality helper, including duplicate handling (a duplicate collapsing to the same set, reordered duplicates, and a duplicate masking a missing member). Passes locally:TestAccKustoCluster_trustedExternalTenantsre-applies the same set of tenants and then asserts an empty plan (PlanOnly: true) for both orderings —["*", tenant_id]and[tenant_id, "*"]. Because plan-only steps do not update state, at least one permutation is guaranteed to differ from Azure's canonical ordering, so the step reliably catches the perpetual diff regardless of which order the API returns. I do not have an Azure subscription set up to run the acceptance test end-to-end; happy for a maintainer to run it or for guidance.go build ./internal/services/kusto/...,go vet ./internal/services/kusto/...,gofmt, andTestProviderschema validation are clean.Change Log
azurerm_kusto_cluster- prevent a perpetual diff ontrusted_external_tenantscaused by the Azure Data Explorer API returning the tenants in a different order than configured [azurerm_kusto_cluster: fix perpetual diff on trusted_external_tenants #32794]This is a (please select all that apply):
Related Issue(s)
Fixes #32793
AI Assistance Disclosure
Extent of AI usage: the code change, the unit/acceptance tests, and this PR description were drafted with the help of an AI coding assistant (GitHub Copilot). All changes were reviewed by me, and the unit tests,
go build,go vet, andgofmtwere run locally to validate them. Responses to review feedback on this PR may also be prepared with AI assistance.