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

0.14.2 panic with nested optional - module_variable_optional_attrs experiment #27269

Closed
bsch150 opened this issue Dec 11, 2020 · 11 comments · Fixed by #28116
Closed

0.14.2 panic with nested optional - module_variable_optional_attrs experiment #27269

bsch150 opened this issue Dec 11, 2020 · 11 comments · Fixed by #28116
Assignees
Labels
bug confirmed a Terraform Core team member has reproduced this issue explained a Terraform Core team member has described the root cause of this issue in code v0.14 Issues (primarily bugs) reported against v0.14 releases

Comments

@bsch150
Copy link

bsch150 commented Dec 11, 2020

While trying out the new module_variable_optional_attrs experiment, I ran into this issue with nested optional attributes. The exact same thing works if I explicitly set the optional_attr to null, and remove the experiment.

Terraform Version

0.14.2

Terraform Configuration Files

Please see the minimal github repo here

There are two examples in the repo. working-without-optional and broken-with-optional. They are identical except the broken example attempts to remove the explicit optional_attr = null using the optional type.

Debug Output

See the debug output in the repo here

Crash Output

See the crash.log in the repo here

Expected Behavior

The two examples should behave exactly the same way. The optional_attr should be implicitly set to null in the broken-with-optional example, and pass tf plan.

Actual Behavior

Terraform panics and crashes.

Steps to Reproduce

git clone https://github.com/bsch150/terraform-14.2-optional-issue.git
cd broken-with-optional
terraform init
terraform plan
@bsch150 bsch150 added bug new new issue not yet triaged labels Dec 11, 2020
@bsch150 bsch150 changed the title 0.14.2 0.14.2 panic with nested optional - module_variable_optional_attrs experiment Dec 11, 2020
@snowsky
Copy link
Contributor

snowsky commented Dec 14, 2020

Does 'optional' need to be used in 'object' type?

variable "in" { type = object({ required = string optional = optional(string) }) }

@bsch150
Copy link
Author

bsch150 commented Dec 15, 2020

Does 'optional' need to be used in 'object' type?

variable "in" { type = object({ required = string optional = optional(string) }) }

@snowsky I'm not sure I understand your question. Are you asking whether the optional is needed there? Or do you mean to point out some way I'm misusing the experiment?

@mildwonkey mildwonkey added confirmed a Terraform Core team member has reproduced this issue v0.14 Issues (primarily bugs) reported against v0.14 releases labels Dec 16, 2020
@mildwonkey
Copy link
Contributor

Thank you for providing the reproduction repo @bsch150 , and I'm sorry about the panic! I've confirmed that the issue still exists in the 0.14 release branch and our master branch.


The crash output gives us a good hint about the problem: convert (in the upstream go-cty library) is getting inconsistent types; in one the list_with_optional is cty.ObjectWithOptionalAttrs and in the other it's just a cty.Object. This is enough information for us to dig into where the problem is occurring. I can't guarantee that we won't find that there's a configuration error in here somewhere, but it definitely should not panic.

Here's the main panic (with some editing and extra newlines, for easier reading):

inconsistent list element types 
(cty.Object(map[string]cty.Type{"nullable_object":cty.Object(map[string]cty.Type{"list_with_optional":cty.List(cty.ObjectWithOptionalAttrs(map[string]cty.Type{"attr":cty.String, "optional_attr":cty.String}, []string{"", "optional_attr"}))})}) 
 cty.Object(map[string]cty.Type{"nullable_object":cty.Object(map[string]cty.Type{"list_with_optional":cty.List(cty.Object(map[string]cty.Type{"attr":cty.String, "optional_attr":cty.String}))})}))

And finally to answer @snowsky 's question, any attribute inside an object can be optional, it does not need to be the entire object.

@mildwonkey mildwonkey added the explained a Terraform Core team member has described the root cause of this issue in code label Dec 16, 2020
@preventhar
Copy link

preventhar commented Dec 16, 2020

I also faced a similar issue today. But, the variable structure I used is a little different. Just wanted to share that.

The variable structure in the bug's description is using optional in a nested object. Like this,

variable "list_of_objects_var" {
  type = list(object({
    nullable_object = object({
      list_with_optional = list(object({
        attr          = string
        optional_attr = optional(string)
      }))
    })
  }))
}

The one I used was a nested optional object. Like this,

