Snowflake-Labs / terraform-provider-snowflake

Terraform provider for managing Snowflake accounts
https://registry.terraform.io/providers/Snowflake-Labs/snowflake/latest
MIT License
549 stars 420 forks source link

snowflake_grant_privileges_to_account_role quote escapes are not consistent when using import #2620

Closed jrobison-sb closed 1 month ago

jrobison-sb commented 8 months ago

Terraform CLI and Provider Versions

$ terraform version
Terraform v1.6.6
on darwin_arm64
+ provider registry.terraform.io/snowflake-labs/snowflake v0.87.2

Terraform Configuration

resource "snowflake_warehouse" "warehouse" {
  name                = "DEV3027"
  warehouse_size      = "small"
  auto_suspend        = 60
  initially_suspended = true
}

resource "snowflake_role" "one" {
  name = "DEV3027_ONE"
}

resource "snowflake_role" "two" {
  name = "DEV3027_TWO"
}

resource "snowflake_role" "three" {
  name = "DEV3027_THREE"
}

resource "snowflake_grant_privileges_to_account_role" "one" {
  privileges        = ["USAGE"]
  account_role_name = snowflake_role.one.name
  on_account_object {
    object_type = "WAREHOUSE"
    object_name = snowflake_warehouse.warehouse.name
  }
}

resource "snowflake_warehouse_grant" "two" {
  warehouse_name    = snowflake_warehouse.warehouse.name
  privilege         = "USAGE"
  roles             = [snowflake_role.two.name]
  with_grant_option = false
}

resource "snowflake_warehouse_grant" "three" {
  warehouse_name    = snowflake_warehouse.warehouse.name
  privilege         = "USAGE"
  roles             = [snowflake_role.three.name]
  with_grant_option = false
}

# resource "snowflake_grant_privileges_to_account_role" "two" {
#   # Import this one using the below command which quote-escapes
#   # the role name and the warehouse name, as per the docs seen
#   # here: https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/docs/resources/grant_privileges_to_account_role.md?plain=1#L79

#   # terraform import snowflake_grant_privileges_to_account_role.two "\"DEV3027_TWO\"|false|false|USAGE|OnAccountObject|WAREHOUSE|\"DEV3027\""
#   privileges        = ["USAGE"]
#   account_role_name = snowflake_role.two.name
#   on_account_object {
#     object_type = "WAREHOUSE"
#     object_name = snowflake_warehouse.warehouse.name
#   }
# }

# resource "snowflake_grant_privileges_to_account_role" "three" {
#   # Import this one using the below command which DOES NOT quote-escape
#   # the role name and the warehouse name, as per the docs seen
#   # here: https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/docs/technical-documentation/resource_migration.md#312-terraform-import

#   # terraform import snowflake_grant_privileges_to_account_role.three "DEV3027_THREE|false|false|USAGE|OnAccountObject|WAREHOUSE|DEV3027"
#   privileges        = ["USAGE"]
#   account_role_name = snowflake_role.three.name
#   on_account_object {
#     object_type = "WAREHOUSE"
#     object_name = snowflake_warehouse.warehouse.name
#   }
# }

Expected Behavior

I expect that when I build a snowflake_warehouse_grant resource initially and then subsequently migrate it to a snowflake_grant_privileges_to_account_role resource, the end result should be the same as if I had built a snowflake_grant_privileges_to_account_role resource right from the start.

Actual Behavior

When I build a snowflake_warehouse_grant resource initially and then subsequently migrate it to a snowflake_grant_privileges_to_account_role resource, the end result is not the same as if I had built a snowflake_grant_privileges_to_account_role resource right from the start.

