Hello! We are developing a custom operator and utilizing fluxCD.

We have the v1alpha1 custom resource which is deployed by fluxCD.
When we upgraded the custom resource operator from v1alpha1 to v1alpha2, flux notified us that dryrun failed with the following error message.

dry-run failed, error: failed to prune fields: failed add back owned items: failed to convert pruned object at version <foo.com>/v1alpha1: conversion webhook for <foo.com>/v1alpha2, Kind=<resource>  returned invalid metadata: invalid metadata of type <nil> in input object

Running the following dry-run command actually sometimes(about once in 5 times...?) fails with the same error message.

$ kubectl apply --server-side --dry-run=server  -f <v1alpha1-resource.yaml> --field-manager kustomize-controller

Error from server: failed to prune fields: failed add back owned items: failed to convert pruned object at version <foo.com>/v1alpha1: conversion webhook for <foo.com>/v1alpha2, Kind=<resource>  returned invalid metadata: invalid metadata of type <nil> in input object

But performing the actual conversion (the following command) never fails

performing the actual conversion also sometimes fails.

$ kubectl apply --server-side -f  <v1alpha1-resource.yaml> --field-manager kustomize-controller

<foo.com>/<resource> serverside-applied

The flakiness might be a key to solving this.

Our conversion code is similar to https://github.com/IBM/operator-sample-go/blob/b79e66026a5cc5b4994222f2ef7aa962de9f7766/operator-application/api/v1alpha1/application_conversion.go#L37

