Don't delete composed resources when we hit an error

[negz]

Fixes https://github.com/crossplane-contrib/function-patch-and-transform/issues/59 Fixes https://github.com/crossplane-contrib/function-patch-and-transform/issues/70

This PR changes how function-patch-and-transform handles patch errors:

• If patching a new composed resource fails because a source field path was required and wasn't found, the function emits a warning result and doesn't add the composed resource to desired state. The new composed resource won't be created.

• If patching an existing composed resource fails because the source field path was required and wasn't found, the function emits a warning result and skips the patch. It does add the composed resource to desired state, so it won't be deleted.

• If patching any kind of resource that always exists (e.g. the XR, the environment) fails because the source field path was required and wasn't found, the function emits a warning result and skips the patch.

• If patching any kind of resource fails because the source field path was optional and wasn't found, the function skips the patch. No result is emitted.

• If patching any kind of resource fails for any other reason, the function emits a fatal result. This will stop the entire function pipeline. This will usually happen due to a misconfigured Composition that needs to be updated to work correctly. The function already returned a fatal error for Compositions that had invalid schemas - for example missing required fields. Now the function will also return a fatal error for issues that it doesn't spot during validation, for example patching the wrong type to a field.

This means that required field paths only meaningfully affects composed resources that don't exist yet. A required field path not existing will block the creation of a composed resource that doesn't exist yet. The function considers a composed resource to not exist if it doesn't appear in observed state.

In any other scenario, a patch from a required field path is treated the same as a patch from an optional field path. The affected resource is added to the desired state, but the patch is skipped. If the resource being patched already exists, the only difference between a missing required field path and a missing optional field path is whether the function emits a warning result. The function will emit a warning result if a required field path not found.

This change ensures that the function won't delete a composed resource when it encounters an error, by omitting the composed resource from its desired state. In some circumstances the function could delete a specific field of an existing composed resource. This would happen if the function had successfully created that field by patching from a field path that was later removed.

[negz]

Marking ready for review. I ran some manual tests with crossplane beta render:

• A missing optional field path skips the patch.
• A missing required field path skips the resource if it doesn't yet exist.
• A missing required field path gives a warning (and doesn't skip the resource) it if does exist.

[phisco]

@negz

If patching an existing composed resource fails because the source field path was required and wasn't found, the function emits a warning result and skips the patch. It does add the composed resource to desired state, so it won't be deleted.

Just to be explicit, this would be not be a different behaviour w.r.t. the native implementation, right? Except for:

In some circumstances the function could delete a specific field of an existing composed resource. This would happen if the function had successfully created that field by patching from a field path that was later removed.

I wonder if we could just "copy" the current value in such case to avoid that 🤔

[negz]

I wonder if we could just "copy" the current value in such case to avoid that

I thought about that. I'm not sure how I feel about it though. It starts to seem less like the function's desired state. I guess you could say the function's desired state is to not change the observed state?

I think my preference would be to proceed as-is and consider doing this if it proves to be an issue.

[phisco]

I guess you could say the function's desired state is to not change the observed state?

Yes, which is what I was hinting at here by adding a way to ignore resources.

[negz]

Yes, which is what I was hinting at https://github.com/crossplane-contrib/function-patch-and-transform/issues/59#issuecomment-1884704718 by adding a way to ignore resources.

Yeah, I'm wary of supporting "partial applies" like this. It could get especially tricky to reason about when stacking desired state from multiple functions:

• The set of desired resources that can actually be applied is the subset of desired resources that don't have an associated error, across all functions in the pipeline.
• When function N returns an error, does function N+1... onward need to be aware and handle that resource in a special way?

Plus, if we know we're hitting an error, do we want to apply as much of the desired state as possible, or as little as possible? As little as possible seems safest/most conservative to me.

[negz]

I wonder if we could just "copy" the current value in such case to avoid that

It occurred to me this morning that this could get us back into the hairy client-side merge problems we hope to avoid by using SSA. For basic/leaf fields it might be fine, but I could imagine it leading down a path to us needing to merge state client-side. Imagine for example a failed patch to an object in a map-like array.

[phisco]

I'm still not entirely sure it's ok to emit resources without a field set because a required patch went missing, in the best case scenario the target field was required and the resource gets rejected, in the worst case we set it to something and we can't go back anymore (or it's costly to do so). I built an image and asked @haarchri for a more real-world feedback.

[negz]

@phisco The other option would be to return a fatal result if a required patch to an existing resource failed. Main downside of this approach would be deadlocking eventually consistent patches, but maybe that's less of an issue when patching existing resources.

I would suggest it's worth proceeding with this even if we're not sure whether the behaviour for required patches to existing resources is ideal. I imagine it's still better than the state we have right now, i.e. deleting resources when we hit an error. I think it's a priority to ship a version that fixes that.

[negz]

Thanks, I'll cut a release this week.