Handle UC Resource Grants with Spaces

[ledbutter]

Changes

In our (private) Terraform module we were trying to configure grants using the databricks_grants resource, but kept running into trouble on apply because it detected duplicate grants to add/remove (snippet from our CI/CD pipeline):

{
"add": [
"CREATE EXTERNAL VOLUME",
"CREATE EXTERNAL TABLE",
"READ FILES",
"WRITE FILES"
],
"principal": "58264056-137c-468c-850f-fff3042498d5",
"remove": [
"CREATE_EXTERNAL_VOLUME",
"READ_FILES",
"CREATE_EXTERNAL_TABLE",
"WRITE_FILES"
]
},

This was because we were missing the _ characters there (notice the format of the privs to add). I've confirmed that the Databricks CLI/API silently accepts this:

databricks grants update external_location my_loc -p dev --json '{"changes": [{"principal": "user", "add": ["ALL PRIVILEGES"]}]}'

So here I add some tests showing the gap, and then added some invocations of the already-existing validate method on CREATE/UPDATE. I certainly am open to other approaches, just wanted to get this out here for discussion.

I'd also be fine with instead updating the "diff" logic here to also look for and replace spaces with underscores, but the existing validate tests seem to already account for that, but I'm not sure where--if at all--validate is called on the mapping in full system calls.

Tests

• make test run locally
• relevant change in docs/ folder
• covered with integration tests in internal/acceptance
• relevant acceptance tests are passing
• using Go SDK

[ledbutter]

Since the underlying "mapping" has gone away since I started on this, I've switched this behavior to match the API and just silently treat " " as _ so that diff'ing can treat "ALL PRIVILEGES" as "ALL_PRIVILEGES" and react accordingly

[alexott]

Does it work correctly? No configuration drift after apply?

[ledbutter]

Does it work correctly? No configuration drift after apply?

Sorry, I should have provided a little more detail originally. No, it does not work.

If I define a resource like this:

resource "databricks_grants" "external_location" {
  external_location = "my_location"

  grant {
    principal  = "user"
    privileges = ["ALL PRIVILEGES"] # this should have an underscore between words
  }
}

The initial attempt fails with an error like this:

