feat: make observe-only resources reflect diff to upstream in conditions (Synced=False)#926
feat: make observe-only resources reflect diff to upstream in conditions (Synced=False)#926jwefers wants to merge 10 commits intocrossplane:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReconciler now emits a ReconcileForbidden Condition (including a message that may contain the observation diff) when updates are disallowed and the external resource differs. Condition APIs and tests were extended to add and assert the ReconcileForbidden reason and the new message path. Changes
Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant External
participant ConditionsAPI
participant ControllerStatus
Reconciler->>External: Observe(resource)
External-->>Reconciler: ExternalObservation{Exists, ResourceUpToDate, Diff}
alt ResourceUpToDate == true
Reconciler->>ConditionsAPI: ReconcileSuccess()
ConditionsAPI-->>ControllerStatus: Condition(TypeSynced, True, ReasonReconcileSuccess)
else ResourceUpToDate == false AND ShouldUpdate()==false
Reconciler->>ConditionsAPI: ReconcileForbidden(message + Diff)
ConditionsAPI-->>ControllerStatus: Condition(TypeSynced, False, ReasonReconcileForbidden, Message)
else ResourceUpToDate == false AND ShouldUpdate()==true
Reconciler->>External: Update(resource)
External-->>Reconciler: UpdateResult (success / error)
alt Update success
Reconciler->>ConditionsAPI: ReconcileSuccess()
ConditionsAPI-->>ControllerStatus: Condition(TypeSynced, True, ReasonReconcileSuccess)
else Update error
Reconciler->>ConditionsAPI: ReconcileFailed(error)
ConditionsAPI-->>ControllerStatus: Condition(TypeSynced, False, ReasonReconcileError)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Thanks — would you like a focused diff walkthrough of the updated test assertions to verify exact conditioned-status expectations? Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/reconciler/managed/reconciler_legacy_test.go (2)
1767-1818:⚠️ Potential issue | 🟡 MinorDuplicate
.WithObservedGeneration(42)call on Line 1781.Nice work splitting the observe-only test into diff vs. no-diff cases — that's a solid improvement! Quick question though: is the double
.WithObservedGeneration(42).WithObservedGeneration(42)on Line 1781 intentional? It looks like a copy-paste artifact. The second call is a no-op but reads like a mistake.- want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42)) + want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42))(I notice the same pattern at Lines 2199 and 2249 in pre-existing tests, so it may be worth a quick cleanup pass across the file.)
2148-2153:⚠️ Potential issue | 🟡 MinorMisleading
reasonstring on Line 2151 — says "ReconcileSuccess" but expectsReconcilePaused.The expected condition on Line 2148 is
ReconcilePaused(), but the reason string on Line 2151 reads"Managed resource should acquire Synced=False/ReconcileSuccess status condition."— could you update this to match the actual expectation? Something like"Managed resource should acquire Synced=False/ReconcilePaused status condition when diff exists but update is not allowed."would be clearer for anyone debugging a test failure.- reason := `Managed resource should acquire Synced=False/ReconcileSuccess status condition.` + reason := `Managed resource should acquire Synced=False/ReconcilePaused status condition when update is not allowed and a diff exists.`
🤖 Fix all issues with AI agents
In `@pkg/reconciler/managed/reconciler.go`:
- Around line 1474-1488: The branch checking observation.ResourceUpToDate is
unreachable because earlier logic returns when observation.ResourceUpToDate is
true; remove the if/else and replace it with a single status.MarkConditions call
that sets xpv1.ReconcilePaused().WithMessage(fmt.Sprintf("External resource
differs from desired state, but will not update: %s", observation.Diff)) when
!policy.ShouldUpdate() is true, keeping the surrounding context that this occurs
in the path where policy.ShouldUpdate() is false; update/remove references to
xpv1.ReconcileSuccess() in this block accordingly so only the paused condition
is emitted here.
🧹 Nitpick comments (4)
pkg/reconciler/managed/reconciler.go (1)
1473-1488: The debug log at line 1473 says "Reconciliation succeeded" but the condition may now beReconcilePaused— is this intentional?Small thing: the log message says "Reconciliation succeeded" regardless of whether the resource is up to date or has a drift. Now that the condition can be
ReconcilePaused(indicating drift), the log message could be confusing when correlating logs with conditions. Would you consider adjusting the log message for the drift case, or is there a reason you'd like to keep it uniform?Also, a quick question on the design choice:
ReconcilePausedis also used when reconciliation is explicitly paused via annotation or empty policy (lines 943-950). Reusing the same Reason for "drift detected but updates not allowed" could make it harder for users to distinguish between "my resource is paused" and "my resource has detected drift." Was a new Reason (e.g.,ReconcileDriftDetected) considered, or is theWithMessage()differentiation sufficient for the intended use cases? Thanks for your thoughts on this!pkg/reconciler/managed/reconciler_modern_test.go (3)
1787-1787: Nit: duplicate.WithObservedGeneration(42)call.Line 1787 chains
.WithObservedGeneration(42).WithObservedGeneration(42)— the second call is redundant. It's functionally harmless (just overwrites with the same value), but it looks like a copy-paste artifact. Would you mind removing the duplicate?Suggested fix
- want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42)) + want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42))
2087-2138: Consider setting a non-emptyDiffin the observation to validate the message end-to-end.Great new test case — thank you for covering this path! One thing I noticed: the
ObserveFnreturnsResourceUpToDate: falsebut leavesDiffas the zero value (empty string), which means the condition message will be"External resource differs from desired state, but will not update: "— ending with a colon and space but no actual diff content. This is a valid scenario, but it might be worth also (or instead) setting a meaningfulDiffvalue to verify the full message formatting. This would more closely simulate the real-world scenario described in the PR objective (users seeing what actually differs).Suggested enhancement
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) { - return ExternalObservation{ResourceExists: true, ResourceUpToDate: false}, nil + return ExternalObservation{ResourceExists: true, ResourceUpToDate: false, Diff: "spec.forProvider.size: 10 != 20"}, nil },And update the expected condition accordingly:
- want.SetConditions(xpv1.ReconcilePaused().WithObservedGeneration(42).WithMessage("External resource differs from desired state, but will not update: ")) + want.SetConditions(xpv1.ReconcilePaused().WithObservedGeneration(42).WithMessage("External resource differs from desired state, but will not update: spec.forProvider.size: 10 != 20"))
1773-1824: Good update — would anObserveOnlySuccessfulObserveWithDiffcounterpart be useful?The rename and
ResourceUpToDate: trueupdate make this test correctly cover the no-diff scenario for ObserveOnly. I noticeManagementPolicyImmutableWithDiffcovers the drift-detected path for a no-Update policy, which exercises the same!policy.ShouldUpdate()code path. That said, since the PR's primary motivation is observe-only resources reflecting diffs (as referenced in the discussion), would adding an explicitObserveOnlySuccessfulObserveWithDifftest case help document the intended behavior for that specific policy? Just a thought — no strong opinion if you feel the immutable test already covers it sufficiently.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/reconciler/managed/reconciler.go (1)
474-484:⚠️ Potential issue | 🟡 MinorNice cleanup of the dead branch — thanks for addressing the earlier feedback! 🎉
One small thing I wanted to flag: at this point
observation.ResourceUpToDateis guaranteedfalse(thetruecase returns early at line 1445), so the unconditionalReconcilePausedis correct. However,observation.Diffis an optional field — not all providers populate it. WhenDiffis"", the condition message becomes:
"External resource differs from desired state, but will not update: "That trailing
": "with no content could look a bit odd to users. Could we handle the empty-diff case to keep the message clean? Something like:Suggested tweak
- status.MarkConditions( - xpv1.ReconcilePaused(). - WithMessage(fmt.Sprintf( - "External resource differs from desired state, but will not update: %s", - observation.Diff))) + msg := "External resource differs from desired state, but will not update." + if observation.Diff != "" { + msg = fmt.Sprintf("External resource differs from desired state, but will not update: %s", observation.Diff) + } + status.MarkConditions(xpv1.ReconcilePaused().WithMessage(msg))What do you think — does this match the intent? Happy to hear if there's a reason to always include the diff suffix.
As per coding guidelines, "Conditions must be actionable for users (not developers), stable/deterministic, with proper Type/Reason/Message format" and "Ensure all error messages are meaningful to end users, not just developers - avoid technical jargon, include context about what the user was trying to do".
Signed-off-by: Julian Wefers <851823+jwefers@users.noreply.github.com>
Signed-off-by: Julian Wefers <851823+jwefers@users.noreply.github.com>
Signed-off-by: Julian Wefers <851823+jwefers@users.noreply.github.com>
Signed-off-by: Julian Wefers <851823+jwefers@users.noreply.github.com>
Signed-off-by: Julian Wefers <851823+jwefers@users.noreply.github.com>
Signed-off-by: Julian Wefers <851823+jwefers@users.noreply.github.com>
…derabbit Signed-off-by: Julian Wefers <851823+jwefers@users.noreply.github.com>
jwefers
force-pushed
the
feature/sync-false-on-observeonly-diffs
branch
from
d440842 to
d8085af
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apis/common/v1/condition.go (1)
58-63:⚠️ Potential issue | 🟠 MajorMissing re-export of
ReasonReconcileForbidden— breaks API consistency.Thanks for adding the
ReconcileForbidden()function at line 141! I noticed that this block re-exports all other synced reason constants (ReasonReconcileSuccess,ReasonReconcileError,ReasonReconcilePaused) from thecommonpackage, butReasonReconcileForbiddenis missing. Consumers of thev1package who need to check a condition's reason (e.g.,condition.Reason == xpv1.ReasonReconcileForbidden) won't find the constant here and would need to importcommondirectly.Could you add the missing re-export to keep things consistent?
Suggested fix
// Reasons a resource is or is not synced. const ( ReasonReconcileSuccess = common.ReasonReconcileSuccess ReasonReconcileError = common.ReasonReconcileError ReasonReconcilePaused = common.ReasonReconcilePaused + ReasonReconcileForbidden = common.ReasonReconcileForbidden )pkg/reconciler/managed/reconciler_modern_test.go (1)
1773-1824:⚠️ Potential issue | 🟡 MinorMissing test for ObserveOnly with a diff?
Thanks for splitting the tests into diff vs. no-diff variants! I notice there's
ObserveOnlySuccessfulObserveNoDiffbut no correspondingObserveOnlySuccessfulObserveWithDifftest. Since the PR's primary goal is to surface diffs for observe-only resources (e.g., showingSynced=False/ReconcileForbidden), would it make sense to add a test for the pureObserveOnlypolicy withResourceUpToDate: false?The
ManagementPolicyImmutableWithDifftest covers a policy withoutUpdate, but it includesCreate,Delete, andLateInitialize— which might follow a slightly different code path than pureObserveOnly. Could you confirm whether the observe-only case is covered elsewhere, or would you be open to adding it here?
🤖 Fix all issues with AI agents
In `@pkg/reconciler/managed/reconciler_legacy_test.go`:
- Around line 1819-1833: Update the test case description for
"ObserveOnlyWithDiffSyncError" so the reason string matches the expected
condition: change the text that currently mentions "ReconcilePaused" to
"ReconcileForbidden" (this is the string assigned in the test case's reason
field within the map key "ObserveOnlyWithDiffSyncError"), ensuring consistency
with the expected xpv1.ReconcileForbidden() condition used later in the
MockStatusUpdate assertion.
In `@pkg/reconciler/managed/reconciler_modern_test.go`:
- Around line 2099-2105: The test sets an expected ReconcileForbidden message
that ends with a trailing colon because the mocked ExternalObservation.Diff is
empty; fix by populating the mock observation's Diff (e.g., "mock diff") in the
modern test path used by MockStatusUpdate/MockSubResourceUpdateFn so the
reconciler appends a non-empty diff, and update the expected message in
want.SetConditions (and any related assertions) to include that diff string to
match the real formatter used by the reconciler.
In `@pkg/reconciler/managed/reconciler.go`:
- Around line 1474-1484: The message built in the ReconcileForbidden condition
includes observation.Diff directly, which can be empty and produce a trailing
colon; update the block that calls
status.MarkConditions(xpv1.ReconcileForbidden().WithMessage(fmt.Sprintf(...)))
to detect when observation.Diff is empty (or only whitespace) and use a fallback
message (e.g., "external resource differs from desired state") or omit the colon
in that case so the final message is clean; keep the existing call to
status.MarkConditions and xpv1.ReconcileForbidden(), only change how the message
string is constructed around observation.Diff.
🧹 Nitpick comments (2)
pkg/reconciler/managed/reconciler_legacy_test.go (1)
2133-2184: NewManagementPolicyImmutableWithDifftest case looks solid.This nicely mirrors the
ObserveOnlyWithDiffSyncErrorcase but for the broader "immutable" policy (Observe + LateInitialize + Create + Delete, without Update). The expectedReconcileForbiddencondition with the diff message is consistent.A thought: both this test and
ObserveOnlyWithDiffSyncErrorhardcode the message format"External resource differs from desired state, but will not update: mock diff". If this message format ever changes in the reconciler, both tests break. That's probably acceptable since it's intentional contract testing, but I wanted to call it out — is this message format defined as a constant or format string in the reconciler? If so, it might be worth referencing that constant here for maintainability.#!/bin/bash # Check if the forbidden message is defined as a constant/format string echo "=== Searching for the forbidden diff message format ===" rg -n "External resource differs" --type=go -g '!*_test.go' rg -n "errFmt.*Forbidden\|errFmt.*Diff\|msgFmt.*update" --type=go -g '!*_test.go' | head -10pkg/reconciler/managed/reconciler_modern_test.go (1)
1787-1787: Redundant.WithObservedGeneration(42)call.Small nit —
.WithObservedGeneration(42)is chained twice here. It's idempotent so it won't break anything, but it looks like a copy-paste artifact. Mind cleaning it up for clarity?✏️ Suggested fix
- want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42)) + want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42))
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Julian Wefers <851823+jwefers@users.noreply.github.com>
jwefers
force-pushed
the
feature/sync-false-on-observeonly-diffs
branch
from
2f99bcd to
a21d83e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/reconciler/managed/reconciler_legacy_test.go (1)
1833-1837:⚠️ Potential issue | 🟡 MinorAlign the failure reason with the ReconcileForbidden scenario.
This case asserts
ReconcileForbidden, but the failure reason still says “successful observation,” which will confuse future readers if the diff mismatch occurs. Could you reword it to reflect the forbidden/ diff case?Suggested edit
- reason := "With ObserveOnly, a successful managed resource observation should be reported as a conditioned status." + reason := "With ObserveOnly and a diff, Synced should be False with ReconcileForbidden."As per coding guidelines, “Check for proper test case naming and reason fields.”
🤖 Fix all issues with AI agents
In `@pkg/reconciler/managed/reconciler_legacy_test.go`:
- Around line 2133-2135: The test case "ManagementPolicyImmutableWithDiff" has a
reason string that says "Reconciliation without being allowed to update should
trigger a Sync Paused condition..." but the test actually asserts for the
ReconcileForbidden condition; update the reason field for the
ManagementPolicyImmutableWithDiff case to reference ReconcileForbidden (e.g.,
"Reconciliation without being allowed to update should trigger a
ReconcileForbidden condition and a requeue after a long wait") so the case name,
reason and assertions are consistent.
In `@pkg/reconciler/managed/reconciler_modern_test.go`:
- Around line 2088-2105: Update the test case's reason string to accurately
describe the expected ReconcileForbidden outcome: replace the misleading
"Successful reconciliation..." message with a concise description such as
"ReconcileForbidden when external resource differs but Update managementPolicy
is missing (skips update and requeues long)" so it matches the asserted
condition ReconcileForbidden in the test that sets the ManagementPolicies and
expects the Synced=False/ReconcileForbidden condition in the MockStatusUpdate
for asModernManaged/newModernManaged.
🧹 Nitpick comments (1)
pkg/reconciler/managed/reconciler_modern_test.go (1)
1787-1788: Trim duplicateWithObservedGenerationcalls.Each of these lines chains
WithObservedGeneration(42)twice. It’s harmless but redundant and adds noise to the expectation.Suggested edit
- want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42)) + want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42))Also applies to: 2153-2154, 2203-2204
| "ManagementPolicyImmutableWithDiff": { | ||
| reason: "Reconciliation without being allowed to update should trigger a Sync Paused condition and a requeue after a long wait.", | ||
| args: args{ |
There was a problem hiding this comment.
Reason text says “Sync Paused,” but expected condition is ReconcileForbidden.
Could we update the case reason to match the actual condition being asserted? It’ll make failures far easier to interpret.
Suggested edit
- reason: "Reconciliation without being allowed to update should trigger a Sync Paused condition and a requeue after a long wait.",
+ reason: "Reconciliation without update permission should trigger ReconcileForbidden and a requeue after a long wait.",As per coding guidelines, “Check for proper test case naming and reason fields.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "ManagementPolicyImmutableWithDiff": { | |
| reason: "Reconciliation without being allowed to update should trigger a Sync Paused condition and a requeue after a long wait.", | |
| args: args{ | |
| "ManagementPolicyImmutableWithDiff": { | |
| reason: "Reconciliation without update permission should trigger ReconcileForbidden and a requeue after a long wait.", | |
| args: args{ |
🤖 Prompt for AI Agents
In `@pkg/reconciler/managed/reconciler_legacy_test.go` around lines 2133 - 2135,
The test case "ManagementPolicyImmutableWithDiff" has a reason string that says
"Reconciliation without being allowed to update should trigger a Sync Paused
condition..." but the test actually asserts for the ReconcileForbidden
condition; update the reason field for the ManagementPolicyImmutableWithDiff
case to reference ReconcileForbidden (e.g., "Reconciliation without being
allowed to update should trigger a ReconcileForbidden condition and a requeue
after a long wait") so the case name, reason and assertions are consistent.
| reason: "Successful reconciliation with upstream diff and skipping update should trigger a requeue after a long wait.", | ||
| args: args{ | ||
| m: &fake.Manager{ | ||
| Client: &test.MockClient{ | ||
| MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { | ||
| mg := asModernManaged(obj, 42) | ||
| mg.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionObserve, xpv1.ManagementActionLateInitialize, xpv1.ManagementActionCreate, xpv1.ManagementActionDelete}) | ||
|
|
||
| return nil | ||
| }), | ||
| MockUpdate: test.NewMockUpdateFn(errBoom), | ||
| MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { | ||
| want := newModernManaged(42) | ||
| want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionObserve, xpv1.ManagementActionLateInitialize, xpv1.ManagementActionCreate, xpv1.ManagementActionDelete}) | ||
| want.SetConditions(xpv1.ReconcileForbidden().WithObservedGeneration(42).WithMessage("External resource differs from desired state, but will not update due to missing 'Update' managementPolicy. Diff:\nmock diff")) | ||
|
|
||
| if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { | ||
| reason := `Managed resource should acquire Synced=False/ReconcileForbidden status condition.` |
There was a problem hiding this comment.
Reword the case reason to match ReconcileForbidden.
This test expects ReconcileForbidden, but the case reason says “Successful reconciliation,” which is misleading. Could you tighten the wording?
Suggested edit
- reason: "Successful reconciliation with upstream diff and skipping update should trigger a requeue after a long wait.",
+ reason: "Reconciliation with upstream diff and no update permission should set ReconcileForbidden and requeue.",As per coding guidelines, “Check for proper test case naming and reason fields.”
🤖 Prompt for AI Agents
In `@pkg/reconciler/managed/reconciler_modern_test.go` around lines 2088 - 2105,
Update the test case's reason string to accurately describe the expected
ReconcileForbidden outcome: replace the misleading "Successful
reconciliation..." message with a concise description such as
"ReconcileForbidden when external resource differs but Update managementPolicy
is missing (skips update and requeues long)" so it matches the asserted
condition ReconcileForbidden in the test that sets the ManagementPolicies and
expects the Synced=False/ReconcileForbidden condition in the MockStatusUpdate
for asModernManaged/newModernManaged.
1 participant
Description of your changes
Previously, a managed resource that is not allowed to Update upstream, did not reflect whether a diff exists between
.spec.forProviderand the external resource, resp..status.atProvider.This was unhelpful when migrating to Crossplane where, even when starting with Observe-Only, the user cannot get confidence that their spec is actually reflective of upstream (except checking every resource manually).
Not anymore: The condition Synced now goes False with reason
ReconcileForbiddenin this case.E.g. when using Argocd, you now get nice little red broken hearts, allowing to gradually updating the spec until it reflects upstream and giving the confidence to turn on all management policies.
The unit tests that deal with no-update resource were refactored and duplicated, now dealing with both cases, diff and no diff separately.
Fixes crossplane/crossplane#6983
TODO:
I have:
./nix.sh flake checkto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.