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

kbld does not fully respect Block Chomping Indicator in manifest #178

Closed
mkurde opened this issue Oct 12, 2021 · 7 comments
Closed

kbld does not fully respect Block Chomping Indicator in manifest #178

mkurde opened this issue Oct 12, 2021 · 7 comments
Labels
bug This issue describes a defect or unexpected behavior carvel accepted This issue should be considered for future work and that the triage process has been completed priority/important-soon Must be staffed and worked on currently or soon.

Comments

@mkurde
Copy link

mkurde commented Oct 12, 2021

What steps did you take:

kbld is the last step after rendering all my manifests in my deploy automation. Therefor, also manifests which do not have docker images are going through this process. I am working on Prometheus Alerting manifests and here is an example:

apiVersion: 'monitoring.coreos.com/v1'
kind: 'PrometheusRule'
metadata:
  name: 'alert-kube-state-container'
  namespace: 'monitoring'
  labels:
    prometheus.monitoring: 'enabled'
    helm.sh/chart: 'alerts-1.0.0'
spec:
  groups:
    - name: 'KubeStateContainer'
      rules:
        - alert: 'Container unready'
          expr: >-
            kube_pod_container_status_ready < 1
            and on (pod, container)
            kube_pod_container_status_terminated_reason{reason="Completed"} == 0
          for: '15m'
          labels:
            severity: 'warning'
            service: 'kube-state-container'
          annotations:
            summary: 'Container(s) unready'
            description: |
              Container `{{ $labels.container }}` has been "Unready" for
              more than 15 minutes.
              Pod: `{{ $labels.pod }}`.
              Check container status and logs.

and the result after kbld:

apiVersion: monitoring.coreos.com/v1
kind: PrometheusRule
metadata:
  labels:
    helm.sh/chart: alerts-1.0.0
    prometheus.monitoring: enabled
  name: alert-kube-state-container
  namespace: monitoring
spec:
  groups:
  - name: KubeStateContainer
    rules:
    - alert: Container unready
      annotations:
        description: |
          Container `{{ $labels.container }}` has been "Unready" for
          more than 15 minutes.
          Pod: `{{ $labels.pod }}`.
          Check container status and logs.
        runbook_url: ""
        summary: Container(s) unready
      expr: kube_pod_container_status_ready < 1 and on (pod, container) kube_pod_container_status_terminated_reason{reason="Completed"}
        == 0
      for: 15m
      labels:
        service: kube-state-container
        severity: warning

The result is the same when the expression is in one line like

          expr: 'kube_pod_container_status_ready < 1 and on (pod, container) kube_pod_container_status_terminated_reason{reason="Completed"} == 0'

What happened:

Next to changes like order and indentation (which is still valid yaml) you see the expr: line. kbld converts the Block Chomping Indicator >- into : and formats the expression into one line except the last chars == 5 which are wrapped into the next line. This breaks yaml.

As you can see, the description with the Block Chomping Indicator | is rendered perfectly fine. I can not use | since it keeps the line breaks.

>, |: "clip": keep the line feed, remove the trailing blank lines.
>-, |-: "strip": remove the line feed, remove the trailing blank lines.

Source: Stackoverflow

What did you expect:

I expect to get valid yaml out of kbld when I use any Block Chomping Indicator like >-. Either the given syntax is preserved or the expression is not wrapped and : is returned.

I won't be bother if the result is a still valid yaml since the original version with the >- syntax is purely used to improve the readability.

Anything else you would like to add:

n/a

Environment:

  • kbld version (use kbld --version):
    • k14s/tap/kbld: stable v0.27.0
    • 0.31.0 brew install vmware-tanzu/carvel/kbld
  • OS (e.g. from /etc/os-release): macOS

Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

@mkurde mkurde added bug This issue describes a defect or unexpected behavior carvel triage This issue has not yet been reviewed for validity labels Oct 12, 2021
@dsiebel
Copy link
Contributor

dsiebel commented Oct 12, 2021

I can confirm that this seems to be a regression that was introduced between 0.30.0 and 0.31.0. I rendered the above manifests using 0.30.0 without the mentioned issue, then upgraded to 0.31.0 and was able to reproduce it.

@dsiebel
Copy link
Contributor

