fix(controllers): use custom CriticalAnnotationUpdater

[wotolom]

Description of your changes

updated description

as suggested by #1953 (comment), this adds a custom implementation of managed.RetryingCriticalAnnotationUpdater - before the change from crossplane/crossplane-runtime#526 - that is used in all controllers.

This aims to revert back the suddenly being effective/working one-time-initialization (of spec.forProvider) based on the AWS Create() response in most many controllers.

Fixes #1951

OG description

This change removes all field/parameter initialization based on the Create() response in ACK generated controllers.
(I did not mark this as breaking, bc this change aims to revert back to pre v0.45 behaviour.)

Fixes #1951

The ACK generated controllers always were using the AWS Create()-Call response to set fields in cr.spec.forProvider and cr.status.atProvider. However, before provider-aws v0.45.0, this did actually never propagate to the resource in the k8s-cluster.
This can be verified by debugging a MR in v0.44 and one in v0.45.
When trying to find out, what change had this effect, respectively why this setting of fields did not propagated to the MR before, I was only able to narrow it down to this commit that bumped - among others - crossplane-runtime to v1.14.1.
Note: This PR comment also was observing this changed behavior, linked to the mentioned commit.

some findings, while debugging:

• https://github.com/crossplane/crossplane-runtime/blob/c8449480cfc562f7b347a96dd5f800199e5a7eb3/pkg/reconciler/managed/reconciler.go#L1096
  this is the place in reconcile.go, where data in (managed.data).spec and .status got lost pre v0.45 and now only .status is lost
• https://github.com/crossplane/crossplane-runtime/pull/526/files#diff-02531dd5ee50ca9686a2ef7c82d0d94ba47ee6f8d07ac963fc839347fb5271adL170-R174
  they changed the description of this function from "reset" to "might get reset"
  Note: However I was not able (yet) to properly test/confirm if this is actually what screwed the controllers over

ToDos before removing Draft-state:

• adapt failing unit tests

I have:

• Read and followed Crossplane's [contribution process].
• Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Tested manually for rds.DBInstance, rds.DBCluster, docdb.DBInstance, docdb.DBCluster and neptune.DBCluster

[MisterMX]

@wotolom thanks for pointing that out. This is quite a tricky breaking change that wasn't pointed out in the release notes of https://github.com/crossplane/crossplane-runtime.

I wonder if it is possible to fix this issue without removing the code from Create? After all there are some cases in AWS where certain information can only be retrieved with the create response. I think it might be better to just change the logic in isUpToDate to adapt to the field changes (if possible).

If we want a quick fix for this issue I guess the easiest solution is to use a custom implementation of managed.RetryingCriticalAnnotationUpdater that is equivalent to the behaviour before crossplane/crossplane-runtime#526 was merged. It can be added as a controller option using managed.WithCriticalAnnotationUpdater. This also results in much less code changes for the hotfix while sticking to code that we know works.

We can then decide how we want to move forward with this issue for v0.46+.

[wotolom]

I wonder if it is possible to fix this issue without removing the code from Create? After all there are some cases in AWS where certain information can only be retrieved with the create response. I think it might be better to just change the logic in isUpToDate to adapt to the field changes (if possible).

also postCreate() would be a legit place to set/unset some fields. E.g. in the specific case of rds.DbInstance where we need to control which fields are (late)init depending on whether parameter DBClusterIdentifier is set or not.

If we want a quick fix for this issue I guess the easiest solution is to use a custom implementation of managed.RetryingCriticalAnnotationUpdater that is equivalent to the behaviour before crossplane/crossplane-runtime#526 was merged. It can be added as a controller option using managed.WithCriticalAnnotationUpdater. This also results in much less code changes for the hotfix while sticking to code that we know works.

However I did go with this solution for now as hotfix, due the uncertainty how many/which resources/controllers are running into this issue besides rds.DBInstance.

[MisterMX]

I am not very happy with this package name. Can we move it to /pkg/utils/reconciler/managed and maybe import it as custommanaged if necessary?

Also it would be good to add a doc string to this package to explain the reason for this special implementation.

[wotolom]

Makes sense, done! :)

[MisterMX]

LGTM. Thank you very much @wotolom for this fix!

[github-actions]

Successfully created backport PR #1956 for release-0.45.