aidanmelen / terraform-provider-snowsql

Terraform SnowSQL provider
https://registry.terraform.io/providers/aidanmelen/snowsql/latest
Other
21 stars 10 forks source link

Update resource lifecycle #60

Closed christophkreutzer closed 1 year ago

christophkreutzer commented 1 year ago

Hi,

I wanted to ask if it was a deliberate decision to only allow create/delete, but no update lifecycle? We have a use case where it would be "more beautiful" if it could be seen as "in-place update" instead of a force replacement.

Would you be open for a PR on that topic?

aidanmelen commented 1 year ago

Have you seen this? There is update support in the latest version.

aidanmelen commented 1 year ago

A PR is welcomed if you find something missing or broken! Thanks for your interest

christophkreutzer commented 1 year ago

It seems I missed your reply, sorry!

Actually, I haven't seen the update for delete lifecycle, but it doesn't solve our issue. May I describe what we are doing: We are working on a kind of registry, where we insert parts of the config into a table for easy retrieval (by ourselfs for easy querying and automated processes). We already implemented the create statement in a way that would allow updates just fine (MERGE). However, if any value changes (and that does happen relatively often), it is shown as a force-replace instead of a in-place update.

Example:

resource "snowsql_exec" "eap_cmdb_inventory" {
  name = "eap_cmdb_inventory"

  create {
    number_of_statements = 1
    statements           = <<-EOT
    MERGE INTO registry t
    USING (
      SELECT
        '${upper(var.app_name)}' AS application_name,
        '${var.owner}' AS owner,
        '${var.accounting_number}' AS accounting_number,
        '${var.version}' AS version,
        parse_json('${jsonencode(var.stages)}') AS stages
    ) s ON t.application_name = s.application_name
    WHEN MATCHED THEN UPDATE SET
      t.owner = s.owner,
      t.accounting_number = s.accounting_number,
      t.version = s.version,
      t.stages = s.stages
    WHEN NOT MATCHED THEN INSERT (application_name, owner, accounting_number, version, stages)
      VALUES (s.application_name, s.owner, s.accounting_number, s.version, s.stages)
    ;
    EOT
  }

  delete {
    number_of_statements = 1
    statements           = <<-EOT
    DELETE FROM registry WHERE APPLICATION_NAME='${upper(var.app_name)}';
    EOT
  }
}

I understand you wanted to make sure that the create statement doesn't change during the lifecycle - I do understand that, that could yield some pretty weird behavior if not noticed.

Would it be possible to add an additional "update" lifecycle command, where we could duplicate the MERGE statement, so that it is clear that will only be called if the resource already exists in the state?

Or do you have any other recommendation?

aidanmelen commented 1 year ago

I like your suggestion for adding an update.statements block. That would be consistent with the terraform-shell-provider, which this code was based on.

Would this be something you would be interested in contributing to the project?

christophkreutzer commented 1 year ago

My Go is a bit rusty, but I'll give it a try!

christophkreutzer commented 1 year ago

Hi @aidanmelen

I've discussed this with a colleague. In our opinion, there are two possibilities:

I'm leaning towards the second option, so code must not be duplicated. Or maybe implementing both, but one could only set one of update.statements or update.update_create_in_place = true. If the update block is missing, or removed afterwards, the provider should fall back to the current behaviour (force-replace).

What do you think?

aidanmelen commented 1 year ago

Thank you for providing multiple solutions. To ensure the most comprehensive approach, only the addition or change to the update.statements should result in the in-place update. If the update block is missing or removed, the provider would fall back to its current behavior of force-replacing.

It's important to understand Terraform lifecycles and Snowflake's object alteration behavior when implementing updates. When the update block is provided during the update lifecycle, the update.statement should be executed. If update block is not supplied, a replacement is expected when the create.statements change.

For instance, consider recreating the snowflake_user resource using the snowsql_exec resource. The following raw Snowflake statements would be used:

-- create
CREATE USER IF NOT EXISTS user1 PASSWORD='abc123' DEFAULT_ROLE = myrole DEFAULT_SECONDARY_ROLES = ('ALL') MUST_CHANGE_PASSWORD = TRUE;

-- read
SHOW USERS LIKE '%user1%';

-- update
ALTER USER IF EXISTS user1 SET EMAIL = 'user1@email.com';

-- delete
DROP USER IF EXISTS user1;

The equivalent Terraform code would look like this:

resource "snowsql_exec" "dcl" {
  name = local.name

  create {
    statements = "CREATE USER IF NOT EXISTS user1 PASSWORD='abc123' DEFAULT_ROLE = myrole DEFAULT_SECONDARY_ROLES = ('ALL') MUST_CHANGE_PASSWORD = TRUE;"
  }

  delete {
    statements = "DROP USER IF EXISTS user1;"
  }
}

On the first apply, the create.statements will be executed and the user will be created. On the second apply, the create block has not changed, therefore no replacement will occur. However, on the third apply, uncommenting the update block should modify the user's email using an in-place update:

resource "snowsql_exec" "dcl" {
  name = local.name

  create {
    statements = "CREATE USER IF NOT EXISTS user1 PASSWORD='abc123' DEFAULT_ROLE = myrole DEFAULT_SECONDARY_ROLES = ('ALL') MUST_CHANGE_PASSWORD = TRUE;"
  }

  update {
    statements = "ALTER USER IF EXISTS user1 SET EMAIL = 'user1@email.com';"
  }

  delete {
    statements = "DROP USER IF EXISTS user1;"
  }
}

What's more, all future changes to the update block should result in the in-place update.

Note that implementing updates is a delicate process as in most cases the operator cannot simply copy and paste the create.statements into update.statements and make changes. Instead, the user must consider Terraform lifecycles and Snowflake's object alteration behavior.

aidanmelen commented 1 year ago

We can keep our Terraform code DRY and maintain control over the Snowflake statements by using the built-in lifecycle meta-argument. Instead of having two similar but different create and update statements, we can reference a local variable in both the create and update blocks. Then, we can ignore changes to the create.statements so that they are only executed during the first apply, unless you remove the lifecycle.ignore and want a replacement again.

locals {
  merge_statement = <<-EOT
    MERGE INTO registry t
    USING (
      SELECT
        '${upper(var.app_name)}' AS application_name,
        '${var.owner}' AS owner,
        '${var.accounting_number}' AS accounting_number,
        '${var.version}' AS version,
        parse_json('${jsonencode(var.stages)}') AS stages
    ) s ON t.application_name = s.application_name
    WHEN MATCHED THEN UPDATE SET
      t.owner = s.owner,
      t.accounting_number = s.accounting_number,
      t.version = s.version,
      t.stages = s.stages
    WHEN NOT MATCHED THEN INSERT (application_name, owner, accounting_number, version, stages)
      VALUES (s.application_name, s.owner, s.accounting_number, s.version, s.stages)
    ;
    EOT
}

resource "snowsql_exec" "eap_cmdb_inventory" {
  name = "eap_cmdb_inventory"

  create {
    statements = local.merge_statement
  }

  update {
    statements = local.merge_statement
  }

  delete {
    statements  = "DELETE FROM registry WHERE APPLICATION_NAME='${upper(var.app_name)}';"
  }

  lifecycle {
    ignore_changes = [
      create,
    ]
  }
}

The update statements are still executed whenever the merge_statement changes, ensuring that the Snowflake statements are updated without being replaced.

I would rather use the lifecycle.ignore_changes for your unique update usecase because it is a well documented behavior in Terraform and we don't have to compensate with bespoke Golang code. Thoughts?

christophkreutzer commented 1 year ago

I would rather use the lifecycle.ignore_changes for your unique update usecase because it is a well documented behavior in Terraform and we don't have to compensate with bespoke Golang code. Thoughts?

I haven't thought of that option, that's much more flexible and also even straight-forward to implement. I'll update the PR asap.

aidanmelen commented 1 year ago

Thanks @christophkreutzer, nice work on the new release (v1.1.0)!