fix: correct parameters order in GenericKubernetesResourceMatcher call to JsonDiff.asJson#3009
Conversation
|
The tests fail, so this seems to break the current behavior. |
|
yes, see the discussion around that: |
|
@xstefank, is there anything I can do about it? How can I run/check those tests? |
| if (!ignoreList.isEmpty()) { | ||
| return nodeIsChildOf(diff, ignoreList); | ||
| } | ||
| return ADD.equals(diff.get(OP).asText()); |
There was a problem hiding this comment.
If you swap the parameter order, I'm pretty sure you have to change the operation here. The issue is that there can be remove or replace, if I recall correctly. So that's maybe why the current parameter order was chosen.
We have used a copy of this code in our own operator to identify changes that need a pod restart. And we never swapped the parameter order due to the (single) add vs. (multiple) remove / replace operations in the diff.
Also, be aware that default values added by K8s will show up here. This is a known limitation of the generic matcher (and is fixed in the SSA based matcher, by filtering out all fields that are not managed by the operator). So you have to anticipate changes in here, that are not just triggered by changes in your desired state, but also from the actual side.
There was a problem hiding this comment.
@Donnerbart I must admit I have no experience with the JOSDK source code, but AFAIK, desired represents the to-be state and actual the as-is state in Kubernetes.
That being said, you might be right, but I don’t think the parameter order of JsonDiff.asJson() depends on the caller’s needs. As I understand it, the outcome of JsonDiff.asJson()—once applied with JsonPatch.apply() to the source—should result in the target parameter.
In JOSDK terms, the patch calculated with JsonDiff.asJson(desiredNode, actualNode)—once applied to actualNode—should result in desiredNode.
As far as I’ve tested, it doesn’t behave that way with the current JsonDiff.asJson() call.
But I may have understood it wrong, and if so, I apologize for wasting everybody’s time with this.
There was a problem hiding this comment.
You are correct about the actual and desired parameters. But I think you might have misunderstood that the outcome of this matcher is not used to calculate a diff that is actually applied to K8s. The purpose of the JsonDiff is just to check if there is a difference between actual and desired, nothing more.
This code is called in the match() code path, not in update(). The actual update of the resource is done via the fabric8 K8s client, either via a client-side client.resource(...).update() or the new server-side-apply client.resource(...).forceConflicts().serverSideApply() approach.
For this code it doesn't matter if you use the diff semantically 100% correct, since we just need to know if actual and desired are equal or not. It's like 10 - 5 vs. 5 - 10 to check if both numbers are equal. As long as the result is not 0, the numbers are not equal.
Also using the "correct" order will not fix the big flaw of the whole approach: on some resources K8s adds default values to actual, that are not defined by your operator in desired. So actual and desired would always mismatch, although you haven't changed a thing in your desired state.
This is why add operations are ignored. This is needed since in this generic matcher we have no idea which fields are managed by our operator, and which are managed by K8s (or other operators). This gap is closed by using SSA and the SSA-based matcher. But without SSA this is the best we can do with a generic matcher.
If you want to change the parameter order for the diff, there is no real gain here, beside being semantically correct. You will still not be able to differentiate changes that come from your desired state vs. other operators. But if you want to change this, you will definitely have to change ignoring the add operation to remove. And I'm pretty sure there is a third replace operation that needs to be considered, so the logic would be more complex. The current implementation is much easier, since we just have to ignore add operations.
There was a problem hiding this comment.
I see your point now., @Donnerbart. Thank you for taking the time to explain it.
…l to JsonDiff.asJson
csviri
force-pushed
the
fix-match-in-GenericKubernetesResourceMatcher
branch
from
c85d891 to
2507bd4
Compare
4 participants
#2997