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

null value is not treated per docs when passed to modules #24142

Closed
oxplot opened this issue Feb 18, 2020 · 20 comments · Fixed by #29832
Closed

null value is not treated per docs when passed to modules #24142

oxplot opened this issue Feb 18, 2020 · 20 comments · Fixed by #29832
Labels
breaking-change bug config v0.12 Issues (primarily bugs) reported against v0.12 releases
Milestone

Comments

@oxplot
Copy link

oxplot commented Feb 18, 2020

Terraform Version

Terraform v0.12.20

Terraform Configuration Files

main.tf:

module mod1 {
  source         = "./mod1"
  optional_input = null
}

mod1/main.tf:

variable optional_input {
  default = 5
}

output foo {
  value = var.optional_input / 5
}

Debug Output

https://pastebin.com/raw/NuxLJi2U

Expected Behavior

As per docs:

null: a value that represents absence or omission. If you set an argument of a resource or module to null, Terraform behaves as though you had completely omitted it — it will use the argument's default value if it has one

Actual Behavior

An error message is shown saying that the said argument must not be null:

  on mod1/main.tf line 6, in output "foo":
   6:   value = var.optional_input / 5
    |----------------
    | var.optional_input is null

Error during operation: argument must not be null.

Steps to Reproduce

  1. terraform init
  2. terraform apply

References

@buddhamangler-cbre
Copy link

This forces module consumers to have knowledge of the default value for the module and specify the same default in their variable definitions. This really should be fixed.

@sean-abbott
Copy link

Can reproduce in 0.12.25, see comment in #21702

@apparentlymart apparentlymart added the v0.12 Issues (primarily bugs) reported against v0.12 releases label Jun 4, 2020
@apparentlymart
Copy link
Contributor

Hi @oxplot,

I agree that the behavior is inconsistent here, and that's confusing. My sense of the ideal behavior is that if a variable value is set to null then the default value should be taken, and that setting a required variable (one without a default) should be an error.

The design challenge in this area is that some modules use null as a valid value, and so changes to the behavior must consider that. In the provider schema system that is represented by explicitly marking a variable as "optional" without providing a default. That approach doesn't directly work for input variables because the presence of default is overloaded to imply "optional", rather than it being a separate argument.

We could potentially address this by allowing default = null as a special way to declare a variable as optional without a default value. That is still inconsistent with the usual meaning of null as being equivalent to omitting the argument -- omitting default would have a different meaning than setting it to null -- but a special case unique to the variable default argument is better than one that applies to all input variables, and default doesn't accept conditional expressions anyway so there isn't really any other useful meaning of default = null.

Unfortunately, given that the current behavior was established in v0.12.0 we cannot just change it now because there will be many modules depending on the current behavior. I'm going to mark this as a breaking change to acknowledge that while it's a valid issue to be addressed we are blocked by finding a responsible way to introduce it that will not cause confusing misbehavior for existing modules. I don't yet know what that approach will be.

In the meantime, I think it'd be best to update the documentation for module blocks to be explicit about this exception to the usual interpretation of null, so that the behavior is at least well-defined even if also inconsistent.

Thanks for reporting this!

@buddhamangler-cbre
Copy link

@apparentlymart I think it's important to consider the converse...a default that is non null that a consumer of the module may or may not set, yet the configuration has to allow for it. This gets even more complex in chained module configurations.

@nshenry03
Copy link

Agreed that this is unexpected behavior, especially considering the documentation for null in the Expressions documentation.

null: a value that represents absence or omission. If you set an argument of a resource or module to null, Terraform behaves as though you had completely omitted it — it will use the argument's default value if it has one, or raise an error if the argument is mandatory.

@007
Copy link

007 commented Aug 24, 2021

The behavior is inconsistent, but the documentation is not inconsistent, it is wrong. Explicitly so.

Fixing the behavior of null to match the docs as a breaking change is one answer (which really should have happened for 0.12.x or at least 0.13), but updating the docs would be a much easier solution. Remove or a module and document it independently that null passed to a module really is null and not "absence or omission".

saliceti pushed a commit to DFE-Digital/cf-monitoring that referenced this issue Sep 2, 2021
Related to terraform issue: hashicorp/terraform#24142

We can't pass a null value to a module as it is passed explicitly
instead of using the default set inside of the module.

As a workaround we set empty string as default and set local variables
conditionally.
saliceti pushed a commit to DFE-Digital/cf-monitoring that referenced this issue Sep 2, 2021
Related to terraform issue: hashicorp/terraform#24142

We can't pass a null value to a module as it is passed explicitly
instead of using the default set inside of the module.

As a workaround we set empty string as default and set local variables
conditionally.
@rabidscorpio
Copy link

rabidscorpio commented Sep 7, 2021

@apparentlymart @jbardin So this issue is now a year and a half old and the documentation still hasn't been updated nor has there been any change to make null work the way the documentation says it's supposed to work.

A null should mean a value that represents absence or omission for everything, not "an absense" for providers but a valid value for modules. It's not just confusing, it doesn't make sense that modules behave completely different from providers.

What's the work around for this? Am I supposed to know default values of variables in a module whenever I call that module? How many modules actually use "null" as a valid value? And why would they do that? Couldn't you just use an empty string?

There have been a lot of tickets that have been closed as duplicates of this issue and yet nothing has been done about this ticket.

@rabidscorpio
Copy link

rabidscorpio commented Sep 8, 2021

Is there any documentation anywhere that says that a null passed in to a module is treated as a literal value as opposed to being ignored? Are modules using null as a passed in value despite documentation that says something completely different?

If modules are ignoring the documentation then this isn't a breaking change, this is a major bug and module maintainers are responsible for using something that completely contradicts documentation.

After doing some digging, the documentation for null was added to the website with this commit d2abdc2 almost 3 years ago so it seems like it was always the intention to have null work the same way for resources and modules. I'm not sure why Hashicorp keeps saying it's a breaking change when it was always intended to work that way.

@jvkubjg
Copy link

jvkubjg commented Sep 20, 2021

Still not updates?

@vynaloze
Copy link

vynaloze commented Oct 5, 2021

@rabidscorpio

I'm not sure why Hashicorp keeps saying it's a breaking change when it was always intended to work that way.

As per Terraform v1.0 Compatibility Promises:

If the actual behavior of a feature differs from what we explicitly documented as the feature's behavior, we will usually treat that as a bug and change the feature to match the documentation, although we will avoid making such changes if they seem likely to cause broad compatibility problems.

And this is precisely a change which will change the behaviour of countless modules. Modules often contain optional variables with reasonable defaults and a valid use case is to opt out of those defaults in favour of resource's defaults by using null as a normal value.

If modules are ignoring the documentation then this isn't a breaking change, this is a major bug and module maintainers are responsible for using something that completely contradicts documentation.

If you would be a maintainer of any module, you'd think otherwise. But hey, the best is to ignore the reality and make it a problem of other people.

@apparentlymart
Is there a reason why the documentation has not been updated for over a year?

@rabidscorpio
Copy link

rabidscorpio commented Oct 5, 2021

@vynaloze null didn't exist before 0.12 and the documentation was written almost 3 years ago when 0.12 was being introduced so any module that uses null as an actual value is doing so knowing there's no documentation to support this usage. If you can point me to any Hashicorp documentation that says that null can be used as a value for modules, I will admit that I'm wrong about this.

The reality is, though, that you have absolutely no idea how many modules use null as an actual value so please refrain from the melodramatic "countless". Yes, in this case, it is other people's problem for doing something that completely contradicts the official documentation. My guess is that very few are because doing so runs the risk that Hashicorp will actually fix the bug that this is.

This is a bug and should have always been treated as a bug.

@vynaloze
Copy link

vynaloze commented Oct 6, 2021

While I have absolutely no idea how many modules are there, neither you do. You suggest to change well-established (as you said: 3 years) behaviour in favour of aligning with documentation (not even a formal language specification).

I value maintaining backwards compatibility, you believe that aligning with documentation is more important. I think we won't agree with each other, yet I think we both agree that Hashicorp should finally make a decision and either fix the documentation or code logic. This inconsistency is good for no one.

@buddhamangler-cbre
Copy link

buddhamangler-cbre commented Oct 6, 2021 via email

@buddhamangler-cbre
Copy link

i want to reiterate my previous point that is related here. If module A defines a variable X with a default of Y and I am designing module B to wrap module A. I want to supply a variable for X in module B that is passed to module A since it is something I wish the consumer the ability to modify, but this means that this issue kicks in. If the consumer of module B supplies a value of null for the variable I get an error just as stated in this issue. If they omit the value, it can work, but only if I replicate the exact same default from module A in module B's variables. This sucks

@rabidscorpio
Copy link

@vynaloze How can this possibly be "well-established" behavior? It's a bug that's not even documented anywhere. If module authors are using this, they did it at their own risk. Why would anyone build something depending on behavior that directly contradicts documentation?

@joeslazaro
Copy link

How about a compromise:

  1. Keep the current default behavior to avoid breaking changes
  2. Add a conditional path in the code to fix the behavior of null to match original intent described the documentation, which is much more convenient behavior.
  3. Define a new environment variable as an "escape hatch" which alters the default behavior of Terraform for issues like this. For example, TF_ALT_BEHAVIOR="nullModule,somethingElse,anotherThing"
  4. Update the documentation to clarify the actual behavior and the intended behavior which can be enabled through the above method.

Wouldn't this satisfy everyone?

@nshenry03
Copy link

Anyone can update the documentation. If you've got the time and want to see this improved, go for it! Simply create a pull request to modify this file: https://github.com/hashicorp/terraform/blob/main/website/docs/language/expressions/types.html.md

There are lots of "This sucks", "Hashicorp should do this", "Hashicorp should have done that" comments. Anyone complaining actually paying anything for Terraform? The truth is, this is fairly big change that has the potential to break existing things (which is never good). Terraform (along w/ the rest of the Hashicorp stack) is a great product that greatly simplifies modern DevSecOps for free (for most anyway)... I agree, I'd love to see this fixed as much as anyone, but truth is, it's a fairly simple workaround if your writing a new module....

main.tf:

module "foo" {
  source = "./mod1"
}

module "bar" {
  source = "./mod1"
  optional_input = 10
}

output "foo" {
  value = module.foo.foo
}

output "bar" {
  value = module.bar.foo
}

mod1/main.tf:

variable "optional_input" {
  description = "An optional number... If not specified, the default is 5"
  type        = number
  default     = null
}

locals {
  # If var.optional_input isn't null, use var.optional_input; otherwise, use 5
  # as the default
  optional_input = var.optional_input != null ? var.optional_input : 5
}

output "foo" {
  value = local.optional_input / 5
}

Works as needed

hnicholas@hnicholas-a01:/tmp/24142$ tfenv use latest
Switching default version to v1.0.8
Switching completed
hnicholas@hnicholas-a01:/tmp/24142$ terraform init
Initializing modules...
- bar in mod1
- foo in mod1

Initializing the backend...

Initializing provider plugins...

Terraform has been successfully initialized!

You may now begin working with Terraform. Try running "terraform plan" to see
any changes that are required for your infrastructure. All Terraform commands
should now work.

If you ever set or change modules or backend configuration for Terraform,
rerun this command to reinitialize your working directory. If you forget, other
commands will detect it and remind you to do so if necessary.
hnicholas@hnicholas-a01:/tmp/24142$ terraform apply -auto-approve

Changes to Outputs:
  + bar = 2
  + foo = 1

You can apply this plan to save these new output values to the Terraform state, without changing any real infrastructure.

Apply complete! Resources: 0 added, 0 changed, 0 destroyed.

Outputs:

bar = 2
foo = 1

@rabidscorpio
Copy link

@nshenry03 Unfortunately, it's not quite that simple a work around. Your solution just omits a parameter if you don't want to set it but that doesn't work if you have to set that parameter and you want to be able to toggle it. This can happen in a number of cases such as having a conditional when passing a module parameter or if you're trying to set a parameter based on a map or a tfvars file. Let's say you have an EKS module and you want to use the default node type for all of your development and staging environments but you want to override that for production.

If you're using Terraform workspaces and you're calling a module where you want to override the module default in some workspaces but you want to keep the module default for all other workspaces, the way you'd expect to do that would be to pass in a null for the parameter when you want to use the module default. Since null is treated as an actual value with modules, it overrides the default and the default becomes useless.

Another example would be calling a module with count or for_each where you want to set parameters for some of the indexes but not for others. You'd expect to be able to use null for parameters where you want to use the module default and then set an actual value when you want to override that default. You can't do this either.

@tjstansell
Copy link

Given the complexity of changing how null is handled and dealing with compatibility errors, perhaps a new default keyword could be allowed as the value of a module variable. This would specifically be treated as though that variable wasn't even specified and could be used in nested module chains or in loops where you sometimes want the default and sometimes want to override a value...

@github-actions
Copy link

github-actions bot commented Dec 2, 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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking-change bug config v0.12 Issues (primarily bugs) reported against v0.12 releases
Projects
None yet
Development

Successfully merging a pull request may close this issue.