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

kyaml panics on duplicate keys #3480

Closed
stefanprodan opened this issue Jan 18, 2021 · 13 comments · Fixed by #3673
Closed

kyaml panics on duplicate keys #3480

stefanprodan opened this issue Jan 18, 2021 · 13 comments · Fixed by #3673
Assignees
Labels
area/kyaml issues for kyaml kind/bug Categorizes issue or PR as related to a bug. kind/feature Categorizes issue or PR as related to a new feature.

Comments

@stefanprodan
Copy link

In Flux v2 we decided to use kustomize libraries in our controllers and we make extensive use of kyaml. We found that if a manifest contains duplicate keys, kyaml panics taking down the whole controller.

Can we please not panic and instead return an error from kyaml containing the manifest name so that both Kustomize and Flux users can track down which object/manifest caused this? Without a hint to the invalid manifest, there is no way for users that have hundreds of manifests in a single kustomization to debug such errors.

Example:

kustomization.yaml

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- resources.yaml

resources.yaml

apiVersion: apps/v1
kind: Deployment
metadata:
  name: podinfo
spec:
  selector:
    matchLabels:
      app: podinfo
  template:
      labels:
        app: podinfo
    spec:
      containers:
      - name: podinfod
        image: ghcr.io/stefanprodan/podinfo:5.0.3 
        command:
        - ./podinfo
        env:
        - name: PODINFO_UI_COLOR
          value: "#34577c"
        env:
          - name: PODINFO_UI_COLOR
            value: "#34577c"

Actual output

 $ kustomize build --enable_kyaml=true
2021/01/18 13:21:50 failed to decode ynode: yaml: unmarshal errors:
  line 50: mapping key "env" already defined at line 47

Kustomize version

$ kustomize version
{Version:kustomize/v3.9.2 GitCommit: BuildDate:2021-01-17T19:01:12+00:00 GoOs:darwin GoArch:amd64}
module github.com/fluxcd/kustomize-controller

require (
	sigs.k8s.io/kustomize/api v0.7.2
)
module github.com/fluxcd/image-automation-controller

// Force downgrade to version used by kyaml.
replace gopkg.in/yaml.v3 => gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c

require (
	sigs.k8s.io/kustomize/kyaml v0.10.5
)
@monopole monopole self-assigned this Jan 19, 2021
@Shell32-Natsu Shell32-Natsu added area/kyaml issues for kyaml kind/feature Categorizes issue or PR as related to a new feature. labels Jan 19, 2021
@pwittrock pwittrock added the kind/bug Categorizes issue or PR as related to a bug. label Jan 27, 2021
@pwittrock
Copy link
Contributor

These are the lines that need to be changed:

func (rn *RNode) Map() map[string]interface{} {

log.Fatalf("failed to decode ynode: %v", err)

@SyamSundarKirubakaran would you like to pick this up? We should also look for anywhere else we log Fatal

@pwittrock
Copy link
Contributor

pwittrock commented Jan 27, 2021

// Map ...
// Use MapOrDie to log.Fatal on error
func (rn *RNode) Map() (map[string]interface{}, error) {
  ...
  return fmt.Errorf(...)
}

func  (rn *RNode) MapOrDie() map[string]interface{} {
  m, err := rn.Map()
  if err != nil {
    log.Fatal(err)
  }
  return m
}

@pwittrock
Copy link
Contributor

Also: go-yaml/yaml#623

@natasha41575
Copy link
Contributor

This is fixed and will be available in the next release

@trynity
Copy link

trynity commented Apr 13, 2021

When will the next release be shipped? We're currently running into this issue where an upstream chart we use has duplicate keys and hasn't fixed it yet, and kustomize is crashing when we're trying to alter things.

@natasha41575
Copy link
Contributor

@trynity what version of kustomize are you using?

@trynity
Copy link

trynity commented Apr 14, 2021

@natasha41575 4.0.5

@natasha41575
Copy link
Contributor

@trynity I would expect 4.0.5 to have this issue fixed (there is a regression test here)

Would you be able to provide a set of files that can reproduce the issue you are having?

Kustomize 4.1.0 was also released yesterday and you can try it.

@bhagyesh18
Copy link

@natasha41575 is there any way the kustomize tool can ignore the duplicate keys? Because I have written a script to patch yamls from helm charts which has duplicate keys, and because of that my script breaks every single time.

Also, version v4.0.5 can show the user where is the error when there are hundreds of manifest as resources.

The latest Kustomize version does not show the user where is the error when there are hundreds of manifest as resources.

@natasha41575
Copy link
Contributor

@bhagyesh18 We can improve the error to show where it is.

I'm not sure that we can ignore the duplicate keys entirely. The error comes from an imported yaml library. I will do some investigation but I'm not sure that we can work around it.

@haim-ari
Copy link

  • Flux v0.16.1
  • kustomize (local) v4.2.0

Flux complains with the following error:

kustomize build failed: map[string]interface (nil): yaml: unmarshal errors: line 11: mapping key "interval" already defined at line 7

In the project there is no indication of such issue in any directory/file

For example if I create such duplication, I can see the error:

Screen Shot 2021-07-14 at 8 14 45

When checking with validate script, there is no indication of such issue.

validate.txt(sh script)

@kingdonb
Copy link

I also have a CustomResourceDefinitions file that doesn't appear to have duplicate keys, but kustomize build fails on it.

This looks like a spurious error and I believe there is still a bug here, though I can't offer much more help than that, I do have a repo which reproduces it:

https://github.com/kingdonb/bb-csh-flux

Running kustomize build . in the tenants/mixed-arch-k3s/okteto-enterprise directory:

okteto-enterprise (clean-up-yaml)$ kustomize build .
2021/07/18 09:22:56 failed to decode ynode: yaml: unmarshal errors:
  line 44: mapping key "jsonPath" already defined at line 40
  line 45: mapping key "name" already defined at line 41
  line 46: mapping key "type" already defined at line 43
  line 684: mapping key "jsonPath" already defined at line 680
  line 685: mapping key "name" already defined at line 681
  line 686: mapping key "type" already defined at line 683
  line 1324: mapping key "jsonPath" already defined at line 1320
  line 1325: mapping key "name" already defined at line 1321
  line 1326: mapping key "type" already defined at line 1323
  line 1965: mapping key "jsonPath" already defined at line 1961
  line 1966: mapping key "name" already defined at line 1962
  line 1967: mapping key "type" already defined at line 1964

The CRDs file is from https://charts.okteto.com/crds.yaml (which comes from the instructions at https://okteto.com/docs/enterprise/install/deployment/ )

An example of the section that the parser is tripping over, (around line 40-46):

https://github.com/kingdonb/bb-csh-flux/blob/70863415d0b72ff64a1169f075da6de4afd3cbde/tenants/mixed-arch-k3s/okteto-enterprise/crds.yaml#L35-L55

These don't look like duplicate hash keys to me, they are just neighboring mappings in the additionalPrinterColumns.

(Let me know if I should open a separate issue for this!)

@kingdonb
Copy link

kingdonb commented Jul 18, 2021

Beg pardon, I was still using an old Kustomize 3.9.3

The error persists and is very similar in Kustomize 4.2.0 though:

$ kustomize build .
Error: map[string]interface {}(nil): yaml: unmarshal errors:
  line 44: mapping key "jsonPath" already defined at line 40
  line 45: mapping key "name" already defined at line 41
  line 46: mapping key "type" already defined at line 43
  line 684: mapping key "jsonPath" already defined at line 680
  line 685: mapping key "name" already defined at line 681
  line 686: mapping key "type" already defined at line 683
  line 1324: mapping key "jsonPath" already defined at line 1320
  line 1325: mapping key "name" already defined at line 1321
  line 1326: mapping key "type" already defined at line 1323
  line 1965: mapping key "jsonPath" already defined at line 1961
  line 1966: mapping key "name" already defined at line 1962
  line 1967: mapping key "type" already defined at line 1964

For reference, this is a common CRD from cert-manager:

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: certificaterequests.cert-manager.io
  annotations:
    cert-manager.io/inject-ca-from-secret: 'okteto/cert-manager-webhook-ca'
  labels:
    app: 'cert-manager'
    app.kubernetes.io/name: 'cert-manager'
    app.kubernetes.io/instance: 'cert-manager'
    app.kubernetes.io/managed-by: 'Helm'
    helm.sh/chart: 'cert-manager-v1.1.0'
...

If I swap out the CRD for cert-manager 1.4.0 CRDs, I no longer get this error. It still seems like a bug, but maybe I don't know how to read CRD and it really is a duplicate key based on some logic I don't understand. (Still based on @haim-ari report and my own experience, I think that a bug remains here.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kyaml issues for kyaml kind/bug Categorizes issue or PR as related to a bug. kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants