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

Changing the sdm_resource type doesn't cause a replacement #49

Open
cailyoung opened this issue Aug 21, 2023 · 5 comments
Open

Changing the sdm_resource type doesn't cause a replacement #49

cailyoung opened this issue Aug 21, 2023 · 5 comments

Comments

@cailyoung
Copy link

To repro:

apply the below:

resource "sdm_resource" "teamcity_database" {
  azure_postgres {
    database = "example"
    hostname = azurerm_mysql_flexible_server.example.fqdn
    name     = "example"

    username = local.readonly_database_username
    password = random_password.readonly_user_password.result
  }
}

Then, realise your error, and update your terraform config to:

resource "sdm_resource" "teamcity_database" {
  azure_mysql {
    database = "example"
    hostname = azurerm_mysql_flexible_server.example.fqdn
    name     = "example"

    username = local.readonly_database_username
    password = random_password.readonly_user_password.result
  }
}

Expected: The provider handles the change by removing the incorrect datasource and adding the right one.

Actual:
Plan shows an update-in-place for the resource, but then apply says:

Error: cannot update Resource rs-75a3e27b64e2fa2f: invalid operation: type does not match original resource (rs-75a3e27b64e2fa2f)
@200sc
Copy link
Contributor

200sc commented Aug 21, 2023

Thanks for the report; we'll add ForceNew: true to the resource type selection, and probably add this for other polymorphic SDK types (nodes) as well. I've opened an internal ticket for tracking this, feel free to raise additional questions or data points.

@200sc
Copy link
Contributor

200sc commented Aug 21, 2023

Unfortunately, attemping a pattern where top level fields are ForceNew results in the following terraform plugin error:
All fields are ForceNew or Computed w/out Optional, Update is superfluous

In other words, Terraform believes that with this configuration, all operations are creates and none are updates, which isn't preferred. If every change caused a re-create, then historical logs, healthchecks, etc will lose consistency in the resource they are referring to.

We (or you) could open a bug with https://github.com/hashicorp/terraform-plugin-sdk to better support this architecture. It appears via tests that subfields underneath top level ForceNew fields do not, in their changes, cause terraform to believe the top level has changed, so this could be considered a bug.

In the meantime I'll raise a (much less simple) note to StrongDM's product team to consider supporting backend type changes of resources with or without terraform.

@cailyoung
Copy link
Author

Thanks for the detailed response. I don't have capacity to report the issue upstream as I'm not particularly confident I understand what you're saying about the underlying problem :)

StrongDM's product team to consider supporting backend type changes of resources

TBH, this is the only TF provider I use that holds TF resources this way. It feels like an antipattern to have a top-level resource with lower-level blocks the way you've designed sdm_resource.

It would feel more idiomatic (to me, anyway) if there was sdm_azure_mysql_datasource as a full-blown resource itself, rather than a 'generic' sdm_resource. If you could pass that feedback on, I'd appreciate it. I recognise that'd be a massive breaking change in the provider, so have no expectations that it'll actually get shipped any time soon!

@aprice-sss
Copy link

@200sc I agree with @cailyoung here - I think the issue is that this is just not a typical structure for a Terraform resource; sdm_resource (and sdm_node etc) actually represents several completely exclusive resource types, which isn't how Terraform expects a resource to behave. The "real" resources are the blocks like ssh_cert or postgres; when you define a sdm_resource you define exactly 1 child block and nothing else, so it's just acting like a sort of pseudo container object for the real resource definition inside it. This is a fundamental deviation from how Terraform resources are structured, so I would not expect them to alter the core functionality to accommodate it.

The most "correct" solution here would seem to be as cailyoung suggested, to replace these "polymorphic" types that are unsupported by Terraform and with more "normal" Terraform resources like "sdm_postgres" or "sdm_ssh_cert" or what have you.

This would have the added (and not insignificant) benefit of making the provider more intuitive to use for Terraform users who are accustomed to the way most Terraform providers behave.

@200sc
Copy link
Contributor

200sc commented Jan 26, 2024

We've opened an internal ticket for discussion on this proposed de-polymorphization; the only downside obvious to me is it would make it much more complicated to query for sets of resources covering multiple types in the datasource equivalent of these types, but that may not be a common use case.

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

No branches or pull requests

3 participants