Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

Throw error when Tuple contains mixed Types #233

Closed
wants to merge 9 commits into from

Conversation

dak1n1
Copy link
Contributor

@dak1n1 dak1n1 commented Jun 14, 2021

In Terraform's type system, Lists and Maps must have consistent element types. However, the Kubernetes API spec defines certain fields as type IntOrString, which can lead to using more than one type in a list. Since Terraform has no equivalent type to translate this to, the solution is make each element of the List or Map the same type. Validation will fail if more than one type is used.

Fixes #231

Description

Release Note

Release note for CHANGELOG:

Fix crash when lists contain more than one element Type

References

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

In Terraform's type system, lists and maps must have consistent element types. However, the Kubernetes API spec defines certain fields as type IntOrString, which can lead to using more than one type in a list. Since Terraform has no equivalent type to translate this to, the solution is make each element of the List or Map the same type. Validation will fail if more than one type is used.

Fixes hashicorp#231
@dak1n1 dak1n1 force-pushed the error_mixed_types branch from a701fc4 to fe2d2fa Compare June 14, 2021 23:17
@dak1n1 dak1n1 marked this pull request as draft June 14, 2021 23:22
dak1n1 added 4 commits June 15, 2021 16:17
This still panics when it hits a list that contains multiple types.
However, it does not panic when the list is all one type.
So it's a step closer to working...
@github-actions github-actions bot added size/S and removed size/XS labels Jun 17, 2021
@dak1n1
Copy link
Contributor Author

dak1n1 commented Jun 17, 2021

There might be a better way to do this, since the SDK includes lists of the Types inside of both Tuples and Objects.

https://pkg.go.dev/github.com/hashicorp/[email protected]/tftypes#Object

https://pkg.go.dev/github.com/hashicorp/[email protected]/tftypes#Tuple

But I couldn't find a way to access those, so I converted the Values into Go values instead.

@dak1n1
Copy link
Contributor Author

dak1n1 commented Jun 17, 2021

This is working for the first 3 conditions I tested for:

Tuple containing Objects of type String and Number. This fails validation, as it should:

  ports:
    - name: http
      port: 8500
      targetPort: 8500
    - name: serflan-tcp
      protocol: "TCP"
      port: 8301
      targetPort: 8301

It passes validation when these are all Strings, as it should:

  ports:
    - name: http
      port: "8500"
      targetPort: "8500"
    - name: serflan-tcp
      protocol: "TCP"
      port: "8301"
      targetPort: "8301"

And it fails validation, as it should, for multiple types within a Tuple (without Objects):

  ports:
    - string
    - 5
    - false

But this is one case where it should not fail validation. It's a valid use case for a Tuple containing an Object of multiple Types:

https://github.com/hashicorp/terraform-helm/blob/9d59da83be654c246d5135248dca662b2ba5a1ef/crds/app.terraform.io_workspaces_crd.yaml#L255-L258

  versions:
  - name: v1alpha1
    served: true
    storage: true
│ Error: Tuple element failed validation:tftypes.Object["name":tftypes.String, "served":tftypes.Bool, "storage":tftypes.Bool]<"name":tftypes.String<"v1alpha1">, "served":tftypes.Bool<"true">, "storage":tftypes.Bool<"true">>
│
│   with kubernetes_manifest.consul,
│   on main.tf line 16, in resource "kubernetes_manifest" "consul":
│   16: resource "kubernetes_manifest" "consul" {
│
│ elements do not all have the same types
╵
╷
│ Error: Config validation failed
│
│   with kubernetes_manifest.consul,
│   on main.tf line 16, in resource "kubernetes_manifest" "consul":
│   16: resource "kubernetes_manifest" "consul" {
│
│ AttributeName("manifest").AttributeName("spec").AttributeName("versions"): elements do not all have the same types
╵

@github-actions github-actions bot added size/M and removed size/S labels Jun 17, 2021
@dak1n1
Copy link
Contributor Author

dak1n1 commented Jun 17, 2021

I was originally trying to implement this as requested by @alexsomesan. He walked me through what the implementation would be like:

You would define a call-back function which the Walk then calls for each attribute of a resource,
 one by one. In that function you'd have to determine if you're dealing with an attribute of type Tuple
 (these get later turned into lists, but they are allowed to have multiple element types so that's where
 the panic comes from).

If it's not a tuple, just move on, otherwise, iterate over the elements of the Tuple and compare their 
types. They should all be the same. If they are not, fail the validation

But after exploring this path, I found that sometimes a Tuple should have different types, so I wasn't able to implement this in the validation function as requested. Instead, I'm now validating that each call to tftypes.NewValue() will not cause a panic. There's a built-in function for validation prior to using NewValue, ValidateValue, which can tell us if the value is compatible with the Type that we're converting it to. I'm opening a new PR for this.

@github-actions
Copy link

I'm going to lock this pull request 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 related to this change, 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 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when mixing integer and string values in list
1 participant