Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ISSUE|FEATURE] Existing Table permissions not detected at first apply #1164

Closed
ebarault opened this issue Mar 8, 2022 · 8 comments · Fixed by #1183
Closed

[ISSUE|FEATURE] Existing Table permissions not detected at first apply #1164

ebarault opened this issue Mar 8, 2022 · 8 comments · Fixed by #1183
Milestone

Comments

@ebarault
Copy link

ebarault commented Mar 8, 2022

Configuration

data "databricks_tables" "tables" {
  catalog_name = local.catalog_name
  schema_name  = local.schema_name
}

resource "databricks_grants" "grants" {
  for_each = data.databricks_tables.tables.ids

  table  = each.value

  dynamic "grant" {
    for_each = var.grants
    content {
      principal  = grant.value.principal
      privileges = grant.value.privileges
    }
  }
}

Expected Behavior

Existing permissions on a Unity Table should be detected and overwritten

Actual Behavior

The module manages new permissions, but does not detect/warns on existing permissions at the first terraform apply

Steps to Reproduce

  1. Create a UC table
CREATE TABLE main.default.test(id   INT);
  1. Grant SELECT permission to a user
GRANT SELECT on main.default.test TO some_user;
  1. (first) terraform apply new permissions to a different user
data "databricks_tables" "tables" {
  catalog_name = "main"
  schema_name  = "default"
}

resource "databricks_grants" "grants" {
  for_each = data.databricks_tables.tables.ids

  table  = each.value

  dynamic "grant" {
    for_each = var.grants
    content {
      principal  = "admin"
      privileges = "MODIFY"
    }
  }
}

Terraform proposes to add the "MODIFY" privilege on the test table but does not detect existing privilege granted to some_user outside of terraform.

  1. (second) terraform apply

Terraform detects the pre-existing privileges on the test table and proposes to remove them.

Terraform and provider versions

databricks provider version 0.5.2

@nfx
Copy link
Contributor

nfx commented Mar 8, 2022

@ebarault what host do you use? why mws alias?

@nfx
Copy link
Contributor

nfx commented Mar 8, 2022

please provide api call logs from debug

@ebarault
Copy link
Author

ebarault commented Mar 8, 2022

@nfx i use a workspace host as with any Unity Catalog config right now, while it is not ported at the account level
nevermind for the mws alias, it is just because i prepared my module for the day the api will be ported at account level

my setup works, it does create new permissions on the UC table and sees drifts on those permissions if altered from outside terraforml ; it just does not manage existing permissions

@nfx
Copy link
Contributor

nfx commented Mar 8, 2022

for the day the api will be ported at account level

i'll be splitting account-level entities into their own provider in the coming months. for now i recommend you splitting account and non-account into their own modules.

my setup works, it does create new permissions on the UC table and sees drifts on those permissions if altered from outside terraforml ; it just does not manage existing permissions

please specify exact step-by-step instructions on how to reproduce this issue.

@ebarault
Copy link
Author

ebarault commented Mar 8, 2022

@nfx I added step-by-step instructions in the issue description

@ebarault
Copy link
Author

ebarault commented Mar 8, 2022

@nfx IMPORTANT: the problem seems to happen only at the first terraform apply. Once the module is applied once, it seems it detects external GRANTS and proposes to remove them

EDIT: yes, I just tested again, that's the way it operates, i updated the description

@ebarault ebarault changed the title [ISSUE|FEATURE] Existing/Added Table permissions not detected [ISSUE|FEATURE] Existing Table permissions not detected on first apply Mar 8, 2022
@ebarault ebarault changed the title [ISSUE|FEATURE] Existing Table permissions not detected on first apply [ISSUE|FEATURE] Existing Table permissions not detected at first apply Mar 8, 2022
@ebarault
Copy link
Author

Hi @nfx, I spotted another edge case:

  • Create a table in a notebook
  • Assign privileges in terraform using the databricks_grants module
  • Delete the table in a notebook
  • Apply again the privileges in terraform: No changes. Your infrastructure matches the configuration.

Which leaves us forced to destroy the module and recreate it

@nfx
Copy link
Contributor

nfx commented Mar 14, 2022

@ebarault yep, this is due to lack of simple "replace permissions" API. thanks for identifying corner cases.

@nfx nfx added this to the v0.5.3 milestone Mar 14, 2022
nfx added a commit that referenced this issue Mar 14, 2022
Read existing permissions from platform before updating them, further improving drift detection. Existing permissions are merged onto diffs as removals.

Fix #1164
nfx added a commit that referenced this issue Mar 14, 2022
Read existing permissions from platform before updating them, further improving drift detection. Existing permissions are merged onto diffs as removals.

Fix #1164
@nfx nfx closed this as completed in #1183 Mar 14, 2022
nfx added a commit that referenced this issue Mar 14, 2022
Read existing permissions from platform before updating them, further improving drift detection. Existing permissions are merged onto diffs as removals.

Fix #1164
@nfx nfx mentioned this issue Mar 14, 2022
michael-berk pushed a commit to michael-berk/terraform-provider-databricks that referenced this issue Feb 15, 2023
Read existing permissions from platform before updating them, further improving drift detection. Existing permissions are merged onto diffs as removals.

Fix databricks#1164
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants