adding a patch item operation with a context and other patch / patch derived operation changes

[shawkins]

Description

This is to address #3067 the current patch operation with only the modified item is dangerous when there are concurrent modifications.

This requires a two argument patch call.

I looked at adding the resourceVersion to a GET call, but the kubernetes support is "no older than" - https://kubernetes.io/docs/reference/using-api/api-concepts/#the-resourceversion-parameter so you are not guaranteed to get back the object of that version. Based upon that, this is the simpler and backwards compatible change.

I'm not sure about the the appropriate way to clone the item in the patch call - or even if that is needed. I can't see any guidance on whether it's expected that the operation may modify the item passed into it.

I also removed the retry logic on patching - it seems you will get a 422 error if the patch can't be applied rather than a conflict.

@rohanKanojia let me know if this is on the right track.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change
• Chore (non-breaking change which doesn't affect codebase;
  test, version modification, documentation, etc.)

Checklist

• Code contributed by me aligns with current project license: Apache 2.0
• I Added CHANGELOG entry regarding this change
• I have implemented unit tests to cover my changes
• I have added/updated the javadocs and other documentation accordingly
• No new bugs, code smells, etc. in SonarCloud report
• I tested my code in Kubernetes
• I tested my code in OpenShift

[centos-ci]

Can one of the admins verify this patch?

[metacosm]

The way we clone in the Java operator SDK: https://github.com/java-operator-sdk/java-operator-sdk/blob/master/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/CustomResourceCache.java#L64-L71 (serialize -> deserialize)

[shawkins]

@metacosm thanks, I've added that clone method as a utility method on the Serialization class.

@rohanKanojia I see there are a lot of internal calls to the single argument path function - given that these have concurrency issues, is that something that needs addressed as well?

[shawkins]

@iocanel I see a relatively recent change to use edit(visitors) via a patch call. I've updated this pr to base that upon a protected method to create the builder when possible - is there currently any way to find/construct the builder for a given type programatically so that this method is not needed?

@iocanel and others - what is the expectation for cloning via constructing a builder with a reference object? for example - new PodBuilder(pod) - from what I can see in the code this is a shallow copy. So for example in calls to edit where an item is associated with the context, that item may be edited. Is this expected? I see in some places this is referred to as a clone:

kubernetes-client/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/apps/v1/DeploymentRollingUpdater.java

Line 41 in 74cc63d

protected Deployment createClone(Deployment obj, String newName, String newDeploymentHash) {

@rohanKanojia another concern is that some of the calls to patch have overriding logic, such as to toggle the cascading flag - that logic will be applied in the case of a call to patch with an object, but it will not be applied when calling the new methods with a patch string. Should there be common enforcement of that logic?

[shawkins]

@iocanel and others - what is the expectation for cloning via constructing a builder with a reference object? for example - new PodBuilder(pod) - from what I can see in the code this is a shallow copy.

Actually this seems to only be an issue for Resource derived from CustomResource (I just assumed it was the same for other classes like Pod). I'll spawn this off as a separate issue - #3076

[shawkins]

The other approach instead of introducing the two argument method would be to expose the withItem method on the Resource interface, so that you could make the patch call:

client.configMaps().inNamespace(currentNamespace).withName(name).withItem(base).patch(modified);

Yet another change would be to deprecate the patch(item) method call, and instead call it something like apply. Edit: The go client does separate out apply and applyStatus.

[rohanKanojia]

I don't remember why this cascading is used in combination with patch. Since cascading is deprecated, I don't think we should use it in our newer methods

[shawkins]

@rohanKanojia @manusa since it seems like it will be a while for these changes to be picked up, I've expanded this pr to include the fixes for #3089 and #3066 as well. With these changes all of the patch logic is consolidated to a single method in OperationSupport.

The expected semantics are:

edit - will call patch with the only the changes between the item passed into the edit operation and the end result.

patch(item) - same behavior as before, similar to an apply in that it tries to force the given changes in the form of a patch against the lastest version from the server. The only difference from the old logic is that retry has been removed.

patch(base, item) - computes the diff between the given objects. This is more consistent that the previous logic which would build the patch off of the server's current version. If base is null, this is acting like the go client's apply in that it creates a merge patch just based upon the item - but with the go client the apply options are explicit, so you get to choose the patch type. merge is a safe default in that not everything supports strategic merge. This could be changed to supply the PatchContext as well - or even not exposed yet as a user callable api method under the assumption that the user should call edit instead. WDYT?

applyStatus(item) - effectively patch(null, item), but against the status subresource.

This pr does not update the mock support for other patch strategies or patching the status subresource. Those would come after #3092

[rohanKanojia]

Some tests are failing. Are they related to this PR?

 Error:  Failures: 
Error:    SecurityContextConstraintsTest.testEdit:117 expected: <1> but was: <0>
Error:  Errors: 
Error:    OpenshiftRoleBindingTest.testPatchWithOnlySubjects:160 » NullPointer
Error:    OpenshiftRoleBindingTest.testPatchWithUserNamesAndGroupsAndNoSubjects:183 » NullPointer
Error:    OpenshiftRoleBindingTest.testPatchWithUserNamesAndGroupsAndOverwriteSubjects:204 » NullPointer
[INFO] 
Error:  Tests run: 744, Failures: 1, Errors: 3, Skipped: 4
[INFO] 
[INFO] ------------------------------------------------------------------------

[shawkins]

Some tests are failing. Are they related to this PR?

Yes they are. There's an assumption down in OperationSupport.checkName that a HasMetadata object will always return a non-null metadata. However these and another test were not written this way. I've now relaxed that assumption, rather than continuing to refine the tests.

[shawkins]

The expected semantics now are:

edit / editStatus - will call patch with the only the changes between the item passed into the edit operation and the end result.

patch(item) - same behavior as before, similar to an apply in that it tries to force the given changes in the form of a patch against the latest version from the server. The only difference from the old logic is that retry has been removed.

patchStatus(item) - generates a merge patch from the item for the status subresource

replaceStatus(item) - similar to replace, but for the status subresource.

patch(patchcontext, item) - see https://github.com/fabric8io/kubernetes-client/pull/3070/files#diff-ab93a52030a548489deda26a47cfd58f43a665df5b1ab6f3e6db56e6c55418ddR38

[metacosm]

LGTM apart from minor wording in exception message.

[sonarcloud]

SonarCloud Quality Gate failed.

1 Bug
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

40.0% Coverage
4.1% Duplication