Skip to content
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

Add merge strategy annotations to managed resources #300

Closed
Tracked by #4951
negz opened this issue Sep 13, 2023 · 10 comments · Fixed by #301
Closed
Tracked by #4951

Add merge strategy annotations to managed resources #300

negz opened this issue Sep 13, 2023 · 10 comments · Fixed by #301
Assignees
Labels
enhancement New feature or request

Comments

@negz
Copy link
Member

negz commented Sep 13, 2023

What problem are you facing?

The new FunctionComposer added in crossplane/crossplane#4500 uses server-side-apply (crossplane/crossplane#4047). This means that if you're using a Composition in mode: Pipeline Crossplane (technically, the API server) is able to correctly merge arrays of objects, among other things.

For this to work, the CRD of the composed resource needs to include merge strategy OpenAPI extensions. For example by default arrays of objects will be replaced entirely - the CRD needs to specify //listType: map and a //listMapKey to change this behavior.

How could Crossplane help solve your problem?

We should start adding these annotations to Crossplane types. The core types defined in crossplane-runtime (e.g. ConditionedStatus) would be a great start, plus very common types like AWS tags.

@negz negz added the enhancement New feature or request label Sep 13, 2023
@negz
Copy link
Member Author

negz commented Sep 13, 2023

CC @ulucinar - just an FYI.

@negz
Copy link
Member Author

negz commented Nov 15, 2023

@ulucinar would it be relatively straightforward to add the //+mapType: granular comment marker to all AWS tags, like https://github.com/upbound/provider-aws/blob/v0.43.1/apis/acm/v1beta1/zz_certificate_types.go#L50?

I think this would have a big impact for folks, e.g. in crossplane-contrib/function-patch-and-transform#50.

@CodyRay
Copy link

CodyRay commented Nov 15, 2023

Will this be possible for uses like helm-provider that can contain arbitrary data. Our usecase is with patch and transform we have a base containing an array inside the value and we'd like to patch in an additional array from the XRD claim. Today we'd do that with resources[i].patches[i].policy.mergeOptions but I noticed that is one of the things that is not supported with function-patch-and-transform

@negz negz transferred this issue from crossplane/crossplane Nov 16, 2023
@negz
Copy link
Member Author

negz commented Nov 16, 2023

@CodyRay No, I don't think this will work for a runtime.RawExtension since the API server has no idea about the structure of the data inside that field and thus SSA can't merge it.

I'm not entirely sure how to handle that case. We could reintroduce mergeOptions, but I'm a little reluctant to do that given that in most cases folks should not use it and should use SSA instead.

Could you raise an issue to track your use case WRT merging/appending RawExtension fields?

@negz negz self-assigned this Nov 16, 2023
@sttts
Copy link

sttts commented Nov 16, 2023

This might be a naive question, but who is the other actor you want to merge with? The provider that completes lists provided by a function?

@negz
Copy link
Member Author

negz commented Nov 16, 2023

@sttts Yes, it's usually (always?) the provider. There's still parts of spec that are "shared" - e.g. AWS tags.

@CodyRay
Copy link

CodyRay commented Nov 16, 2023

Could you raise an issue to track your use case WRT merging/appending RawExtension fields?

Would that issue belong in https://github.com/crossplane/upjet or https://github.com/crossplane-contrib/function-patch-and-transform ?

@negz
Copy link
Member Author

negz commented Nov 16, 2023

@CodyRay Option 3 - https://github.com/crossplane/crossplane. 😉

@erikgb
Copy link

erikgb commented Dec 12, 2023

@CodyRay Did you ever create an issue for this?

@CodyRay
Copy link

CodyRay commented Dec 13, 2023

I didn't end up creating a ticket. We found some really problematic behavior before we made it to production with mergeOptions where array items weren't removed from the destination when they are deleted from the source composite resource. Since I think that in the long term server-side apply is going to be better, we ended up pivoting to use a function and I suspect we'll eventually use https://github.com/crossplane-contrib/function-go-templating or do the appending within our helm chart instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants