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

Need improved conditional for creating multiple resources #26811

Closed
ajklotz opened this issue Nov 4, 2020 · 11 comments
Closed

Need improved conditional for creating multiple resources #26811

ajklotz opened this issue Nov 4, 2020 · 11 comments

Comments

@ajklotz
Copy link

ajklotz commented Nov 4, 2020

Current Terraform Version

Terraform v0.13.4
+ provider registry.terraform.io/hashicorp/azurerm v2.31.1
+ provider registry.terraform.io/hashicorp/random v3.0.0

Use-cases

As I started to write my Terraform configuration I got to a point where I needed to conditionally create resources. I found the the count argument was the way to do this. However, when I attempted to conditionally create resources, Terraform wanted to destroy and create these resources it had already created from an earlier plan. It was pointed out that the address of the resource was different because of the use of count. This means, that any resources created prior to adding the conditional would have to be moved to this new address before the apply. It seems that the count argument was really meant for creating n of resources so it naturally wants to create an array of the resource up to however many there are. But in my use-case, I'm using count with a ternary for true/false evaluation to create the resource or not.

Attempted Solutions

Proposal

There should be a way to conditionally create resources without having to use count as an argument so that the address of resources do not change, forcing the person to manually move resources to the new address.

References

#26809

@ajklotz ajklotz added enhancement new new issue not yet triaged labels Nov 4, 2020
@favoretti
Copy link

You can use for_each, but you will end up with the same problem (array), although much less error-prone (no index, named references, etc).

@pkolyvas pkolyvas removed the new new issue not yet triaged label Nov 11, 2020
@Tbohunek
Copy link

@jbardin thanks for pointing out #26980 was the same, but I believe that the proposal I raised within went into more detail. Any thoughts on that?

@jbardin
Copy link
Member

jbardin commented Nov 19, 2020

Hi @Tbohunek,

The issues are linked, so we can always refer to both for more detail. You can certainly add more information here if you would like, but it's much easier to track a particular use-case scenario that we may want to solve in one place, rather than various implementation details spread across multiple issues.

As for adding an optional deploy attribute, since that is a breaking change for all providers it's mostly likely an approach we could not take in the near term. For most applications, using count and for_each is sufficient, so what we really need to do is collect information about deficiencies in using count or for_each, and this thread is the place for that.

Thanks!

@Tbohunek
Copy link

Thanks for clarification @jbardin ! I agree it makes sense in one thread.

Why do you see deploy attribute as breaking change? If it always defaulted to false, it would not interfere with any existing deployment that's not using it. The only caveat is deploy could not be specified if count or for_each is already specified and vice-versa. Would that still be breaking?

@jbardin
Copy link
Member

jbardin commented Nov 19, 2020

deploy is not a reserved name, so any providers using deploy as an attribute name in their own resource schemas would be broken by this change.

@Tbohunek
Copy link

Point taken. Any other name would have that issue.

Can you see anyhow whether certain providers use it or not? Is there any chance you would only add this feature to selected providers such as azurerm, aws and google? I'm not a fan of partial solutions, but technically those three are probably the most important and beneficial, and maintained by you.

@jbardin
Copy link
Member

jbardin commented Nov 19, 2020

We don't have a comprehensive list of all providers (there are customers with providers which are not publicly available at all), and changing behavior based on the provider type is not an option. The point I was trying to make earlier is that we don't need to discuss the implementation details so much as the use case itself. Until we have a clear case for implementing another way to make resources conditional, the details of how that would work don't matter. Very often as we drill down into the underlying need, a more appropriate solution presents itself.

@Tbohunek
Copy link

@jbardin The use-case is clear. count is not well-suited for yes/no deployment of only (and explicitly) one instance of a resource.

@Tbohunek
Copy link

Hi @jbardin, I'd like to generate some momentum so here's a use-case.

I deploy Landing Zone for 200 Products from my landing-zone module, which actually consists of different modules such as network, key-vault, rbac, des and so on.
In some cases, deployment of a feature requires other inputs. e.g. to deploy disk encryption set des, I need first the Security team to deploy the HSM key into a vault that I deploy via key-vault of the landing-zone in the first place.
Yes, I can deploy the des module extra outside the landing-zone module, but this means to add 200 instances of des module call with many variables passed into. This is compared to a deploy_des variable flag.

However, when using count, the id like .[0] is added to the resource path in Terraform graph, leading to potential issues in case there sometimes appeared a .[1].
Moreover, turning a "hard-deploy" module into "counted" module means you need to at least terraform state mv all resources so that they contain the .[0] even though you never intent to deploy more than 1 instance of this resource. This may be costly and risky operation, and it may discourage teams from even trying to make their code more flexible.

What do you think of this use-case?

@jbardin
Copy link
Member

jbardin commented Feb 26, 2021

Coalescing with #21953 right now since that issue has more visibility.

@jbardin jbardin closed this as completed Feb 26, 2021
@ghost
Copy link

ghost commented Mar 29, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants