crossplane-contrib / provider-aws

Crossplane AWS Provider
Apache License 2.0
440 stars 374 forks source link

Aurora DBInstance .spec.forProvider field is modified and makes the CR invalid #2106

Open drewwells opened 1 month ago

drewwells commented 1 month ago

Crossplane is setting spec.forProvider.{masterUsername,kmsKeyID,port} on a spec that is disallowed when those fields are set.

What happened?

Crossplane attempts to recreate a deleted aurora instance and fails because the forProvider block was modified to an invalid state. kmsKeyID, masterusername and port were populated.

For example, these errors are seen:

The requested DB Instance will be a member of a DB Cluster. Set master user name for the DB Cluster.
create failed: cannot create DBInstance in AWS: InvalidParameterCombination:
      The requested DB Instance will be a member of a DB Cluster. Set database endpoint
      port number for the DB Cluster.\n\tstatus code: 400

How can we reproduce it?

Provision an aurora dbcluster and dbinstance Delete the dbinstance from AWS console Trigger crossplane to reconcile the dbinstance which is missing an aws resource now Crossplane reports confusing errors that the dbcluster is missing fields port, masterUsername, and kmsKeyID. It's actually because the dbinstance now includes those fields.

What environment did it happen in?

provider-aws version: 0.46.0 crossplane: 1.17.0

MisterMX commented 1 month ago

Crossplane does not support external deletion of resources. The deletion process should always be triggered from within Crossplane.

If there is a way to prevent this error from happening without breaking existing implementations feel free to open a pull request.

drewwells commented 1 month ago

Do you know why the provider is modifying the spec fields? Its changing fields to make the spec configuration invalid

Drew

On October 22, 2024, Anthony Sottile @.***> wrote:

Crossplane does not support external deletion of resources. The deletion process should always be triggered from within Crossplane.

If there is a way to prevent this error from happening without breaking existing implementations feel free to open a pull request.

— Reply to this email directly, view it on GitHub https://github.com/crossplane-contrib/provider-aws/issues/2106#issuecomment-2429256607, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB2ME2R34JQOW35D4NXV7TZ4ZFUVAVCNFSM6AAAAABP73RLCKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMRZGI2TMNRQG4 . You are receiving this because you authored the thread.Message ID: @.***>

MisterMX commented 3 weeks ago

The controller intentionally late-initializes all unset, optional properties with the values retrieved from AWS. I haven't looked into the code specifically but I think you issue is caused by the external deletion of the DBInstance which the controller does not seem to support.

drewwells commented 3 weeks ago

Those properties can't be set, they have to be left blank. Repo steps are pretty easy.

Create an aurora dbinstance CR Save the modified spec properties, rename it (metadata.name) Create it again You will need to remove the 3 properties specified in the description to create a new dbinstance object

MisterMX commented 3 weeks ago

Now I'm not sure anymore if I understand the problem correctly. If you think there is an issue with how the controller builds and sends the creation / update request to AWS feel free to open a PR with a fix.

drewwells commented 3 weeks ago

Okay, is there a dummy's guide to get started on developing for provider-aws?

The issue is spec fields get populated as the provider reconciles the object. This is a pretty universal problem with this provider though. Controller should stick to updating the status field (in my opinion). Especially in cases like this one, when the object becomes invalid by updates from the controller.

-Drew

On October 31, 2024, Anthony Sottile @.***> wrote:

Now I'm not sure anymore if I understand the problem correctly. If you think there is an issue with how the controller builds and sends the creation / update request to AWS feel free to open a PR with a fix.

— Reply to this email directly, view it on GitHub https://github.com/crossplane-contrib/provider-aws/issues/2106#issuecomment-2449669815, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB2ME4TJQXGEVZJIJNEEF3Z6IK6PAVCNFSM6AAAAABP73RLCKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBZGY3DSOBRGU . You are receiving this because you authored the thread.Message ID: @.***>

MisterMX commented 2 weeks ago

Every controller populates empty, optional fields with the state of the resource it receives from the external API (here: AWS). That is common across most Crossplane provider and we won't change that.

However, each controller can decide individually which field should be set in the request it sends to AWS. Given the case above: If a single-instance specific field should not be part of the response, we can remove it from the creation or update request.

drewwells commented 2 weeks ago

That's what I was hoping to hear. So the provider should zero out those 3 fields before sending a create/update request. I wasn't sure what the expected operation here was.

On November 4, 2024, Anthony Sottile @.***> wrote:

Every controller populates empty, optional fields with the state of the resource it receives from the external API (here: AWS). That is common across most Crossplane provider and we won't change that.

However, each controller can decide individually which field should be set in the request it sends to AWS. Given the case above: If a single-instance specific field should not be part of the response, we can remove it from the creation or update request.

— Reply to this email directly, view it on GitHub https://github.com/crossplane-contrib/provider-aws/issues/2106#issuecomment-2455207343, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB2ME7W3KJSBLEJBAGQEQ3Z66QKZAVCNFSM6AAAAABP73RLCKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJVGIYDOMZUGM . You are receiving this because you authored the thread.Message ID: @.***>