cannot create grants: permissions for external_location-dev_my_location &{[{user [ALL_PRIVILEGES] [Principal]}] but have to be {[{user [ALL PRIVILEGES] []}]}

Retries fail with:

│ Error: cannot create grants: Duplicate privileges to add and delete.
│
│ with databricks_grants.external_location["my_location"],
│ on externalLocations.tf line 85, in resource "databricks_grants" "external_location":
│ 85: resource "databricks_grants" "external_location" {

Which matches the behavior in the CLI:

databricks grants update external_location my_location -p my_profile --json '{"changes": [{"principal": "user", "add": ["ALL PRIVILEGES"]},{"principal": "user", "remove": ["ALL_PRIVILEGES"]} ]}'
Error: Duplicate privileges to add and delete.

The diff'ing logic doesn't take into consideration that "ALL PRIVILEGES" is the same as "ALL_PRIVILEGES" in the databricks API/CLI (it silently just accepts it without the underscore).

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.58%. Comparing base (e78079d) to head (c4e9df0).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3292      +/-   ##
==========================================
- Coverage   83.59%   83.58%   -0.01%     
==========================================
  Files         178      178              
  Lines       16759    16769      +10     
==========================================
+ Hits        14009    14017       +8     
- Misses       1899     1900       +1     
- Partials      851      852       +1     

Files	Coverage Δ
catalog/resource_grant.go	81.25% <100.00%> (+0.81%)	⬆️
catalog/resource_grants.go	84.12% <100.00%> (+0.52%)	⬆️
common/resource.go	63.87% <ø> (ø)
qa/testing.go	73.21% <ø> (ø)

... and 1 file with indirect coverage changes

[mgyucht]

@ledbutter Thanks for this contribution! We've discussed this problem and your approach seems to solve one half of the problem. The other half is making sure that there is no diff from TF perspective at plan time if the only difference is underscores. Can you add logic to CustomizeDiff for these resources to ensure there is no diff is the only difference between config & state is an underscore in the permission level?

[ledbutter]

I've pushed changes for just the singular grant resource so far, but I'm still trying to figure out how to test this properly: I can't find prior art for a scenario like this with the CustomizeDiff being tested like this. The closest I've found is validating the "health" attribute is cleared on sql endpoint, but that doesn't require any actual setup. I'll keep grinding on this as I get the time.

[ledbutter]

@ledbutter Thanks for this contribution! We've discussed this problem and your approach seems to solve one half of the problem. The other half is making sure that there is no diff from TF perspective at plan time if the only difference is underscores. Can you add logic to CustomizeDiff for these resources to ensure there is no diff is the only difference between config & state is an underscore in the permission level?

After more research, it seems like DiffSuppressFunc is what we actually want to use in this case. I'm having a hard time validating it is working correctly however.

[nkvuong]

@ledbutter the only test I've seen that test this behaviour is TestCatalogSuppressCaseSensitivity under catalog/resource_catalog_test.go

[ledbutter]

@ledbutter the only test I've seen that test this behaviour is TestCatalogSuppressCaseSensitivity under catalog/resource_catalog_test.go

Right, and this is so trivial I don't see much value in it, but I will certainly add those tests if desired. What I'd really like to validate is the actual plan/apply suppresses this as expected, especially for the grants case where I'm a little less sure of the approach.

[nkvuong]

@ledbutter the only test I've seen that test this behaviour is TestCatalogSuppressCaseSensitivity under catalog/resource_catalog_test.go

Right, and this is so trivial I don't see much value in it, but I will certainly add those tests if desired. What I'd really like to validate is the actual plan/apply suppresses this as expected, especially for the grants case where I'm a little less sure of the approach.

For unit testing, this is the way to test as we should validate that the ExpectedDiff is calculated correctly by Terraform. For integration test, you could simply create a test that creates a grant resource with space in privileges, e.g. similar to TestUcAccGrantsForIdChange. The integration test fixture will create the resources, validate that the tf plan is clean (which should confirm that the suppressdiff works as expected)

[ledbutter]

@ledbutter the only test I've seen that test this behaviour is TestCatalogSuppressCaseSensitivity under catalog/resource_catalog_test.go

Right, and this is so trivial I don't see much value in it, but I will certainly add those tests if desired. What I'd really like to validate is the actual plan/apply suppresses this as expected, especially for the grants case where I'm a little less sure of the approach.

For unit testing, this is the way to test as we should validate that the ExpectedDiff is calculated correctly by Terraform. For integration test, you could simply create a test that creates a grant resource with space in privileges, e.g. similar to TestUcAccGrantsForIdChange. The integration test fixture will create the resources, validate that the tf plan is clean (which should confirm that the suppressdiff works as expected)

I think I've done this with my latest commits, but I'm uncertain about how I'm trying to assert the integration tests...

[nkvuong]

it should suffice to just have a single step with ALL PRIVILEGES, the test would fail if there is still a diff shown in the plan

[ledbutter]

Ok, updated

[nkvuong]

@ledbutter integration test failed, so I looked at this in a bit more details. DiffSuppressFunc only works for primitive, i.e. string-like attributes. For lists, we need to look at CustomizeDiff instead.

@tanmay-db we probably need to re-add a lightweight version of CustomizeDiff to handle this scenario, as it is causing permanent drifts

[ledbutter]

I initially tried using CustomizeDiff, but that only works for computed keys, as I mentioned here. Are you saying that there needs to be a lower level change before this can be accomplished?

[ledbutter]

@nkvuong @mgyucht So I have half of the problem solved, but I'm not sure how--if at all, given the current tooling--to solve the other half: the suppression of any diffs during a plan operation. Thoughts?

I see the options as:

• Roll forward with just the "normalization" and leave the phantom diff as-is
• Abandon this
• Figure out a way to handle customizing diffs in this way (I'd need help on this as it seems to be tied to limitations in the overall tooling)

[ledbutter]

@nkvuong Thanks for fixing this in a better way! I'll close this now since it is no longer needed