ecr.repository: Add forceDelete to delete images

[chlunde]

Description of your changes

Add flag forceDelete to the CRD directly under spec. This enabled the
force flag on the DeleteRepository AWS API call, which will also delete
images if the repository contains any.

Without this option, AWS blocks deletion of repositories without images.

Updates #495

Implementation notes:

• I'm testing the proper flags in the mock. Not sure if you think this is abuse of the current mocks, but I think it is important to test, and without a different mock approach I don't know of any other way than this.
• We should consider the name forceDelete carefully, does it make sense for recursive delete on S3 too? I guess it's pretty generic (unlike recursiveDelete which makes sense for S3 and ECR but not "delete an active bundle" as discussed in Force Delete Flag for some resources  #495)

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

• Unit tests
• Test IRL with a non-empty repo and ensure it is not deleted without the flag
• Set the flag and ensure it is deleted

[ulucinar]

Hi @chlunde,
Do I understand correctly that in your case involving ECR, AWS already provides the force-delete option as part of its API?

It may make sense to put such a flag under spec.forProvider because it’s a parameter that’s part of the AWS API (it's for the provider). In the case mentioned here, AWS does not include a similar parameter in its API.

I also have a concern about naming: Per API conventions, we had better use declarative API names instead of imperative ones. Maybe something like forceDeletion or deletionPolicy.

@hasheddan, what's your opinion here?

[chlunde]

@ulucinar you're right, in this case the provider does support the option. The name is simply force under the delete API request, so if I should move it I would like to defer to you and @hasheddan for the name :)

I notice secrets manager has forProvider.forceDeleteWithoutRecovery - but this name matches the provider option and forProvider.force is surely not the correct name :D

[hasheddan]

Apologies for the delayed response here folks! @chlunde I think putting this under forProvider as @ulucinar suggests is the right move, and I also think calling it something like forceDelete is appropriate here. The only other route I would consider going is an embedded struct under forProvider with something like delete.force, but I agree that just using forceDelete is more aligned with fields like skipFinalSnapshotBeforeDeletion, etc.

[hasheddan]

LGTM! Thanks @chlunde! Did you test this against an actual ECR repository?

[chlunde]

Yeah, I tested on an actual cluster. I have not re-tested yet after moving the field.

[chlunde]

re-tested OK:

2021-08-02T20:20:31.613+0200	DEBUG	controller-runtime.manager.events	Warning	{"object": {"kind":"Repository","name":"example","uid":"70fd7f05-cd0e-4267-9ff0-21b326c8972d","apiVersion":"ecr.aws.crossplane.io/v1alpha1","resourceVersion":"4837"}, "reason": "CannotDeleteExternalResource", "message": "failed to delete the repository resource: RepositoryNotEmptyException: The repository with name 'example' in registry with id '21913760....' cannot be deleted because it still contains images"}

$ kubectl patch repository.ecr example --type=merge --patch '{"spec": {"forProvider": {"forceDelete": true}}}'
repository.ecr.aws.crossplane.io/example patched

2021-08-02T20:25:19.267+0200	DEBUG	provider-aws	Reconciling	{"controller": "managed/repository.ecr.aws.crossplane.io", "request": "/example"}
2021-08-02T20:25:19.534+0200	DEBUG	provider-aws	Successfully requested deletion of external resource	{"controller": "managed/repository.ecr.aws.crossplane.io", "request": "/example", "uid": "70fd7f05-cd0e-4267-9ff0-21b326c8972d", "version": "5338", "external-name": "example", "deletion-timestamp": "2021-08-02 20:20:21 +0200 CEST"}
2021-08-02T20:25:19.535+0200	DEBUG	controller-runtime.manager.events	Normal	{"object": {"kind":"Repository","name":"example","uid":"70fd7f05-cd0e-4267-9ff0-21b326c8972d","apiVersion":"ecr.aws.crossplane.io/v1alpha1","resourceVersion":"5338"}, "reason": "DeletedExternalResource", "message": "Successfully requested deletion of external resource"}
2021-08-02T20:25:19.555+0200	DEBUG	provider-aws	Reconciling	{"controller": "managed/repository.ecr.aws.crossplane.io", "request": "/example"}

[AaronME]

This works in testing. Confirmed that forceDelete: false results in an error on delete of populated repo. forceDelete: true successfully forces delete of the repo even with images.

Thanks, @chlunde!