Better support for non-deterministic external-names by updating critical annotations

[twobiers]

Description of your changes

This PR ensures that critical annotations are stored when a resource is created. Some resources have non-deterministic external-names, which are never updated when the mangementPolicies don't contain LateInitialize. So far, there is an implicit contract that an Observation updating the external-name will be eventually stored as part of the LateInitialize process. However, that should only affect updates to the spec and not the annotations like external-name. More Context can be found here: https://github.com/crossplane/crossplane/issues/5918

I'm not sure how a unit test can be expressed, as I'm not really familiar with the setup. If the change is approved, I think it would make sense to backport it aswell.

Fixes:

• https://github.com/crossplane/crossplane/issues/5918
• https://github.com/crossplane-contrib/provider-upjet-azuread/issues/159
• https://github.com/crossplane-contrib/provider-upjet-aws/issues/1567
• https://github.com/crossplane-contrib/provider-upjet-azure/issues/802

Maybe fixes aswell:

• https://github.com/crossplane-contrib/provider-upjet-aws/issues/1482

I have:

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

Need help with this checklist? See the cheat sheet.

[jeanduplessis]

@twobiers, we are interested in progressing the solution you propose in this PR. Are you currently able to give this attention? If so, would you mind resolving the conflicts and pushing up an updated version? If you aren't currently able to give this attention would you be happy for us to take it over and get it over the line (you'll be credited with the contribution)?

[twobiers]

@jeanduplessis Thanks for your kind comment. I'm able to work on this and have rebased the branch onto master.

[jeanduplessis]

Thanks, @twobiers. @erhancagirici will be doing a thorough review in the next few days, with a specific focus on making sure it's compatible with Upjet's async approach.

[bobh66]

Closing and reopening to kick the CI

[coderabbitai[bot]]

Walkthrough

The changes refactor the critical annotation update mechanism to use JSON merge patch operations for annotation-only updates rather than modifying entire objects. A new reconciliation block calls this mechanism after resource creation or existence detection. Test scaffolding is extended to mock patch operations across multiple test suites.

Changes

Cohort / File(s)	Summary
Core annotation update logic pkg/reconciler/managed/api.go	Introduces package-level variable criticalAnnotations and updates RetryingCriticalAnnotationUpdater.UpdateCriticalAnnotations to use JSON merge patch strategy. Builds a map of critical annotations present on the object, short-circuits if none exist, and applies patch via client.Patch with MergePatchType. On conflict, fetches latest object and reapplies annotations while preserving existing changes.
Core annotation tests pkg/reconciler/managed/api_test.go	Updates test helper from setLabels to setAnnotations and replaces MockUpdate with MockPatch mocks to align with annotation-based patch operations.
Reconciler integration pkg/reconciler/managed/reconciler.go	Adds new blocks to call UpdateCriticalAnnotations after resource creation or existence detection. Logs debug messages and emits warning events on update failures, with status requeue.
Test scaffolding for legacy reconciler pkg/reconciler/managed/reconciler_legacy_test.go	Extends mock client setup across numerous test cases by adding MockPatch: test.NewMockPatchFn(nil) to enable patch operation handling in test scenarios.
Test scaffolding for modern reconciler pkg/reconciler/managed/reconciler_modern_test.go	Adds MockPatch field with test.NewMockPatchFn(nil) across multiple test case configurations and applies minor cosmetic formatting adjustments to align MockGet entries.

Sequence Diagram

sequenceDiagram
    participant Reconciler
    participant UpdateCritical as UpdateCriticalAnnotations
    participant Client
    participant APIServer

    Reconciler->>UpdateCritical: UpdateCriticalAnnotations(obj)
    UpdateCritical->>UpdateCritical: Build map of critical<br/>annotations from object
    alt No annotations present
        UpdateCritical-->>Reconciler: Early return
    else Annotations found
        UpdateCritical->>Client: Patch(MergePatchType,<br/>metadata.annotations)
        alt Patch succeeds
            Client->>APIServer: Apply JSON merge patch
            APIServer-->>Client: Updated object
            Client-->>UpdateCritical: Success
            UpdateCritical-->>Reconciler: Return
        else Conflict error
            Client-->>UpdateCritical: Conflict
            UpdateCritical->>Client: Get(latest object)
            Client->>APIServer: Fetch latest
            APIServer-->>Client: Latest object
            Client-->>UpdateCritical: Object data
            UpdateCritical->>UpdateCritical: Reapply critical<br/>annotations
            UpdateCritical->>Client: Patch(MergePatchType)
            Client->>APIServer: Retry merge patch
            APIServer-->>Client: Updated
            Client-->>UpdateCritical: Success
            UpdateCritical-->>Reconciler: Return
        end
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~28 minutes

Areas requiring extra attention:

• pkg/reconciler/managed/api.go: Review merge patch JSON construction logic, conflict retry mechanism, and field ownership handling in the Patch call—verify that only metadata.annotations are updated and that conflict recovery preserves concurrent changes.
• pkg/reconciler/managed/reconciler.go: Verify placement of the new UpdateCriticalAnnotations calls ensures critical annotations are persisted after both creation and existence detection without introducing unintended requeue loops.
• pkg/reconciler/managed/reconciler_legacy_test.go and reconciler_modern_test.go: Ensure MockPatch additions align with the new patch-based code paths and that test expectations accurately reflect the modified reconciliation flow.

Poem

🐰 Patches now whisper soft and true, Annotations dance in merge-patch brew, No conflicts here—we fetch, we mend, Critical notes on which we depend! 🪵

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Better support for non-deterministic external-names by updating critical annotations' accurately summarizes the main change: implementing critical annotation updates to support non-deterministic external names.

✨ Finishing touches

• [ ] 📝 Generate docstrings

🧪 Generate unit tests (beta)

• [ ] Create PR with unit tests
• [ ] Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[twobiers]

I just had the idea that a function in the pipeline could also ensure those annotations are persisted. It would not be an ideal solution, but at least a possible hotfix while async is not fully supported.