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

Wrap aws_lambda_function.environment.variables type in &schema.Schema #2847

Closed

Conversation

jimmy-btn
Copy link
Contributor

@jimmy-btn jimmy-btn commented Jan 3, 2018

Wrap element type in &schema.Schema so that it conforms to the expected Terraform Schema structure

@Ninir Ninir added the crash Results from or addresses a Terraform crash or kernel panic. label Jan 4, 2018
@Ninir
Copy link
Contributor

Ninir commented Jan 4, 2018

Hi @jimmy-btn

Could you give more details about how this is happening?
If you can provide debug logs, HCL configurations and steps to reproduce, this would be greatly appreciated. Thanks!

@Ninir Ninir added the waiting-response Maintainers are waiting on response from community or contributor. label Jan 4, 2018
@jimmy-btn
Copy link
Contributor Author

Hi Ninir!

tl;dr This isn't an issue today, because Terraform doesn't validate the types of elements passed to Maps properly, but will cause issues in the future.

This one is a bit tricky - I noticed it while making some changes inside Terraform, but haven't worked out how to tickle it from the public interface because Terraform doesn't validate the types for elements passed to maps via the schema provider interface.

This is definitely an invalid schema - although I mis-understood why. Terraform is happy to have an Elem field for TypeMap, but expects Elem to be a *Schema or a *Resource. I've modified this PR to wrap the field correctly.

Why is this a problem?
In some cases, Terraform will traverse the full Schema, and expects Elem to be a *Schema or a *Resource.

See CoreConfigSchema() function in helper/schema/core_schema.go for an example of code which is going to panic if this is not the case.

Reproducing
This doesn't cause any real problems today for Maps, but we can reproduce the issue using a List.

Add a field to an existing resource which implements a list of Integers, and specify the type in Elem directly (as we have with the environment.variables map for aws_lambda_function):

"test": {
    Type:     schema.TypeList,
    Optional: true,
    Elem:     Type: schema.TypeInt,
},

Terraform will not pick up on the error when validating the schema, but if you try and create or import a resource which uses that field but supplies the incorrect type, Terraform will panic and crash. For example, the following resource will cause a panic:

resource "my_resource" "name" {
    test = ["string_not_integer"]
}

If you specify Elem as a *Schema:

"test": {
    Type:     schema.TypeList,
    Optional: true,
    Elem:     &schema.Schema(Type: schema.TypeInt),
},

Terraform will correctly identify the type mismatch and output
test.0: cannot parse '' as bool: strconv.ParseBool: parsing "string_not_integer": invalid syntax instead. Much nicer!

In hashicorp/terraform#17037 I am proposing adding an explicit test for this to InternalValidate to make sure all provider schemas are in good shape, in case we ever want to add type validation for Maps.

@jimmy-btn jimmy-btn changed the title Remove invalid Elem field from aws_lambda_function.environment.variables Wrap aws_lambda_function.environment.variables type in &schema.Schema Jan 5, 2018
@jimmy-btn
Copy link
Contributor Author

I'm going to drop this PR and replace with one which covers all instances of this issue in the codebase. Thank you!

@jimmy-btn jimmy-btn closed this Jan 8, 2018
@jimmy-btn jimmy-btn deleted the fix-lambda-function-schema branch January 8, 2018 00:55
@ghost
Copy link

ghost commented Apr 8, 2020

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 8, 2020
@breathingdust breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
crash Results from or addresses a Terraform crash or kernel panic.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants