Regression: Patch drops properties that are null or are an empty array

[ephesused]

I see this problem starting in v3.8.0.

root@28be5a6d76a0:/tmp/patch-removal# ./kustomize-v3.8.0 version
{Version:kustomize/v3.8.0 GitCommit:6a50372dd5686df22750b0c729adaf369fbf193c BuildDate:2020-07-05T14:08:42Z GoOs:linux GoArch:amd64}
root@28be5a6d76a0:/tmp/patch-removal#

When a resource defines a property with null, or an array as empty, the patch drops the property completely. The drops lead to failures when a transformer later attempts to work with the (newly) missing entry.

Setup

kustomization.yaml

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- resources.yaml
patchesStrategicMerge:
- patches.yaml
# transformers:
# - transformers.yaml

resources.yaml

Note the null selector and the empty array for imagePullSecrets.

apiVersion: apps/v1
kind: Deployment
metadata:
  name: testing123
spec:
  replicas: 1
  selector: null
  template:
    spec:
      containers:
      - name: event
        image: testing123
        imagePullPolicy: IfNotPresent
      imagePullSecrets: []

patches.yaml

apiVersion: apps/v1
kind: Deployment
metadata:
  name: testing123
spec:
  template:
    spec:
      affinity:
        nodeAffinity:
          requiredDuringSchedulingIgnoredDuringExecution:
            nodeSelectorTerms: []

Expected behavior

This is the behavior I see in v3.7.0:

root@28be5a6d76a0:/tmp/patch-removal# ./kustomize-v3.7.0 version
{Version:kustomize/v3.7.0 GitCommit:42d1f7b792a80af34828ec4c89af99e1043351a7 BuildDate:2020-07-04T19:15:46Z GoOs:linux GoArch:amd64}
root@28be5a6d76a0:/tmp/patch-removal#

Note the (retained) null selector and the empty array for imagePullSecrets:

root@28be5a6d76a0:/tmp/patch-removal# ./kustomize-v3.7.0 build .
apiVersion: apps/v1
kind: Deployment
metadata:
  name: testing123
spec:
  replicas: 1
  selector: null
  template:
    spec:
      affinity:
        nodeAffinity:
          requiredDuringSchedulingIgnoredDuringExecution:
            nodeSelectorTerms: []
      containers:
      - image: testing123
        imagePullPolicy: IfNotPresent
        name: event
      imagePullSecrets: []
root@28be5a6d76a0:/tmp/patch-removal#

Actual behavior

Note the absence of the selector and the imagePullSecrets.

root@28be5a6d76a0:/tmp/patch-removal# ./kustomize-v3.8.0 build .
apiVersion: apps/v1
kind: Deployment
metadata:
  name: testing123
spec:
  replicas: 1
  template:
    spec:
      affinity:
        nodeAffinity:
          requiredDuringSchedulingIgnoredDuringExecution:
            nodeSelectorTerms: []
      containers:
      - image: testing123
        imagePullPolicy: IfNotPresent
        name: event
root@28be5a6d76a0:/tmp/patch-removal#

Failure scenario

Add transformers.yaml and uncomment its inclusion in the kustomization.yaml.

transformers.yaml

apiVersion: builtin
kind: PatchTransformer
metadata:
  name: patch-transformer
patch: |-
  - op: add
    path: /spec/template/spec/imagePullSecrets/-
    value:
      name: added-image-pull-secrets
target:
  group: apps
  kind: Deployment
  name: .*
  version: v1

The results are as follows - v3.7.0 succeeds and v3.8.0 fails:

root@28be5a6d76a0:/tmp/patch-removal# cat kustomization.yaml
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- resources.yaml
patchesStrategicMerge:
- patches.yaml
transformers:
- transformers.yaml
root@28be5a6d76a0:/tmp/patch-removal# ./kustomize-v3.7.0 build .
apiVersion: apps/v1
kind: Deployment
metadata:
  name: testing123
spec:
  replicas: 1
  selector: null
  template:
    spec:
      affinity:
        nodeAffinity:
          requiredDuringSchedulingIgnoredDuringExecution:
            nodeSelectorTerms: []
      containers:
      - image: testing123
        imagePullPolicy: IfNotPresent
        name: event
      imagePullSecrets:
      - name: added-image-pull-secrets
root@28be5a6d76a0:/tmp/patch-removal# ./kustomize-v3.8.0 build .
Error: add operation does not apply: doc is missing path: "/spec/template/spec/imagePullSecrets/-": missing value
root@28be5a6d76a0:/tmp/patch-removal#

[ephesused]

A couple workaround options that use patching to inject the empty array (or, similarly, a null):

1. Use patchesStrategicMerge with something like ...

   apiVersion: apps/v1
   kind: Deployment
   metadata:
   name: testing123
   spec:
   template:
   spec:
     imagePullSecrets: []

2. Use transformers with something like ...

   apiVersion: builtin
   kind: PatchTransformer
   metadata:
   name: patch-transformer
   patch: |-
   - op: add
   path: /spec/template/spec/imagePullSecrets
   value: []
   target:
   group: apps
   kind: Deployment
   version: v1

[ricochet]

We're an ISV and intentionally provide manifests with empty fields so that downstream overlays can be additive.

1. We have a set of optional overlays that we provide as examples. If the upstream base changes, the downstream overlays do not need to change, e.g. we originally do not have a volume mount on a pod, then later add a default mount for /tmp.
2. Customers also create their own patches. Similar to the example above, we do not want to break their patches when we add volume mounts.
3. We want adding patches to arrays to be simple.

Given this perspective, I view this as a breaking change that occurred between minor releases.

[monopole]

@ricochet - thanks for these comments.

There is a one PR difference between v3.7.0 and v3.8.0 - a change in how strategic merge patch is implemented.

It's not an API change, so it's not a major release, but it's riskier than a patch release (which are often taken automatically). Hence the change in the minor release digit (7 becomes 8). Thanks for trying v3.8.0.

Fallback to v3.7.0 for old behavior, but the v3.8.0 branch is the way forward for fixes, and these comments will be taken into account.

We'll look more into how people are declaring fields with null values and empty arrays. That seems like an attempt to use buggy behavior to get desired behavior. Hoping we can identify a better way to handle this.

[monopole]

@Shell32-Natsu - high priority for getting 3.8.1 out

The first thing to determine here would be desired behavior.

Can we establish clear reasons as to why we'd need to declare fields with null values or empty lists? Isn't is sufficient to write patches that create fields / lists if they don't exist?

[ricochet]

Thanks @monopole! I agree that this approach has always had a smell. If kustomize handled the logic of "if array doesn't exist, create it, then add", then that would solve this problem. Based on the most common questions that I receive and in slack, this would go a long way in smoothing out bumps in writing patches for kustomize.

For example, an add operation using a non-positional like "-". This could imply create if necessary, then add.

- op: add
  path: /spec/ports/-
  value:
    name: datacache
    port: 35001
    protocol: TCP
    targetPort: datacache

[hornpolish]

Thanks @monopole and @ricochet , here is a more concrete example and our goal. we are an ISV, we hope to have many customers who kustomize from our bases. for grins, suppose we have 10 deployment objects, 5 with init containers, 5 without.

a) we want to make it easy for the customer to insert their own init container into all 10 deployment objects. we dont want them to have to inspect our bases and handle the "has init container" deployments different to the "has not". if we have empty arrays for the "have not" 5, our instructions for the customer are easier. "add your init container to the array". @ricochet suggestion of "create it if you need it" works great in this scenario

b) we want these bases to be resilient/durable over time as well. some future version will happen, and now 6 deployments need init containers. we don't want to break the customer who has kustomizations that worked last month when we introduce a fresh init container next month

we are big fans of kustomize, and its additive mantra. we hope to make it easy to add in a consistent and future proof way.

[monopole]

OK, #2713 should allow empty lists in resources to be explicitly declared and retained as such.

So YAML like

  someList: []

won't be dropped in the course of transformation or formatting, as may happen to other empty fields.

Present but declaratively empty list (array) fields are handy as documentation and as targets for array extension under the fine print of rfc6902.

It's likely going to be important to allow patches to add entries to list fields that are completely missing (creating such fields in the process, as long as openapi type declarations are respected), but that's a different issue.

[monopole]

@ricochet, @hornpolish please give v3.8.1 a try.

Standard install from binary should work:

curl -s "https://raw.githubusercontent.com/\
kubernetes-sigs/kustomize/master/hack/install_kustomize.sh"  | bash

Please reopen this if still and issue, or open new issues for new problems.

[ephesused]

A run against master (0b359d0) shows that empty arrays are retained as desired (thanks!), but nulls and empty mappings still are getting dropped. Would you prefer I reopen here, or start a new issue for nulls and empty mappings?

Comparing manifest.yaml between 3ddc9af (3ddc9af6c5119e77cf0eaa34728f8dc6df5556b5) and master (0b359d0ef0272e6545eda0e99aacd63aef99c4d0)...
16973d16972
<       creationTimestamp: null
17272,17275c17271,17272
<       - emptyDir: {}
<         name: security-ssh
<       - emptyDir: {}
<         name: tmp
---
>       - name: security-ssh
>       - name: tmp

As expected, v3.8.1's behavior matches master (0b359d0).

[Shell32-Natsu]

@monopole Please let me know your opinion about the empty map and null.

[monopole]

@ephesused Thanks for your help here.

In the example way up top with selector: null, why would you like to see that field retained in the output?

[ephesused]

@monopole, I don't have enough depth with kustomize to know if there would be a functional difference like there is with arrays. If there's no functional implication, then I agree that dropping null fields is fine. (In that case, the only reason I can think to retain them would be consistency - consistency with the input resource, and consistency with previous kustomize versions.)

[hornpolish]

@monopole - sorry for the delay, i was trying to narrow it down, but probably should bring y'all up to date

cat >kustomization.yaml <<EOF
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- resource.yaml
patchesStrategicMerge:
- patch.yaml
EOF

cat >resource.yaml <<EOF
apiVersion: apps/v1
kind: Deployment
metadata:
  name: sas-crunchy-data-postgres-operator
spec:
  replicas: 1
  template:
    metadata:
      creationTimestamp: null
    spec:
      serviceAccountName: postgres-operator
      containers:
      - name: apiserver
        image: sas-crunchy-data-operator-api-server
        imagePullPolicy: IfNotPresent
        ports:
        - containerPort: 8443
        envFrom: []
        volumeMounts:
        - name: security-ssh
          mountPath: /security-ssh
        - mountPath: /tmp
          name: tmp
      imagePullSecrets: []
      volumes:
      - emptyDir: {}
        name: security-ssh
      - emptyDir: {}
        name: tmp
EOF

cat >patch.yaml <<EOF
apiVersion: apps/v1
kind: Deployment
metadata:
  name: sas-crunchy-data-postgres-operator
  labels:
    workload.sas.com/class: stateless
spec:
  template:
    metadata:
      labels:
        workload.sas.com/class: stateless
    spec:
      tolerations:
        - key: "workload.sas.com/class"
          operator: "Equal"
          value: "stateful"
          effect: "NoSchedule"
        - key: "workload.sas.com/class"
          operator: "Equal"
          value: "stateless"
          effect: "NoSchedule"
---
EOF

if i kustomize build . on this, - emptyDir: {} as well as creationTimestamp: null gets removed from the object that is built, even though i'm not patching that part of the object.

if i remove/comment the patchesStrategicMerge from kustomization.yaml, then the - emptyDir: {} and creationTimestamp: null passes thru into the result

almost as those null empty fields are not retained as you build the object in preparation (or during) the patchesStrategicMerge process

i'll reopen the Issue