crossplane / terrajet

Generate Crossplane Providers from any Terraform Provider
https://crossplane.io
Apache License 2.0
290 stars 38 forks source link

resource is re-created even if there is already an external name assigned and the new name is not overriden #122

Closed stoehdoi closed 2 years ago

stoehdoi commented 2 years ago

What happened?

When deleting the external resource, terrajet controller re-creates the object in the external provider but does not update the external name annotation in the CR.

How can we reproduce it?

  1. Install provider-tf-aws
  2. Create a custom resource (e.g. TransitGatewayVpcAttachment)
  3. Wait for the external resource creation to complete successfully
  4. Delete the external resource via AWS Console/cli
  5. Wait for the external resource to be recreated
  6. Compare the external-name annotation value in the CR with the actual name of the re-created external resource

Workaround

Deleting the external-name annotations forces the controller to update the CR with the latest external name

muvaf commented 2 years ago

Hi @stoehdoi ! The external name annotation is used as the identifier of the resource in Crossplane. While in some cases, we're able to recreate the resources if it doesn't exist, i.e. the cases where user can decide on the identifier. In cases where the ID is decided by the provider, like AWS VPC, it's risky to create another resource and override the external name annotation. In fact, we never replace external name annotation in any case to make sure we don't leak anything unless we are sure that we just created a new resource and it needs to be saved somewhere. So, I don't think we'll implement a mechanism that can replace an existing external name annotation due to the risk involved there.

muvaf commented 2 years ago

In cases where the ID is decided by the provider, like AWS VPC, it's risky to create another resource and override the external name annotation.

Quoting myself, in cases where the ID is generated by the provider, we don't want the resource to be recreated with a new ID if there is an existing external name on the annotation because of the risks involved. I realized on the second read of your comment that what you expected was recreation+update of external name but the provider did only recreation. The correct behavior should've been that if you delete the resource from cloud console and its ID is generated by the cloud provider, then we shouldn't re-create it because it should always query the ID we already have on the resource, if there is one. So, the re-creation in that case is actually a bug.

Now I see your point that if it re-creates, it should update to not leak the resource, however, it shouldn't have re-created in the first place.

stoehdoi commented 2 years ago

Yes, exactly @muvaf. The resource was recreated even though TGW attachments get a random ID assigned by the cloud provider. In my opinion, one of the strongest advantages of crossplane over pure terraform is the reconciliation logic that ensures the desired state is met. Don't you think you would lose a major benefit if you did not recreate the resource?

muvaf commented 2 years ago

@stoehdoi just opened an issue in Crossplane repo to define the behavior and apply it to all https://github.com/crossplane/crossplane/issues/2704

I think there are some inconsistencies in how we do the re-creation and the discussion in that issue should help.