Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't use yaml.v2 2.3.0 which has a breaking change #1259

Merged
merged 1 commit into from Oct 18, 2020

Conversation

marckhouzam
Copy link
Collaborator

@marckhouzam marckhouzam commented Oct 16, 2020

This is to start a discussion to see what would be the best way forward.

PR #1153 moved Cobra to yaml.v2 2.3.0

This change has introduced a breaking change which is preventing kubectl and helm from using Cobra 1.1 (and probably others). See kubernetes/kubernetes#95571 and helm/helm#8890 (comment)

The breaking change has to do with line wrapping and was introduced by go-yaml/yaml#571

The reason behind Cobra upgrading to yaml.v2 2.3.0 was to address a CVE reported by Kubernetes in kubernetes/kubernetes#89535. However the fix for Kubernetes was to move to yaml.v2 2.2.8 as seen in kubernetes/kubernetes#87467. yaml.v2 2.2.8 does not contain the breaking change.

Therefore, Cobra could instead use yaml.v2 2.2.8 and avoid bringing in the breaking change of 2.3.0. This PR makes the suggested change.

This would require doing an quick release of Cobra to address the current problem.

What are the maintainers thoughts on this?
@jharshman @jpmcb @wfernandes

The longer term solution will need to be addressed by the yaml.v2 project.

yaml.v2 contains a breaking change from
go-yaml/yaml#571

yaml.v2 2.2.8 does not have that change and also addresses the CVE that
was behind the move to yaml.v2 2.3.0.  This commit reverts to using
yaml.v2 2.2.8

Signed-off-by: Marc Khouzam <marc.khouzam@montreal.ca>
@marckhouzam
Copy link
Collaborator Author

marckhouzam commented Oct 16, 2020

@eparis may have an opinion on this.

@umarcor
Copy link
Contributor

umarcor commented Oct 16, 2020

Can kubectl and helm use 1.0.0 until the issue is fixed in yaml.v2 (or yaml.v3)?

@marckhouzam
Copy link
Collaborator Author

marckhouzam commented Oct 16, 2020

Can kubectl and helm use 1.0.0 until the issue is fixed in yaml.v2 (or yaml.v3)?

Sure, that's what they are doing now. It's just that the 1.1 release isn't that useful if multiple projects can't use it. So really, this discussion is about trying to get Cobra improvements in the hands of users.

@umarcor
Copy link
Contributor

umarcor commented Oct 16, 2020

My only point is that, depending on the reponsiveness of yaml.v2 maintainers, it might be less disruptive to wait until their next release, and then bump cobra. Was it reported in go-yaml already? I didn't find any reference in the linked issues.

@marckhouzam
Copy link
Collaborator Author

marckhouzam commented Oct 16, 2020

Someone from kubectl was supposed to reach out to them. Should be happening soon.

@thaJeztah
Copy link
Contributor

thaJeztah commented Oct 17, 2020

would it be possible to do a new release with this PR and #1255 (which fixes packaging .deb packaging failures on the latest Ubuntu release)

@jpmcb
Copy link
Collaborator

jpmcb commented Oct 18, 2020

I will work on resolving this today.

I think what we should do is cut a quick patch release and deprecate the version of Yaml to what kubectl and helm need.

Question: @marckhouzam was this an unintended breaking change? Otherwise, I would have expected to see a yaml 3.x version denoting the breaking change.

jpmcb
jpmcb approved these changes Oct 18, 2020
Copy link
Collaborator

@jpmcb jpmcb left a comment

👍

@jpmcb jpmcb merged commit f32f4ef into spf13:master Oct 18, 2020
2 checks passed
@marckhouzam marckhouzam deleted the fix/revertyaml branch Oct 18, 2020
@marckhouzam
Copy link
Collaborator Author

marckhouzam commented Oct 18, 2020

@jpmcb Thank you for jumping on this!

Question: @marckhouzam was this an unintended breaking change? Otherwise, I would have expected to see a yaml 3.x version denoting the breaking change.

From what I understand, the fact that it was a breaking change was overlooked.
Looking at what the author of the change says here kubernetes/kubernetes#95571 (comment), he does realize the change is breaking. The change was brought in by go-yaml/yaml#571, without much discussion.

@jpmcb
Copy link
Collaborator

jpmcb commented Oct 18, 2020

I see - thanks much for the quick response.

From what I understand, the fact that it was a breaking change was overlooked.

That is unfortunate. We will go ahead and call this a bug in 1.1.x and fix it in 1.1.1 shortly. While I have you @marckhouzam any other bug fixes that should get into this release I'm about to cut?

@marckhouzam
Copy link
Collaborator Author

marckhouzam commented Oct 18, 2020

While I have you @marckhouzam any other bug fixes that should get into this release I'm about to cut?

For a 1.1.1 release, I believe the two PRs you just merged (this one and the manpages one) seem to be it.

@jpmcb
Copy link
Collaborator

jpmcb commented Oct 18, 2020

twpayne added a commit to cilium/hubble that referenced this pull request Oct 18, 2020
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>
twpayne added a commit to cilium/hubble that referenced this pull request Oct 18, 2020
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>
@marckhouzam
Copy link
Collaborator Author

marckhouzam commented Oct 18, 2020

https://github.com/spf13/cobra/releases/tag/v1.1.1 is live!

Thanks! It's very appreciated!

twpayne added a commit to cilium/hubble that referenced this pull request Oct 19, 2020
Version 2.3.0 changes the default line wrapping from 80 characters to
none. This potentially breaks Kubernetes and Helm. See
spf13/cobra#1259 for discussion.

Signed-off-by: Tom Payne <tom@isovalent.com>
twpayne added a commit to cilium/hubble that referenced this pull request Oct 19, 2020
gopkg.in/yaml.v2 version 2.3.0 changes the default line wrapping from 80
characters to none. This potentially breaks Kubernetes and Helm. See
spf13/cobra#1259 for discussion.

Signed-off-by: Tom Payne <tom@isovalent.com>
twpayne added a commit to cilium/hubble that referenced this pull request Oct 19, 2020
gopkg.in/yaml.v2 version 2.3.0 changes the default line wrapping from 80
characters to none. This potentially breaks Kubernetes and Helm. See
spf13/cobra#1259 for discussion.

Signed-off-by: Tom Payne <tom@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants