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

At "sigs.k8s.io/yaml/goyaml.v2", marshal output is incorrect when the string field is long and contains Colon and Space(: ) #116

Closed
koba1t opened this issue Sep 16, 2024 · 7 comments
Labels
priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. triage/needs-information Indicates an issue needs more information in order to work on it.

Comments

@koba1t
Copy link
Member

koba1t commented Sep 16, 2024

What happened?

The marshal output is incorrect when the string field is long and contains Colon and Space(: ) at "sigs.k8s.io/yaml/goyaml.v2" pkg.

How can we reproduce it (as minimally and precisely as possible)?

code

https://go.dev/play/p/WDmfH-PWqtW

package main

import (
	"fmt"
	"log"

	yaml "sigs.k8s.io/yaml/goyaml.v2"
)

type StructA struct {
	A string `yaml:"a"`
}

var data = `
a: "12345678901234: .s"
`

var failedData = `
a: "12345678901234567890123456789012345678901234567890123456789012341234567890123456789012345678901234567890123456789012345678901234: .s"
`

func processYAML(input string) {
	var a StructA
	if err := yaml.Unmarshal([]byte(input), &a); err != nil {
		log.Printf("cannot unmarshal data: %v", err)
		return
	}

	yamlBytes, err := yaml.Marshal(a)
	if err != nil {
		log.Printf("cannot marshal data: %v", err)
		return
	}

	fmt.Println(string(yamlBytes))
}

func main() {
	processYAML(data)
	processYAML(failedData)
}

output

a: '12345678901234: .s'

a: '12345678901234567890123456789012345678901234567890123456789012341234567890123456789012345678901234567890123456789012345678901234:
  .s'

Additionally

related to kubernetes-sigs/kustomize#947

@liggitt
Copy link

liggitt commented Sep 16, 2024

it's not clear to me that the resulting yaml is invalid... it round-trips correctly when unmarshaled (see https://go.dev/play/p/_v1ZSI-MnM3), and other yaml linters (e.g. https://www.yamllint.com/, https://yaml-online-parser.appspot.com/) validate it and appear to interpret it correctly

@liggitt liggitt added priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. triage/needs-information Indicates an issue needs more information in order to work on it. labels Sep 16, 2024
@koba1t
Copy link
Member Author

koba1t commented Sep 18, 2024

Sorry, I didn't know about 'line folding' in the YAML specification.
https://yaml.org/spec/1.2.2/#65-line-folding

I mistakenly thought it looked like an unexpected extra line break had been added.

a: '12345678901234567890123456789012345678901234567890123456789012341234567890123456789012345678901234567890123456789012345678901234:
  .s'

/close

@k8s-ci-robot
Copy link

@koba1t: Closing this issue.

In response to this:

Sorry, I didn't know about 'line folding' in the YAML specification.
https://yaml.org/spec/1.2.2/#65-line-folding

I mistakenly thought it looked like an unexpected extra line break had been added.

a: '12345678901234567890123456789012345678901234567890123456789012341234567890123456789012345678901234567890123456789012345678901234:
 .s'

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@r10r
Copy link

r10r commented Oct 16, 2024

@liggitt Do you mind to reopen this issue ?

I'm facing the same issue and according to go-yaml/yaml#455 (comment)
folding should already have been disabled in goyaml.v2. But it has not.

The YAML output becomes difficult to read and edit due to the line wrapping.
So it should be at least possible to disable the line wrapping in the encoder.
This should be fairly easy looking at the following pull requests:

What about merging go-yaml/yaml#329 into the fork of goyaml.v2 ?

@liggitt
Copy link

liggitt commented Oct 16, 2024

We won't reopen this specific issue, since the output is not actually incorrect.

Given that changing the line wrapping default in go-yaml broke things, and they had to back that change out, I'm not inclined to import that problem to this repo. More configurability / options in yaml output wouldn't help users of things like kubectl / kustomize, which would continue to keep compatible defaults using current wrapping behavior.

@r10r
Copy link

r10r commented Oct 16, 2024

Thanks for the explanation. I think about creating a fork, but I wonder if this is a good idea. Do you know what exactly breaks when the maximum line width is changed? I'm fine with increasing the maximum line to a fixed width, let's say 200 chars. Will this generate invalid yaml ?

@liggitt
Copy link

liggitt commented Oct 16, 2024

Do you know what exactly breaks when the maximum line width is changed?

For kubectl / kustomize users, anything expecting stable (byte-equivalent) output in yaml format release-to-release. It's still semantically equal, but was not byte-equal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. triage/needs-information Indicates an issue needs more information in order to work on it.
Projects
None yet
Development

No branches or pull requests

4 participants