vendor: pin yaml.v2 to v2.2.8

[twpayne]

gopkg.in/yaml.v2 2.3.0 changed the default line wrapping which broke
some k8s tests upstream.

Signed-off-by: Tom Payne tom@isovalent.com

[rolinh]

Generally speaking, I really want us to move away from using replace directives as this makes the code non go-get'able. For this particular case however, it seems that it's probably the best course of action as downgrading gopkg.in/yaml.v2 to v2.2.8 means that github.com/go-openapi/runtime gets downgraded from v0.19.20 to v0.19.15 and github.com/go-openapi/validate from v0.19.10 to v0.19.8.

EDIT: the only difference between v2.2.8 and v2.3.0 is the breaking change regarding line wrapping: go-yaml/yaml@v2.2.8...v2.3.0

[aanm]

@twpayne do you have more information? How does this affect Cilium? Is Cilium affected by this line wrapping default?

[twpayne]

@aanm There's an active discussion in the Cilium and eBPF Slack.

[aanm]

@aanm There's an active discussion in the Cilium and eBPF Slack.

Slack threads will expire after 30 days so we should keep the history the commit messages or PRs

[twpayne]

Slack threads will expire after 30 days so we should keep the history the commit messages or PRs

Good point. I'll copy the Slack history here when this PR is merged.

[aanm]

test-me-please

[aanm]

marked for backport. This changes the output of the generated CRDs, with this PR its output will be exactly the same as the one gotten from kubectl get crd ... -o yaml.

[twpayne]

test-me-please

[twpayne]

Test failure is flake #13181.

[twpayne]

Slack thread for posterity:

Tam Mach Yesterday at 11:44 AM
👋 , should the same be done for cilium cilium/hubble#391 (highlighted by @tom Payne) ?
twpaynetwpayne
#391 vendor: bump cobra to v1.1.1
cobra 1.1.0 inadvertently included a breaking YAML change affecting
kubectl and helm. See spf13/cobra#1259 for
details.
Signed-off-by: Tom Payne tom@isovalent.com
Labels
ready-to-merge, release-note/misc
Comments
2
https://github.com/cilium/hubble|cilium/hubblecilium/hubble | Oct 18th | Added by GitHub

22 replies

Tom Payne 1 day ago
I suspect so. Do you want to create the PR? Otherwise, I'm happy to do it.

aanm 1 day ago
cilium has v1.0.0 so it's unlikely affected? (edited)

Tam Mach 1 day ago
but yaml is v2.3.0, which is the breaking change underlying.

Tom Payne 1 day ago
I think (but have not verified) that the breaking change was in the k8s tests where YAML outputs are compared byte-for-byte, so changing the default line wrapping (which happened for yaml.v2 v2.3.0) broke the tests.
:face_with_monocle:
1

Tom Payne 1 day ago
However, I think it's worth being consistent across Cilium and Hubble so I would advocate for using the same versions of libraries in both.

rolinh 1 day ago
Comparing yaml v2.2.8 and v2.3.0: go-yaml/yaml@v2.2.8...v2.3.0
The only change is the breaking one about line wrapping.
I'd advocate to downgrade to gopkg.in/yaml.v2 v2.2.8 in Cilium as well so we can also get rid of the replace directive in Hubble. v2.2.8 is the version we have in Cilium v1.8 fwiw.

Tom Payne 1 day ago
OK, on it.
🚀
1

Tom Payne 1 day ago
#13620

twpaynetwpayne
#13620 vendor: pin yaml.v2 to v2.2.8
gopkg.in/yaml.v2 2.3.0 changed the default line wrapping which broke
some k8s tests upstream.
Signed-off-by: Tom Payne tom@isovalent.com
Labels
area/build, release-note/misc
Reviewers
@rolinh, @aanm, @cilium/vendor
https://github.com/cilium/cilium|cilium/ciliumcilium/cilium | Yesterday at 1:52 PM | Added by GitHub

rolinh 1 day ago
Is a dependency pulling in v2.3.0 explicitly? Phrased differently: can we avoid the replace directive?

Tom Payne 1 day ago
Short answer: I don't know. Longer answer: I don't know, but pinning the version is safer as it's more robust to changes in our dependencies dependencies.
:face_with_monocle:
1

Tom Payne 1 day ago
I would suggest keeping the pin for now, and then checking back in a week or two to see how k8s are handling it.

Tam Mach 1 day ago
Short answer: I don't know. Longer answer: I don't know, but pinning the version is safer as it's more robust to changes in our dependencies dependencies.
I usually run go mod graph to check, there was a visualisation tool as well, but it's too much for big project. Please find below output for master branch.
$ go mod graph | grep "gopkg.in/yaml.v2@v2.3.0"
github.com/cilium/cilium gopkg.in/yaml.v2@v2.3.0
github.com/go-openapi/validate@v0.19.10 gopkg.in/yaml.v2@v2.3.0
github.com/go-openapi/runtime@v0.19.20 gopkg.in/yaml.v2@v2.3.0
gopkg.in/yaml.v2@v2.3.0 gopkg.in/check.v1@v0.0.0-20161208181325-20d25e280405
(edited)

Tom Payne 1 day ago
That's a good tip, thanks.

rolinh 1 day ago
Yup, this is what I noticed as well. Downgrading go-yaml means also downgrading github.com/go-openapi/runtime from v0.19.20 to v0.19.15 and github.com/go-openapi/validate from v0.19.10 to v0.19.8. Not sure what the implications are there but it's maybe best to sigh use yet another replace directive for the time being as Tom suggests.
:cilium-new:
1

glibsm 1 day ago
The replace directive was made for cases such as these. Common lib that is used by a lot of things. We don’t necessarily have to downgrade everything, just replace yaml out until it’s fixed (otherwise it can easily creep back in).
:cilium-new:
1

glibsm 1 day ago
The only scenarios where it doesn’t work is if lib A which depends on YAML uses a new feature from the latest YAML bugged release.

Tom Payne 1 day ago
Hmm, this seems to affect us too: we do byte-for-byte comparisons on YAML files. https://github.com/cilium/cilium/pull/13620/checks?check_run_id=1275837527

aanm 1 day ago
@tom Payne run make manifests the k8s code generator generates different files depending on the library version (edited)
👍
1

Tom Payne 1 day ago
Thanks, PR updated.

aanm 1 day ago
@tom Payne you need to also run make go-bindata
✔️
1

aanm 1 day ago
cc @chris Tarazi for visibility. This will change the output of the manifests files. kubectl get crd ... -o yaml returns an output exactly as the one proposed in the above PR. (edited)
👍
1

Chris Tarazi 1 day ago
LGTM on the CRD changes