LastAsyncOperation isn't cleared after async success: should it read previousAsyncOperation ?

[gberche-orange]

What happened?

The following conditions are not friendly for crossplane users: the LastAsyncOperation condition reports a failure however a more recent AsyncOperation reports a success. This is confusing as this may be interpreted as an error. Does the condition LastAsyncOperation rather aims at describing the previousAsyncOperation ?

  conditions:
  - lastTransitionTime: "2023-07-04T14:16:16Z"
    reason: Available
    status: "True"
    type: Ready
  - lastTransitionTime: "2023-07-04T14:16:15Z"
    reason: ReconcileSuccess
    status: "True"
    type: Synced
  - lastTransitionTime: "2023-07-04T13:36:06Z"
    message: 'apply failed: Error, failed to create instance kuttl-medium-mysql-instance-6q268-6gs5q:
      googleapi: Error 400: Invalid request: Invalid label: Label value ''a-string-with-dot-.-which-is-invalid-char'
      violates format constraints. The value can only contain lowercase letters, numeric
      characters, underscores and dashes. The value can be at most 63 characters long.
      International characters are allowed.., invalid: '
    reason: ApplyFailure
    status: "False"
    type: LastAsyncOperation
  - lastTransitionTime: "2023-07-04T14:16:16Z"
    reason: Finished
    status: "True"
    type: AsyncOperation

here is the current declaration for this condition
https://github.com/upbound/upjet/blob/018e6404baef55366ddd86d8d9cbf04c9bcb2426/pkg/resource/conditions.go#L30-L40

It was previously named AsyncOperationFinished into 3953d88

How can we reproduce it?

First provision a gcp cloud sql with an invalid label

apiVersion: sql.gcp.upbound.io/v1beta1                                                                                                                                                                             
kind: DatabaseInstance                                                                                                                                                                                             
metadata:                                                                                                                                                                                                          
  name: kuttl-medium-mysql-instance-6q268-6gs5q                                                                                                                                                                    
spec:                                                                                                                                                                                                              
  forProvider:                                                                                                                                                                                                     
[...]
    databaseVersion: MYSQL_8_0_28                                                                                                                                                                                  
    settings:                                                                                                                                                                                                      
[...]
      userLabels:                                                                                                                                                                                                  
        k8s_cluster_api: a-string-with-dot-.-which-is-invalid-char

Then fix it

apiVersion: sql.gcp.upbound.io/v1beta1                                                                                                                                                                             
kind: DatabaseInstance                                                                                                                                                                                             
metadata:                                                                                                                                                                                                          
  name: kuttl-medium-mysql-instance-6q268-6gs5q                                                                                                                                                                    
spec:                                                                                                                                                                                                              
  forProvider:                                                                                                                                                                                                     
[...]
    databaseVersion: MYSQL_8_0_28                                                                                                                                                                                  
    settings:                                                                                                                                                                                                      
[...]
      userLabels:                                                                                                                                                                                                  
        k8s_cluster_api: a-valid-string-without-invalid-chars

[mbbush]

I think the issue here is that AsyncOperation has a misleading name. As I understand it, that condition is supposed to signify when a resource has started an async terraform operation which has not yet completed. The condition becomes true once the operation finishes, regardless of whether it succeeds or fails (that's why it says "Finished", not "Success").

I've also observed cases where the AsyncOperation is not present, despite the provider debug logs showing an ongoing async update (I updated a aws kafka cluster, which takes tens of minutes to finish), so I think that either my understanding of what the AsyncOperation condition is supposed to represent is incorrect, or we aren't applying it consistently.