Steps to Reproduce

  1. terraform apply the HCL seen above, with snowflake_grant_privileges_to_account_role.two and snowflake_grant_privileges_to_account_role.three commented out. This will build successfully.
  2. terraform state show snowflake_grant_privileges_to_account_role.one and notice that the account_role_name and the object_name do not use any weird quote-escaping, even though the ID does.
  3. Modify the HCL to comment out lines 29-41 (snowflake_warehouse_grant.two and snowflake_warehouse_grant.three) and uncomment lines 43-69 (snowflake_grant_privileges_to_account_role.two and snowflake_grant_privileges_to_account_role.three) to get ready for import migration.
  4. Drop snowflake_warehouse_grant.two and snowflake_warehouse_grant.three from state
    • terraform state rm snowflake_warehouse_grant.two
    • terraform state rm snowflake_warehouse_grant.three
  5. Import snowflake_grant_privileges_to_account_role.two and snowflake_grant_privileges_to_account_role.three to migrate to the new resource types. We'll import these two ways, using quote-escapes and also not-using quote-escapes, just to cover all bases.
    • terraform import snowflake_grant_privileges_to_account_role.two "\"DEV3027_TWO\"|false|false|USAGE|OnAccountObject|WAREHOUSE|\"DEV3027\""
    • terraform import snowflake_grant_privileges_to_account_role.three "DEV3027_THREE|false|false|USAGE|OnAccountObject|WAREHOUSE|DEV3027"
  6. Run terraform plan, and here is where I expect to see a clean plan with no diffs, but in fact there are diffs:

    # snowflake_grant_privileges_to_account_role.three must be replaced
    -/+ resource "snowflake_grant_privileges_to_account_role" "three" {
      ~ account_role_name = "\"DEV3027_THREE\"" -> "DEV3027_THREE" # forces replacement
      ~ id                = "DEV3027_THREE|false|false|USAGE|OnAccountObject|WAREHOUSE|DEV3027" -> (known after apply)
        # (5 unchanged attributes hidden)
    
      ~ on_account_object {
          ~ object_name = "\"DEV3027\"" -> "DEV3027" # forces replacement
            # (1 unchanged attribute hidden)
        }
    }
    
    # snowflake_grant_privileges_to_account_role.two must be replaced
    -/+ resource "snowflake_grant_privileges_to_account_role" "two" {
      ~ account_role_name = "\"DEV3027_TWO\"" -> "DEV3027_TWO" # forces replacement
      ~ id                = "\"DEV3027_TWO\"|false|false|USAGE|OnAccountObject|WAREHOUSE|\"DEV3027\"" -> (known after apply)
        # (5 unchanged attributes hidden)
    
      ~ on_account_object {
          ~ object_name = "\"DEV3027\"" -> "DEV3027" # forces replacement
            # (1 unchanged attribute hidden)
        }
    }
  7. Okay, that's weird, edit the HCL again and add jsonencode() on lines 50, 53, 64, 67, which are the account_role_name's and the object_name's. jsonencode() will wrap the string values with quote-escapes to make the above diffs go away.
  8. terraform plan again, and see that the diffs are gone and our migration was successful.
  9. But why do we have to use jsonencode() on lines 50, 53, 64, 67 when we didn't have to use it on lines 22 and 25?
  10. (Optional) terraform destroy to clean up after reproducing this issue.

How much impact is this issue causing?

Medium

Logs

No response

Additional Information

Thanks for all your efforts on this.

sfc-gh-jcieslak commented 8 months ago

Hey @jrobison-sb We are aware that identifiers and their representation through HCL is not ideal. Soon, we'll be working on making them consistent (reference). Right now, I think there may be inconsistency between account level objects and the rest. The account-level objects may be required to not be quoted and the rest (e.g. database-level objects) should be quoted, which seems to be correct in this case I guess. I'll take a closer look at this case tomorrow.

jrobison-sb commented 8 months ago

@sfc-gh-jcieslak thanks for your attention on this.

Note that this issue doesn't illustrate an inconsistency between account level objects versus other types of objects. This illustrates an inconsistency between a warehouse grant with snowflake_grant_privileges_to_account_role when using terraform import versus a warehouse grant with snowflake_grant_privileges_to_account_role when not-using terraform import.

The underlying SQL would be GRANT USAGE ON WAREHOUSE $WAREHOUSE_NAME TO ROLE $ROLE_NAME; in both cases.

sfc-gh-jcieslak commented 8 months ago

Hey 👋 I followed the steps to reproduce the issue and when I added quotes around identifiers and Terraform didn't produce any plan

resource "snowflake_grant_privileges_to_account_role" "two" {
  # Import this one using the below command which quote-escapes
  # the role name and the warehouse name, as per the docs seen
  # here: https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/docs/resources/grant_privileges_to_account_role.md?plain=1#L79

  # terraform import snowflake_grant_privileges_to_account_role.two "\"DEV3027_TWO\"|false|false|USAGE|OnAccountObject|WAREHOUSE|\"DEV3027\""
  privileges        = ["USAGE"]
  account_role_name = "\"${snowflake_role.two.name}\""
  on_account_object {
    object_type = "WAREHOUSE"
    object_name = "\"${snowflake_warehouse.warehouse.name}\""
  }
}

resource "snowflake_grant_privileges_to_account_role" "three" {
  # Import this one using the below command which DOES NOT quote-escape
  # the role name and the warehouse name, as per the docs seen
  # here: https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/docs/technical-documentation/resource_migration.md#312-terraform-import

  # terraform import snowflake_grant_privileges_to_account_role.three "DEV3027_THREE|false|false|USAGE|OnAccountObject|WAREHOUSE|DEV3027"
  privileges        = ["USAGE"]
  account_role_name = "\"${snowflake_role.three.name}\""
  on_account_object {
    object_type = "WAREHOUSE"
    object_name = "\"${snowflake_warehouse.warehouse.name}\""
  }
}

Does this solve your issue? At least for now, until we'll work on identifiers. Quotes in the id field shouldn't matter (at least in this case) because the parsing function we have right now can handle both.

jrobison-sb commented 8 months ago

Hi again @sfc-gh-jcieslak, thanks for reproducing this.

Adding quotes around those identifiers is basically the same thing I mentioned in the initial report with jsonencode():

  1. Okay, that's weird, edit the HCL again and add jsonencode() on lines 50, 53, 64, 67, which are the account_role_name's and the object_name's. jsonencode() will wrap the string values with quote-escapes to make the above diffs go away.

But it's inconsistent to have to use quotes in the HCL for snowflake_grant_privileges_to_account_role.two (which was imported) versus not using quotes in the HCL for snowflake_grant_privileges_to_account_role.one (which wasn't imported).

The HCL code in inconsistent:

$ grep -n object_name main.tf 
25:    object_name = snowflake_warehouse.warehouse.name
53:    object_name = jsonencode(snowflake_warehouse.warehouse.name)
67:    object_name = jsonencode(snowflake_warehouse.warehouse.name)

And the state is inconsistent too:

$ terraform state show snowflake_grant_privileges_to_account_role.one | grep object_name
        object_name = "DEV3027"

$ terraform state show snowflake_grant_privileges_to_account_role.two | grep object_name
        object_name = "\"DEV3027\""

The reason I care about this is that we have hundreds of deprecated snowflake_*_grant resources and we'll need to migrate those to snowflake_grant_privileges_to_account_role resources by way of terraform import. I worry that at some point in the future this inconsistency will be fixed in the provider, and at that point we'll run into something like this:

  # snowflake_grant_privileges_to_account_role.two must be replaced
-/+ resource "snowflake_grant_privileges_to_account_role" "two" {
      ~ id                = "\"DEV3027_TWO\"|false|false|USAGE|OnAccountObject|WAREHOUSE|\"DEV3027\"" -> (known after apply)
        # (6 unchanged attributes hidden)

      ~ on_account_object {
          ~ object_name = "\"DEV3027\"" -> "DEV3027" # forces replacement
            # (1 unchanged attribute hidden)
        }
    }

Plan: 1 to add, 0 to change, 1 to destroy.

The reason we're migrating our old snowflake_*_grant resources by way of terraform import is that we specifically do not want to recreate our grants and do all the QA checking that goes along with that. If we can complete the migration entirely within the terraform state, then there's no change in Snowflake and no risk to our deployed resources. But if our deployed resources are going to eventually be recreated anyway when the quoting inconsistency gets fixed, then perhaps it's better for us to wait and do our migrations after that happens.

Do you have any thoughts on if the eventual fix for this will require recreating all of our affected resources?

sfc-gh-jcieslak commented 8 months ago

Hey @jrobison-sb If I would choose between quoted and unquoted I would go for quoted, because you can then have names with dots, etc. (it would be an issue without quotes). Regarding config/state inconsistencies, we are aware they're there. We started utilizing state upgraders to help users with migration (reference). They automatically update the state, so the only thing you would do is upgrade to the latest version -> adjust config to match the new state representation (e.g. add/remove quotes from identifiers). But I don't guarantee it will be applicable in the case of identifiers. Also, I'm not sure what changes will be applied, because the analysis for the identifiers is not started yet (it should in a few days/weeks). tldr; I don't have data to confidently say you won't be recreating your grants after identifiers change, but we'll try to use tools like state migrators to prevent recreations.

jrobison-sb commented 7 months ago

I notice that a new feature in Terraform 1.8 seems very relevant to this use case:

