search

carvel-dev / ytt

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

adding duplicate key into map via an overlay should error before serialization time checks #230

Open DennisDenuto opened 3 years ago

DennisDenuto commented 3 years ago

Steps to reproduce

  1. template yml:
---
clients:
- client1:
    secret: blah1
- client2:
    secret: blah2

overlay.yml

#@ load("@ytt:overlay", "overlay")
#@overlay/match by=overlay.all
---
clients:
#@overlay/match by=overlay.all,expects="1+"
- 
  #@overlay/match by="secret", expects=0
  client1:
    secret: foo
  1. ytt -f template.yml -f overlay.yml

Expected

A useful error message

Actual

a go stack trace

error: panic: Unexpected duplicate key: client1

goroutine 1 [running]:
github.com/k14s/ytt/pkg/yamlmeta.convertToGo(0x99cac0, 0xc00017efa0, 0x0, 0xc000096728)
    github.com/k14s/ytt@/pkg/yamlmeta/convert.go:65 +0x7de
github.com/k14s/ytt/pkg/yamlmeta.convertToGo(0x99c840, 0xc00017ed20, 0xc000073fb0, 0x0)
    github.com/k14s/ytt@/pkg/yamlmeta/convert.go:74 +0x47e
github.com/k14s/ytt/pkg/yamlmeta.convertToGo(0x99cac0, 0xc00017e730, 0x7f46f377b6d0, 0x0)
    github.com/k14s/ytt@/pkg/yamlmeta/convert.go:67 +0x5fd
github.com/k14s/ytt/pkg/yamlmeta.(*Document).AsYAMLBytes(0xc00017e3c0, 0x20, 0x952800, 0xc000199701, 0xc000159bc0, 0xc000199758)
    github.com/k14s/ytt@/pkg/yamlmeta/document.go:25 +0x3c
github.com/k14s/ytt/pkg/yamlmeta.(*YAMLPrinter).Print(0xc000159bc0, 0xc00017e3c0, 0xb25800, 0xc000159bc0)
    github.com/k14s/ytt@/pkg/yamlmeta/printers.go:36 +0x42
github.com/k14s/ytt/pkg/yamlmeta.(*DocumentSet).AsBytesWithPrinter(0xc000153110, 0xa91800, 0x1, 0x1, 0xc000096730, 0x1, 0x1)
    github.com/k14s/ytt@/pkg/yamlmeta/document_set.go:58 +0xec
github.com/k14s/ytt/pkg/yamlmeta.(*DocumentSet).AsBytes(...)
    github.com/k14s/ytt@/pkg/yamlmeta/document_set.go:43
github.com/k14s/ytt/pkg/workspace.(*LibraryLoader).Eval(0xc000199a68, 0xc00006f4c0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
    github.com/k14s/ytt@/pkg/workspace/library_loader.go:114
...
Truncated
...
danielhelfand commented 3 years ago

We were able to reproduce this in ytt playground. Digging further, there are actual panic calls within pkg/yamlmeta/convert.go that is leading to the panic. The actual error is shown here.

It would be an improved user experience to have a simple error message without the stack dump from panic, so maybe it would be worthwhile refactoring this?

cppforlife commented 3 years ago

Digging further, there are actual panic calls within pkg/yamlmeta/convert.go that is leading to the panic. The actual error is shown here.

that's right it's coming from serialization layer. that's a layer of defense (aka internal consistency check). the "true" problem is that overlay code does not check when it adds a new keys that it's not duplicate. we should fix this problem at the overlay level. (and live last layer of defense as is).