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

tk export produces wrapped lines #607

Open
pharaujo opened this issue Sep 9, 2021 · 15 comments
Open

tk export produces wrapped lines #607

pharaujo opened this issue Sep 9, 2021 · 15 comments

Comments

@pharaujo
Copy link

pharaujo commented Sep 9, 2021

tk export sometimes produces invalid Kubernetes manifests due to wrapping of long lines. Most fields where long lines might exist are resistant to \n in their values, but some fields are not.

This is a sample file where we can see the line wrapping occurring:

// environments/default/main.jsonnet
{
  service: {
    apiVersion: 'v1',
    kind: 'Service',
    metadata: {
      annotations: {
        k: 'aaaaa bbbbb ccccc ddddd eeeee fffff ggggg hhhhh iiiii jjjjj kkkkk mmmmm nnnnn',
      },
      labels: {
        name: 'grafana',
      },
      name: 'grafana',
    },
    spec: {
      ports: [{
        name: 'grafana-ui',
        port: 3000,
        targetPort: 3000,
      }],
      selector: { name: 'grafana' },
      type: 'NodePort',
    },
  },
}

tk export output environments/default/main.jsonnet produces the following (notice the k annotation):

apiVersion: v1
 kind: Service
 metadata:
     annotations:
         k: aaaaa bbbbb ccccc ddddd eeeee fffff ggggg hhhhh iiiii jjjjj kkkkk mmmmm
             nnnnn
     labels:
         name: grafana
     name: grafana
     namespace: default
 spec:
     ports:
       - name: grafana-ui
         port: 3000
         targetPort: 3000
     selector:
         name: grafana
     type: NodePort

In our particular use-case, we're using tanka to generate helm charts, and so some values are actually Helm templating, e.g.:

{
  spec+: {
    template+: {
      metadata+: {
        annotations+: {
          local tplname(obj) = '%s.%s-%s' % [obj.apiVersion, obj.kind, obj.metadata.name],
          'checksum/grafana-config': '{{ include (print $.Template.BasePath "/generated/%s.yaml") . | sha256sum }}' % tplname($.grafana.config),
        },
      },
    },
  },
}

The output of this snippet makes helm templating fail because it breaks the {{ }} markers into different lines.

This has been a known surprising behavior of go-yaml.v2 for quite some time, so much so that v3 changed to not wrap long lines by default.

I tried this patch locally and it fixes the issue, but I don't know if it will have unintended side-effects elsewhere:

diff --git a/pkg/kubernetes/manifest/manifest.go b/pkg/kubernetes/manifest/manifest.go
index 51f577d..55a6102 100644
--- a/pkg/kubernetes/manifest/manifest.go
+++ b/pkg/kubernetes/manifest/manifest.go
@@ -9,7 +9,7 @@ import (
        "github.com/Masterminds/sprig/v3"
        "github.com/pkg/errors"
        "github.com/stretchr/objx"
-       yaml "gopkg.in/yaml.v2"
+       yaml "gopkg.in/yaml.v3"
 )

 // Manifest represents a Kubernetes API object. The fields `apiVersion` and

Another option would be to use yaml.v2's FutureLineWrap() to disable line wrapping, if you're not in a place where you can migrate to v3.

@Duologic
Copy link
Member

I can remember a few discussions about yaml.v2/v3 but I can't recall what implications that may have. @sh0rez might have more context.

@pharaujo
Copy link
Author

Hey, it's been a month. I'm happy to get a PR going to fix this particular issue, let me know if you have any opinions on the approach to take.

@julienduchesne
Copy link
Member

To be noted that this causes a massive amount of indent changes. The kustomize Kubernetes team has the same issue with go-yaml. They have their own fork that allows to customize the indents (reducing the migration impact). This issue has been updated semi-recently so it might be good to see what comes of it: go-yaml/yaml#720 before forcing a massive YAML update on users. Also, it does seem like kubectl still imports v2.4.0 🤔

@stale
Copy link

stale bot commented Jan 9, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 9, 2022
@pharaujo
Copy link
Author