variable "test_optional" {
  type = list(object({
    level_one_req_attr = string
    level_one_optional_attr = optional(object({
      level_two_req_attr = string
      level_two_optional_attr = optional(string)
    }))
  }))
}

And the input to the above variable is like this,

test_optional = [
    {
      level_one_req_attr = "sample1"
    },
    {
      level_one_req_attr = "sample2"
      level_one_optional_attr = {
        level_two_req_attr = "sample2"
      }
    },
    {
      level_one_req_attr = "sample3"
      level_one_optional_attr = {
        level_two_req_attr = "sample3"
        level_two_optional_attr = "sample3"
      }
    }
  ]

And the main panic is,

inconsistent list element types
(cty.Object(map[string]cty.Type{"level_one_optional_attr":cty.ObjectWithOptionalAttrs(map[string]cty.Type{"level_two_optional_attr":cty.String, "level_two_req_attr":cty.String}, []string{"", "level_two_optional_attr"}), "level_one_req_attr":cty.String}) then
cty.Object(map[string]cty.Type{"level_one_optional_attr":cty.Object(map[string]cty.Type{"level_two_optional_attr":cty.String, "level_two_req_attr":cty.String}), "level_one_req_attr":cty.String}))

The reason for inconsistent list element types is exactly the same as @mildwonkey described above.

@bsch150
Copy link
Author

bsch150 commented Dec 16, 2020

Thanks for the responses all - especially @mildwonkey!

I just made an interesting discovery around this issue - if you flip the order of the input array, plan passes.

in my broken-with-optional folder, my inputs to the module are:

module "test" {
  source = "./module"
  list_of_objects_var = [
    {
      nullable_object = null
    },
    {
      nullable_object = {
        list_with_optional = [
          {
            attr = "line"
          }
        ]
      }
    }
  ]
}

Now, if I switch the two elements of the list:

module "test" {
  source = "./module"
  list_of_objects_var = [
    {
      nullable_object = {
        list_with_optional = [
          {
            attr = "line"
          }
        ]
      }
    },
    {
      nullable_object = null
    }
  ]
}

With this change I can run plan successfully. However, this is not a solution for my use case as the order does matter for me.

Having never looked into your source code I'll avoid making any guesses why that would be - hopefully this is helpful.

@Matthewsre
Copy link

@mildwonkey, this issue has come up a few times for a microservices module I have been working on and is a big key to unlock some of the features we have been holding back on. Is there any way for me to view how items like this are prioritized?

You can see how we are leveraging optional in our module and have been unable to expand:

Module:
https://registry.terraform.io/modules/Matthewsre/microservices/azurerm/latest

Direct link to variable we would like to expand:
https://github.com/Matthewsre/terraform-azurerm-microservices/blob/master/variables.tf#L88

@Matthewsre
Copy link

Matthewsre commented Feb 19, 2021

To provide a little more context, here is an example usage where I have to force provide some values that I would rather have optional:

image

If the partitional_key_path and max_throughput were optional they could be omitted and would fall back to the default values. There are several places I would like to use that pattern, but have held off in hopes this would be resolved in the near future :)

@mildwonkey
Copy link
Contributor

Unfortunately we don't have a public roadmap at this time, but it's something we would love to do.

Issues with experimental features tend to be lower on the list of priorities unless those features are directly tied to a larger project. Since we're all pretty focused on the 0.15 release, I'm not sure when anyone will look into this issue. I know that isn't much of an answer at all, and I'm sorry that this has been a continued problem for you.

@alisdair
Copy link
Member

alisdair commented Mar 4, 2021

@bsch150 Thanks again for such an excellent reproduction case! I have proposed a fix in the upstream cty library which addresses this issue: zclconf/go-cty#88. This will require review and release before we can bring it into Terraform.

@preventhar, I was unable to reproduce this problem with the configuration you specified in your comment when testing against Terraform 0.15.0-beta1. From looking at the panic I believe it should be fixed by the above PR, but if you can provide a minimal reproduction case which panics, I'd appreciate it.

@ghost
Copy link

ghost commented Apr 16, 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 Apr 16, 2021
@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!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug confirmed a Terraform Core team member has reproduced this issue explained a Terraform Core team member has described the root cause of this issue in code v0.14 Issues (primarily bugs) reported against v0.14 releases
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants