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

Breaking migration from deprecated to new grant types #2340

Closed
jakemingolla opened this issue Jan 11, 2024 · 7 comments
Closed

Breaking migration from deprecated to new grant types #2340

jakemingolla opened this issue Jan 11, 2024 · 7 comments
Assignees
Labels
bug Used to mark issues with provider's incorrect behavior

Comments

@jakemingolla
Copy link

Terraform CLI and Provider Versions

Terraform v1.3.9
on linux_amd64
+ provider registry.terraform.io/aidanmelen/snowsql v1.3.3
+ provider registry.terraform.io/hashicorp/aws v4.0.0
+ provider registry.terraform.io/snowflake-labs/snowflake v0.82.0

Terraform Configuration

resource "snowflake_database" "role_testing" {
  name = "ROLE_TESTING"
}

resource "snowflake_role" "role_a" {
  name = "ROLE_A"
}

resource "snowflake_role" "role_b" {
  name = "ROLE_B"
}

resource "snowflake_database_grant" "usage_on_role_testing_database" {
  depends_on = [
    snowflake_database.role_testing,
    snowflake_role.role_a,
    snowflake_role.role_b
  ]
  database_name = snowflake_database.role_testing.name
  privilege     = "USAGE"
  roles = [
    snowflake_role.role_a.name,
    snowflake_role.role_b.name
  ]
}

Expected Behavior

Overview

This series of steps will introduce an incompatibility between snowflake_database_grant and snowflake_grant_privileges_to_role for the same underlying Snowflake resource. Attempting to upgrade between the two is prone to a race condition where grants on the same resource overwrite each other. Furthermore, subsequents plans using the two terraform resources causes a permanent diff due to contention for grants on the same resource.

Baseline

Applying the above configuration creates expected grants on the resources:

Screenshot 2024-01-11 at 2 53 22 PM

Attempted upgrade to snowflake_grant_privileges_to_role

Then, change ROLE_B to be granted USAGE via a separate snowflake_grant_privileges_to_role resource:

resource "snowflake_database" "role_testing" {
  name = "ROLE_TESTING"
}

resource "snowflake_role" "role_a" {
  name = "ROLE_A"
}

resource "snowflake_role" "role_b" {
  name = "ROLE_B"
}

resource "snowflake_database_grant" "usage_on_role_testing_database" {
  depends_on = [
    snowflake_database.role_testing,
    snowflake_role.role_a
  ]
  database_name = snowflake_database.role_testing.name
  privilege     = "USAGE"
  roles = [
    snowflake_role.role_a.name,
  ]
}

resource "snowflake_grant_privileges_to_role" "usage_on_role_testing_database" {
  depends_on = [
    snowflake_database.role_testing,
    snowflake_role.role_b
  ]
  privileges = ["USAGE"]
  role_name  = snowflake_role.role_b.name
  on_account_object {
    object_type = "DATABASE"
    object_name = snowflake_database.role_testing.name
  }
}

This results in the follow plan:

Terraform will perform the following actions:

  # snowflake_database_grant.usage_on_role_testing_database will be updated in-place
  ~ resource "snowflake_database_grant" "usage_on_role_testing_database" {
        id                     = "ROLE_TESTING|USAGE|false|ROLE_A,ROLE_B|"
      ~ roles                  = [
          - "ROLE_B",
            # (1 unchanged element hidden)
        ]
        # (5 unchanged attributes hidden)
    }

  # snowflake_grant_privileges_to_role.usage_on_role_testing_database will be created
  + resource "snowflake_grant_privileges_to_role" "usage_on_role_testing_database" {
      + all_privileges    = false
      + id                = (known after apply)
      + on_account        = false
      + privileges        = [
          + "USAGE",
        ]
      + role_name         = "ROLE_B"
      + with_grant_option = false

      + on_account_object {
          + object_name = "ROLE_TESTING"
          + object_type = "DATABASE"
        }
    }

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

During the apply, the following output is produced

Warning

Note the order of operations between the start & end of the modifications and creation of role grants