Will you consider using the aforementioned FutureLineWrap() in manifest.go to enable v3's line wrapping while v2/v3 migration discussion is ongoing?

@stale stale bot removed the stale label Jan 25, 2022
@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@xvzf
Copy link
Contributor

xvzf commented Jul 14, 2023

Hey folks 👋
Is there a way to reconsider moving to V3? Semantically, it should not make a difference so tools like FluxCD/ArgoCD/... won't apply any changes to target environments

@Duologic
Copy link
Member

Perhaps it would be more beneficial to poke upstream to fix the issue?

@zerok
Copy link
Contributor

zerok commented May 27, 2024

Tanka is now on v3 and I can no longer reproduce this issue 🙂

@zerok zerok closed this as completed May 27, 2024
@zc-devs
Copy link

zc-devs commented Jul 10, 2024

> tk --version
v0.27.1

Tanka's definition

local keycloakContainer =
  core.container.new('keycloak', 'quay.io/keycloak/keycloak')\
  + core.container.withArgs('start aaaaa bbbbb ccccc ddddd eeeee fffff ggggg hhhhh iiiii jjjjj kkkkk mmmmm nnnnn')

produces

    spec:
      containers:
      - args:
        - start aaaaa bbbbb ccccc ddddd eeeee fffff ggggg hhhhh iiiii jjjjj kkkkk
          mmmmm nnnnn
        image: quay.io/keycloak/keycloak

Reproduced on other resources as well.

@zerok
Copy link
Contributor

zerok commented Jul 11, 2024

You are absolutely right ... now I wonder why I couldn't reproduce it before.

@zerok zerok reopened this Jul 11, 2024
@zerok
Copy link
Contributor

zerok commented Jul 11, 2024

Looks like that's a problem with gopkg.in/yaml.v2 v2.4.0. It was "fixed" before but since it introduces a behavioral change, it got reverted and is now only fixed in v3 🙁

go-yaml/yaml#571

Unfortunately that's also consistent with how sigs.k8s.io/yaml marshals things.

@zerok
Copy link
Contributor

zerok commented Jul 11, 2024

Maybe we will only be able to fix that once we are able to upgrade to yaml.v3 everywhere. Let's put it this way: This has been on our wishlist for a while...

@zc-devs
Copy link

zc-devs commented Jul 11, 2024

I open exported yamls in Visual Studio Code. And TBH, example with container didn't show errors.

Reproduced on other resources as well

But I was afraid of my Traefik Ingress Route:

k8s.traefik.v1alpha1.traefik.ingressRouteRule.new('Host(`%s`) && (PathPrefix(`/realms`,`/resources`,`/js`) || Path(`/robots.txt`))' % conf.hostname)

which produces

  - kind: Rule
    match: Host(`keycloak.example.com`) && (PathPrefix(`/realms`,`/resources`,`/js`)
      || Path(`/robots.txt`))

which in VS Code looks like
Screenshot 1

that's also consistent with how sigs.k8s.io/yaml marshals things

Maybe because of that, but Kubernetes took that yaml without complains and fixed it a little (added >-):

    - kind: Rule
      match: >-
        Host(`keycloak.example.com`) && (PathPrefix(`/realms`,`/resources`,`/js`)
        || Path(`/robots.txt`))

In the end this is not urgent issue to me. For now at least.

Most fields where long lines might exist are resistant to \n in their values, but some fields are not

upgrade to yaml.v3 everywhere

That would be great. Thank you.

@zerok
Copy link
Contributor

zerok commented Jul 12, 2024

Ahhhhh, looks like in YAML linebreaks in list-item values are ignored... So your initial example output

    spec:
      containers:
      - args:
        - start aaaaa bbbbb ccccc ddddd eeeee fffff ggggg hhhhh iiiii jjjjj kkkkk
          mmmmm nnnnn
        image: quay.io/keycloak/keycloak

is valid and the arg contains everything including nnnnn. I then also tried it with yq and putting the string into an object property. This also produced the line-wrapping but yq was again able to parse it 😐

As far as I can tell, as long as you are not trying to parse the produced YAML with something that does not support this language "feature", then you should be good to go also in your Traefik example 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

7 participants