Providers can now transfer the ownership of a remote object between resources of different types, for situations where there are two different resource types that represent the same remote object type. This extends the moved block behavior to support moving between two resources of different types only if the provider for the target resource type declares that it can convert from the source resource type. Refer to provider documentation for details on which pairs of resource types are supported.

It would be great if the Snowflake provider supported this, then users could do:

moved {
  from = snowflake_warehouse_grant.a
  to   = snowflake_grant_privileges_to_account_role.b
}

Then any given grant wouldn't need to be recreated and users wouldn't need to redo QA checking of their applications.

sfc-gh-jcieslak commented 7 months ago

Hmm, that's interesting. Thanks for sharing :), I'll discuss the use of it with the team, but it looks like it's a feature only enabled in the Terraform Plugin Framework and we're still on SDK v2 :/ I think the move state implementation part is described here: https://developer.hashicorp.com/terraform/plugin/framework/resources/state-move - couldn't find anything similar on the SDK v2 docs.

jrobison-sb commented 7 months ago

FWIW Terraform 1.8 has been released today and does include the "transfer the ownership of a remote object between resources of different types" feature. https://github.com/hashicorp/terraform/releases/tag/v1.8.0

PedroMartinSteenstrup commented 4 months ago

We started re-writing our resources to keep up with the new releases toward provider version 1, and stumbled upon this as well. We're using terragrunt. Ex:

terragrunt import 'snowflake_grant_privileges_to_account_role.on_integration_object["UNLOADER_MARKETING|HTG-DATALAKE-MARKETING"]' "UNLOADER_MARKETING|false|false|USAGE|OnAccountObject|INTEGRATION|HTG-DATALAKE-MARKETING"

And land on similar diff reported on this issue

  # snowflake_grant_privileges_to_account_role.on_integration_object["UNLOADER_MARKETING|HTG-DATALAKE-MARKETING"] must be replaced
-/+ resource "snowflake_grant_privileges_to_account_role" "on_integration_object" {
      ~ account_role_name = "\"UNLOADER_MARKETING\"" -> "UNLOADER_MARKETING" # forces replacement
      ~ id                = "UNLOADER_MARKETING|false|false|USAGE|OnAccountObject|INTEGRATION|HTG-DATALAKE-MARKETING" -> (known after apply)
        # (5 unchanged attributes hidden)

      ~ on_account_object {
          ~ object_name = "\"HTG-DATALAKE-MARKETING\"" -> "HTG-DATALAKE-MARKETING" # forces replacement
            # (1 unchanged attribute hidden)
        }
    }

We have thousands of stuff to migrate, and probably a few hundreds where re-applying grants would be very disruptive to our production environment.

If I would choose between quoted and unquoted I would go for quoted, because you can then have names with dots, etc. (it would be an issue without quotes). I'm not sure what changes will be applied, because the analysis for the identifiers is not started yet (it should in a few days/weeks). tldr; I don't have data to confidently say you won't be recreating your grants after identifiers change, but we'll try to use tools like state migrators to prevent recreations.

We would therefore really appreciate a definitive answer on the quoting before we start moving on the upgrade path, as in should we hardcode escape sequence in each terraform resource, or wait until changes are brought in provider to handle imports? And thanks for all the work on this provider!

sfc-gh-jcieslak commented 4 months ago

Hey @PedroMartinSteenstrup We're currently going through identifiers rework, right now we're gathering cases that are connected to the bad identifiers handling. Fixing issues connected to the identifiers is a part of the rework, so certainly we'll look into this case and try to make the identifiers' quoting consistent. Unfortunately, I cannot give you the exact date when that will happen as we're also preparing the essential resources for V1, but in the upcoming weeks we'll be working on them.

sfc-gh-jcieslak commented 3 months ago

Hey @jrobison-sb The next release should have a change that enables users to specify identifiers in both ways. The plan won't be produced because of the Diff Suppression on quotes. More on that will be posted in the identifiers rework summary.

sfc-gh-jcieslak commented 2 months ago

Hey 👋 Yesterday, we released a new version of the provider (v0.95.0) that shouldn't produce any plans related to quoting. We also prepared a document concluding identifiers rework (here) that should clear things up regarding identifiers.

sfc-gh-jcieslak commented 1 month ago

Closing due to long inactivity, the solution was provided and documented in the doc concluding identifiers rework (comment above). In case there's still something wrong with identifiers, please create another issue. Thanks :)