ytt silently skips yaml merge key

[zhouhaibing089]

What steps did you take:

merge key is described here, however ytt silently ignores those special syntax.

What happened:

I have a yaml like belowfoo.yaml:

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: clustertasks.tekton.dev
  labels:
    app.kubernetes.io/instance: default
    app.kubernetes.io/part-of: tekton-pipelines
    pipeline.tekton.dev/release: "v0.22.0"
    version: "v0.22.0"
spec:
  group: tekton.dev
  preserveUnknownFields: false
  versions:
    - &version
      name: v1alpha1
      served: true
      storage: false
      schema:
        openAPIV3Schema:
          type: object
          x-kubernetes-preserve-unknown-fields: true
      subresources:
        status: {}
    - !!merge <<: *version
      name: v1beta1
      storage: true
  names:
    kind: ClusterTask
    plural: clustertasks
    categories:
      - tekton
      - tekton-pipelines
  scope: Cluster
  conversion:
    strategy: Webhook
    webhook:
      conversionReviewVersions: ["v1beta1"]
      clientConfig:
        service:
          name: tekton-pipelines-webhook
          namespace: tekton-pipelines

You can see &version and !!merge <<: *version. Then I run ytt -f foo.yaml, and no error reported, but the result YAML is incorrect:

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: clustertasks.tekton.dev
  labels:
    app.kubernetes.io/instance: default
    app.kubernetes.io/part-of: tekton-pipelines
    pipeline.tekton.dev/release: v0.22.0
    version: v0.22.0
spec:
  group: tekton.dev
  preserveUnknownFields: false
  versions:
  - name: v1alpha1
    served: true
    storage: false
    schema:
      openAPIV3Schema:
        type: object
        x-kubernetes-preserve-unknown-fields: true
    subresources:
      status: {}
  - name: v1beta1
    storage: true
  names:
    kind: ClusterTask
    plural: clustertasks
    categories:
    - tekton
    - tekton-pipelines
  scope: Cluster
  conversion:
    strategy: Webhook
    webhook:
      conversionReviewVersions:
      - v1beta1
      clientConfig:
        service:
          name: tekton-pipelines-webhook
          namespace: tekton-pipelines

What did you expect:

I'd expect those merge key tags are preserved or expanded like below:

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: clustertasks.tekton.dev
  labels:
    app.kubernetes.io/instance: default
    app.kubernetes.io/part-of: tekton-pipelines
    pipeline.tekton.dev/release: v0.22.0
    version: v0.22.0
spec:
  group: tekton.dev
  preserveUnknownFields: false
  versions:
  - name: v1alpha1
    served: true
    storage: false
    schema:
      openAPIV3Schema:
        type: object
        x-kubernetes-preserve-unknown-fields: true
    subresources:
      status: {}
  - name: v1beta1
    served: true
    storage: true
    schema:
      openAPIV3Schema:
        type: object
        x-kubernetes-preserve-unknown-fields: true
    subresources:
      status: {}
  names:
    kind: ClusterTask
    plural: clustertasks
    categories:
    - tekton
    - tekton-pipelines
  scope: Cluster
  conversion:
    strategy: Webhook
    webhook:
      conversionReviewVersions:
      - v1beta1
      clientConfig:
        service:
          name: tekton-pipelines-webhook
          namespace: tekton-pipelines

Anything else you would like to add:

None.

Environment:

• ytt version (use ytt --version): ytt version 0.30.0
• OS (e.g. from /etc/os-release): osx

[zhouhaibing089]

See tektoncd/pipeline#3794 as well.

[gcheadle-vmware]

Hi @zhouhaibing089,

I see that the version of ytt that you are using is v0.30.0. If you take a peek at the v0.31.0 release notes, you should see in that version we now support the merge operator.
With your example, I made some slight canges to foo.yml:

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: clustertasks.tekton.dev
  labels:
    app.kubernetes.io/instance: default
    app.kubernetes.io/part-of: tekton-pipelines
    pipeline.tekton.dev/release: "v0.22.0"
    version: "v0.22.0"
spec:
  group: tekton.dev
  preserveUnknownFields: false
  versions:
    - &version
      name: v1alpha1
      served: true
      storage: false
      schema:
        openAPIV3Schema:
          type: object
          x-kubernetes-preserve-unknown-fields: true
      subresources:
        status: {}
    -
      <<: *version
      #@yaml/map-key-override
      name: v1beta1
      #@yaml/map-key-override
      storage: true
(removed end of file for brevity...)

then running ytt -f foo.yml:

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: clustertasks.tekton.dev
  labels:
    app.kubernetes.io/instance: default
    app.kubernetes.io/part-of: tekton-pipelines
    pipeline.tekton.dev/release: v0.22.0
    version: v0.22.0
spec:
  group: tekton.dev
  preserveUnknownFields: false
  versions:
  - name: v1alpha1
    served: true
    storage: false
    schema:
      openAPIV3Schema:
        type: object
        x-kubernetes-preserve-unknown-fields: true
    subresources:
      status: {}
  - served: true
    schema:
      openAPIV3Schema:
        type: object
        x-kubernetes-preserve-unknown-fields: true
    subresources:
      status: {}
    name: v1beta1
    storage: true
(removed end of file for brevity...)

Let me know if this helps, and please feel free to reach out/respond if you have any further questions.

[aaronshurley]

Hey @zhouhaibing089, did @gcheadle-vmware's comment above help?

[zhouhaibing089]

Thanks @gcheadle-vmware.

Without the modifications:

$ ytt-new -f ./foo.yaml                                                                                                                                                           [10:50:16]
ytt: Error:
- __ytt_tpl2_start_node: expected key 'name' to not be specified again (unless 'yaml/map-key-override' annotation is added)
    in <toplevel>
      foo.yaml:25 |       name: v1beta1

In my opinion, foo.yaml should stay as is(This is a published file which as client, we don't have control with), and ytt should be able to parse this YAML without issues.

[cppforlife]

i would agree with @zhouhaibing089 for the case of plain yaml files. we recently, on develop branch, added feature to interpret certain yml files as regular yaml files so for those files i would imagine we should also relax duplicate key check.

wdyt @jtigger?

[pivotaljohn]

we recently, on develop branch, added feature to interpret certain yml files as regular yaml files

Yes: #324

so for those files i would imagine we should also relax duplicate key check.

A given YAML input is:

1. parsed from a stream of bytes into as YAML object model;
2. (if is a template) compiled and evaluated as a template.

It's during that second step that ytt explicitly checks for duplicate keys.

With #324, files like the subject of this issue would not be evaluated as templates and this would "just work." (edit: and we're one step closer... but need to relax how these values are handled on conversion)

p.s. when generating output, ytt does not check map keys:
https://github.com/vmware-tanzu/carvel-ytt/blob/01e2421ff04a2fa22c634b7cb3eedf04e054d7aa/pkg/yamlmeta/convert.go#L30-L38
p.s.s. here's where during conversion, we are checking map keys:
https://github.com/vmware-tanzu/carvel-ytt/blob/01e2421ff04a2fa22c634b7cb3eedf04e054d7aa/pkg/yamlmeta/convert.go#L60-L70

[pivotaljohn]

This exact issue was also identified in #310.

To keep things clear, let's close this issue as a duplicate. @zhouhaibing089 if you believe this is an error, please let us know.

Thank you for reporting this, @zhouhaibing089.