Fix synchronization between claim and the counterpart composite #4896

pedjak · 2023-10-25T15:11:21Z

Description of your changes

Removing of a claim annotation, label, field and/or array member is properly propagated to the counterpart composite by using k8s server-side apply feature. The same approach is used by propagating composite changes back to the claim.

Implementing the server-side apply requested the following changes on the claim reconciler:

Given that a submitted patch needs to contain all fields that a reconciler care about and wants to take the ownership, the existing configurators and binders cannot perform updates. Instead, they are only adding the requested changes to the patch, that gets applied in one shot.
The claim configurator requires access to actual claim and composite, but produces a claim patch, similar goes for the composite configurator. Hence, Configurator was not applicable anymore got removed.
Given that WithClaimConfigurator and WithCompositeConfigurator are used only for mocking in the unit tests, their visibility is reduced.
Field manager name follows the convention: crossplane-claim-controller/<CLAIM_GROUP_KIND_LOWERCASE>
Claim status subresource is still modified by sending UPDATE requests, because the controller should own it completely. That is expressed explicitly by setting the status field ownership.
Given that Applicator is not used anymore, Reconciler.client is redeclared to client.Client
Unit tests reworked/updated to verify the new implementation

Following the proposal stated in #4581, the following composite fields are propagated back to the claim:

spec.compositionRef
spec.compositionSelector
spec.compositionUpdatePolicy
spec.compositionRevisionSelector
status minus status.conditions

Fixes:

An e2e test added for verifying the above fixes and catching future regressions.

Part of #4047 epic.

Implements: #4581

I have:

Read and followed Crossplane's contribution process.
Run make reviewable to ensure this PR is ready for review.
Added or updated unit tests.
Added or updated e2e tests.
~~[ ] Linked a PR or a docs tracking issue to document this change.~~
~~[ ] Added backport release-x.y labels to auto-backport this PR.~~

sttts · 2023-10-25T15:25:56Z

internal/controller/apiextensions/claim/configurator.go

@@ -125,35 +126,37 @@ func ConfigureComposite(_ context.Context, cm resource.CompositeClaim, cp resour
 	}

 	claimSpecFilter := xcrd.GetPropFields(wellKnownClaimFields)
-	ucp.Object["spec"] = filter(spec, claimSpecFilter...)
+	cpPatch.(*composite.Unstructured).Object["spec"] = filter(spec, claimSpecFilter...)


this is ugly. Why not use that type for cpPatch directly? A type cast is always suspicious, unguarded even more so.

I see why you do that. Still ugly, but I don't see a better way.

Me neither, but I see this sort of casting often in this part of the codebase. This gives me a signal that we have just one implementation of resource.Composite or resource.CompositeClaim interface and perhaps it would be better to use it directly where needed. Using casts mean that we have single data model, and hence there is no much benefit of hiding it behind some interfaces.

I have removed type casting in this commit. Let me know what do you think. I like it.

internal/controller/apiextensions/claim/configurator.go

sttts · 2023-10-25T15:31:03Z

internal/controller/apiextensions/claim/configurator.go

-	if err := merge(ucm.Object["status"], ucp.Object["status"],
+	udesiredCm, ok := cmPatch.(*claim.Unstructured)
+	if !ok {
+		return nil


what does this mean? Similar to above: don't cast. Use the proper types directly.

internal/controller/apiextensions/claim/reconciler.go

internal/controller/apiextensions/claim/configurator.go

internal/controller/apiextensions/claim/reconciler.go

pedjak · 2023-11-29T18:11:15Z

@turkenh #5062 got merged and I have rebased this one on the top of the latest master, answered your comments as well. PTAL.

turkenh

I tried an upgrade test and noticed that the composite keep getting updates, see below:

updates.mp4

Reproduction steps:

Install Crossplane v1.14.2
Follow the steps here.
Upgrade Crossplane to a local build from this branch.
Run kubectl get xnopresources.nop.example.org -w

Could you investigate that?

Well, actually this is true for just regular install as well without upgrading Crossplane:

Reproduction steps:

Install Crossplane with a local build from this branch.
Follow the steps here.
Run kubectl get xnopresources.nop.example.org -w

install.mp4

It become True/True only after around 5mins:

internal/controller/apiextensions/claim/configurator.go

internal/controller/apiextensions/claim/configurator_test.go

internal/controller/apiextensions/claim/reconciler.go

pedjak · 2023-12-11T06:39:13Z

@turkenh I have addressed your comments. PTAL.

turkenh · 2023-12-11T08:53:31Z

@turkenh I have addressed your comments. PTAL.

Did you have a chance to do some testing and see what was going on here?

Removing of a claim annotation, label, field and/or array member is properly propagated to the counterpart composite by using [k8s server-side apply](https://kubernetes.io/docs/reference/using-api/server-side-apply/) feature. The same approach is used by propagating composite changes back to the claim. Implementing the server-side apply requested the following changes on the claim reconciler: * Given that a submitted patch needs to contain all fields that a reconciler care about and wants to take the ownership, the existing configurators and binders cannot perform updates. Instead, they are only adding the requested changes to the patch, that gets applied in one shot. * The claim configurator requires access to actual claim and composite, but produces a claim patch, similar goes for the composite configurator. Hence, `Configurator` was not applicable anymore got removed. * Given that `WithClaimConfigurator` and `WithCompositeConfigurator` are used only for mocking in the unit tests, their visibility is reduced. * Field manager name follows the convention: `crossplane-claim-controller/<CLAIM_GROUP_KIND_LOWERCASE>` * Claim status subresource is still modified by sending UPDATE requests, because the controller should own it completely. That is expressed explicitly by setting the status field ownership. * Given that `Applicator` is not used anymore, `Reconciler.client` is redeclared to `client.Client` * unit tests reworked/updated to verify the new implementation Following the proposal stated in crossplane#4581, the following composite fields are propagated back to the claim: * `spec.compositionRef` * `spec.compositionSelector` * `spec.compositionUpdatePolicy` * `spec.compositionRevisionSelector` * `status` minus `status.conditions` Signed-off-by: Predrag Knezevic <predrag.knezevic@upbound.io>

pedjak · 2023-12-11T18:11:27Z

Did you have a chance to do some testing and see what was going on #4896 (review)

Yes, fixed, but please double check.

Signed-off-by: Predrag Knezevic <predrag.knezevic@upbound.io>

turkenh

Great work @pedjak, see how many (long running issues) fixed by this single PR 💪
Thanks for bearing with me during reviews 🙌

pedjak requested review from negz and a team as code owners October 25, 2023 15:11

pedjak requested a review from turkenh October 25, 2023 15:11

pedjak self-assigned this Oct 25, 2023

sttts reviewed Oct 25, 2023

View reviewed changes

internal/controller/apiextensions/claim/configurator.go Outdated Show resolved Hide resolved

sttts reviewed Oct 25, 2023

View reviewed changes

internal/controller/apiextensions/claim/reconciler.go Outdated Show resolved Hide resolved

sttts reviewed Oct 25, 2023

View reviewed changes

internal/controller/apiextensions/claim/reconciler.go Outdated Show resolved Hide resolved

pedjak force-pushed the fix-claim-xr-sync branch 2 times, most recently from e44abc9 to 44eb7ed Compare October 30, 2023 12:57

turkenh reviewed Nov 14, 2023

View reviewed changes

pedjak force-pushed the fix-claim-xr-sync branch 2 times, most recently from ab2cef4 to 53a3081 Compare November 29, 2023 18:09

turkenh reviewed Dec 5, 2023

View reviewed changes

pedjak force-pushed the fix-claim-xr-sync branch from 53a3081 to 71f75c4 Compare December 11, 2023 06:37

pedjak force-pushed the fix-claim-xr-sync branch 3 times, most recently from 44d4181 to f0f7c14 Compare December 11, 2023 17:40

Addressed reviewers comments

b390a75

Signed-off-by: Predrag Knezevic <predrag.knezevic@upbound.io>

pedjak force-pushed the fix-claim-xr-sync branch from f0f7c14 to b390a75 Compare December 12, 2023 10:01

turkenh approved these changes Dec 13, 2023

View reviewed changes

turkenh merged commit 841258b into crossplane:master Dec 13, 2023
16 of 17 checks passed

turkenh mentioned this pull request Dec 13, 2023

Generate XR name locally by appending random suffix to the claim name #4780

Closed

5 tasks

pedjak mentioned this pull request Dec 13, 2023

Tag deleted from claim spec is added back #3421

Closed

phisco mentioned this pull request Dec 15, 2023

tests(e2e): properly teardown resources in TestPropagateFieldsRemovalToXR #5141

Merged

6 tasks

pedjak mentioned this pull request Dec 18, 2023

Indicate in log/events the composite readiness #5145

Merged

2 tasks

negz mentioned this pull request Jan 22, 2024

Clear Ownership of Fields Between Claim and Composite #4581

Closed

negz added the release/breaking-behavior-change label Jan 23, 2024

negz mentioned this pull request Jan 25, 2024

Put SSA-based claim controller behind a feature flag #5287

Closed

negz mentioned this pull request Mar 25, 2024

Functions can't modify XR metadata or spec #5434

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix synchronization between claim and the counterpart composite #4896

Fix synchronization between claim and the counterpart composite #4896

pedjak commented Oct 25, 2023 •

edited

sttts Oct 25, 2023

sttts Oct 25, 2023

pedjak Oct 26, 2023

pedjak Oct 26, 2023

sttts Oct 25, 2023

pedjak commented Nov 29, 2023

turkenh left a comment •

edited

pedjak commented Dec 11, 2023

turkenh commented Dec 11, 2023

pedjak commented Dec 11, 2023

turkenh left a comment •

edited

Fix synchronization between claim and the counterpart composite #4896

Fix synchronization between claim and the counterpart composite #4896

Conversation

pedjak commented Oct 25, 2023 • edited

Description of your changes

sttts Oct 25, 2023

Choose a reason for hiding this comment

sttts Oct 25, 2023

Choose a reason for hiding this comment

pedjak Oct 26, 2023

Choose a reason for hiding this comment

pedjak Oct 26, 2023

Choose a reason for hiding this comment

sttts Oct 25, 2023

Choose a reason for hiding this comment

pedjak commented Nov 29, 2023

turkenh left a comment • edited

Choose a reason for hiding this comment

pedjak commented Dec 11, 2023

turkenh commented Dec 11, 2023

pedjak commented Dec 11, 2023

turkenh left a comment • edited

Choose a reason for hiding this comment

pedjak commented Oct 25, 2023 •

edited

turkenh left a comment •

edited

turkenh left a comment •

edited