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

Long lines are broken with space in them #3969

Closed
jsturtevant opened this issue Jun 8, 2021 · 17 comments
Closed

Long lines are broken with space in them #3969

jsturtevant opened this issue Jun 8, 2021 · 17 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@jsturtevant
Copy link

Describe the bug

When upgrading to the 4.0.5+ from 4.0.4 longs lines with spaces in the yaml are broken. Looks similiar to #947 but that was fixed in older versions (this wasn't an issue in 3.8.x+).

Files that can reproduce the issue
Example:

kustomization.yaml

resources:
 - test.yaml

test.yaml

apiVersion: controlplane.cluster.x-k8s.io/v1alpha4
kind: KubeadmControlPlane
metadata:
  name: ${CLUSTER_NAME}-control-plane
  namespace: default
spec:
  kubeadmConfigSpec:
    postKubeadmCommands:
    - sed -i '\#--listen-client-urls#s#$#,https://127.0.0.1:2379#' /etc/kubernetes/manifests/etcd.yaml
    - echo "DNSStubListener=no" >> /etc/systemd/resolved.conf
    - mv /etc/resolv.conf /etc/resolv.conf.OLD && ln -s /run/systemd/resolve/resolv.conf /etc/resolv.conf
    - systemctl restart systemd-resolved

Expected output

apiVersion: controlplane.cluster.x-k8s.io/v1alpha4
kind: KubeadmControlPlane
metadata:
  name: ${CLUSTER_NAME}-control-plane
  namespace: default
spec:
  kubeadmConfigSpec:
    postKubeadmCommands:
    - sed -i '\#--listen-client-urls#s#$#,https://127.0.0.1:2379#' /etc/kubernetes/manifests/etcd.yaml
    - echo "DNSStubListener=no" >> /etc/systemd/resolved.conf
    - mv /etc/resolv.conf /etc/resolv.conf.OLD && ln -s /run/systemd/resolve/resolv.conf /etc/resolv.conf
    - systemctl restart systemd-resolved

Actual output

apiVersion: controlplane.cluster.x-k8s.io/v1alpha4
kind: KubeadmControlPlane
metadata:
  name: ${CLUSTER_NAME}-control-plane
  namespace: default
spec:
  kubeadmConfigSpec:
    postKubeadmCommands:
    - sed -i '\#--listen-client-urls#s#$#,https://127.0.0.1:2379#' /etc/kubernetes/manifests/etcd.yaml
    - echo "DNSStubListener=no" >> /etc/systemd/resolved.conf
    - mv /etc/resolv.conf /etc/resolv.conf.OLD && ln -s /run/systemd/resolve/resolv.conf
      /etc/resolv.conf
    - systemctl restart systemd-resolved

Kustomize version
4.0.5+ including 4.1.x

Platform
linux

Additional context

@jsturtevant jsturtevant added the kind/bug Categorizes issue or PR as related to a bug. label Jun 8, 2021
@Shell32-Natsu Shell32-Natsu added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jun 8, 2021
@Shell32-Natsu
Copy link
Contributor

Looks like this is the issue in go-yaml go-yaml/yaml#387. kustomize depends on go-yaml v2 indirect through sigs.k8s.io/yaml v1.2.0. So we need to wait for sigs.k8s.io/yaml to update.

@cadiegoc
Copy link

Hi,
My team is having the same issue with Kustomize and long lines being broken.
I can see that Kustomize v4.2.0 was released 25 days ago. Do you know if that version fixes it?
If not, do you know if the next version (v4.3.0?) will fix it? If it does, do you know the expected date for the new release?
Thank you in advance!

@tobernguyen
Copy link

I'm experiencing this in kustomize v3.9.4 too which is the version ArgoCD v2.0.4 is using.

@grzesrap
Copy link

grzesrap commented Sep 7, 2021

I have this issues with helm:
Input kustomize.yaml

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

namespace: monitor-system

resources:
  - ../../base
  - 00-pvc.yml

helmCharts:
- name: prometheus
  repo: https://myhelmrel/virthelm-monit
  releaseName: prometheus
  valuesInline:
    server:
      sidecarContainers:
        thanos-sidecar:
          image: quay.io/thanos/thanos:v0.22.0
          args:
            - sidecar
            - --log.level=info
            - --tsdb.path=/data/
            - --prometheus.url=http://127.0.0.1:9090
            - '--objstore.config={type: S3, config: {bucket: thanos, endpoint: s3.bucket, insecure: true, access_key: aa, secret_key: bb}}'

