Consider extending the crate with merge functionality

[soenkeliebau]

We (and by that I mean @teozkr) have created functionality to merge objects with objects of the same type in our framework.

The use case that we are trying to solve with this is that our crds can have a hierarchy of configuration objects, with the same object being defined globally, then again for every role and every role can then have many rolegroups.

global/
├─ role1/
│  ├─ rolegroup11/
├─ role2/
│  ├─ rolegroup12/
│  ├─ rolegroup21/

When processing this, we basically iterate over all rolegroups and then need to create the config that is valid for that rolegroup by merging configs that are set "further up the tree" into the config set for this rolegroup, with anything that is "overridden" in more specific configs taking precedence.

The following example is taken from the rustdoc comments and might help make it clearer:

 # use stackable_operator::config::merge::Merge;

 #[derive(Merge, Debug, PartialEq, Eq)]
 struct Foo {
     bar: Option<u8>,
     baz: Option<u8>,
 }

 let mut config = Foo {
     bar: Some(0),
     baz: None,
 };
 config.merge(&Foo {
     bar: Some(1),
     baz: Some(2),
 });
 assert_eq!(config, Foo {
     bar: Some(0), // Overridden by `bar: Some(0)` above
     baz: Some(2), // Fallback is used
 });

The full code can be found here.

This has been working well for us, but we now do have a use case where it would be useful being able to merge structs from k8s-openapi as well.

Specifically what we'd like to do is allow users of our operators the ability to specify an "override pod template", which the operater will then merge with the pod template for statefulsets it would create before writing these. This would allow a sort of final, catch-all mechanism to adjust operator behavior and work around missing features/bugs.

Disclaimer: You absolutely have to know what you are doing here! We are aware of the risks of a function like this!

That being said, there could probably be more use-cases where merging functionality might turn out useful.

The purpose of this ticket is to check if there'd be an interest in functionality like this, we'd be happy to do the work of pulling out the code and creating a PR, but wanted to discuss the usefulness before actually doing the work :)

[Arnavion]

https://github.com/Arnavion/k8s-openapi/commit/d3718bfd050987ab6aa2d605fa94e2e7998b9385 ?

[nightkr]

Oh wow, managed to miss that completely. Will have to investigate!

[nightkr]

Hm, it looks like this doesn't implement strategic merges, right? So merging the following two documents:

apiVersion: v1
kind: Pod
spec:
  containers:
  - name: nginx
    image: nginx
---
apiVersion: v1
kind: Pod
spec:
  containers:
  - name: nginx
    args: ["-t"]

Would give you the following document:

apiVersion: v1
kind: Pod
spec:
  containers:
  - name: nginx
    image: nginx
  - name: nginx
    args: ["-t"]

But containers is actually an ordered map, so the correct behaviour would have been to merge the two container definitions with the same name, like so:

apiVersion: v1
kind: Pod
spec:
  containers:
  - name: nginx
    image: nginx
    args: ["-t"]

[nightkr]

In fact, the documented concat behaviour doesn't match any of K8s' merge strategies (see x-kubernetes-map-type and x-kubernetes-list-type).

[Arnavion]

Yes, IIUC @dpc 's use case wasn't directly related to merge patches, just a custom merge-like thing they were doing locally, so they didn't implement it to follow the Kubernetes strategy specifically. But also IIUC their use case would be unaffected if we did implement the strategy for Vec<T> in accordance with Kubernetes. ie we can repurpose the trait rather than making a whole new one.

So it seems the generated impl incorporate the x-kubernetes-patch-merge-key and x-kubernetes-patch-strategy properties of the fields, and DeepMerge::merge_from would need to take them as a parameter.

[nightkr]

Either that or you could use the strategy to replace the DeepMerge::merge_from call.

[lfrancke]

Thank you both for your work on this!