-
Notifications
You must be signed in to change notification settings - Fork 902
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge options are declared per patch, but are actually scoped to the entire MR being composed #5629
Comments
I'm not super familiar with the code base, please let me know if this analysis is not correct. I think this raises a few questions:
I'm willing to contribute with whatever is necessary, but at this point I think we need to further discuss what direction we should move to solve this. |
Great details to explain this behavior, how it can be reproduced, and some analysis of the code base - really impressive effort here @LCaparelli, that accelerates folks understanding of the issue! I don't have the full deep context of the codebase, but I didn't catch anything obviously wrong with your analysis yet. One thing to note is that the behavior to consider the So this behavior has indeed been present for awhile - I'm anticipating there's a good reason for it, perhaps to avoid conflicting behavior across patches, but I don't have the depth of context to really know why. That being said, a couple questions for you:
image:
- name: argoproj/rollouts-demo
tag: blue
# claim user can optionally add additional entries to this array
- name: container2
tag: some-image:v0.0.1 |
Thanks for taking the time to have a look at this @jbw976!
We ended up setting a static limit to the number of additional containers (usually 5 sidecars is already quite a lot), so that we can have patches for specific indexes while still keeping the abstraction level we wanted for the main container (sidecars can be something we don't maintain ourselves, so we want that to be flexible and allow users to set whatever container fields they want). # the static patches solve temporary issues regarding containers duplication
- type: FromCompositeFieldPath
fromFieldPath: spec.additionalContainers[0]
toFieldPath: spec.forProvider.manifest.spec.template.spec.containers[1]
- type: FromCompositeFieldPath
fromFieldPath: spec.additionalContainers[1]
toFieldPath: spec.forProvider.manifest.spec.template.spec.containers[2]
- type: FromCompositeFieldPath
fromFieldPath: spec.additionalContainers[2]
toFieldPath: spec.forProvider.manifest.spec.template.spec.containers[3]
- type: FromCompositeFieldPath
fromFieldPath: spec.additionalContainers[3]
toFieldPath: spec.forProvider.manifest.spec.template.spec.containers[4]
- type: FromCompositeFieldPath
fromFieldPath: spec.additionalContainers[4]
toFieldPath: spec.forProvider.manifest.spec.template.spec.containers[5] Will probably solve this more elegantly with
I'll give that a swing (hopefully) soon, still hadn't had time to do it. 😅 Replying right now just to share the workaround we found. |
What happened?
I have a patch that alters an array at index 0. Then I have another patch that alters the same array, but by appending to it, using the
appendSlice
merge option.This second patch sources data from an optional input field in the claim. For simplicity's sake, in the example I'll list, this input is always null/empty. This implies that the patch should be ignored altogether, but you'll see that's not entirely accurate.
Since the
appendSlice
merge option is only specified in the second patch, I expected only the input from that patch to be appended to the resulting array at the managed resource being composed.Instead, the merge option also affects the first patch, causing the changes meant to be written at index 0 to actually be written to a new element of the array being appended to the original array.
Initial State at the managed resource (irrelevant fields omitted for brevity):
After updating the image to v0.0.2 in the claim, actual outcome:
Expected outcome:
How can we reproduce it?
XRD
Composition
You can either install the kubernetes-provider locally, that provides the
Object
managed resource as an API, or directly install this CRD + RBAC which I adapted from the provider's current version to make sure the provider itself has no role in this behavior:Object CRD + RBAC
After applying the provided XRD and composition, apply this workload:
Check the resulting object, it should hold a single container (which is fine and expected). Now patch the image, simulating a bump:
The resulting
Object
(MR being composed) now has 2 containers:Code Analysis
It seems we render the entire composed resource based on the patches first in this loop:
crossplane/internal/controller/apiextensions/composite/composition_pt.go
Line 200 in a2f63b3
Then apply in this loop that comes right after:
crossplane/internal/controller/apiextensions/composite/composition_pt.go
Line 261 in a2f63b3
When applying, we incorporate the merge options defined in any of the patches from the composition:
crossplane/internal/controller/apiextensions/composite/composition_pt.go
Lines 273 to 275 in a2f63b3
This effectively means that the merge options are scoped to the entire MR being composed, not scoped to the patch where they're declared.
Additionally, because the source field for this second patch is null/empty, this also means that the merge option defined in patch that should be ignored causes side-effects.
What environment did it happen in?
Crossplane version: v1.15.2
The text was updated successfully, but these errors were encountered: