duplicate key check in plain yamls

[mhubig]

ytt crashes when running against the current tekton release:

ytt --file-mark '*.yaml:type=yaml-plain' -f https://storage.googleapis.com/tekton-releases/pipeline/previous/v0.21.0/release.yaml

panic: Unexpected duplicate key: name

goroutine 1 [running]:
github.com/k14s/ytt/pkg/yamlmeta.convertToGo(0x158b5c0, 0xc0002d5a40, 0x0, 0x0)
        github.com/k14s/ytt/pkg/yamlmeta/convert.go:65 +0x7ca
github.com/k14s/ytt/pkg/yamlmeta.convertToGo(0x158b300, 0xc0002d54a0, 0xc000197e50, 0x0)
        github.com/k14s/ytt/pkg/yamlmeta/convert.go:74 +0x454
github.com/k14s/ytt/pkg/yamlmeta.convertToGo(0x158b5c0, 0xc0002d5380, 0xc000197df0, 0x0)
        github.com/k14s/ytt/pkg/yamlmeta/convert.go:67 +0x5ed
github.com/k14s/ytt/pkg/yamlmeta.convertToGo(0x158b5c0, 0xc0002d4f00, 0x10ff7a7, 0xc00046853a)
        github.com/k14s/ytt/pkg/yamlmeta/convert.go:67 +0x5ed
github.com/k14s/ytt/pkg/yamlmeta.(*Document).AsYAMLBytes(0xc00032e8a0, 0xc00014c940, 0x4, 0x4, 0x4, 0x0)
        github.com/k14s/ytt/pkg/yamlmeta/document.go:25 +0x3c
github.com/k14s/ytt/pkg/yamlmeta.(*YAMLPrinter).Print(0xc0003ce5e0, 0xc00032e8a0, 0x0, 0x0)
        github.com/k14s/ytt/pkg/yamlmeta/printers.go:36 +0x42
github.com/k14s/ytt/pkg/yamlmeta.(*DocumentSet).AsBytesWithPrinter(0xc000116000, 0x1674e98, 0x2e, 0x30, 0xc000220800, 0x2e, 0x40)
        github.com/k14s/ytt/pkg/yamlmeta/document_set.go:58 +0xe8
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(0xc0000d9ad8, 0xc0000b0fc0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
        github.com/k14s/ytt/pkg/workspace/library_loader.go:151 +0x343
github.com/k14s/ytt/pkg/cmd/template.(*TemplateOptions).RunWithFiles(0xc0000d1540, 0xc0000ac0d0, 0x1, 0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        github.com/k14s/ytt/pkg/cmd/template/cmd.go:168 +0x55c
github.com/k14s/ytt/pkg/cmd/template.(*TemplateOptions).Run(0xc0000d1540, 0x0, 0x0)
        github.com/k14s/ytt/pkg/cmd/template/cmd.go:97 +0x2b6
github.com/k14s/ytt/pkg/cmd/template.NewCmd.func1(0xc0000f6dc0, 0xc0000b0ec0, 0x0, 0x4, 0x0, 0x0)
        github.com/k14s/ytt/pkg/cmd/template/cmd.go:61 +0x2a
github.com/cppforlife/cobrautil.WrapRunEForCmd.func1.1(0xc0000f6dc0, 0xc0000b0ec0, 0x0, 0x4, 0x0, 0x0)
        github.com/cppforlife/cobrautil@v0.0.0-20200514214827-bb86e6965d72/misc.go:45 +0xaf
github.com/cppforlife/cobrautil.WrapRunEForCmd.func1.1(0xc0000f6dc0, 0xc0000b0ec0, 0x0, 0x4, 0x0, 0x0)
        github.com/cppforlife/cobrautil@v0.0.0-20200514214827-bb86e6965d72/misc.go:45 +0xaf
github.com/spf13/cobra.(*Command).execute(0xc0000f6dc0, 0xc0000b6010, 0x4, 0x4, 0xc0000f6dc0, 0xc0000b6010)
        github.com/spf13/cobra@v1.0.0/command.go:842 +0x453
github.com/spf13/cobra.(*Command).ExecuteC(0xc0000f6dc0, 0xc0000f6dc0, 0x1a87b00, 0xc0000c01f8)
        github.com/spf13/cobra@v1.0.0/command.go:950 +0x349
github.com/spf13/cobra.(*Command).Execute(...)
        github.com/spf13/cobra@v1.0.0/command.go:887
main.main()
        github.com/k14s/ytt/cmd/ytt/ytt.go:21 +0x112

I think this is because of the strange merge syntax within the manifest:

- !!merge <<: *version
   name: v1beta1
   storage: true

Environment:

• ytt version (use ytt --version): 0.31.0
• OS (e.g. from /etc/os-release): macOS Big Sur (11.2.1)

[joaopapereira]

Hello @mhubig
After some scavenging, I was able to reproduce your issue. Definitely, it is related to that piece of YAML there.
Unfortunately at this point in time ytt does not support the merge operator as described in https://yaml.org/type/merge.html

We do have another open issue with the same ask #299 but in your case, it is causing a panic because the file is marked as plain YAML and ytt is not try to process it.
Another interesting part of your issue is that the panic is not very helpful, so I think I would keep this issue open to tracking the enhancement of this panic with maybe the filename and line to at least help us find the culprit line more quickly.

[cppforlife]

Unfortunately at this point in time ytt does not support the merge operator as described in https://yaml.org/type/merge.html

ytt does support merge. can be demonstrated by following that works successfully+correctly:

stuff: &version 
  non_name_key: val
array:
- !!merge <<: *version
  name: v1beta1

the underlying problem here is that ytt is strict about setting same key in a map twice (there is a flag to control that feature --implicit-map-key-overrides). in the example above *version already includes a name, and the next line tries to set name again. this is a safety feature which is quite useful in other contexts but may not be as useful as when consuming third party yaml.

i think we need to think hard about what kind of functionality we want to have for this. for example, should we do something similar to what we are planning to do with unknown comments (disable that check for plain yaml files -- non ytt templates).

[aaronshurley]

For this issue, we would like to make a couple of improvements here:

• provide a better UX for this scenario (we shouldn't panic, consider referencing the --implicit-map-key-overrides flag)
• ensure that documentation around the --implicit-map-key-overrides is clear

As @cppforlife mentioned, we should think more about the functionality that we want for plain yaml vs ytt templates.

[pivotaljohn]

FWIW, at least in this specific Tekton case, this was resolved for users on the source side: tektoncd/pipeline#3842

[pivotaljohn]

i think we need to think hard about what kind of functionality we want to have for this. for example, should we do something similar to what we are planning to do with unknown comments (disable that check for plain yaml files -- non ytt templates).

Idea:

1. for now, extend the --implicit-map-key-override to plain YAML file evaluation.
2. restore the balance of usability with security by introducing a new File Mark: options which is a CSV with possible values implicit-map-key-overrides, ignore-unknown-comments, strict.
3. deprecate and then remove the all-or-nothing mode of these flags
   • if someone wants everything to be configured to allow implicit map keys, glob it;
   • this reduces interface complexity: there's one mechanisms to set the parser options;
   • it also encourages being judicious in disabling such checks.

Example:

$ ytt -f plain.yml -f template.yml --file-mark 'plain.yml:options=implicit-map-key-overrides`

[pivotaljohn]

Looking deeper, other YAML processing tools override keys, implicitly.

In ytt, once we've determined that a YAML file is "plain YAML", there's no need to require the --implicit-map-key-override flag to perform the override for that document.

This is the approach we took in #550. That work is done and will be included in the next release. 👍🏻