snowflake_grant_privileges_to_role.usage_on_role_testing_database: Creating...
snowflake_database_grant.usage_on_role_testing_database: Modifying... [id=ROLE_TESTING|USAGE|false|ROLE_A,ROLE_B|]
snowflake_grant_privileges_to_role.usage_on_role_testing_database: Creation complete after 1s [id=ROLE_B|USAGE|false|false|false|true|false|false|false|false|DATABASE|ROLE_TESTING||false||false|]
snowflake_database_grant.usage_on_role_testing_database: Modifications complete after 1s [id=ROLE_TESTING|USAGE|false|ROLE_A|]

Apply complete! Resources: 1 added, 1 changed, 0 destroyed.

Race condition

There is a race condition when contending for the USAGE grants on the ROLE_TESTING database. Looking at the history of the grants applied in terraform (read top to bottom in chronologically descending order):

Screenshot 2024-01-11 at 2 50 31 PM

Because the GRANT completes before the REVOKE the resulting grants are not as expected:

Screenshot 2024-01-11 at 2 48 38 PM

"Fixing" it

Attempting to re-plan the configuration yields the following:

Terraform will perform the following actions:

  # snowflake_grant_privileges_to_role.usage_on_role_testing_database will be updated in-place
  ~ resource "snowflake_grant_privileges_to_role" "usage_on_role_testing_database" {
        id                = "ROLE_B|USAGE|false|false|false|true|false|false|false|false|DATABASE|ROLE_TESTING||false||false|"
      ~ privileges        = [
          + "USAGE",
        ]
        # (4 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

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

After re-applying, both roles are successfully granted USAGE to the database

Screenshot 2024-01-11 at 2 52 53 PM

However...

Subsequent plans are polluted with the following diff:

Note

The snowflake_database_grant no longer manages ROLE_B, that is handled by the non-deprecated snowflake_grant_privileges_to_role

Terraform will perform the following actions:

  # snowflake_database_grant.usage_on_role_testing_database will be updated in-place
  ~ resource "snowflake_database_grant" "usage_on_role_testing_database" {
        id                     = "ROLE_TESTING|USAGE|false|ROLE_A|"
      ~ roles                  = [
          - "ROLE_B",
            # (1 unchanged element hidden)
        ]
        # (5 unchanged attributes hidden)
    }

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

Actual Behavior

As shown above, sharing Snowflake resources between snowflake_database_grant and snowflake_grant_privileges_to_role terraform constructs results in both broken applies and subsequently confusing plans. This makes migrating from the deprecated role resource types to the new snowflake_grant_privileges_to_role almost impossible in a large terraform configuration with hundreds of grants split across a multitude of resources and roles.

It would be nice if a dedicated migration guide was provided for migrating off of the deprecated resource types onto the new snowflake_grant_privileges_to_role. Simply revoking and re-adding all of the grants in subsequent commits would result in downtime and isn't a reasonable solution for production-grade Snowflake resources we expose to end-user facing roles.

(Note that while snowflake_database_grant was the sole resource used in the example above it impacts other resource types as well).

Steps to Reproduce

See above. I'm happy to provide more details if anything is unclear.

How much impact is this issue causing?

High

Logs

No response

Additional Information

No response

@jakemingolla jakemingolla added the bug Used to mark issues with provider's incorrect behavior label Jan 11, 2024
@sfc-gh-jcieslak
Copy link
Collaborator

sfc-gh-jcieslak commented Jan 15, 2024

@jakemingolla
Hey 👋
To do a "no downtime" update to the snowflake_grant_privileges_to_role resource from the deprecated ones I can see one solution for now. It would be changing the configuration and altering the state.

As you described when removing one and creating the other some race conditions may occur. We can avoid such issues and achieve the "no downtime" update by changing the Terraform configuration and altering the state, so Terraform wouldn't have to run anything on the infra (because we'll only change the state). You can achieve it by following this guide, but to show my point I'll make step-by-step with your case.

Let's start with the config

resource "snowflake_database" "test" {
  name = "snow_test_db"
}

resource "snowflake_role" "a" {
  name = "snow_test_role_a"
}

