azurerm_storage_data_lake_gen2_path/azurerm_storage_data_lake_gen2_filesystem - ACLs generated by default are no longer stored in state to prevent perpetual state diffs

[dkuzmenok]

Description

By default, API is adding default rules for ACLs. You can overwrite them, modify, but they are always being added to a specific filesystem path.

That created a never-ending apply/plan cycle, when terraform expects to have specific list of ACLs and API adjusts it from their side.

Changes

The change is checking whether terraform wants to modify default ACLs, and if no - hides them from the list.

That makes sure we don't mix auto-added ACLs with the ones we do not expect to see. That is applied only to non-custom ACL entries.

Issues

Resolves #10814.
Resolves #11435.

Tests

# go test -v -timeout 3000s -run ^TestAccStorageDataLakeGen2 github.com/hashicorp/terraform-provider-azurerm/internal/services/storage/
=== RUN   TestAccStorageDataLakeGen2FileSystem_basic
=== PAUSE TestAccStorageDataLakeGen2FileSystem_basic
=== RUN   TestAccStorageDataLakeGen2FileSystem_requiresImport
=== PAUSE TestAccStorageDataLakeGen2FileSystem_requiresImport
=== RUN   TestAccStorageDataLakeGen2FileSystem_withDefaultACL
=== PAUSE TestAccStorageDataLakeGen2FileSystem_withDefaultACL
=== RUN   TestAccStorageDataLakeGen2FileSystem_UpdateDefaultACL
=== PAUSE TestAccStorageDataLakeGen2FileSystem_UpdateDefaultACL
=== RUN   TestAccStorageDataLakeGen2FileSystem_properties
=== PAUSE TestAccStorageDataLakeGen2FileSystem_properties
=== RUN   TestAccStorageDataLakeGen2FileSystem_handlesStorageAccountDeletion
=== PAUSE TestAccStorageDataLakeGen2FileSystem_handlesStorageAccountDeletion
=== RUN   TestAccStorageDataLakeGen2FileSystem_withOwnerGroup
=== PAUSE TestAccStorageDataLakeGen2FileSystem_withOwnerGroup
=== RUN   TestAccStorageDataLakeGen2FileSystem_withSuperUsers
=== PAUSE TestAccStorageDataLakeGen2FileSystem_withSuperUsers
=== RUN   TestAccStorageDataLakeGen2Path_basic
=== PAUSE TestAccStorageDataLakeGen2Path_basic
=== RUN   TestAccStorageDataLakeGen2Path_requiresImport
=== PAUSE TestAccStorageDataLakeGen2Path_requiresImport
=== RUN   TestAccStorageDataLakeGen2Path_withSimpleACLAndUpdate
=== PAUSE TestAccStorageDataLakeGen2Path_withSimpleACLAndUpdate
=== RUN   TestAccStorageDataLakeGen2Path_withSimpleACL
=== PAUSE TestAccStorageDataLakeGen2Path_withSimpleACL
=== RUN   TestAccStorageDataLakeGen2Path_withACLWithSpecificUserAndDefaults
=== PAUSE TestAccStorageDataLakeGen2Path_withACLWithSpecificUserAndDefaults
=== RUN   TestAccStorageDataLakeGen2Path_withOwner
=== PAUSE TestAccStorageDataLakeGen2Path_withOwner
=== RUN   TestAccStorageDataLakeGen2Path_withSuperUsers
=== PAUSE TestAccStorageDataLakeGen2Path_withSuperUsers
=== CONT  TestAccStorageDataLakeGen2FileSystem_basic
=== CONT  TestAccStorageDataLakeGen2Path_basic
=== CONT  TestAccStorageDataLakeGen2Path_withACLWithSpecificUserAndDefaults
=== CONT  TestAccStorageDataLakeGen2FileSystem_properties
--- PASS: TestAccStorageDataLakeGen2FileSystem_basic (127.50s)
=== CONT  TestAccStorageDataLakeGen2Path_withSuperUsers
--- PASS: TestAccStorageDataLakeGen2Path_basic (149.22s)
=== CONT  TestAccStorageDataLakeGen2Path_withOwner
--- PASS: TestAccStorageDataLakeGen2Path_withACLWithSpecificUserAndDefaults (162.82s)
=== CONT  TestAccStorageDataLakeGen2FileSystem_withDefaultACL
--- PASS: TestAccStorageDataLakeGen2FileSystem_properties (249.68s)
=== CONT  TestAccStorageDataLakeGen2FileSystem_UpdateDefaultACL
--- PASS: TestAccStorageDataLakeGen2Path_withSuperUsers (170.32s)
=== CONT  TestAccStorageDataLakeGen2FileSystem_requiresImport
--- PASS: TestAccStorageDataLakeGen2Path_withOwner (285.87s)
=== CONT  TestAccStorageDataLakeGen2Path_withSimpleACLAndUpdate
--- PASS: TestAccStorageDataLakeGen2FileSystem_withDefaultACL (274.82s)
=== CONT  TestAccStorageDataLakeGen2Path_withSimpleACL
--- PASS: TestAccStorageDataLakeGen2FileSystem_requiresImport (183.59s)
=== CONT  TestAccStorageDataLakeGen2FileSystem_withOwnerGroup
--- PASS: TestAccStorageDataLakeGen2FileSystem_UpdateDefaultACL (282.75s)
=== CONT  TestAccStorageDataLakeGen2FileSystem_withSuperUsers
--- PASS: TestAccStorageDataLakeGen2Path_withSimpleACL (142.06s)
=== CONT  TestAccStorageDataLakeGen2Path_requiresImport
--- PASS: TestAccStorageDataLakeGen2FileSystem_withOwnerGroup (107.48s)
=== CONT  TestAccStorageDataLakeGen2FileSystem_handlesStorageAccountDeletion
--- PASS: TestAccStorageDataLakeGen2Path_withSimpleACLAndUpdate (274.19s)
--- PASS: TestAccStorageDataLakeGen2FileSystem_withSuperUsers (178.01s)
--- PASS: TestAccStorageDataLakeGen2FileSystem_handlesStorageAccountDeletion (172.19s)
--- PASS: TestAccStorageDataLakeGen2Path_requiresImport (217.87s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/storage       797.602s
#

[aristosvo]

Hi @dkuzmenok ! Just taking a quick look, I was under the impression that this might change the behaviour of this resource. I'm just not sure whether this is considered breaking :)

If this changes behaviour in a breaking way, functionality should be hidden behind a flag.., WDYT?

[dkuzmenok]

@aristosvo It would not change planned changes, instead, it would properly report planned changes when you don't want to modify default ACLs, which you cannot deny or prevent. If you don't override defaults, they will always be there and your plan would always complain that you have ACLs returned from API, that you don't expect.

[dkuzmenok]

I feel that description is not clear enough.

For every filesystem items API adds "default" ACLs:

scope: default
id: *empty*
type: user

scope: default
id: *empty*
type: group

scope: default
id: *empty*
type: other

scope: default
id: *empty*
type: mask

If you don't set ACLs for those scope+id+type, you will get it from API anyway, and you planned changes would reflect difference between expectations and reality.
This PR is checking if you want to override any default - you will get it from API response, otherwise default ACLs are filtered.

[dkuzmenok]

If this PR brings doubts, then maybe we can enable this functionality by a new feature ? Then default behavior will not change.

[dkuzmenok]

Any thoughts about this PR?

[mbfrahry]

Hey @dkuzmenok, thanks for opening this PR. This will help so much for sure but the only negative I see is that we won't be storing default acls into state that haven't been specified in the config file.

Do you have an idea of if it would be helpful to still have that default acl information stored in the state file for someone to view? If so, we could add another attribute called default_acl or something like that that will be computed and store that information. But if you don't think that information would be helpful then this PR looks good!

[dkuzmenok]

@mbfrahry As far as I understand, we cannot modify anything, that goes in state, on the initial apply (without refresh).
That is why, instead of modifying what people fill, I modify what API returns.

Is there any example of modifying state (or what comes to state) to take a look?

[mbfrahry]

You can use a debugger if you're coding with an IDE or just return an error just before this line to see what is being returned and saved into state

  return fmt.Errorf("Ace List: %+v", FlattenDataLakeGen2AceList(d, acl))

Though, I don't think you need to modify state for what I'm asking. To frame it a different way, right now default acls are being saved into state which is causing issues with people that haven't specified those default acls in their config file. Your PR stops those default acls from being saved in state unless they've been specified in the config.

That could be considered a breaking change as the attribute was storing information people could be using. Thought I can't imagine people were referencing default ACLs for anything? That's what I'm not sure about. I think we'll just merge this in and if people do want to see default acls, we can add a new property later. Thanks for this @dkuzmenok!

[mbfrahry]

LGTM

[dkuzmenok]

@mbfrahry

Thought I can't imagine people were referencing default ACLs for anything? That's what I'm not sure about. I think we'll just merge this in and if people do want to see default acls, we can add a new property later.

That was exactly my thought. If people don't want to change default values - they should not come up as a "difference" with an Azure state response. But if you want to override something - you should save that in the state and properly compare with Azure state.

[github-actions]

This functionality has been released in v3.39.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.