KUBERNETES_PATCH_PATH fails when applying back original resource with modification.

[GrahamDumpleton]

Expected behavior (what you expected to happen):

Hook script is configured to use snapshots and so gets complete copies of resources from the cluster.

A resource, in this case a Secret, is taken from the snapshot and modified. The complete resource with all original fields and the one modified field is passed back to the shell-operator using KUBERNETES_PATCH_PATH file, with something like:

operation: CreateOrUpdate
object:
  apiVersion: v1
  kind: Secret
  metadata:
    namespace: namespace-2
    name: secret-1
    ownerReferences:
      ...
    resourceVersion: ...
    ...
  type: Opaque
  data:
    key: dmFsdWU=

Expected behaviour is that this should be applied back to the cluster and the original version updated.

IOW, expecting this would work just like if had run kubectl apply. However, it doesn't.

Actual behavior (what actually happened):

The update of the existing resource fails with the error:

Hook failed. Will retry after delay. Failed count is 1. Error: 1 error occurred:\n\t* Create object: v1/Secret/namespace-2/secret-1: resourceVersion should not be set on objects to be created\n\n"

In other words, it fails when supplying the original resource with just the modification applied due to the existence of the original resourceVersion property. Even though it should be an update since the resource does exist, the message indicates creation of an object is being attempted. The existence of resourceVersion property doesn't cause an issue if you were to take the same modified object and use kubectl apply.

If one explicitly removes resourceVersion from what is written back to KUBERNETES_PATCH_PATH then it works, but that isn't really desirable. For the case of an update you actually really want it to verify the resource version is the same and make the update so that can catch if resource in cluster had changed in the interim, or even deleted.

What doesn't make sense is that in the code it does:

	case Create:
		return NewCreateOperation(spec.Object,
			WithSubresource(spec.Subresource))
	case CreateIfNotExists:
		return NewCreateOperation(spec.Object,
			WithSubresource(spec.Subresource),
			IgnoreIfExists())
	case CreateOrUpdate:
		return NewCreateOperation(spec.Object,
			WithSubresource(spec.Subresource),
			UpdateIfExists())

so at that point it seems to treat them all as being a create operation. I can't follow the code through after that to work out what actually happens, but the docs say I should expect:

CreateOrUpdate — accept a Kubernetes object. It retrieves an object, and if it already exists, computes a JSON Merge Patch and applies it (will not update .status field). If it does not exist, we create the object.

So I would expect that it should be realising that the resource already exists and creates a patch for the differences and applies that, but that doesn't look like what is happening.

For now I have switched to using:

operation: MergePatch
kind: Secret
namespace: namespace-2
name: secret-1
ignoreMissingObject: true
mergePatch:
  type: Opaque
  data:
    key: value

instead, but that isn't going to work for other cases will have where is too hard to determine a small patch. In those cases will just have to drop the resourceVersion and hope that will be okay, although the case where resource deleted since will be an issue since could potentially recreate it when don't want it to be.

Environment:

• Shell-operator version: 1.0.6
• Kubernetes version: 1.21

Additional information for debugging (if necessary):

Hook script

#!/bin/bash
if [[ $1 == "--config" ]]; then

cat <<EOF

configVersion: v1

settings:

executionMinInterval: 5s

executionBurst: 1

schedule:

name: periodic-checking

crontab: "* * * * *"

group: secret-copier

kubernetes:
name: secretexports

apiVersion: example.com/v1alpha1

kind: SecretExport

executeHookOnEvent:

Added
Modified

group: secret-copier


name: secretimports

apiVersion: example.com/v1alpha1

kind: SecretImport

executeHookOnEvent:

Added
Modified

group: secret-copier


name: secrets

kind: Secret

executeHookOnEvent:

Added
Modified

group: secret-copier

EOF

exit 0

fi



ytt --data-value-file contexts=$BINDING_CONTEXT_PATH -f ../handlers >$KUBERNETES_PATCH_PATH

[nabokihms]

@GrahamDumpleton hello and thank you for submitting this issue. Most of our teammates have days off till the end of this week because of the New Year's holiday season in Russia. We will reach you next week and see together what we can do.

[GrahamDumpleton]

Alas, I can't use MergePatch as that can't be used to remove keys/properties and using JSONPatch will be too messy. So will drop the resourceVersion for now and hope it is okay until get a proper solution.

[GrahamDumpleton]

Thinking whether there should be a distinct Update operation rather than relying on CreateOrUpdate. I don't want it created if it doesn't exist. For Update there should be a flag like ignoreMissingObject so doesn't error if had since been deleted.

[diafour]

Hello!

CreateOrUpdate — accept a Kubernetes object. It retrieves an object, and if it already exists, computes a JSON Merge Patch and applies it (will not update .status field). If it does not exist, we create the object.

Oops, this one is not correct. CreateOrUpdate will try to create first and then update if create call return 'AlreadyExists' error. We'll fix the description.

So I would expect that it should be realising that the resource already exists and creates a patch for the differences and applies that, but that doesn't look like what is happening.

The closest to this scenario is a JQPatch operation. It retrieves object, apply changes with jq expression and then call Update. Also, this operation respects ignoreMissingObject flag (not mentioned in docs, oops again).

Thinking whether there should be a distinct Update operation rather than relying on CreateOrUpdate. I don't want it created if it doesn't exist. For Update there should be a flag like ignoreMissingObject so doesn't error if had since been deleted.

Sounds good. @zuzzas what do you think about adding Update operation?

[GrahamDumpleton]

Another example where behaviour of CreateOrUpdate, and lack of Update (which behaves like kubectl apply), is an issue is with persistent volume claims. If you try to update an existing persistent volume claim by taking original resource, changing resource request values, but apply whole resource back to the cluster with CreateOrUpdate you get an error:

time="2022-01-27T02:40:02Z" level=error msg="Hook failed. Will retry after delay. Failed count is 1. Error: 1 error occurred:\n\t* Create object: v1/PersistentVolumeClaim/default/registry-registry-1: PersistentVolumeClaim \"registry-registry-1\" is invalid: spec: Forbidden: spec is immutable after creation except resources.requests for bound claims\n\u00a0\u00a0core.PersistentVolumeClaimSpec{\n\u00a0\u00a0\t... // 2 identical fields\n\u00a0\u00a0\tResources:        {Requests: ││  {s\"storage\": {i: {...}, s: \"10Gi\", Format: \"BinarySI\"}}},\n\u00a0\u00a0\tVolumeName:  \"\",\n-\u00a0\tStorageClassName: nil,\n+\u00a0\tStorageClassName: &\"local-path\",\n\u00a0\u0 ││ 0a0\tVolumeMode:       &\"Filesystem\",\n\u00a0\u00a0\tDataSource:       nil,\n\u00a0\u00a0\tDataSourceRef:    nil,\n\u00a0\u00a0}\n\n\n" binding=imageregistries event=kubernetes hook=image-registry.sh queue=main task=HookRun

In other words, it errors even though the fields outside of resource requests are the same values as originals.

Not much choice at this point but to not use KUBERNETES_PATCH_PATH and use kubectl apply in the hook script. :-(