Use yaml.UnmarshalStrict() during strict validation

[aeijdenberg]

Earlier this week the go-yaml/yaml#307 landed which adds checking of duplicate keys when unmarshaling yaml, which would have saved me a few hours with a buggy pipeline that I wrote.

This PR changes to the behaviour of verify-pipeline to use yaml.UnmarshalStrict(), only when the existing --strict option is set. This also has the benefit of picking up any field names in a yaml file that don't match a struct (when unmarshalled into a map, anything is allowed) which can help detect typos.

Note that in order for these tests to pass, you will need to vendor in a new version of gopkg.in/yaml.v2. I'm not sure how they plan to update their branch - in my testing of below I updated to the tip of the devel branch at https://github.com/go-yaml/yaml, which I verified passes the full set of integration tests (testflight).

diff --git a/src/gopkg.in/yaml.v2 b/src/gopkg.in/yaml.v2
index e4d366fc..031c9222 160000
--- a/src/gopkg.in/yaml.v2
+++ b/src/gopkg.in/yaml.v2
@@ -1 +1 @@
-Subproject commit e4d366fc3c7938e2958e662b4258c7a89e1f0e3e
+Subproject commit 031c922227a592b2b562a1833438308381f9a8bf

(maybe you'll want to wait for a newer release from them?)

Anyway, submitting the PR now with some basic tests.

It might be interesting to consider using yaml.UnmarshalStrict() everywhere that yaml.Unmarshal() is used, but that's a bigger change to contemplate.

[aeijdenberg]

Build failure is expected. Will need a newer version of gopkg.in/yaml.v2 to get UnmarshalStrict() at all, and then a newer version again (tip of the devel branch) in order to make all of the tests pass.

[vito]

Thanks! Seems worth considering just always being strict. Are there any downsides/breakages from that, besides obviously breaking for people with invalid configurations which were silently being accepted before?

I'm gonna wait for go-yaml/yaml to have it in the v2 branch before merging.

[aeijdenberg]

Hi @vito - I can't think of any reason not to use it everywhere, assuming that it does what it says on the box correctly, and if writing new code I definitely would. I'm not sure whether Concourse stores any yaml that it later reads out again, and whether stricter parsing might cause issues then?

When prepping this PR I did find that of the code in fly that calls yaml.Unmarshal() is actually in the bosh-cli package, so it might be a more involved change to try to push it everywhere.

[aeijdenberg]

@vito - looks like a new version of gopkg.in/yaml.v2 has been released which has the changes we need, so this ought to be able to be merged now.

e.g. https://gopkg.in/yaml.v2 points to https://github.com/go-yaml/yaml/tree/v2.1.1 which includes go-yaml/yaml@77b384e

[marco-m]

Hello any news ?

[vito]

Taking a look now! I'll merge this manually by bumping gopkg at the same time if the tests pass locally.

[vito]

Thanks! All good.

[marco-m]

Man! beer++ if we meet one day!