search

carvel-dev / ytt

YAML templating tool that works on YAML structure instead of text
https://carvel.dev/ytt
Apache License 2.0
1.66k stars 135 forks source link

When struct keys are not strings, should error, not panic #764

Open hmarko opened 1 year ago

hmarko commented 1 year ago

When a non-string value has been provided for an key in a struct, ytt should report an error

ytt: Error:
- expected string for data value key, was bool
    config.yaml:7 |         on:

rather than panic (see below).

👇🏻 original report 👇🏻

when using on as a key in yaml (see below example) ytt panics. changing it to anything else solves it (just change the on: to on1: for example)

config.yaml:

#@data/values
---
general:
    notifications:
        recipients:
        - admin@test.com
        on:
        - any

data.yaml:

#@ load("@ytt:data", "data")
- name: Delete 
  type: multistep 
  steps: 
  - type: ansible
    inventory: hosts
    #@ if/end hasattr(data.values.general,"notifications"):
    notifications: #@ data.values.general.notifications

ytt panics with the following stack trace: 

panic: expected struct key %!s(bool=true) to be string

goroutine 1 [running]:
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.dictAsStarlarkValue.func1({0x8924a0, 0xd61ba8}, {0x88fee0, 0xc00009b818})
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:100 +0x127
github.com/vmware-tanzu/carvel-ytt/pkg/orderedmap.(*Map).Iterate(0xaaaaaaaaaaaaa, 0xc00018cdf8)
        github.com/vmware-tanzu/carvel-ytt/pkg/orderedmap/map.go:71 +0x79
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.dictAsStarlarkValue({{0x9004e0, 0xc00009b7b8}, {0xe8, 0x0}}, 0xda58e0)
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:96 +0xe9
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.asStarlarkValue({{0x9004e0, 0xc00009b7b8}, {0xf4, 0x0}}, {0x9004e0, 0xc00009b7e8})
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:83 +0x2fc
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.dictAsStarlarkValue.func1({0x896f20, 0xc00017ab90}, {0x9004e0, 0xc00009b7e8})
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:98 +0x98
github.com/vmware-tanzu/carvel-ytt/pkg/orderedmap.(*Map).Iterate(0x0, 0xc00018d008)
        github.com/vmware-tanzu/carvel-ytt/pkg/orderedmap/map.go:71 +0x79
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.dictAsStarlarkValue({{0x9004e0, 0xc00009b7b8}, {0xd0, 0x0}}, 0xc00018d090)
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:96 +0xe9
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.asStarlarkValue({{0x9004e0, 0xc00009b7b8}, {0xf4, 0x0}}, {0x9004e0, 0xc00009b7d0})
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:83 +0x2fc
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.dictAsStarlarkValue.func1({0x896f20, 0xc00017ab80}, {0x9004e0, 0xc00009b7d0})
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:98 +0x98
github.com/vmware-tanzu/carvel-ytt/pkg/orderedmap.(*Map).Iterate(0x18, 0xc00018d218)
        github.com/vmware-tanzu/carvel-ytt/pkg/orderedmap/map.go:71 +0x79
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.dictAsStarlarkValue({{0x9004e0, 0xc00009b7b8}, {0xb8, 0x0}}, 0xc00018d2f8)
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:96 +0xe9
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.asStarlarkValue({{0x9004e0, 0xc00009b7b8}, {0x8, 0x0}}, {0x9004e0, 0xc00009b7b8})
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:83 +0x2fc
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.AsStarlarkValue(...)
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:36
github.com/vmware-tanzu/carvel-ytt/pkg/yttlibrary.NewDataModule(0xa84f00, {0xa84f00, 0xc00019daa0})
        github.com/vmware-tanzu/carvel-ytt/pkg/yttlibrary/data.go:27 +0x45
github.com/vmware-tanzu/carvel-ytt/pkg/workspace.(*TemplateLoader).EvalYAML(0xc0001ae7e0, {0xc0000ba690, 0xc0000ba690}, 0xc0000ba5f0)
        github.com/vmware-tanzu/carvel-ytt/pkg/workspace/template_loader.go:198 +0x47c
github.com/vmware-tanzu/carvel-ytt/pkg/workspace.(*LibraryExecution).eval(0xc00018da58, 0xc0001a82c0, {0x0, 0xc0001969f0, 0xc00018d7a0}, {0x0, 0xa96c88, 0xc00019ff40})
        github.com/vmware-tanzu/carvel-ytt/pkg/workspace/library_execution.go:225 +0x91e
github.com/vmware-tanzu/carvel-ytt/pkg/workspace.(*LibraryExecution).Eval(0xc00018da58, 0x0, {0x0, 0x0, 0xc00009a948}, {0x0, 0xc00008b860, 0x0})
        github.com/vmware-tanzu/carvel-ytt/pkg/workspace/library_execution.go:177 +0x46
github.com/vmware-tanzu/carvel-ytt/pkg/cmd/template.(*Options).RunWithFiles(0xc0000c7540, {{0xc00008b8b0, 0x2, 0x14}}, {0xa8a930, 0xc000135290})
        github.com/vmware-tanzu/carvel-ytt/pkg/cmd/template/cmd.go:164 +0x570
github.com/vmware-tanzu/carvel-ytt/pkg/cmd/template.(*Options).Run(0xc0000c7540)
        github.com/vmware-tanzu/carvel-ytt/pkg/cmd/template/cmd.go:100 +0x4b7
github.com/vmware-tanzu/carvel-ytt/pkg/cmd.NewCmd.func1(0x0, {0xc0000b89e0, 0x0, 0x0})
        github.com/vmware-tanzu/carvel-ytt/pkg/cmd/template.go:22 +0x1d
github.com/cppforlife/cobrautil.WrapRunEForCmd.func1.1(0x0, {0xc0000b89e0, 0x0, 0x2})
        github.com/cppforlife/cobrautil@v0.0.0-20200514214827-bb86e6965d72/misc.go:45 +0x6d
github.com/cppforlife/cobrautil.WrapRunEForCmd.func1.1(0xc000120a00, {0xc0000b89e0, 0x0, 0x2})
        github.com/cppforlife/cobrautil@v0.0.0-20200514214827-bb86e6965d72/misc.go:45 +0x6d
github.com/spf13/cobra.(*Command).execute(0xc000120a00, {0xc00008c160, 0x2, 0x2})
        github.com/spf13/cobra@v1.5.0/command.go:872 +0x624
github.com/spf13/cobra.(*Command).ExecuteC(0xc000120a00)
        github.com/spf13/cobra@v1.5.0/command.go:990 +0x3bc
github.com/spf13/cobra.(*Command).Execute(...)
        github.com/spf13/cobra@v1.5.0/command.go:918
main.main()
        github.com/vmware-tanzu/carvel-ytt/cmd/ytt/ytt.go:21 +0xd6

Environment: ytt version 0.43.0 CentOS Linux release 7.8.2003 (Core)

hmarko commented 1 year ago

This beheviour happens on words like: on, off, true, false used as yaml keys.

mamachanko commented 1 year ago

@hmarko thanks for reporting this!

Panicking is not great 😢

Treating on as a boolean is YAML as per spec. Unless quoted a string matching y|Y|yes|Yes|YES|n|N|no|No|NO|true|True|TRUE|false|False|FALSE|on|On|ON|off|Off|OFF is treated as a boolean. See this example on the playground.

To be able to use the key on - as shown in your example - quote it:

#@data/values
---
general:
    notifications:
        recipients:
        - admin@test.com
        "on":
        - any
hmarko commented 1 year ago

Thank you ! I think it worth taking care of it to prevent nasty panics from happening. tools like ytt should template this anyway (what will happen if I want to change true to false as per the yaml spec using ytt ?)

mamachanko commented 1 year ago

Thank you ! I think it worth taking care of it to prevent nasty panics from happening.

I agree, the panic isn't right. However, I think the right behavior according to spec is to treat your data values like so:

#@data/values
---
general:
    notifications:
        recipients:
        - admin@test.com
        true: #! note the boolean
        - any

tools like ytt should template this anyway (what will happen if I want to change true to false as per the yaml spec using ytt ?)

I am not sure I follow you here. Can you expound and maybe provide an example?

hmarko commented 1 year ago

Ok, I think I got it now :-) My YAML uses the on: boolean as both key and value in the hierarchy. I cannot really use it as a key according to the spec.

pivotaljohn commented 1 year ago

@hmarko it's not you, this was a "good intention, bad impact" idea that we actually had intended to "fix". But that work has not yet been prioritized by the maintainers. See #407.

What remains, tho, is the better handling. As @mamachanko noted above: panics are suitable for when something goes terribly wrong internally. When the problem arises directly from our inputs, we should error out gracefully with a helpful + actionable message. 👍🏻

I'm going to adjust this issue to capture that scope.