`KubernetesDependentResource` should preserve metadata on updates (currently replace used)

[csviri]

This should be revisited, and it should do smart "apply". Maybe support two modes, that will remove parts which are not specified by the desired state, like additional labels?

[csviri]

One question is if we should completely replace the specs on update (or maybe have feature flag for this?). Thus just replace everything with the values from desired value.
In this case default values and mutation hooks will always apply anyways on update(?).
Well on mutation hook this is not necessarily true, since it might be configured just for create.
Can we generally assume, that a correct configuration/implementation for a mutation hook is to have it always configured on both create and on update (and have the implementation idempotent of course).

Considering (special cases) ConfigMap or Secret the values should be probably replaced. In Deployment if a sidecart is added it's not that straighforward.

What do you think @metacosm ?

[metacosm]

The problem is that it's difficult to offer a generic solution since what needs to be preserved is quite operator- or use-case-dependent… I think we should wait and gather more feedback on this topic before implementing something.

[csviri]

The problem is that the current implementation simply does not do for example optimistic locking, and does not preserve any annotations or labels added or finalizers added. It's an interesting question what to assume here on who adds the finalizers and how. But since there is API for that and it's a common case to add additional finalizers.

In general probably the easiest and simple what we can do now is just replace the spec on the actual resource. And that is general enough, will preserve all the metadata. If somebody wants a different more tailored behavior can still override the update.

[csviri]

Just for the record, experimented with this:

What would be a little nicer, since would preserves / overrides values from metadata not just from spec. The problem is with the move operation generated from JsonPatch.

 public R preserveValuesForDesiredFromActual(R actual, R desired) {
    try {
      var desiredSpecNode = objectMapper.valueToTree(desired);
      var actualSpecNode = objectMapper.valueToTree(actual);
      var diffJsonPatch = JsonDiff.asJson(desiredSpecNode, actualSpecNode);
      ArrayNode patchToApply = objectMapper.createArrayNode();

      for (int i = 0; i < diffJsonPatch.size(); i++) {
        ObjectNode objectNode = (ObjectNode) diffJsonPatch.get(i);
        String operation = objectNode.get("op").asText();
        switch (operation) {
          case "add":
            patchToApply.add(objectNode);
            break;
          case "move":
            // todo don't generate move (fix is to not do compat in JsonDiff)
            objectNode.set("op", TextNode.valueOf("add"));
            patchToApply.add(objectNode);
            break;
          case "replace":
          case "remove":
            break;
          default:
            throw new IllegalStateException("Not expected operation: " + operation);
        }
      }
      var patchedDesire = JsonPatch.apply(patchToApply, desiredSpecNode);
      return objectMapper.treeToValue(patchedDesire, resourceClass);
    } catch (JsonProcessingException e) {
      throw new IllegalStateException(e);
    }
  }

It would be simple to do modifyng the JsonDiff (io.fabric8.zjsonpatch) , removing the compactDiffs

  public static JsonNode asJson(final JsonNode source, final JsonNode target) {
        final List<Diff> diffs = new ArrayList<Diff>();
        List<Object> path = new LinkedList<Object>();
        /**
         * generating diffs in the order of their occurrence
         */
        generateDiffs(diffs, path, source, target);
        /**
         * Merging remove & add to move operation
         */
        compactDiffs(diffs);

        return getJsonNodes(diffs);
    }

Just we would have to copy the source code.