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

Direct support for conditional single-instance resources #1306

Open
staizen-stephen opened this issue Feb 24, 2024 · 14 comments
Open

Direct support for conditional single-instance resources #1306

staizen-stephen opened this issue Feb 24, 2024 · 14 comments
Assignees
Labels
enhancement New feature or request needs-rfc This issue needs an RFC prior to being accepted or, if it's accepted, prior to being implemented. pending-decision This issue has not been accepted for implementation nor rejected. It's still open to discussion.

Comments

@staizen-stephen
Copy link

OpenTofu Version

None. Currently using Hashicorp Terraform.

Use Cases

We often use count = var.xxxx ? 1 : 0 in various flavours but with the same purpose of determining to create or not create a single resource.

While this is currently supported it means that resources require an indexed reference, and this also means that if you later change something to be conditional then you have a lot of manual state manipulation required.

I'd like to see this natively supported as it would simplify a lot of code.

Attempted Solutions

Workarounds are using count = ... expressions.

Proposal

Add an additional meta-data atttribute opentofu-options and a sub-attribute resource-enabled, for example:

resource "aws_key_pair" "my-key-pair" {
  ...
  opentofu-options {
    resource-enabled = var.create-key-pair
  }
}

When a resource is not enabled (known at plan time), then any reference to it in the graph becomes invalid and this would prevent a bunch of munging such as coalesce(aws-key_pair.my-key-pair.*.name, "") or equivalent.

References

@staizen-stephen staizen-stephen added enhancement New feature or request pending-decision This issue has not been accepted for implementation nor rejected. It's still open to discussion. labels Feb 24, 2024
@cam72cam cam72cam added the needs-rfc This issue needs an RFC prior to being accepted or, if it's accepted, prior to being implemented. label Feb 26, 2024
@cam72cam
Copy link
Contributor

I've marked this as needs-rfc as it requires some pretty careful consideration around how this could be designed. hashicorp/terraform#21953 (comment) is a good summary of the current state of discussion.

I see this as two distinct discussions that are linked. One is to add the enabled flag to resources/modules/etc... and handle the result of type.name.attr being an invalid reference if disabled. The other is syntax sugar around accessing these optional values.

On initial reading, I'm leaning toward setting a disabled resource/module/etc... to a typed unknown and making sure that references behave as expected, using expressions like type.name =! null ? type.name.attr : other_value.

I'd recommend that any discussion of "syntax sugar" like the if_enabled or try functions happen in a followup issue.

@staizen-stephen
Copy link
Author

Hi @cam72cam,

Thanks for spending time on this and being willing to reopen discussion. I agree an RFC process makes sense.

I also agree that there are potentially multiple stages in this. For my own use, I haven't ever seen a need for introspection on values that aren't defined. I can usually use the same flag that controls instantiation to control behaviour elsewhere. But maybe others see more benefit from this.

On implementation, I'd always favour outcomes that are equivalent to the resource block being absent from the input, so literally not defined. I don't know if this is practical from the code base and I can imagine it's not ideal for the graph traversal. At the same time, to avoid creating downstream dependencies in providers, I wouldn't expect them to be aware of the culling process.

Functionally, I'd imagine it would be similar to omitting a value from a for_each block, reducing a count or excluding a resource completely. If we take the for_each case, this is exactly the behaviour we'd expect: a fully specified resource is no longer present.

Coming at it from a completely different angle, perhaps there's value in revisiting how singleton resources are represented in the state such that a singleton resource and a resource with a count of one are pseudonyms for the same state? I'm just thinking aloud 😁

Thanks again and I look forward to further comments from the community.

@cam72cam
Copy link
Contributor

I think part of what was not explained well in the original issue is that the evaluator needs to know all of the types / existence in an expression in order for it to be valid.

In the example:

resource "foo" "bar" {
   enabled = local.isenabled
}

output "myout" {
   value = local.isenabled ? foo.bar.attribute : null
}

In that example, we need to know that foo.bar.attribute could exist and is of a certain type. HCL does not do any short circuiting currently, even for && and || operators. These sort of implementation details muddied the waters of the original issue. We need to remember that HCL is a declarative config language that is being shoe-horned into being something close to a programming language.

@staizen-stephen
Copy link
Author

Yes, that makes sense.

What about if that expression was disallowed but instead a new HCL function was invented that allowed a dynamic lookup but required the type to be stated?

When I have to solve similar expressions, I sometimes use a contrived coalesce expression to workaround these cases. The coalesce default value presents a known and typed value that is only present in the case that the resource is absent.

Could that approach work?

@cam72cam
Copy link
Contributor

I think it's as simple as making foo.bar a "typed unknown" or null value. We know that foo.bar could be a "foo resource" or it might not exist. We don't care if it exists or not until we actually try to access the value. Knowing that it might have a attribute should be enough for the expression traversal not to complain, though that level of knowledge may not even be nessesary.

Again, this is getting into the weeds of the OpenTofu and HCL internals and might be confusing the issue.

From a user facing perspective, local.isenabled ? foo.bar.attribute : null would work as intended. I also believe it would be fairly easy to support foo.bar != null ? foo.bar.attribute : null. If you messed up or forgot the ternary expression and tried to access foo.bar.attribute when it is not enabled it would throw an error.

Would allowing those two forms of expression suffice for your use cases?

I also think you could get fancy with it, without adding syntax sugar:

resource "foo" "bar" {
  enabled = var.isenabled
}

locals {
  fallback = {attr = "default_value")
}

output "out" {
// Four Equivalent expressions

  // 1. Use the same expression as the enabled field
  value = var.isenabled ? foo.bar.attr : "default_value"
  
  // 2. Check if the resource is null directly
  value = foo.bar != null ? foo.bar.attr : "default_value"
  
  // 3. Use coalesce with an inline default
  value = coalesce(foo.bar, {attr = "default_value"}).attr
  
  // 4. Use coalesce with a local default 
  value = coalesce(foo.bar, local.fallback).attr
}

All four of those would work under the same set of logic I'm proposing above. I'd probably only ever use 2 or 4 myself, but it is flexible to do any of those four variants.

@staizen-stephen
Copy link
Author

All of these work beautifully for me 💯

@serialseb
Copy link

serialseb commented Mar 4, 2024

Retranscripting my chat from github:

The typical example is this:

variable "generate_policy" { type = bool }
resource "aws_iam_policy" "myPolicy" {
  if = var.generate_policy
}

In replacement of the common occurence i see which makes a [0] a part of weird code

variable "generate_policy" { type = bool }
resource "aws_iam_policy" "myPolicy" {
  count = var.generate_policy == true ? 1 : 0
}

it can then flow naturally considering the same syntax as depends_on, returning a bool in those circumstances it's being tested against:

resource "aws_iam_policy_role_attachment" "aws_managed" {
  if = aws_iam_policy.myPolicy
  # which replaces the traditiona count = length(aws_iam_policy) > 0 ? 1 : 0
  policy_arn = aws_iam_policy.myPolicy
  role = "MyDefinedRole"
}

I think it's a choice between introduing null or making the resource booley in those scenarios (null not being my favourite keyword in any language)

@tom-reinders
Copy link

Would this also be considered for dynamic blocks?

resource "azurerm_linux_function_app" "function_app" {
  // ...

  site_config {
    // ...

    dynamic "application_stack" {
      for_each = var.dotnet_version != "" ? [var.dotnet_version] : []
      content {
        dotnet_version              = application_stack.value
        use_dotnet_isolated_runtime = var.dotnet_isolated
      }
    }

    // ...
  }

  // ...
}

becomes something like

resource "azurerm_linux_function_app" "function_app" {
  // ...

  site_config {
    // ...

    dynamic "application_stack" {
      if = var.dotnet_version != ""
      content {
        dotnet_version              = var.dotnet_version
        use_dotnet_isolated_runtime = var.dotnet_isolated
      }
    }

    // ...
  }

  // ...
}

@cam72cam
Copy link
Contributor

cam72cam commented Mar 4, 2024

@tom-reinders I think it's a different set of technical issues, but should follow the same syntax.

@staizen-stephen
Copy link
Author

@tom-reinders @cam72cam - up to you but I don't see a need to change the dynamic blocks, at least not as a strict need. You can already create zero entries using the for_each option with any filtering you want and it doesn't change the state versus now. So it may be nice from a style perspective (although it may equally confuse).

@ikegentz
Copy link

ikegentz commented Mar 14, 2024

Don't want to distract from the main discussion but wanted to throw this out there; as a TF user that's been tracking hashicorp/terraform#21953 for some time (and the previous issue dating back to 2021), it appears there's a lot of things to think about from a backwards-compatibility perspective. FWIW, I'd happily refactor my code to a new major version if that's what it takes to get this over the line, instead of trying to design it to be perfectly backwards-compatible with all 1.y.z versions. Thank you for all your hard work on this!

@RockyMM
Copy link

RockyMM commented Apr 15, 2024

Speaking as a member of the community:

@ikegentz
enabled could be a feature of 1.9+.y.z version, not necessarily 2.y.z.

But I just noticed this issue. Is opentofu-options a thing? I find the usage of the kebab case very un-Terraformy.

However, probably in the future something like _opentofu_options should be a thing. Is there an issue where this is discussed?

@staizen-stephen
Copy link
Author

Speaking as a member of the community:

@ikegentz enabled could be a feature of 1.9+.y.z version, not necessarily 2.y.z.

But I just noticed this issue. Is opentofu-options a thing? I find the usage of the kebab case very un-Terraformy.

However, probably in the future something like _opentofu_options should be a thing. Is there an issue where this is discussed?

Sorry, my fault. opentofu-options was my invention for the discussion. It should obviously match the style of the rest of the platform, that also includes other fields.

@janosdebugs
Copy link
Contributor

We have discussed this with the core team and we feel that an RFC detailing the exact implementation would be good to get this issue moving. @cam72cam will write an RFC to detail the implementation.

@cam72cam cam72cam self-assigned this May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-rfc This issue needs an RFC prior to being accepted or, if it's accepted, prior to being implemented. pending-decision This issue has not been accepted for implementation nor rejected. It's still open to discussion.
Projects
None yet
Development

No branches or pull requests

7 participants