Add server-side apply merge strategy markers #301
Conversation
This commit adds SSA merge strategy markers that allow the API server to granularly merge lists, rather than atomically replacing them. Composition functions use SSA, so this change means that a function can return only the list elements it desires (e.g. tags) and those elements will be merged into any existing elements, without replacing them. For the moment I've only covered two cases: * Lists that we know are sets of scalar values (generated from Terraform sets) * Maps of scalar values (generated from Terraform maps) I'm hopeful that in both of these cases it _should_ be possible to allow the map or set to be granularly merged, not atomically replaced. https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy Signed-off-by: Nic Cope <[email protected]>
| // TODO(negz): Can we reliably add SSA markers for lists of objects? Do we | ||
| // have cases where we're turning a Terraform map of maps into a list of | ||
| // objects with a well-known key that we could merge on? |
There was a problem hiding this comment.
Happy to expand this PR to cover this case too, if it's possible. Are there heuristics I can use to detect TF schemas that become Kubernetes slices-of-structs, where some fields in the struct (e.g. name) uniquely identify them?
There was a problem hiding this comment.
Hi @negz,
Here are some statistics regarding the object lists across the generated APIs of upbound/provider-{aws,azure,gcp}:
provider-aws -> map[embedded:1397 list-of-objects:165]
provider-azure -> map[embedded:1165 list-of-objects:471]
provider-gcp -> map[embedded:1335 list-of-objects:291]
Here, embedded counts represent object lists with a MaxItems constraint of 1 whether that item is optional or not. The importance of this is that we can, in theory, convert these lists (of length 1) into embedded objects. A good example is the spec.forProvider.vpcConfig field of Cluster.eks, which is also the subject of crossplane-contrib/provider-upjet-aws#975. Due to HCL syntax, such embedded objects are also represented as lists, which is also reflected in our generated APIs. Converting such object lists of length 1 into required (MinItems == 1) or optional embedded objects will relieve us from having to specify the list map keys for the list map SSA merge strategy. But we still need to be careful about the possibility of the changing MaxItems constraints if the semantics of the API could potentially allow this.
The list-of-objects counts represent other object lists without a MaxItems constraint or a MaxItems constraint greater than 1. The question is then how we will determine the index set, i.e., the list map keys for those associative lists. We have also examined 10 object lists belonging to MRs of different API groups and 3-4 of them already had their indexing (naming) keys (such as a unique identifier for the list item) and 6-7 of them did not have a natural index set (no subset of the scalar object fields constitute an index set that can uniquely identify a list item).
So my current understanding is that it's not possible to reliably determine the associative list object keys in our generated APIs. We will even need to inject some naming fields into the generated CRD schemas to be able to specify an SSA merge strategy of type map, so that the API server can apply its SSA merge semantics for the objects in the list.
On the other hand, if we just inject a naming field that will not overlap with the already existing fields in the list object schema, then it will potentially result in unnormalized APIs where there is already a set of naming fields (index set mentioned above).
Another complexity we need to handle is any field that goes into the list map keys is not nullable, it has to be either a required field or should have a default value. For injected keys of objects of lists with a length constraint greater than 1 (or no length constraint), we cannot do the defaulting via the kubebuilder markers and we will have to resort to something like defaulting webhooks that will compute a proper unique identifier (index set) for a given list object (two index sets must be equal iff the corresponding objects are equal). So, to do this properly, I believe we will need to have a manual configuration API in upjet through which we can specify the logic to configure & compute an index set.
My suggestion is to leave the object lists case unresolved for now and introduce a configuration API for this purpose in a follow-up PR so that we can address issues similar to crossplane-contrib/provider-upjet-aws#975 via SSA merge strategies.
| // AddServerSideApplyMarkers adds server-side apply comment markers to indicate | ||
| // that scalar maps and sets can be merged granularly, not replace atomically. | ||
| func AddServerSideApplyMarkers(f *Field) { | ||
| switch f.Schema.Type { //nolint:exhaustive |
There was a problem hiding this comment.
We may consider moving this switch-case to inside the buildSchema function. Because there is a similar switch-case block inside of this function.
|
Hi @sergenyalcin, |
…okens - Do not add topological markers for sensitive fields Signed-off-by: Alper Rifat Ulucinar <[email protected]>
@ulucinar This plan is perfectly fine with me. As you mentioned, we want to fix the relevant issue (related to AWS eks.Cluster); also, fixing and merging this PR is a chance to include the |
Description of your changes
Fixes #300 (mostly?)
This commit adds SSA merge strategy markers that allow the API server to granularly merge lists, rather than atomically replacing them. Composition functions use SSA, so this change means that a function can return only the list elements it desires (e.g. tags) and those elements will be merged into any existing elements, without replacing them.
For the moment I've only covered two cases:
I'm hopeful that in both of these cases it should be possible to allow the map or set to be granularly merged, not atomically replaced.
https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy
I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested
I tried to add a case to
TestBuild, but then noticed that it appears to be completely unaware of comments. If you'd like this tested, please give me some direction on how and where to do so.See crossplane-contrib/provider-upjet-aws#968 for a PR generated using this change.