dsiebel commented Oct 12, 2021

Looks like this might be an issue with go-yaml which was update from 2.3.0 to 2.4.0 in kbld 0.31.0.
v2.3.0...v2.4.0#diff-18f184a5d0

EDIT:
It looks like it can be controlled with this

// The API does not allow this process to be reversed as it's intended
// for migration only. v3 drops this method and instead offers more
// control on a per encoding basis.
yaml.FutureLineWrap()

@aaronshurley
Copy link
Contributor

@mkurde Thank you for reporting this bug and @dsiebel thank you for digging into this. I was able to replicate this with the provided information. We did have quite a few dependency bumps in the 0.31.0 release which likely led to this regression. I'll mark this as important-soon since it's a regression. Due to KubeCon this week, it might be unlikely that we get to this until next week or so.

We'd be open to a PR if either of you would like to dig into this, just let us know if you run into any issues/questions.

@aaronshurley aaronshurley added carvel accepted This issue should be considered for future work and that the triage process has been completed priority/important-soon Must be staffed and worked on currently or soon. and removed carvel triage This issue has not yet been reviewed for validity labels Oct 12, 2021
@cppforlife
Copy link
Contributor

...formats the expression into one line except the last chars == 5 which are wrapped into the next line. This breaks yaml....
I won't be bother if the result is a still valid yaml since the original version with the >- syntax is purely used to improve the readability.

im not sure i fully groked the issue. result is a valid yaml, though line folded (as example shows below ytt for example using a yaml library reads that yaml).

is there particular yaml parser that does not understand that line folding?

example:

% pbpaste|kbld -f-
---
apiVersion: monitoring.coreos.com/v1
kind: PrometheusRule
metadata:
  labels:
    helm.sh/chart: alerts-1.0.0
    prometheus.monitoring: enabled
  name: alert-kube-state-container
  namespace: monitoring
spec:
  groups:
  - name: KubeStateContainer
    rules:
    - alert: Container unready
      annotations:
        description: |
          Container `{{ $labels.container }}` has been "Unready" for
          more than 15 minutes.
          Pod: `{{ $labels.pod }}`.
          Check container status and logs.
        summary: Container(s) unready
      expr: kube_pod_container_status_ready < 1 and on (pod, container) kube_pod_container_status_terminated_reason{reason="Completed"}
        == 0
      for: 15m
      labels:
        service: kube-state-container
        severity: warning
% pbpaste|kbld -f-|ytt -f-
apiVersion: monitoring.coreos.com/v1
kind: PrometheusRule
metadata:
  labels:
    helm.sh/chart: alerts-1.0.0
    prometheus.monitoring: enabled
  name: alert-kube-state-container
  namespace: monitoring
spec:
  groups:
  - name: KubeStateContainer
    rules:
    - alert: Container unready
      annotations:
        description: |
          Container `{{ $labels.container }}` has been "Unready" for
          more than 15 minutes.
          Pod: `{{ $labels.pod }}`.
          Check container status and logs.
        summary: Container(s) unready
      expr: kube_pod_container_status_ready < 1 and on (pod, container) kube_pod_container_status_terminated_reason{reason="Completed"} == 0
      for: 15m
      labels:
        service: kube-state-container
        severity: warning

@aaronshurley
Copy link
Contributor

@mkurde @dsiebel ^

@mkurde
Copy link
Author

mkurde commented Oct 14, 2021

hi @cppforlife, after some more work on our charts we discovered that the mentioned problem is not affecting kubernetes. The IDE still complains due to the wrapped line but kubernetes allow to apply the manifests.

We can close the issue here. If that wrapping will become a problem in the future, I will reopen the issue.

@mkurde mkurde closed this as completed Oct 14, 2021
@mkurde
Copy link
Author

mkurde commented Oct 14, 2021

To add some additional notes:

@github-actions github-actions bot added the carvel triage This issue has not yet been reviewed for validity label Oct 14, 2021
@aaronshurley aaronshurley removed the carvel triage This issue has not yet been reviewed for validity label Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue describes a defect or unexpected behavior carvel accepted This issue should be considered for future work and that the triage process has been completed priority/important-soon Must be staffed and worked on currently or soon.
Projects
None yet
Development

No branches or pull requests

4 participants