hashicorp / terraform

Terraform enables you to safely and predictably create, change, and improve infrastructure. It is a source-available tool that codifies APIs into declarative configuration files that can be shared amongst team members, treated as code, edited, reviewed, and versioned.
https://www.terraform.io/
Other
42.57k stars 9.54k forks source link

Move mutually exclusive resources to the same destination #31170

Open Unichron opened 2 years ago

Unichron commented 2 years ago

Use-cases

It might be a niche use-case, but I came across it nevertheless. I have a module in which I create one of two versions of the same resource depending on an input variable:

variable "with_labels" {
  type = bool
}

resource "kubernetes_config_map" "with_labels" {
  count = var.with_labels ? 1 : 0
  metadata {
    name      = "test"
    namespace = "default"
    labels = {
      foo = "bar"
    }
  }
}

resource "kubernetes_config_map" "without_labels" {
  count = var.with_labels ? 0 : 1
  metadata {
    name      = "test"
    namespace = "default"
  }

  lifecycle {
    ignore_changes = [metadata[0].labels]
  }
}

I now would like to refactor this code to "merge" these two resources into one. The end result would look like this:

resource "kubernetes_config_map" "this" {
  metadata {
    name      = "test"
    namespace = "default"
  }

  lifecycle {
    ignore_changes = [metadata[0].labels]
  }
}

Attempted Solutions

I tried to use moved blocks:

moved {
  from = kubernetes_config_map.with_labels[0]
  to   = kubernetes_config_map.this
}

moved {
  from = kubernetes_config_map.without_labels[0]
  to   = kubernetes_config_map.this
}

but it throws an error: Each resource instance can have moved from only one source instance.

Proposal

Not sure if there is any good solution to this, if only static expressions can be used inside the moved blocks. Maybe terraform could check the existence of the from resource and ignore the block if it doesn't exist, and on top of that this could be controlled via a flag in the block to prevent accidents.

Aabhusan commented 2 years ago

tried this as well didn't worked :(

moved { from = var.with_labels ? kubernetes_config_map.with_labels : kubernetes_config_map.without_labels to = kubernetes_namespace.this }

Error: Invalid expression │ │ on main.tf line 15, in moved: │ 15: from = var.manage_labels ? kubernetes_namespace.with_labels : kubernetes_namespace.without_labels │ │ A single static variable reference is required: only attribute access and indexing with constant keys. No calculations, function calls, template expressions, etc are allowed here.

apparentlymart commented 2 years ago

Hi @Unichron! Thanks for sharing this use-case.

The intent of this rule about only having one source is to avoid having to resolve a potential ambiguous situation where both of the specified source addresses have objects bound to them, and so Terraform would have no way to know which one to keep and which one to discard.

There are two key design constraints here which we need to keep in mind when thinking about solutions:

Given that, I don't have a ready idea about how we could change Terraform to meet the need you have described.

If the caller of this module is in the same "module package" (e.g. the same Git repository) then you could potentially declare moved in the parent module instead, since that module presumably "knows" whether it is setting with_labels or not. But if course that is not ideal because it requires the caller to know about the internals of the child module.

Although it's tangential to the problem statement you shared here, I wonder if it would be better in your case to look for ways to use only a single kubernetes_config_map resource and instead set the labels argument conditionally. Writing a conditional expression directly in that argument should achieve the same effect in principle, with the "true" result set to the map of labels and the "false" result set to null to represent leaving the argument unset in that case.

apparentlymart commented 2 years ago

On re-read I see that the other resource also has a differing ignore_changes, and so indeed it does seem required to have two separate resources in this case, as long as it isn't acceptable to ignoring changes to the labels in the case where labels are specified

Also, I misunderstood that you were trying to migrate from having to separate resources to only having one, rather than the other way around, and so indeed you are already trying to move to the structure I proposed, having previously solved it with two resources.

Sorry for not reading more closely the first time!

apparentlymart commented 2 years ago

One hypothetical design that could work here is to allow writing a single moved block where from uses the [ ... ] sequence syntax to specify multiple source addresses in order of preference:

moved {
  # INVALID: One possible future design to address this use-case
  from = [
    kubernetes_config_map.with_labels[0],
    kubernetes_config_map.without_labels[0],
  ]
  to = kubernetes_config_map.this
}

This structure would in principle allow us to resolve the ambiguity by having addresses that appear earlier in the list take priority over ones later in the list in any situation where multiple are bound to objects in the prior state. In the case where both of those addresses were bound, Terraform would move kubernetes_config_map.with_labels[0] to kubernetes_config_map.this and then leave kubernetes_config_map.without_labels[0] at its previous address, which would then cause Terraform to plan to destroy it (since it's no longer in the configuration).

While this could technically work, I'm not yet convinced that it would be intuitive to a future reader what exactly the above example means, and it may also be hard for Terraform to give good feedback to the user about why it's planning to destroy kubernetes_config_map.without_labels[0] even though it appears in a moved block.

I think some more design iteration is warranted here then, but did want to share the above as a starting point.

(I realize that in your case it's impossible for both to exist in the prior state due to the count expressions being mutually exclusive, but a design to meet this use-case must also contend with the more annoying case where multiple are present, in order to avoid breaking our rule that the prior state alone cannot make moved statement resolution fail.)

garypendlebury commented 2 years ago

@Unichron This kind of workaround worked for me:

moved {
  from = kubernetes_config_map.with_labels[0]
  to   = kubernetes_config_map.without_labels[0]
}

moved {
  from = kubernetes_config_map.without_labels[0]
  to   = kubernetes_config_map.this
}
apparentlymart commented 2 years ago

Thanks for sharing that solution, @garypendlebury.

That solution can work as long as there is not currently either a kubernetes_config_map.with_labels[0] or a kubernetes_config_map.without_labels[0] defined in your configuration, because it exploits the "chaining rule" in our move execution algorithm:

Since I was talking above about the ambiguity of two objects trying to move to the same location, it's perhaps worth showing how Terraform would treat that with the above example too:

In effect then, this is functionally equivalent to the hypothetical I shared of presenting from as a list of possible source addresses. The only significant difference is the way Terraform resolves the ambiguity: for chaining it just falls out as a logical consequence of the chaining rule, whereas for a list of source addresses there would need to be some extra logic to try moving from each source address in turn until either one succeeds or we run out of source addresses to try. Either way though the effect is the same: one object will be moved to the new location, and the other will be proposed for destruction.

novas0x2a commented 2 years ago

Chiming in with a similar use-case! In my case, I'm taking some roots that were (supposed to be) functionally identical copy/pastes that I'm resolving into one. A quick illustration is:

A:

resource "null_module" "thing_prod" {}

B:

resource "null_module" "thing_dev" {}

And I want to resolve these into:

# THIS MOVED PAIR IS ILLEGAL
moved {
    from = null_module.thing_prod
    to   = null_module.thing
}
moved {
    from = null_module.thing_dev
    to   = null_module.thing
}
resource "null_module" "thing" {}

Obviously, this is currently illegal for the same reason as the top of the thread (reuse of the same to). In my case, the chained resources mentioned in https://github.com/hashicorp/terraform/issues/31170#issuecomment-1173741286 works for me (thanks a ton for that) but I just wanted to call out a similar but different use-case.

pathob commented 1 year ago

@Unichron This kind of workaround worked for me:

moved {
  from = kubernetes_config_map.with_labels[0]
  to   = kubernetes_config_map.without_labels[0]
}

moved {
  from = kubernetes_config_map.without_labels[0]
  to   = kubernetes_config_map.this
}

Clever! ;-) Thanks for sharing.