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

defaults() leaves child objects null #28344

Closed
ghost opened this issue Apr 13, 2021 · 15 comments
Closed

defaults() leaves child objects null #28344

ghost opened this issue Apr 13, 2021 · 15 comments
Labels
enhancement functions new new issue not yet triaged v0.15 Issues (primarily bugs) reported against v0.15 releases

Comments

@ghost
Copy link

ghost commented Apr 13, 2021

Terraform Version

Terraform v0.15.0-rc2

Terraform Configuration Files

terraform {
  experiments      = [module_variable_optional_attrs]
}

variable "my_var" {
  type = object({
    some_config1 = optional(object({
      my_something = optional(bool)
    }))

    deep_config = optional(object({
      deep_1 = optional(object({
        something_1 = string
        something_2 = number
      }))

      deep_2 = optional(object({
        something_1 = string
        something_2 = number
      }))
    }))
  })
}

locals {
  my_var = defaults(var.my_var, {
    some_config1 = {
      my_something = true
    }

    deep_config = {
      deep_1 = {
        something_1 = "asdf"
        something_2 = 1
      }

      deep_2 = {
        something_1 = "fdsa"
        something_2 = 5
      }
    }
  })
}

output "my_var" {
  value = local.my_var
}

Debug Output

Not necessary

Crash Output

Not necessary

Expected Behavior

If I provide var.my_var with {} and have defaults for all child objects, I expect defaults() to expand all child objects to include those defaults.

Actual Behavior

Child objects remain null, see output:

var.my_var
  Enter a value: {}

Changes to Outputs:
  + my_var = {
      + deep_config  = null
      + some_config1 = null
    }

I understand this is by design: Terraform will recursively visit all of the attributes or elements of the nested value and repeat the same defaults-merging logic one level deeper.; this is a poor design. It means that no sub-objects can be optional at the root of the type = object({}); so therefore should change. It also leaves a bad impression when someone in the future Google's this only to learn that objects cannot be optional; which means the only way to fill in the defaults would be to remove optional(object({})) on the child objects, then requiring <object> = {} just for terraform to expand the "default" value into the child object. This requirement kinda makes this functionality useless; I may as well just declare the whole value then instead of a whole bunch of my_obj = {}, and so on.

We have some config that goes pretty deep, for example:

    zookeeper = object({
      replica_count = optional(number)
      resources = optional(object({
        requests = optional(object({
          cpu    = optional(string)
          memory = optional(string)
        }))
        limits = optional(object({
          cpu    = optional(string)
          memory = optional(string)
        }))
      }))
      volumes = optional(object({
        data_size    = optional(string)
        dataLog_size = optional(string)
      }))
    })

The only way to make ☝️ work is to make only the primitive variables optional and then "empty" nest the object declarations. The other option would be to "flatten" the objects, but that doesnt look nearly as nice.

Steps to Reproduce

  1. Copy above included tf
  2. terraform plan

Additional Context

NA

References

@ghost ghost added bug new new issue not yet triaged labels Apr 13, 2021
@ghost
Copy link
Author

ghost commented Apr 13, 2021

@alisdair tagging you in this because you were on #27385 ; hopefully that is correct ;)

@alisdair alisdair added enhancement experiment/module_variable_optional_attrs functions v0.15 Issues (primarily bugs) reported against v0.15 releases and removed bug labels Apr 13, 2021
@alisdair
Copy link
Contributor

Thanks for your feedback on this! I've relabelled this as an enhancement, as the defaults function appears to be working as designed. As you note, this issue is a critique of the design, which will require a different process than fixing a bug.

I didn't contribute to the design of the defaults function, so this is only speculation, but I believe that it was not intended to be used with deeply nested "global config" object trees like the one you are specifying here. Instead I think it was intended to work with shallow objects, which is why the functionality for nested maps and lists and so on may not work as you expect.

The other option would be to "flatten" the objects, but that doesnt look nearly as nice.

While it may not be aesthetically pleasing, I think this is the most viable workaround. Without more context of your use case, I can't be sure that this recommendation will work for you, but if there is one set of configuration options for zookeeper, flattening those out should hopefully work.

@ghost
Copy link
Author

ghost commented Apr 13, 2021

For any future users that read this, you have two options:

  1. "flatten" the objects
  2. "empty" declare the objects until you get to the primitive types

@ghost
Copy link
Author

ghost commented Apr 13, 2021

So, option 2 above actually doesn't work if you provide a value; defaults() will only make empty {} have defaults.

@apparentlymart
Copy link
Contributor

Thanks for sharing this feedback, @withernet!

The main reason why this feature remains experimental is that there is still some unsettled debate about how to handle different kinds of omissions, so I appreciate you sharing your use-case here as a contribution to that discussion.

The current design is aiming to allow modules to distinguish between an object being unset and an object being set with all of its attributes null, which some modules use to determine whether a particular feature will be active at all, separately from configuring it when it is active.

I understand is not a distinction that is relevant for your module and so you'd rather have the objects just always be set so you don't have to work around them sometimes being null. Unfortunately we've yet to establish a common base case that suits everyone's requirements, and so clearly some further design work is needed before we can finalize this feature.

@ghost
Copy link
Author

ghost commented Apr 13, 2021

Why not add a parameter to defaults(), maybe call it deep, and if it's true it fills in all defaults? Or, a parameter calling it levels, and it defaults to 1; allowing the user to specify an override for how "deep" to go for defaults?

@ghost
Copy link
Author

ghost commented Apr 14, 2021

Also want to confirm that even if you "flatten"; it's not usable at all. I just flattened our module and if you specify a single defined var, defaults just skips the rest of the object and leaves all the other variables null. This is literally not usable.

@apparentlymart
Copy link
Contributor

Hi @withernet,

Your feedback has been noted! This feature will remain experimental until we've found a suitable answer to all of the feedback, including this issue. Thanks!

@edobry
Copy link

edobry commented May 25, 2021

Just chiming in to say that I'm also experiencing this issue; currently I'm using option 2 (empty-declaring), but this isn't very sustainable, given how deeply-nested my config is, and flattening isn't an option. Unless the design of defaults changes to accommodate this, we'll likely be switching to writing complex modules like this using cdktf in Typescript instead.

@sandwyrm
Copy link

sandwyrm commented Jul 16, 2021

I disagree this issue should be viewed as an enhancement, and believe it should be marked as a bug. If the user is providing a default value using defaults, it would seem their use case implies an expected default to be applied. If the user has supplied neither a value nor a default, then it would be expected that a null to be returned. Currently, the function is ignoring user provided default values, which seems contrary to the point of having the defaults function exist.

This appears to be a simple change in the defaults.go file line 112 to switch the OR test; if umInput.IsNull() || umFb.IsNull() instead to an AND test; if umInput.IsNull() && umFb.IsNull() thereby only falling through if both the original and the defaults value are null. I'm not a Go developer and don't really understand the requisites it would take to establish a build environment and test any modifications for this project, or I would contribute a pull request myself. However, this feels like a trivial modification if my understanding is correct.

Interim Workaround

For others looking for an interim solution, the nested object types can be provided as {} as well to fully fill in the defaults. Essentially, all objects in a tree must exist for defaults to be applied. Using the original example in the issue, if the input provided includes the sub-keys the output matches expectations:

var.my_var
  Enter a value: { some_config1 = {}, deep_config = { deep_1 = {}, deep_2 = {} } }


Changes to Outputs:
  + my_var = {
      + deep_config  = {
          + deep_1 = {
              + something_1 = "asdf"
              + something_2 = 1
            }
          + deep_2 = {
              + something_1 = "fdsa"
              + something_2 = 5
            }
        }
      + some_config1 = {
          + my_something = true
        }
    }

(I had to mark the contents of deep_1 and deep_2 as optional as well for this example)

As a handy helper for CLI runtime usage, the empty defaults template can be added to the description property of the variable for quick copy-and-pase:

variable "my_var" {
   description = "Use Defaults Template:\n{ some_config1 = {}, deep_config = { deep_1 = {}, deep_2 = {} } }"
   ...

This makes it much easier than remembering the structure required without opening the file again to inspect:

var.my_var
  Use Defaults Template:
  { some_config1 = {}, deep_config = { deep_1 = {}, deep_2 = {} } }

  Enter a value: 

@pulecp
Copy link

pulecp commented Jul 21, 2021

Thanks for pointing out this issue. We observe a similar behavior when a default value of an input variable is null or an empty map and default values are specified in defaults() function as the second option. The input variable is nested object with all fields optional. We want to achieve the possibility to override any single key in the input variable with a custom value. The issue in this case is that the objects are null when they are not overridden.

See this example. The default value for cpu is 50m and I would like to override it to 100m in terraform.tfvars.

Terraform version

$ terraform -version
Terraform v1.0.1
on linux_amd64

main.tf

terraform {                                                                     
  experiments = [module_variable_optional_attrs]                                
}                                                                               
                                                                                
variable "k8s_resources" {                                                      
  description = "The K8s resource requests and limits."                         
  type = object({                                                               
    resources = optional(object({                                               
      requests = optional(object({                                              
        cpu    = optional(string)                                               
        memory = optional(string)                                               
      })),                                                                      
      limits = optional(object({                                                
        memory = optional(string)                                               
      }))                                                                       
    }))                                                                         
  })                                                                            
  default = null                                                                
}                                                                               
                                                                                
locals {                                                                        
  k8s_values = defaults(var.k8s_resources, {                                    
    resources = {                                                               
      requests = {                                                              
        cpu    = "50m"                                                          
        memory = "256Mi"                                                        
      },                                                                        
      limits = {                                                                
        memory = "256Mi"                                                        
      },                                                                        
    },                                                                          
  })                                                                            
}                                                                               
                                                                                
output "k8s_values" {                                                           
  value = local.k8s_values                                                      
}

terraform.tfvars

k8s_resources = {
  resources = {
    requests = {
      cpu = "100m"
    }
  }
}

Expected output

[...]
Outputs:

k8s_values = {
  "resources" = {
    "limits" = {
      "memory" = "256Mi"
    }
    "requests" = {
      "cpu" = "100m"
      "memory" = "256Mi"
    }
  }
}

Actual output

[...]
Outputs:

k8s_values = {
  "resources" = {
    "limits" = null /* object */
    "requests" = {
      "cpu" = "100m"
      "memory" = "256Mi"
    }
  }
}

I would appreciate when this behavior could be well documented at least. A fix is welcomed.

@dmikalova
Copy link

dmikalova commented Nov 6, 2021

Using for expressions it is possible to edit objects exactly how you want to - for the kubernetes_manifest resource I wanted to remove keys with null values, as well as set a custom default resources block, albeit empty, so this is what I did:

variable "conf" {
  type = object({
    description = string
    name        = string
    namespace   = string
    params = list(object({
      default     = optional(string)
      description = string
      name        = string
      type        = string
    }))
    resources = optional(object({
      inputs = optional(list(object({
        name = string
        type = string
      })))
      outputs = optional(list(object({
        name = string
        type = string
      })))
    }))
    results = optional(list(object({
      name        = string
      description = string
    })))
    steps = list(object({
      args    = optional(list(string))
      command = optional(list(string))
      env = optional(list(object({
        name  = string
        value = string
      })))
      image = string
      name  = string
      resources = optional(object({
        limits = optional(object({
          cpu    = string
          memory = string
        }))
        requests = optional(object({
          cpu    = string
          memory = string
        }))
      }))
      script     = optional(string)
      workingDir = string
    }))
  })
}

locals {
  conf = merge(
    defaults(var.conf, {}),
    { for k, v in var.conf : k => v if v != null },
    { resources = { for k, v in var.conf.resources : k => v if v != null } },
    { steps = [for step in var.conf.steps : merge(
      { resources = {} },
      { for k, v in step : k => v if v != null },
    )] },
  )
}

It would be convenient to have a function that would deeply remove keys with null values.

@korinne
Copy link

korinne commented Jun 8, 2022

Hi all, we recently released the Terraform v1.3 alpha, which includes the ability to mark object type attributes as optional and set default values.

I'm following up with issues that have provided feedback on the previous experiment, and wanted to invite you all to try the alpha and provide any feedback on this updated design. You can learn more/add comments here. Thank you so much in advance, and hope you like the feature!

@apparentlymart
Copy link
Contributor

Seeing the notification for this also made me realize we missed one when we were closing out some issues to represent that the defaults function is removed altogether in the alpha, in favor of building its functionality directly into the type constraints syntax.

For that reason I'm going to close this issue -- it's discussing a behavior of a feature that's no longer present in Terraform in the main branch, and that we don't plan to change in the v1.2 release series -- though I would still echo @korinne's request to try out the replacement with the v1.3.0 alpha and send any feedback as described in the feedback request topic. Thanks!

@apparentlymart apparentlymart closed this as not planned Won't fix, can't repro, duplicate, stale Jun 9, 2022
@github-actions
Copy link
Contributor

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 Jul 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement functions new new issue not yet triaged v0.15 Issues (primarily bugs) reported against v0.15 releases
Projects
None yet
Development

No branches or pull requests

7 participants