and output in deployment (broken lines)

      - args:
        - sidecar
        - --log.level=info
        - --tsdb.path=/data/
        - --prometheus.url=http://127.0.0.1:9090
        - '--objstore.config={type: S3, config: {bucket: thanos, endpoint: s3.bucket,
          insecure: true, access_key: aa, secret_key: bb}}'

This bug still exists in version v4.3.0. Is this work scheduled for the next release? Is there any way to fix this bug now?

@natasha41575
Copy link
Contributor

This bug still exists in version v4.3.0. Is this work scheduled for the next release? Is there any way to fix this bug now?

The bug is likely in the go-yaml libraries that we use. Kustomize does not have the resources to address this bug at the moment, but anyone is welcome to investigate and submit a PR to either kustomize or go-yaml for review.

@hancockjt
Copy link

This seems to happen for any string longer than 80 characters in your patches. I'm surprised it's not getting a lot more attention. It's a pretty huge limitation. I'm having to resort to using python yaml parsing to handle these operations.

@zhouhaibing089
Copy link
Contributor

zhouhaibing089 commented Oct 4, 2021

If I understand correctly, kustomize needs to bump kubernetes-sigs/yaml to v1.3.0?

#4222 is submitted for review.

@cadiegoc
Copy link

Does anyone know if this issue has been fixed?

alamont added a commit to orikami-nl/pulsar-helm-chart that referenced this issue Dec 16, 2021
@karlschriek
Copy link

karlschriek commented Feb 3, 2022

Bumping sigs.k8s.io/yaml to v1.3.0 doesn't seem to fix it (at least not by bumping it on v4.3.0, I cannot speak for later releases).

@karlschriek
Copy link

Actually, interesting, I discovered that infinite lines was brought in with go-yaml v2.30 (https://github.com/go-yaml/yaml/releases/tag/v2.3.0) but was then reverted again (apparently largely because the Kubernetes community wanted it) in v2.40 (https://github.com/go-yaml/yaml/releases/tag/v2.4.0)

kubernetes-sigs/yaml v1.3.0 bumps go-yaml to from v2.2.8 to v2.4.0.

I will investigate with v2.3.0, but I am guessing it will have some surprises as well if folks wanted it reverted.

@karlschriek
Copy link

karlschriek commented Feb 3, 2022

A few things I learnt: kustomize doesn't just use sigs.k8s.io/yaml but also uses go-yaml directly in several places.

There is also a forked go-yaml that is sometimes used.

Anyway, I tried setting all direct references to 2.3.0 and also created a forked sigs.k8s.io/yaml that I set to use 2.3.0 as well. Also had to fork apimachinery since it was also pegged to 2.4.0.

None of this appears to have had any effect. Long lines are still broken up, so either I am missing a reference somewhere, or the solution requires additional code changes that I have no idea where to implement.

Could anyone from the core kustomize team comment?

@karlschriek
Copy link

karlschriek commented Feb 3, 2022

Ok so I finally figured it out.

The direct references to go-yaml don't play any role whatsoever. sigs.k8s.io/yaml is the key. This needs to be at version 1.3.0 so that go-yaml is at 2.4.0 (not 2.3.0, that isn't actually needed).

This right here is where the magic happens: https://github.com/kubernetes-sigs/yaml/blob/v1.3.0/yaml.go#L82.

To have this work without line breaks you just need to call yaml.FutureLineWrap() (i.e. this right here https://github.com/go-yaml/yaml/blob/v2.4.0/yaml.go#L476) somewhere (for example within the JSONtoYaml function or as an init when calling kustomize).

If it really is as simple as this, could we get this as command line arg?

@cprivitere
Copy link
Member

For the record, kustomize 4.5.2 still exhibits this behavior.

@cprivitere
Copy link
Member

@natasha41575 Given that 4.5.2 doesn't fix it and what @karlschriek has found, do you have any further ideas about what needs to happen here?

@karlschriek
Copy link

Let me also link this PR, which would actually solve the issue #4222 (comment), although I doubt whether it will be merged in its current form. The consensus from the Kubernetes community seems at the moment to be that the line-wrapping should be kept, since changing it would causes several tests to break. For us it would be sufficient to have kustomize "init" with yaml.FutureLineWrap() based on some flag, e.g. kustomize build --future-line-wrap .

@KnVerey
Copy link
Contributor

KnVerey commented Apr 1, 2022

/close
/kind duplicate

Duplicate of #947, which is older / has more history. Please watch that one intead.

@KnVerey KnVerey closed this as completed Apr 1, 2022
@k8s-ci-robot
Copy link
Contributor

@KnVerey: The label(s) kind/duplicate cannot be applied, because the repository doesn't have them.

In response to this:

/close
/kind duplicate

Duplicate of #947, which is older / has more history. Please watch that one intead.

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/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests