Add a warning if patch doesn't succeed

[mnarodovitch]

What do these changes do?

The situation

• kopf submits a merge-patch+json patch
• kubernetes API doesn't apply the merge-patch+json patch but responds 200 OK

leads to bugs and confusion, when testing operators.

With this change, kopf will check, if the merge-patch+json patch was merged and be verbose, if the patch was not exactly successful.

Description

Before: When a patch wasn't merged, then kopf didn't report anything.

After: When a patch wasn't merged, then kopf will report if ran in --verbose mode.

This will help users to debug their operators. Especially, when they use OpenAPI schemas in their CRDs.

Not sure if the check is in the right place and the logger is setup appropriately.

No risky changes.

Issues/PRs

If a patch is not merged, then it is usually because of a schema mismatch of the CRD. For this reason it was discussed to move the internal kopf status to the annotations. #308 #321

Type of changes

• Bug fix (non-breaking change which fixes an issue)

Checklist

• The code addresses only the mentioned problem, and this problem only
• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• If you provide code modification, please add yourself to CONTRIBUTORS.txt

[zincr]

🤖 zincr found 1 problem , 0 warnings

❌ Approvals
✅ Large Commits
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below

---

Approvals

All proposed changes must be reviewed by project maintainers before they can be merged

Not enough people have approved this pull request - please ensure that 1 additional user, who have not contributed to this pull request approve the changes.

• ✅ Approved by PR author @michaelnarodovitch
• ❌ 1 additional approval needed
   

[zincr]

🤖 zincr found 0 problems , 0 warnings

✅ Large Commits
✅ Specification
✅ Dependency Licensing

[lgtm-com]

This pull request introduces 2 alerts when merging 3b75a8b into df4e51b - view on LGTM.com

new alerts:

• 2 for Unused import

[nolar]

I see the pain point and the purpose of this change. Indeed, it was highly unpleasant to notice that K8s API silently drops the data, thus breaking Kopf's functionality without raising any errors.

But I tend to consider this as a K8s's or K8s API's issue.

I'm not sure that we want to implement the K8s server-side logic on the client side (in the framework). It can change over time, and keeping it in sync will require effort. It works as it works. Or it fails.

The original problem is now solved with #331 — the state is persisted in annotations since kopf>=0.27rc1. So, the pain should be less painful (despite it remains for e.g. function results).

---

It is also unreliable to assume that body+patch == patched_body. Some fields can be changed on the server side after we have got our body, before the patch was applied — e.g. by other operators/controllers/k8s itself. Then, the patched_body will be different from body+patch, and it would be totally okay.

What we can do here, is to return the patch's response from the client function, and validate the body-vs-patch match on the top-level (in kopf.reactor.processing.apply_reaction_outcomes()) to see the the returned body matches all the fields in the patch (without merging the patch into the body).

---

There was also an intention to extend the patch object to support regular JSON patches, as K8s and K8s clients prefer it for some reason. Not a priority though. But this change would make it even more difficult to maintain on the client side.

[mnarodovitch]

Thx for all the comments and suggestions. Had to add quite a bit of code. I think quality is OK and changes are safe.

Missing is integration tests with the Kubernetes API responses, and especially integration tests with the status_patch merging. What I did was based of an educated guess from reviewing https://github.com/kubernetes/apiextensions-apiserver. If we wanted to be 100% safe, we would also have to test the PATCH API of the canonical Kubernetes objects.

[nolar]

Conceptually and architecturally, it looks good now. I will dive deep into the detailed code review (especially, tests) a little bit later, if you don't mind.

[nolar]

Implemented in nolar/kopf#527, available since kopf>=0.28rc2