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

kapp has yaml Norway issues #967

Open
Red-M opened this issue Jun 14, 2024 · 7 comments
Open

kapp has yaml Norway issues #967

Red-M opened this issue Jun 14, 2024 · 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

Comments

@Red-M
Copy link

Red-M commented Jun 14, 2024

What steps did you take:

  1. Along the lines of helm template | kapp deploy -f -
  2. kapp errors out

What happened:
Helm shows the labels in the (I'm using a HPA as an example) manifests as:

apiVersion: autoscaling/v2
kind: HorizontalPodAutoscaler
metadata:
  name: "thing1"
  namespace: "default"
  labels:
    app: "thing1"
    job_id: "42"
    other_label: "test"

However kapp's diff shows:

@@ update horizontalpodautoscaler/thing1 (autoscaling/v2) namespace: default @@
  ...
  9,  9       app: thing1
 10     -     job_id: "42"
     10 +     job_id: 42
     11 +     kapp.k14s.io/app: "1718329494750920866"
     12 +     kapp.k14s.io/association: v1.28f428fa5fef94f7deadd11917f4bd7d
 13, 13       other_label: test

Which produces this error from kapp:

2:08:17AM: ---- applying 3 changes [0/3 done] ----
2:08:17AM: update horizontalpodautoscaler/thing1 (autoscaling/v2) namespace: default
kapp: Error: update horizontalpodautoscaler/thing1 (autoscaling/v2) namespace: default:
  Updating resource horizontalpodautoscaler/thing1 (autoscaling/v2) namespace: default:
    API server says:
      HorizontalPodAutoscaler in version "v2" cannot be handled as a HorizontalPodAutoscaler: json: cannot unmarshal number into Go struct field ObjectMeta.metadata.labels of type string (reason: BadRequest)

What did you expect:
The deployment to succeed and no errors from kapp.

Anything else you would like to add:
While deploying an existing helm chart by passing the manifests from helm to kapp, I found that when kapp went to make ownership changes on ANY resource in the chart, kapp rewrote the entire stanza of yaml (in this case, the metadata.labels section) and removed some very critical " characters, which turned a string into an integer but that integer is in metadata.labels which is against kubernetes manifest specifications. This could also cause issues in other parts of manifests which take a string but have the value of said string set to a value that can be converted to an integer, thus making the yaml manifest have an integer instead of the expected (and requested! (because of the explicit usage of "!!)) string based value.

I'm passing kapp manifests from helm because the environment I'm working in already has a large existing base of helm charts deployed (and moving to something like ytt is not feasible due to time and risk) but helm as a deployment tool is painful for automated deployments from CICD pipelines, which is why I want to move to using kapp over helm as a deployment tool.

Example HPA manifest:

apiVersion: autoscaling/v2
kind: HorizontalPodAutoscaler
metadata:
  name: "thing"
  namespace: "default"
  labels:
    app: "thing1"
    job_id: "42"
    other_label: "test"
spec:
  scaleTargetRef:
    apiVersion: apps/v1
    kind: Deployment
    name: "thing1"
  minReplicas: 2
  maxReplicas: 6
  metrics:
    - type: Resource
      resource:
        name: "cpu"
        target:
          type: Utilization
          averageUtilization: 50
    - type: Resource
      resource:
        name: "memory"
        target:
          type: Utilization
          averageUtilization: 50
  behavior:
    scaleDown:
      stabilizationWindowSeconds: 30

Environment:

  • kapp version: 0.62.1
  • OS: Alpine/Debian
  • Kubernetes version: v1.29.4-eks-036c24b
@Red-M Red-M added bug This issue describes a defect or unexpected behavior carvel triage This issue has not yet been reviewed for validity labels Jun 14, 2024
@Red-M
Copy link
Author

Red-M commented Jun 14, 2024

I've also just noticed that kapp did the same thing in the annotations of a service which has AWS LBC annotations:

@@ update service/thing1 (v1) namespace: default @@
  ...
  6,  6       alb.ingress.kubernetes.io/healthcheck-protocol: HTTP
  7     -     alb.ingress.kubernetes.io/success-codes: "200"
      7 +     alb.ingress.kubernetes.io/success-codes: 200

@100mik
Copy link
Contributor

100mik commented Jun 14, 2024

Could you share an example template we can reproduce this with?

@100mik
Copy link
Contributor

100mik commented Jun 14, 2024

However, my best bet is that running helm install introduces some sort of handling for these cases.
It would be interesting to see if the output of helm template has the quotes as well, I would believe it won't as kapp amends the YAML but does not manipulate existing fields.

@Red-M
Copy link
Author

Red-M commented Jun 14, 2024

Could you share an example template we can reproduce this with?

The output I provided for the example manifest in the OP is what is being passed to kapp, there is nothing that helm is doing other than rendering the template and passing that output to kapp. So no template is required.

However, my best bet is that running helm install introduces some sort of handling for these cases. It would be interesting to see if the output of helm template has the quotes as well, I would believe it won't as kapp amends the YAML but does not manipulate existing fields.

Nope, helm template is showing quotes and while the chart was originally installed with helm, all of this output is from kapp, kapp wanted to make these fields into integers, even though the manifest provided is asking for a string.

@Red-M
Copy link
Author

Red-M commented Jun 18, 2024

I found it, the issue is upstream: kubernetes-sigs/yaml#108

its in this call

@renuy renuy moved this to To Triage in Carvel Jul 12, 2024
@renuy renuy added carvel accepted This issue should be considered for future work and that the triage process has been completed and removed carvel triage This issue has not yet been reviewed for validity labels Jul 12, 2024
@renuy renuy moved this from To Triage to Prioritized Backlog in Carvel Jul 24, 2024
@ringerc
Copy link

ringerc commented Nov 27, 2024

For anyone else reading this, the missing detail in this ticket seems to be that there's a template variable being substituted in these manifests.

kapp's yaml exporter sees that "${template_variable}" can be represented legally in yaml as bare ${template_variable} so it omits the quotes when outputting the yaml.

Then helm substitutes ${template_variable} for 42.

Both components are individually behaving correctly, but the net result is incorrect, because "${template_variable}" got substituted to 42 not "42".

The same behaviour can be observed when using kustomize to generate the yaml manifests, it is not unique to kapp. IMO this is mostly a problem arising from (mis)using string templating instead of yaml aware semantic templating, but to a large degree we're all stuck with that for various reasons including lots of structured format documents nested as strings in other structured format documents.

@Red-M
Copy link
Author

Red-M commented Nov 27, 2024

For anyone else reading this, the missing detail in this ticket seems to be that there's a template variable being substituted in these manifests.

kapp's yaml exporter sees that "${template_variable}" can be represented legally in yaml as bare ${template_variable} so it omits the quotes when outputting the yaml.

Then helm substitutes ${template_variable} for 42.

Both components are individually behaving correctly, but the net result is incorrect, because "${template_variable}" got substituted to 42 not "42".

Please read the reproducible example, I'm not using kapp's helm, I literally passed it raw manifests.

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
Projects
Status: Prioritized Backlog
Development

No branches or pull requests

4 participants