resource "snowflake_role" "b" {
  name = "snow_test_role_b"
}

resource "snowflake_database_grant" "old" {
  depends_on = [ snowflake_database.test, snowflake_role.a, snowflake_role.b ]
  database_name = snowflake_database.test.name
  privilege = "USAGE"
  roles = [ snowflake_role.a.name, snowflake_role.b.name ]
}

# resource "snowflake_grant_privileges_to_role" "new" {
#   depends_on = [ snowflake_database.test, snowflake_role.a, snowflake_role.b ]
#   for_each = toset([ snowflake_role.a.name, snowflake_role.b.name ])
#   privileges = [ "USAGE" ]
#   role_name = each.key
#   on_account_object {
#     object_type = "DATABASE"
#     object_name = snowflake_database.test.name
#   }
# 
  1. List the state
    Run terraform state list to search the grants we're looking for (you can use grep to filter), for example, terraform state list | grep "snowflake_database_grant"

  2. Remove from the state
    Now you can go and choose which one you want to migrate next and remove it from the state with terraform state rm <resource_address>. In our example terraform state rm snowflake_database_grant.old. After running the command you can remove the resource from the configuration (removing the state will "detach" it from the resource block, so after removing it the Terraform won't try to revoke USAGE from our roles).

  3. Import into the state
    Now, we have to import the state into our new grant resource. You can uncomment the new resource that is equivalent to the previous old version. Now we have to run a proper terraform import statement to "attach" the state to the new resource, so it won't try to grant anything again. In the example, I've used for_each, so I had to modify the receiving address a bit (notice [] in the resource address). The syntax for the snowflake_grant_privileges_to_role import is here at the bottom. It will be changed, probably in the next version to be easier to write (similar to this one).
    So, to import both roles you have to run the following commands

terraform import 'snowflake_grant_privileges_to_role.new["snow_test_role_a"]' "snow_test_role_a|USAGE|false|false|false|true|false|false|false|false|DATABASE|snow_test_db||false||false||"
terraform import 'snowflake_grant_privileges_to_role.new["snow_test_role_b"]' "snow_test_role_b|USAGE|false|false|false|true|false|false|false|false|DATABASE|snow_test_db||false||false||"

After that sequence of steps, grants should be granted as they were and Terraform shouldn't produce any plan when the terraform plan is run.

Side note: As I was writing this I think you can also skip importing and let it "create" itself again. This will cause to grant things that are already there (this may be not true for granting ALL PRIVILEGES or granting on_all privileges), which is fine and easier when migrating a large number of grants.

I hope this helps :)
Feel free to ask for any additional info. Soon, I think we should publish such a migration guide in the repo, so stay tuned for that. Although It will contain probably the same information as here, we may mention our plans on breaking changes, migration guides, and methods we would like to use to improve migrations from deprecated resources / parameters.

@jakemingolla
Copy link
Author

Hi @sfc-gh-jcieslak, thanks for the quick response!

After reading through the recommended approach, I'm not sure how well this will work in practice with hundreds of grants across multiple roles in separate Snowflake accounts. Relying on human intervention to remove and import each grant is likely to be error prone and difficult to synchronize across Terraform configurations. Do you know if there are any examples of public repositories undergoing migrations for large Terraform configurations to use as a blueprint?

One potential workaround I've been thinking about would be if it would be possible to add functionality to the deprecated role grant types to ignore roles not directly controlled by the resource. This would allow new snowflake_grant_privileges_to_role to control the same underlying Snowflake objects without the snowflake_database_grant resources polluting subsequent plans.

In addition, reducing the -parallelism parameter to 1 during terraform apply could combine to eliminate the race condition documented above. If each Terraform resource fully completes modifying Snowflake grants before moving onto the next one we'd avoid the immediate revoking of grants.

Let me know if any of the above needs clarification.

@sfc-gh-jcieslak
Copy link
Collaborator

sfc-gh-jcieslak commented Jan 18, 2024

One potential workaround I've been thinking about would be if it would be possible to add functionality to the deprecated role grant types to ignore roles not directly controlled by the resource. This would allow new snowflake_grant_privileges_to_role to control the same underlying Snowflake objects without the snowflake_database_grant resources polluting subsequent plans.

I believe this can be somewhat achieved by lifecycle meta-tags. For example

resource "snowflake_database_grant" "grant" {
    # ...
    lifecycle {
        ignore_changes = all
    }
}

will ignore all changes to the parameters and won't update (afaik this doesn't prevent from creating or deleting, so before deleting the resource it would be best to remove it from the state and let the snowflake_grant_privilege_to_role fully take over).

After reading through the recommended approach, I'm not sure how well this will work in practice with hundreds of grants across multiple roles in separate Snowflake accounts. Relying on human intervention to remove and import each grant is likely to be error prone and difficult to synchronize across Terraform configurations. Do you know if there are any examples of public repositories undergoing migrations for large Terraform configurations to use as a blueprint?

I know that grant resources may be hard to migrate manually, but on the other hand, the resource could be generated with a simple scripts that would minimize human error. But before that, I highly recommend waiting a bit for the snowflake_grant_privileges_to_account_role resource. It should replace snowflake_grant_privilege_to_role (a new one will be pretty much the same when it comes to configuration, except one or two fields, but its internal state is different and that can cause issues with migrating again). After snowflake_grant_privilege_to_account_role is added I would recommend creating the script that would migrate one type of grant at a time. An example could be a Python script that would call into Snowflake to retrieve needed roles and databases, and then produce configuration based on some kind of template (e.g. jinja). Another similar solution would be to generate import statements with ID - https://developer.hashicorp.com/terraform/language/import/generating-configuration and use terraform plan to generate configurations.

We are aware that migrating a large amount of resources (mostly that's the case for grant resources) can be hard and we have plans to consider introducing a tool for automated migrations, but right now we don't have the capacity to do so.

@jakemingolla
Copy link
Author

With the lifecycle meta argument, will that also account for other competing terraform resources for grants on the same Snowflake object? For example, after applying:

resource "snowflake_database_grant" "a" {
  database_name = snowflake_database.role_testing.name
  privilege     = "USAGE"
  roles = [
    snowflake_role.role_a.name,
  ]
  lifecycle {
    ignore_changes = all
  }
}

resource "snowflake_grant_privileges_to_role" "b" {
  privileges = ["USAGE"]
  role_name  = snowflake_role.role_b.name
  on_account_object {
    object_type = "DATABASE"
    object_name = snowflake_database.role_testing.name
  }
}

Would the snowflake_database_grant still have plan to remove the grant for role B, or would that behavior be covered by the lifecycle rule? I wasn't sure if terraform considers that a 'change' since there's nothing being changed at the configuration level, only at the underlying Snowflake object level.

@sfc-gh-jcieslak
Copy link
Collaborator

Any change (by another resource or manually) in Snowflake won't be considered by the resource when ignore_chanegs = all is specified. Here's a test

resource "snowflake_database" "db" {
  name = "test-db"
}

resource "snowflake_role" "test_role" {
  name = "test_role_321"
}

resource "snowflake_database_grant" "a" {
  database_name = snowflake_database.db.name
  privilege     = "USAGE"
  roles = [
    snowflake_role.test_role.name,
  ]
#  lifecycle {
#    ignore_changes = all
#  }
}

Run the configuration above (with lifecycle commented). See that the database has usage granted:

show grants on database "test-db";

Revoke usage manually:

revoke usage on database "test-db" from role "test_role_321";

Run terraform plan. It should return a plan to update the resource by granting usage again. Now, uncomment the lifecycle block and run the terraform plan again. It will say that there're no changes because any change in Snowflake is ignored by the resource.

@sfc-gh-jcieslak sfc-gh-jcieslak self-assigned this Jan 19, 2024
@sfc-gh-jcieslak
Copy link
Collaborator

@jakemingolla Hey 👋 can we close the issue or is there anything else you would like to know?

@jakemingolla
Copy link
Author

sure, feel free to close it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to mark issues with provider's incorrect behavior
Projects
None yet
Development

No branches or pull requests

2 participants