grafana / tanka

Flexible, reusable and concise configuration for Kubernetes
https://tanka.dev
Apache License 2.0
2.32k stars 165 forks source link

`tk export` produces wrapped lines #607

Open pharaujo opened 2 years ago

pharaujo commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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: https://github.com/go-yaml/yaml/issues/720 before forcing a massive YAML update on users. Also, it does seem like kubectl still imports v2.4.0 🤔

stale[bot] commented 2 years ago

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.

pharaujo commented 2 years ago

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

stale[bot] commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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

zerok commented 3 months ago

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

zc-devs commented 1 month ago
> 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 commented 1 month ago

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

zerok commented 1 month ago

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 🙁

https://github.com/go-yaml/yaml/pull/571

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

zerok commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 🙂