We checked the conversion log. Just one dryrun command called ConvertTo function for 3 times and ConvertFrom function for 3 times. For the last one time for each ConvertTo and ConvertFrom, we noticed that the request has lacking the information of metadata and spec when it fails.
The error log is like "metadata":{"creationTimestamp":null},"spec":{}
(The normal log is like "metadata":{"name":"<foo>","namespace":"<foo>","uid":"09b69792-56d5-4217-b23c-4d418d3f904b","resourceVersion":"1707796","generation":3,"creationTimestamp":"2022-09-16T07:28:54Z","labels":{"kustomize.toolkit.fluxcd.io/name":"<foo>","kustomize.toolkit.fluxcd.io/namespace":"flux-system"}},"spec":{"attribute1":[{...)

We could confirm that this weird thing happens when the managedField has two components(kustomization-controller and our-operator) as follows:

apiVersion: <foo.com>/v1alpha2
kind: <MyResource>
metadata:
  creationTimestamp: "2022-09-15T04:52:03Z"
  generation: 1
  labels:
    kustomize.toolkit.fluxcd.io/name: operator-sample
    kustomize.toolkit.fluxcd.io/namespace: flux-system
  managedFields:
  - apiVersion: <foo.com>/v1alpha1
    fieldsType: FieldsV1
    fieldsV1:
      f:metadata:
        f:labels:
          f:kustomize.toolkit.fluxcd.io/name: {}
          f:kustomize.toolkit.fluxcd.io/namespace: {}
      f:spec:
        f:attribute1: {}
        f:attribute2: {}
    manager: kustomize-controller
    operation: Apply
    time: "2022-09-15T04:52:03Z"
  - apiVersion: <foo.com>/v1alpha2
    fieldsType: FieldsV1
    fieldsV1:
      f:status:
        f:attribute1: {}
        f:attribute2: {}
    manager: <our-operator>
    operation: Update
    time: "2022-09-15T04:52:04Z"
  name: v1alpha1-flux
  namespace: flux
  resourceVersion: "483157"
  uid: 696bed77-a12b-45d0-b240-8d685cf790e0

spec:
  ...
status:
  ...

I asked this question in the flux repo but I could not get the reason why.
fluxcd/flux2#3105

I got stuck in this for more than one week and any ideas are really appreciated.
Thanks!

Atomic things only need a single owner stored, so, if we go through existing schemas and ensure that everything that should be atomic is atomic, we will have fewer leaf fields which need managers stored.

Hi, is there and example/some doc you can point me on how to use structured merge diff from a controller

Hi,

I was playing around with server-side apply and hit into a problem. Say I have a pod like this (very reduced case of course):

# pod.yml
apiVersion: v1
kind: Pod
metadata:
  name: test
spec:
  containers:
  - name: test 
    ports:
    - containerPort: 1234

If I run kubectl apply -f pod.yml --server-side I get the following error:

Error from server: failed to create typed patch object: .spec.containers[name="test"].ports: element 0: associative list with keys has an element that omits key field "protocol"

This is unexpected, because ContainerPort specifies the protocol as an optional field, which defaults to TCP. I traced this error back to this repository, so I figured this is where it could be resolved.

$ kubectl version
Client Version: version.Info{Major:"1", Minor:"16", GitVersion:"v1.16.2", GitCommit:"c97fe5036ef3df2967d086711e6c0c405941e14b", GitTreeState:"clean", BuildDate:"2019-10-15T19:18:23Z", GoVersion:"go1.12.10", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"16", GitVersion:"v1.16.3", GitCommit:"b3cbbae08ec52a7fc73d334838e18d17e8512749", GitTreeState:"clean", BuildDate:"2019-11-16T01:01:59Z", GoVersion:"go1.12.12", Compiler:"gc", Platform:"linux/amd64"}

CVE-2022-28948 - Medium Severity Vulnerability

Vulnerable Library - github.com/go-yaml/yaml-v2.4.0

YAML support for the Go language.

Dependency Hierarchy:
- github.com/kubernetes-sigs/structured-merge-diff/v4-v4.2.1
- ❌ github.com/go-yaml/yaml-v2.4.0 (Vulnerable Library)

Found in base branch: master

Vulnerability Details

An issue in the Unmarshal function in Go-Yaml v3 causes the program to crash when attempting to deserialize invalid input.

Publish Date: 2022-05-19

URL: CVE-2022-28948

CVSS 3 Score Details (5.5)

Base Score Metrics:

• Exploitability Metrics:
  • Attack Vector: Local
  • Attack Complexity: Low
  • Privileges Required: None
  • User Interaction: Required
  • Scope: Unchanged
• Impact Metrics:
  • Confidentiality Impact: None
  • Integrity Impact: None
  • Availability Impact: High

For more information on CVSS3 Scores, click here.

Suggested Fix

Type: Upgrade version

Release Date: 2022-05-19

Fix Resolution: v3.0.0

The approach taken by json-iterator and its reflect library is not long-term stable (it pins to internal details of the stdlib reflect package), and requires maintenance per go release, which does not appear to be sustainable golang/go#48238 (comment)

Minimum versions to work with go1.18+ are:

• github.com/json-iterator/go v1.1.12
• github.com/modern-go/reflect2 v1.0.2

In addition to the maintenance aspect, it is hard to reason about the safety of the implementation (golang/go#48238 (comment))

I'm exploring what it would look like to use the stdlib json decoder as a base and add in the features we rely on (case-sensitive keys, preservation of ints/floats, etc) in kubernetes/kubernetes#105030

I would encourage this project to consider the long-term sustainability/safety of the json-iterator dependency as well.

When server side apply validation fails with type mismatch, the returned error message contains go specific struct. For example:

.s: expected string, got &value.valueUnstructured{Value:[]interface {}{"a"}}

Wondering if having info such as []interface {}{"a"} inside returned message would be a concern.

Thought of converting to JSON and saw the previous concern here.

Currently when extracting a path such we extract everything recursively below a given path.

For example:
I apply with “metadata”:{}
Someone else adds “metadata”: { “annotations”: {“foo”:”bar”}}
When I extract with Fieldset .metadata we are going to return metadata:{“annotations”:{“foo”:”bar”}} but we want to instead return metadata:{}

So what this means for ExtractItems is that every path should be treated as a leaf and should empty out the contents of the final path element regardless of what is there.

Another example,
if we have ”metadata”: {“annotations”: {“foo”: “bar”, “caz”: “dak”}}
The only way to receive all the annotations should be to Extract a fieldset with both .metadata.annotations.foo and .metadata.annotations.caz because if you tried to extract .metadata.annotations we want this to return ”metadata”:{“annotations”:{}}}

Atomic fields are an exception to this whereby extracting a path to an atomic field should return everything in that field.

k8s version is 1.19.1.

I first created the following by using kubectl apply --server-side -f backendconfig.yaml

apiVersion: cloud.google.com/v1
kind: BackendConfig
metadata:
  name: bar
  namespace: default
spec:
  connectionDraining:
    drainingTimeoutSec: 30
  securityPolicy:
    name: bol-default-policy

The following is what has been created and persisted:

apiVersion: cloud.google.com/v1
kind: BackendConfig
metadata:
  creationTimestamp: "2020-09-11T18:18:40Z"
  generation: 1
  managedFields:
  - apiVersion: cloud.google.com/v1
    fieldsType: FieldsV1
    fieldsV1:
      f:spec:
        f:connectionDraining:
          f:drainingTimeoutSec: {}
        f:securityPolicy:
          f:name: {}
    manager: kubectl
    operation: Apply
    time: "2020-09-11T18:18:40Z"
  name: bar
  namespace: default
  resourceVersion: "214762"
  selfLink: /apis/cloud.google.com/v1/namespaces/default/backendconfigs/bar
  uid: 8d90b477-994f-4091-b631-95f19ad10730
spec:
  connectionDraining:
    drainingTimeoutSec: 30
  securityPolicy:
    name: bol-default-policy

Then I tried to re-apply the following using kubectl apply --server-side -f backendconfig.yaml

apiVersion: cloud.google.com/v1
kind: BackendConfig
metadata:
  name: bar
  namespace: default
spec:
  # <=========== connectionDraining field is removed.
  securityPolicy:
    name: bol-default-policy

I got error:

The BackendConfig "bar" is invalid: spec.connectionDraining: Invalid value: "null": spec.connectionDraining in body must be of type object: "null"

I also tried to re-apply the following using kubectl apply --server-side -f backendconfig.yaml

apiVersion: cloud.google.com/v1
kind: BackendConfig
metadata:
  name: bar
  namespace: default
spec:
  connectionDraining: null  # <======= connectionDraining is set to null
  securityPolicy:
    name: bol-default-policy

I got the same error:

The BackendConfig "bar" is invalid: spec.connectionDraining: Invalid value: "null": spec.connectionDraining in body must be of type object: "null"

On the other hand, I tried to update the created object with kubectl replace -f backendconfig.yaml

apiVersion: cloud.google.com/v1
kind: BackendConfig
metadata:
  name: bar
  namespace: default
spec:
  connectionDraining: null  # <======= connectionDraining is set to null
  securityPolicy:
    name: bol-default-policy

I got the same schema validation error as SSA above.

But if I run kubectl replace with the following, it can successfully remove the field w/o error.

apiVersion: cloud.google.com/v1
kind: BackendConfig
metadata:
  name: bar
  namespace: default
spec:
  # <========== connectionDraining field is removed.
  securityPolicy:
    name: bol-default-policy

I have played with SMD v4 libary a bit, my understanding is that:
SMD v4 set the field as null in the merged object when it infers the user want to delete the field.
In this case, initially it is

spec:
  connectionDraining:
    drainingTimeoutSec: 30
  securityPolicy:
    name: bol-default-policy

Then I SSA:

spec:
  securityPolicy:
    name: bol-default-policy

In the apiserver, SMD v4 merged the object as the following before the object goes to the validation:

spec:
  connectionDraining: null
  securityPolicy:
    name: bol-default-policy

The OpenAPI schema validation rejects the object since the connectionDraining field is an object:

              connectionDraining:
                description: ConnectionDrainingConfig contains configuration for connection
                  draining. For now the draining timeout. May manage more settings
                  in the future.
                properties:
                  drainingTimeoutSec:
                    description: Draining timeout in seconds.
                    format: int64
                    type: integer
                type: object

I'm told that 1.16 supports interpreting a CRD which has x-kubernetes-list-map-type and x-kubernetes-list-map-keys properties.

Please document somewhere an example of a CRD that uses these, so the syntax is clear.

Please also document what happens if I have an already-installed CRD with existing CR state, and I am already using SSA, and I update a property of my CRD from not having x-kubernetes-list-* annotations to having them. That is, what happens if an existing user adopts this nice new feature? Does it DTRT or explode?
For existing CRs that have been applied before. That haven't? For new CRs?

@apelisse we just discussed this.

Another approach towards making the serialized size smaller: if at a given level, the manager owns everything below, store something indicating this rather than enumerating every leaf field. We currently store a . to indicate "owns this"; we could store a ! to indicate "owns this recursively".

Pros:

• If large sections of objects have a single owner, this could make the size much much smaller

Cons:

• On apply, when someone takes ownership of a leaf field, it might drastically change another manager's managed fields. But if we do the compression on encode & decode, it might not need to complicate the code very much.

Not sure if this is what we want. If someone applies null for a field, we'll keep the null values, and we'll let Kubernetes deserialize the null value into whatever type we have. I'm not sure that's always what we want.

Hello,
We have observed in Crossplane project's CNCF fuzzing tests that runtime.DefaultUnstructuredConverter.ToUnstructured (from k8s.io/[email protected]) consuming sigs.k8s.io/structured-merge-diff/[email protected] panics with the following sample program:

package main

import (
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
	"k8s.io/apimachinery/pkg/runtime"
)

type Test struct {
	Time *metav1.Time
}

func main() {
	if _, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&Test{}); err != nil {
		panic(err)
	}
}

, producing the following stacktrace:

panic: value method k8s.io/apimachinery/pkg/apis/meta/v1.Time.ToUnstructured called using nil *Time pointer

goroutine 1 [running]:
k8s.io/apimachinery/pkg/apis/meta/v1.(*Time).ToUnstructured(0x100000001?)
        <autogenerated>:1 +0x48
sigs.k8s.io/structured-merge-diff/v4/value.TypeReflectCacheEntry.ToUnstructured({0x1, 0x0, 0x0, 0x0, 0x1, 0x0, 0x0, {0x0, 0x0, 0x0}}, ...)
        /Users/alper/data/workspaces/go/pkg/mod/sigs.k8s.io/structured-merge-diff/[email protected]/value/reflectcache.go:188 +0x6b0
k8s.io/apimachinery/pkg/runtime.toUnstructured({0x100f7cba0?, 0x140001aa3d0?, 0x0?}, {0x100efef60?, 0x14000193f10?, 0x0?})
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:655 +0x8a0
k8s.io/apimachinery/pkg/runtime.structToUnstructured({0x100f1f260?, 0x140001aa3d0?, 0x100f7f5e0?}, {0x100f0a3c0?, 0x140001aa3d8?, 0x100a61548?})
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:843 +0x7ac
k8s.io/apimachinery/pkg/runtime.toUnstructured({0x100f1f260?, 0x140001aa3d0?, 0x140001aa3d0?}, {0x100f0a3c0?, 0x140001aa3d8?, 0x140000021a0?})
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:692 +0x7f4
k8s.io/apimachinery/pkg/runtime.(*unstructuredConverter).ToUnstructured(0x101277530, {0x100edca80?, 0x140001aa3d0})
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:586 +0x364
main.main()
        /Users/alper/data/workspaces/github.com/ulucinar/crossplane/crossplane/cmd/test/main.go:13 +0x44

The same behavior is observed also with k8s.io/[email protected], which depends on sigs.k8s.io/structured-merge-diff/[email protected].

As a workaround, if a json tag is added with the omitempty specifier, then no panic is observed:

package main

import (
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
	"k8s.io/apimachinery/pkg/runtime"
)

type Test struct {
	Time *metav1.Time `json:"time,omitempty"`
}

func main() {
	if _, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&Test{}); err != nil {
		panic(err)
	}
}

As the stacktrace points, it looks like we are calling converter.ToUnstructured with a nil converter here.

Would adding a nil check for the converter variable there be appropriate (and maybe returning a nil, nil so that we do not attempt to convert a nil value, or return an error)?

In some case I need translator fieldsV1 to fieldpath

from

.spec.template.spec.containers[name="simple-web-0"].resources.limits.cpu

to

spec.template.spec.containers[0].resources.limits.cpu

json-iterator just cut a new release to address incompatibilities with the current go development version (will be go1.18)

xref:

• kubernetes/kubernetes#104949
• kubernetes/kubernetes#104947
• https://github.com/json-iterator/go/releases/tag/v1.1.12
• https://github.com/modern-go/reflect2

kubectl apply --server-side of this yaml:

apiVersion: v1
kind: Service
metadata:
name: test
spec:
type: ClusterIP
selector:
app: test
ports:
- name: http
port: 80
targetPort: 8080

Failed with
Error from server: failed to create typed patch object: .spec.ports: element 0: associative list with keys has an element that omits key field "protocol" (and doesn't have default value)

Expected to default it with "TCP" (like client side patch/apply does)

$ kubectl version
Client Version: version.Info{Major:"1", Minor:"20", GitVersion:"v1.20.1", GitCommit:"c4d752765b3bbac2237bf87cf0b1c2e307844666", GitTreeState:"clean", BuildDate:"2020-12-19T07:38:38Z", GoVersion:"go1.15.5", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"20", GitVersion:"v1.20.1", GitCommit:"c4d752765b3bbac2237bf87cf0b1c2e307844666", GitTreeState:"clean", BuildDate:"2020-12-18T12:00:47Z", GoVersion:"go1.15.5", Compiler:"gc", Platform:"linux/amd64"}

There is inconsistent behavior between server side apply and client side apply on type mismatch error handling. In server side apply, type mismatch error will be caught during structural merge and stop proceeding to further validation.

For CRD example:

......
openAPIV3Schema:
        type: object
        properties:
          a:
            type: array
            items:
              type: string
            x-kubernetes-validations:
            - rule: self != []
          s:
            type: string
            x-kubernetes-validations:
            - rule: self != ""
          i:
            type: integer
            x-kubernetes-validations:
            - rule: self != 0

When try to create CR like:

apiVersion: example.com/v1
kind: Mismatch
metadata:
  name: string
a: "a"
s: 1
i: "a"

kubectl apply --server-side -f cr_mismatch_string.yaml --validate=false will return:

Error from server: failed to create typed patch object (default/string; example.com/v1, Kind=Mismatch): errors:
  .i: expected numeric (int or float), got string
  .s: expected string, got &value.valueUnstructured{Value:1}
  .a: expected list, got &{a}

While kubectl apply -f cr_mismatch_string.yaml --validate=false will return:

The Mismatch "string" is invalid: 
* s: Invalid value: "integer": s in body must be of type string: "integer"
* a: Invalid value: "string": a in body must be of type array: "string"
* i: Invalid value: "string": i in body must be of type integer: "string"
* a: Invalid value: "a": invalid data, expected []interface{} to match the provided schema with type=array evaluating rule: self != []
* i: Invalid value: "a": invalid data, expected int, got string evaluating rule: self != 0
* s: Invalid value: 1: invalid data, expected string, got int64 evaluating rule: self != ""

Crossplane's v1.MapTransform type has an inlined map and using the runtime.DefaultUnstructuredConverter.FromUnstructured to convert a map[string]interface{} into a v1.Composition (which embodies a v1.MapTransform results in a panic:

panic: reflect: NumField of non-struct type map[string]v1.JSON

goroutine 1 [running]:
reflect.(*rtype).NumField(0x0?)
        /Users/alper/.goenv/versions/1.19.1/src/reflect/type.go:1033 +0x6c
sigs.k8s.io/structured-merge-diff/v4/value.buildStructCacheEntry({0x1066b18b8, 0x1063a48e0}, 0x0?, {0x14000121b30, 0x1, 0x1})
        /Users/alper/data/workspaces/go/pkg/mod/sigs.k8s.io/structured-merge-diff/[email protected]/value/reflectcache.go:146 +0x60
sigs.k8s.io/structured-merge-diff/v4/value.buildStructCacheEntry({0x1066b18b8, 0x106439c80}, 0x10640aae0?, {0x0, 0x0, 0x0})
        /Users/alper/data/workspaces/go/pkg/mod/sigs.k8s.io/structured-merge-diff/[email protected]/value/reflectcache.go:157 +0x3dc
sigs.k8s.io/structured-merge-diff/v4/value.typeReflectEntryOf(0x106402540?, {0x1066b18b8, 0x106439c80?}, 0x0?)
        /Users/alper/data/workspaces/go/pkg/mod/sigs.k8s.io/structured-merge-diff/[email protected]/value/reflectcache.go:119 +0x1e4
sigs.k8s.io/structured-merge-diff/v4/value.TypeReflectEntryOf({0x1066b18b8, 0x106439c80})
        /Users/alper/data/workspaces/go/pkg/mod/sigs.k8s.io/structured-merge-diff/[email protected]/value/reflectcache.go:94 +0xf4
k8s.io/apimachinery/pkg/runtime.fromUnstructured({0x1063a4580?, 0x140009e9dd0?, 0x1400091d728?}, {0x106439c80?, 0x14000011590?, 0x14000a3a750?}, 0x106342080?)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:346 +0x194
k8s.io/apimachinery/pkg/runtime.pointerFromUnstructured({0x1063a4580?, 0x140009e9dd0?, 0x1066b18b8?}, {0x10647c500?, 0x14000a3aa68?, 0x104ab1b2c?}, 0x1063a4580?)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:508 +0x27c
k8s.io/apimachinery/pkg/runtime.fromUnstructured({0x1063a4580?, 0x140009e9dd0?, 0x2?}, {0x10647c500?, 0x14000a3aa68?, 0x98?}, 0x12f2a2300?)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:357 +0x320
k8s.io/apimachinery/pkg/runtime.structFromUnstructured({0x1063a4580?, 0x140009e9da0?, 0x1400091da28?}, {0x1065176a0?, 0x14000a3aa50?, 0x104b26b3c?}, 0x1400091f110)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:550 +0x6cc
k8s.io/apimachinery/pkg/runtime.fromUnstructured({0x106386600?, 0x1400047ed40?, 0x1?}, {0x1065176a0?, 0x14000a3aa50?, 0x104e4aa04?}, 0x106402540?)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:359 +0x2c8
k8s.io/apimachinery/pkg/runtime.sliceFromUnstructured({0x1062abb00?, 0x14000515e90?, 0x1066b18b8?}, {0x1062a6800?, 0x140006a9ac0?, 0x104ab1b2c?}, 0x1400091f110)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:488 +0x73c
k8s.io/apimachinery/pkg/runtime.fromUnstructured({0x1062abb00?, 0x14000515e90?, 0x5?}, {0x1062a6800?, 0x140006a9ac0?, 0x98?}, 0x1400091dec8?)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:355 +0x2f4
k8s.io/apimachinery/pkg/runtime.structFromUnstructured({0x1063a4580?, 0x140009e9d70?, 0x1400091e008?}, {0x106544460?, 0x140006a9a90?, 0x1077f6da8?}, 0x1400091f110)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:550 +0x6cc
k8s.io/apimachinery/pkg/runtime.fromUnstructured({0x106386600?, 0x140009d9b10?, 0x3?}, {0x106544460?, 0x140006a9a90?, 0x104e4aa04?}, 0x106402540?)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:359 +0x2c8
k8s.io/apimachinery/pkg/runtime.sliceFromUnstructured({0x1062abb00?, 0x14000515ea8?, 0x1066b18b8?}, {0x1062a6700?, 0x14000301bf0?, 0x104ab1b2c?}, 0x1400091f110)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:488 +0x73c
k8s.io/apimachinery/pkg/runtime.fromUnstructured({0x1062abb00?, 0x14000515ea8?, 0x2?}, {0x1062a6700?, 0x14000301bf0?, 0x98?}, 0x14000a17200?)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:355 +0x2f4
k8s.io/apimachinery/pkg/runtime.structFromUnstructured({0x1063a4580?, 0x140009e9b60?, 0x1400091e5e8?}, {0x106517400?, 0x14000301bc0?, 0x1400039df18?}, 0x1400091f110)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:550 +0x6cc
k8s.io/apimachinery/pkg/runtime.fromUnstructured({0x106386600?, 0x140002eb580?, 0xb?}, {0x106517400?, 0x14000301bc0?, 0x104e4aa04?}, 0x106402540?)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:359 +0x2c8
k8s.io/apimachinery/pkg/runtime.sliceFromUnstructured({0x1062abb00?, 0x14000515f50?, 0x1066b18b8?}, {0x1062a65c0?, 0x1400039df40?, 0x104ab1b2c?}, 0x1400091f110)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:488 +0x73c
k8s.io/apimachinery/pkg/runtime.fromUnstructured({0x1062abb00?, 0x14000515f50?, 0x2?}, {0x1062a65c0?, 0x1400039df40?, 0x98?}, 0x0?)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:355 +0x2f4
k8s.io/apimachinery/pkg/runtime.structFromUnstructured({0x1063a4580?, 0x140009e9020?, 0x1066b18b8?}, {0x106526060?, 0x1400039df08?, 0x104ab1b2c?}, 0x1400091f110)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:550 +0x6cc
k8s.io/apimachinery/pkg/runtime.fromUnstructured({0x1063a4580?, 0x140009e9020?, 0x2?}, {0x106526060?, 0x1400039df08?, 0x98?}, 0x14000a161b0?)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:359 +0x2c8
k8s.io/apimachinery/pkg/runtime.structFromUnstructured({0x1063a4580?, 0x140009e8f60?, 0x0?}, {0x1064a9de0?, 0x1400039de00?, 0x104c70218?}, 0x1400091f110)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:550 +0x6cc
k8s.io/apimachinery/pkg/runtime.fromUnstructured({0x1063a4580?, 0x140009e8f60?, 0x19b0?}, {0x1064a9de0?, 0x1400039de00?, 0x195?}, 0x1400091f0c8?)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:359 +0x2c8
k8s.io/apimachinery/pkg/runtime.(*unstructuredConverter).FromUnstructuredWithValidation(0x1077ba580, 0x140009e8f60, {0x1065fffc0?, 0x1400039de00}, 0x0)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:247 +0x32c
k8s.io/apimachinery/pkg/runtime.(*unstructuredConverter).FromUnstructured(...)
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/[email protected]/pkg/runtime/converter.go:274
github.com/upbound/upjet/pkg/migration.(*PlanGenerator).convertComposition(0x1400091ff18, {{0x140009e8f60}, {{0x14000055020, 0x55}, {0x0, 0x0}, {0x0, 0x0}}})
        /Users/alper/data/workspaces/github.com/ulucinar/upbound/upbound-upjet/pkg/migration/plan_generator.go:236 +0x7c
github.com/upbound/upjet/pkg/migration.(*PlanGenerator).convert(0x1400091ff18)
        /Users/alper/data/workspaces/github.com/ulucinar/upbound/upbound-upjet/pkg/migration/plan_generator.go:128 +0x338
github.com/upbound/upjet/pkg/migration.(*PlanGenerator).GeneratePlan(0x0?)
        /Users/alper/data/workspaces/github.com/ulucinar/upbound/upbound-upjet/pkg/migration/plan_generator.go:107 +0x28
main.main()
        /Users/alper/data/workspaces/github.com/ulucinar/migration/cmd/migrator/main.go:48 +0x5f8

Process finished with the exit code 2

Looks like we are not checking the inlined field's kind before making a recursive call here.

We've determined that most of the performance impact is due to the large size change. So, we need to fix that. This issue will tackle improving the format, which is the most obvious avenue for fixing this problem.

Goals:

• concise
• human-readable-ish (just have to beat gzip'd json or proto)

Ideas:

1. Switch to a list format rather than the current map format. Instead of {"<keytype><key>" : { <children> }}, we do [ keytype, <key>, [ <children> ]]. This flattens every string such that we don't double-encode any strings in keys. keytypes can be numeric if we wish, which shortens them (no need for "s. This would be a slight improvement in size since we wouldn't need as many escape characters in strings.
2. Make a "string table". Every time we go to encode a string, look it up from a string table, adding it if it isn't there.
3. The obvious thing to do is number the strings, but we can do better from a readability point of view: use just enough characters to make it unique. We store the short encoding in the string table so that it'll be future proof (adding more strings won't make existing short names ambiguous).
4. We now have to store the string table somewhere. We get the most benefit if we store it once per object rather than once per manager (we still have benefit there from list item keys being repetitive). The format of the string table can just be a json map, and we don't have to duplicate the short name in the long name: {"me": "tadata", "sp": "ec", "st": "atus", ...}

Finally, we can provide a default table, so that the shortcuts for common field names don't depend on the order in which they get added to the string table. In terms of storage space it'd be best to just version this default and only encode the differences, but for readability it'd be best to include it in the object. It might make a big difference.

I think this should get us fairly close to what gzip does. gzip has the advantage that it can lose 1 bit per ascii character. But in JSON we'll give that back by base64 encoding it. And what I've described should be much faster than compression/decompression.

Of course we can do even better if we made a truly binary format (imagine combining the [<keytype>, into a single byte), but that won't be more readable than gzip.

When comparing list fields within two objects, a simple equality check isn't enough. We need to implement a semantic check on the individual list items to ensure accurate comparisons.

• Code Reference: The current equality check can be found here: https://github.com/kubernetes-sigs/structured-merge-diff/blob/v4.4.1/typed/compare.go#L148

• Example: The following two objects should be evaluated as identical, even though their list item order differs:

kind: Role
metadata:
  name: test-role
  namespace: test-ns
rules:
- apiGroups:
  - batch/v1
  resources:
  - jobs
  verbs:
  - '*'
- apiGroups:
  - apps/v1
  resources:
  - deployments
  verbs:
  - '*'

and

kind: Role
metadata:
  name: test-role
  namespace: test-ns
rules:
- apiGroups:
  - apps/v1
  resources:
  - deployments
  verbs:
  - '*'
- apiGroups:
  - batch/v1
  resources:
  - jobs
  verbs:
  - '*'

The following setup can lead to a panic. The issue seems to be with embedding a pointer to a struct, when using runtime.DefaultUnstructuredConverter.ToUnstructured.

type A struct {
  *B `json:",inline"`
}

type B struct {
  Foo string `json:"foo"`
}

The panic observed is:

... Panic: reflect: NumField of non-struct type *v2.CiliumNetworkPolicy (PC=0x46A7A8)

/usr/lib/go/src/runtime/panic.go:975
  in gopanic
/usr/lib/go/src/reflect/type.go:974
  in rtype.NumField
/home/chris/code/cilium/cilium/vendor/sigs.k8s.io/structured-merge-diff/v4/value/reflectcache.go:146
  in buildStructCacheEntry
/home/chris/code/cilium/cilium/vendor/sigs.k8s.io/structured-merge-diff/v4/value/reflectcache.go:153
  in buildStructCacheEntry
/home/chris/code/cilium/cilium/vendor/sigs.k8s.io/structured-merge-diff/v4/value/reflectcache.go:119
  in typeReflectEntryOf
/home/chris/code/cilium/cilium/vendor/sigs.k8s.io/structured-merge-diff/v4/value/reflectcache.go:94
  in TypeReflectEntryOf
/home/chris/code/cilium/cilium/vendor/k8s.io/apimachinery/pkg/runtime/converter.go:480
  in toUnstructured
/home/chris/code/cilium/cilium/vendor/k8s.io/apimachinery/pkg/runtime/converter.go:413
  in unstructuredConverter.ToUnstructured
unknown_fields.go:102
  in detectUnknownFields
validator.go:138
  in NPValidator.ValidateCCNP
validator_test.go:189
  in CNPValidationSuite.Test_GoodCCNP
/usr/lib/go/src/reflect/value.go:336
  in Value.Call
/usr/lib/go/src/runtime/asm_amd64.s:1374
  in goexit

The following patch fixes the issue:

diff --git a/vendor/sigs.k8s.io/structured-merge-diff/v4/value/reflectcache.go b/vendor/sigs.k8s.io/structured-merge-diff/v4/value/reflectcache.go
index 49e6dd169..ff16bd52f 100644
--- a/vendor/sigs.k8s.io/structured-merge-diff/v4/value/reflectcache.go
+++ b/vendor/sigs.k8s.io/structured-merge-diff/v4/value/reflectcache.go
@@ -150,7 +150,11 @@ func buildStructCacheEntry(t reflect.Type, infos map[string]*FieldCacheEntry, fi
 			continue
 		}
 		if isInline {
-			buildStructCacheEntry(field.Type, infos, append(fieldPath, field.Index))
+			e := field.Type
+			if field.Type.Kind() == reflect.Ptr {
+				e = field.Type.Elem()
+			}
+			buildStructCacheEntry(e, infos, append(fieldPath, field.Index))
 			continue
 		}
 		info := &FieldCacheEntry{JsonName: jsonName, isOmitEmpty: isOmitempty, fieldPath: append(fieldPath, field.Index), fieldType: field.Type}

Currently the reconcileWithSchemaWalker will believe that a list/map node has changed from atomic to granular if the node was previously atomic (and the current element relationship is not atomic).

The way it determines whether a node is atomic is by naively assuming that if the node’s fieldSet contains an path element that is a member, but that path element has no children, than the node must be atomic.

This is incorrect because there are situations where a node might be an empty list/map, but not be atomic. For example, if a manager wants to own just the spec.metadata field to ensure it doesn't get removed, but does not want to own any of its contents.

The guiding example that exposes this issue is the following proposed test to be added to the merge package

                // BROKEN
                "no_change_list": {
                        Ops: []Operation{
                                // Apply an empty list with the first manager
                                // Meaning we want the manager to own the list itself
                                // but none of its elements
                                Apply{
                                        Manager: "one",
                                        Object: `
                                                list:
                                        `,
                                        APIVersion: "v1",
                                },
                                // Apply specific elements to the list created by manager one.
                                // Manager two should own these individual elements in the list.
                                // Manager one should continue to own the list itself but none of the elements.
                                Apply{
                                        Manager: "two",
                                        Object: `
                                                list:
                                                 - a
                                                 - b
                                        `,
                                        APIVersion: "v1",
                                },
                                // Prior to this apply, manager one SHOULD own the list but none of the individual
                                // elements, but when we use manager two to remove element "a" from the list,
                                // we see that actually manager one becomes an owner of all the elements in the list.
                                //
                                // This occurs because `reconcileWithSchemaWalker` thinks the list's schema has
                                // changed from atomic to granular.
                                //
                                // It was never atomic in the first place, but the reconciler determines node
                                // atomicity by checking if a node (in this case the list itself) exists as a member in the
                                // fieldSet but this node has no children (because the list is empty).
                                Apply{
                                        Manager: "two",
                                        Object: `
                                                list:
                                                 - b
                                        `,
                                        APIVersion: "v1",
                                },
                        },
                        // BROKEN: expected:
                        //Object: `
                        //      list:
                        //      - b
                        //`,
                        // but actually got:
                        Object: `
                                list:
                                - a
                                - b
                        `,
                        APIVersion: "v1",
                        Managed: fieldpath.ManagedFields{
                                "apply-one": fieldpath.NewVersionedSet(
                                        // BROKEN expected:
                                        //_NS(
                                        //      _P("list"),
                                        //),
                                        // but actually got:
                                        _NS(
                                                _P("list"),
                                                _P("list", _V("a")),
                                                _P("list", _V("b")),
                                        ),
                                        "v1",
                                        false,
                                ),
                                "apply-two": fieldpath.NewVersionedSet(
                                        _NS(
                                                _P("list", _V("b")),
                                        ),
                                        "v1",
                                        false,
                                ),
                        },
                },

A perfectly correct solution may not be possible because there is no way of determining what the previous schema was.

One proposal is to maintain a holistic view of things we know not to be atomic (because some other applier was able to apply individual elements) and then for our atomicity check we look at that holistic view to see whether or not any node was previously not atomic.

While not perfect, this is more correct than the current situation.

It looks like structured-merge-diff is a dependency of k8s.io/apiserver, but there are no corresponding tags for kubernetes versions like there are in other k8s components.

For example, when using kubernetes-1.14.0 tag of k8s.io/apiserver, I have to manually pin structured-merge-diff to e85c7b244fd2 when using go modules.

If anyone comes here with the same issue:

go get -m sigs.k8s.io/structured-merge-diff@e85c7b244fd2

Is there any chance we can start tagging this repo as well?

Bug Report

Problem
This page has one broken link of contributor cheat sheet which needs to fix.

solution
need to locate the correct path of README.

/assign
/kind bug
/kind documentation

We should block merges for anything where go test ... doesn't pass.

i was hitting above line when using an atomic map as the element of a set-list. is there a plan for supporting such schema? thanks! @apelisse @jennybuckley

Right now, we have some lists that are marked as listType=map that are not strictly maps in Kubernetes. In that case, everything goes wrong in smd because the list is invalid (and can't be handled), so it's fully rejected. We can't even track the fields, which causes us to fail and return an error which is ignored by Kubernetes, and the managed fields are entirely reset.

Unfortunately, it means that objects that have such a list can't be server-side applied at all. We think it'd be simpler to merely ignore these (fairly rare) occurences and try to do the best thing possibe rather than fail.

This means:

1. When we validate an object with such a list, we should probably not fail altogether (though we should probably reject a request to server-side apply an object like this).
2. When tracking the fields, we should give duplicate fields to the same owner.
3. When server-side applying without the field, we need to be careful about how the field should be removed.
4. When we merge a list that has such a field, we need to be careful, especially if the server-side apply contains the field (should we remove both other occurences? Probably).

The latest / v4.1.1 release of structured-merge-diff still uses v2.2.1 of gopkg.in/yaml.v2 (go.mod), which still contains the medium vulnerability CVE-2019-11254 which can cause excessive CPU while parsing certain YAML. This is fixed in v2.2.8 of gopkg.in/yaml.v2 , specifically this commit.