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

Multi-Line strings are marshalled with incorrect line breaks injected into the result #789

Open
chrisprijic opened this issue Oct 4, 2021 · 7 comments

Comments

@chrisprijic
Copy link

chrisprijic commented Oct 4, 2021

Specifically referencing a ticket on Kustomize (kubernetes-sigs/kustomize#4201) that mentioned it might be a go-yaml issue -- I was able to get go-yaml to reproduce the issue directly (see below).

When I have the YAML as follows:

apiVersion: batch/v1
kind: Job
metadata:
  name: "pulsar-pulsar-init"
  namespace: default
spec:
  template:
    spec:
      containers:
        - name: "myContainer"
          image: "my/image"
          imagePullPolicy: IfNotPresent
          command: ["sh", "-c"]
          args:
            - >

              bin/my executable \
                --myParam A \
                --myParam2 B

it ends up unmarshalled and marshaled back into this (notice the injected newline on line 13):

apiVersion: batch/v1
kind: Job
metadata:
  name: pulsar-pulsar-init
  namespace: pulsar
spec:
  template:
    spec:
      containers:
      - args:
        - >4
          bin/my executable \

            --myParam A \
            --myParam2 B
        command:
        - sh
        - -c
        image: my/image
        imagePullPolicy: IfNotPresent
        name: myContainer

Here's a simpler example that can be executed on Go Playground: https://play.golang.org/p/rDAQcO_csLI

This definitely breaks the usage of the value and does not reference the original value correctly.

@niemeyer
Copy link
Contributor

niemeyer commented Oct 4, 2021

Thanks for the reproducer. I'll look into it.

@rogpeppe
Copy link
Contributor

rogpeppe commented Oct 4, 2021

FWIW, looking at the playground example only, both the original YAML and the final YAML unmarshal to the same value, so there's no round trip issue where Go is being used AFAICS.

A slightly simpler example: https://play.golang.org/p/CL2RI6cgUzO

@chrisprijic
Copy link
Author

chrisprijic commented Oct 4, 2021

@rogpeppe I think it has to do specifically with unmarshalling and marshalling with yaml.Node -- check this out: https://play.golang.org/p/iN2COX5_VCs

The value in the unmarshalled struct is correct but becomes incorrect once marshalled.

EDIT:

Also unmarshalling the \n\n one with yaml.Node removes it? Here: https://play.golang.org/p/qiD8yAaIbli

@rogpeppe
Copy link
Contributor

rogpeppe commented Oct 5, 2021

@chrisprijic

The value in the unmarshalled struct is correct but becomes incorrect once marshalled.

I don't see that - when we unmarshal the YAML that you're saying is incorrect, we get a matching result, so I think it's hard to argue that it's not right, given that it round-trips OK:

https://play.golang.org/p/vuPSb9x-VRf

@chrisprijic
Copy link
Author

chrisprijic commented Oct 5, 2021

@rogpeppe I think the below output of your playground example should help clarify what I'm talking about:

image

The original value has a single newline (1), while the marshalled value has two (2). When the final value is unmarshalled again, the double newlines become a single newline again (3).

The use-case in kubernetes-sigs/kustomize#4201 is that it reads, modifies, and writes YAML and needs this pattern to work with go-yaml.

When this marshalled output:

>4
    bin/my_executable \

        --myParam A \
        --myParam B

is a command being run by /bin/sh via Kustomize + Kubectl, the extra newline there breaks the command and renders it unusable (I received output along the lines of "command not found: --myParam").

Even though both unmarshal into the same output, they don't match each others original YAML strings, and I see that as incorrect behavior.

EDIT:

Or maybe it has to do with the YAML spec -- that empty lines are ignored, and that's why this behavior is deemed ok?

@rogpeppe
Copy link
Contributor

rogpeppe commented Oct 6, 2021

Or maybe it has to do with the YAML spec -- that empty lines are ignored, and that's why this behavior is deemed ok?

I thought that was the case when using folded style, but I don't think it is in fact, having tried to understand the YAML spec a bit more.

There's definitely something funky going on here. Here's a simpler test case that shows it failing to round trip a string that starts with a newline: https://play.golang.org/p/Vn6fXJLBHCg
The old version doesn't exhibit the same issue.

Also, it turns out that I got the code in my example above wrong; oops, sorry! When I fix it to actually round trip, it does demonstrate the problem:

https://play.golang.org/p/yibN7e8S2_Y

@chrisprijic
Copy link
Author

@niemeyer were you able to figure anything out related to this? I might have time later this week to also take a look if not but I've been deep in other things atm. I'm hoping stepping through the code with a debugger should be enough to see where things go awry.

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

No branches or pull requests

3 participants