Repository: crossplane/provider-aws (go)

Issue:
PublicDNSNamespace resources are not removed from AWS even with the correct deletionPolicy
### What happened?

- I was trying the PublicDNSNamespace resource in the AWS provider, and even though I use "deletionPolicy: Delete" I noticed that the resource was not removed from AWS when doing "kubectl delete -f dns.yaml".


### How can we reproduce it?

I have this manifest called dns.yaml:

```yaml
---
apiVersion: servicediscovery.aws.crossplane.io/v1alpha1
kind: PublicDNSNamespace
metadata:
  name: dns-aunes-leda-1
spec:
  deletionPolicy: Delete
  forProvider:
    region: eu-west-3
    name: aunes-leda-1.dfds.cloud
    tags:
      - key: perftest
        value: perftest
  providerConfigRef:
    name: aws-provider-config-aunes
  writeConnectionSecretToRef:
    namespace: perftest
    name: secret-dns-aunes-leda-1
```

1. kubectl apply -f dns.yaml
2. Observe that dns-aunes-leda-1 is created as PublicDNSNamespace in Kubernetes.
3. Observe that aunes-leda-1.dfds.cloud is created in Cloud Map and Route 53 on AWS
4. kubectl delete -f dns.yaml
5. Observe that dns-aunes-leda-1 is deleted as PublicDNSNamespace in Kubernetes.
6. Observe that it is not removed in Cloud Map and Route 53 on AWS. (I have actualy waited up to 16 hours, and the resource is still there on AWS).

### What environment did it happen in?
Crossplane version: 

Crossplane version (installed through Helm): 1.5.0
Crossplane CLI version: v1.5.0
aws-provider version: v0.20.1
Kubernetes version: v1.22.3
Kubernetes distribution: minikube v1.24.0
OS: Darwin 12.0.1 (macOS)



Pull request:
Servicediscovery delete fix

### Description of your changes

Fixes #944

This PR moves the storage of the OperationID returned by the AWS Cloud Map [API call](https://docs.aws.amazon.com/cloud-map/latest/api/API_CreatePublicDnsNamespace.html) into an annotation so that it is persisted.  

I have:

- [x] Read and followed Crossplane's [contribution process].
- [x] Run `make reviewable test` to ensure this PR is ready for review.

### How has this code been tested

I created and deleted the following AWS resources, ensuring the resources were created and deleted in the AWS console. 

- HTTPNamespace
- PrivateDNSNamespace (with VPC)
- PublicDNSNamespace

[contribution process]: https://git.io/fj2m9


Patch:
diff --git a/apis/servicediscovery/v1alpha1/custom_types.go b/apis/servicediscovery/v1alpha1/custom_types.go
index 13b4dc61a..bd5e1269c 100644
--- a/apis/servicediscovery/v1alpha1/custom_types.go
+++ b/apis/servicediscovery/v1alpha1/custom_types.go
@@ -1,6 +1,15 @@
 package v1alpha1
 
-import xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
+import (
+	"github.com/crossplane/crossplane-runtime/pkg/meta"
+	xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
+
+	aws "github.com/crossplane/provider-aws/pkg/clients"
+)
+
+// AnnotationKeyOperationID is the key in the annotations map of a
+// Cloud Map managed resource for the OperationId returned by API calls
+const AnnotationKeyOperationID = Group + "/operation-id"
 
 // CustomServiceParameters are custom parameters for Services.
 type CustomServiceParameters struct{}
@@ -27,12 +36,15 @@ type CustomPublicDNSNamespaceParameters struct{}
 
 // GetOperationID returns the last operation id.
 func (in *HTTPNamespace) GetOperationID() *string {
-	return in.Status.AtProvider.OperationID
+	if val, ok := in.GetAnnotations()[AnnotationKeyOperationID]; ok {
+		return &val
+	}
+	return nil
 }
 
 // SetOperationID sets the last operation id.
 func (in *HTTPNamespace) SetOperationID(id *string) {
-	in.Status.AtProvider.OperationID = id
+	meta.AddAnnotations(in, map[string]string{AnnotationKeyOperationID: aws.StringValue(id)})
 }
 
 // GetDescription returns the description.
@@ -47,12 +59,15 @@ func (in *HTTPNamespace) SetDescription(d *string) {
 
 // GetOperationID returns the last operation id.
 func (in *PrivateDNSNamespace) GetOperationID() *string {
-	return in.Status.AtProvider.OperationID
+	if val, ok := in.GetAnnotations()[AnnotationKeyOperationID]; ok {
+		return &val
+	}
+	return nil
 }
 
 // SetOperationID sets the last operation id.
 func (in *PrivateDNSNamespace) SetOperationID(id *string) {
-	in.Status.AtProvider.OperationID = id
+	meta.AddAnnotations(in, map[string]string{AnnotationKeyOperationID: aws.StringValue(id)})
 }
 
 // GetDescription returns the description.
@@ -67,12 +82,15 @@ func (in *PrivateDNSNamespace) SetDescription(d *string) {
 
 // GetOperationID returns the last operation id.
 func (in *PublicDNSNamespace) GetOperationID() *string {
-	return in.Status.AtProvider.OperationID
+	if val, ok := in.GetAnnotations()[AnnotationKeyOperationID]; ok {
+		return &val
+	}
+	return nil
 }
 
 // SetOperationID sets the last operation id.
 func (in *PublicDNSNamespace) SetOperationID(id *string) {
-	in.Status.AtProvider.OperationID = id
+	meta.AddAnnotations(in, map[string]string{AnnotationKeyOperationID: aws.StringValue(id)})
 }
 
 // GetDescription returns the description.
diff --git a/pkg/controller/servicediscovery/httpnamespace/setup.go b/pkg/controller/servicediscovery/httpnamespace/setup.go
index bedada204..7f509bd33 100644
--- a/pkg/controller/servicediscovery/httpnamespace/setup.go
+++ b/pkg/controller/servicediscovery/httpnamespace/setup.go
@@ -43,6 +43,7 @@ func SetupHTTPNamespace(mgr ctrl.Manager, l logging.Logger, rl workqueue.RateLim
 		func(e *external) {
 			h := commonnamespace.NewHooks(e.kube, e.client)
 			e.preCreate = preCreate
+			e.postCreate = postCreate
 			e.delete = h.Delete
 			e.observe = h.Observe
 		},
@@ -66,3 +67,8 @@ func preCreate(_ context.Context, cr *svcapitypes.HTTPNamespace, obj *svcsdk.Cre
 	obj.CreatorRequestId = awsclient.String(string(cr.UID))
 	return nil
 }
+
+func postCreate(_ context.Context, cr *svcapitypes.HTTPNamespace, resp *svcsdk.CreateHttpNamespaceOutput, cre managed.ExternalCreation, err error) (managed.ExternalCreation, error) {
+	cr.SetOperationID(resp.OperationId)
+	return cre, err
+}
diff --git a/pkg/controller/servicediscovery/privatednsnamespace/setup.go b/pkg/controller/servicediscovery/privatednsnamespace/setup.go
index 1218462f4..3c212e986 100644
--- a/pkg/controller/servicediscovery/privatednsnamespace/setup.go
+++ b/pkg/controller/servicediscovery/privatednsnamespace/setup.go
@@ -43,6 +43,7 @@ func SetupPrivateDNSNamespace(mgr ctrl.Manager, l logging.Logger, rl workqueue.R
 		func(e *external) {
 			h := commonnamespace.NewHooks(e.kube, e.client)
 			e.preCreate = preCreate
+			e.postCreate = postCreate
 			e.delete = h.Delete
 			e.observe = h.Observe
 		},
@@ -68,3 +69,8 @@ func preCreate(_ context.Context, cr *svcapitypes.PrivateDNSNamespace, obj *svcs
 	obj.Vpc = cr.Spec.ForProvider.VPC
 	return nil
 }
+
+func postCreate(_ context.Context, cr *svcapitypes.PrivateDNSNamespace, resp *svcsdk.CreatePrivateDnsNamespaceOutput, cre managed.ExternalCreation, err error) (managed.ExternalCreation, error) {
+	cr.SetOperationID(resp.OperationId)
+	return cre, err
+}
diff --git a/pkg/controller/servicediscovery/publicdnsnamespace/setup.go b/pkg/controller/servicediscovery/publicdnsnamespace/setup.go
index 099b38dbd..5d51a57a7 100644
--- a/pkg/controller/servicediscovery/publicdnsnamespace/setup.go
+++ b/pkg/controller/servicediscovery/publicdnsnamespace/setup.go
@@ -45,6 +45,7 @@ func SetupPublicDNSNamespace(mgr ctrl.Manager, l logging.Logger, rl workqueue.Ra
 			e.preCreate = preCreate
 			e.delete = h.Delete
 			e.observe = h.Observe
+			e.postCreate = postCreate
 		},
 	}
 	return ctrl.NewControllerManagedBy(mgr).
@@ -66,3 +67,8 @@ func preCreate(_ context.Context, cr *svcapitypes.PublicDNSNamespace, obj *svcsd
 	obj.CreatorRequestId = awsclient.String(string(cr.UID))
 	return nil
 }
+
+func postCreate(_ context.Context, cr *svcapitypes.PublicDNSNamespace, resp *svcsdk.CreatePublicDnsNamespaceOutput, cre managed.ExternalCreation, err error) (managed.ExternalCreation, error) {
+	cr.SetOperationID(resp.OperationId)
+	return cre, err
+}


Test patch:
diff --git a/pkg/controller/servicediscovery/commonnamespace/hooks_test.go b/pkg/controller/servicediscovery/commonnamespace/hooks_test.go
index 307098709..7b0a98aa2 100644
--- a/pkg/controller/servicediscovery/commonnamespace/hooks_test.go
+++ b/pkg/controller/servicediscovery/commonnamespace/hooks_test.go
@@ -106,32 +106,32 @@ func TestObserve(t *testing.T) {
 				},
 				kube: nil,
 				cr: &svcapitypes.HTTPNamespace{
+					ObjectMeta: v1.ObjectMeta{
+						Annotations: map[string]string{
+							"servicediscovery.aws.crossplane.io/operation-id": validOpID,
+						},
+					},
 					Spec: svcapitypes.HTTPNamespaceSpec{
 						ForProvider: svcapitypes.HTTPNamespaceParameters{
 							Region: "eu-central-1",
 							Name:   aws.String("test"),
 						},
 					},
-					Status: svcapitypes.HTTPNamespaceStatus{
-						AtProvider: svcapitypes.HTTPNamespaceObservation{
-							OperationID: aws.String(validOpID),
-						},
-					},
 				},
 			},
 			want: want{
 				cr: &svcapitypes.HTTPNamespace{
+					ObjectMeta: v1.ObjectMeta{
+						Annotations: map[string]string{
+							"servicediscovery.aws.crossplane.io/operation-id": validOpID,
+						},
+					},
 					Spec: svcapitypes.HTTPNamespaceSpec{
 						ForProvider: svcapitypes.HTTPNamespaceParameters{
 							Region: "eu-central-1",
 							Name:   aws.String("test"),
 						},
 					},
-					Status: svcapitypes.HTTPNamespaceStatus{
-						AtProvider: svcapitypes.HTTPNamespaceObservation{
-							OperationID: aws.String(validOpID),
-						},
-					},
 				},
 				result: managed.ExternalObservation{
 					ResourceExists: true,
@@ -155,32 +155,32 @@ func TestObserve(t *testing.T) {
 				},
 				kube: nil,
 				cr: &svcapitypes.HTTPNamespace{
+					ObjectMeta: v1.ObjectMeta{
+						Annotations: map[string]string{
+							"servicediscovery.aws.crossplane.io/operation-id": validOpID,
+						},
+					},
 					Spec: svcapitypes.HTTPNamespaceSpec{
 						ForProvider: svcapitypes.HTTPNamespaceParameters{
 							Region: "eu-central-1",
 							Name:   aws.String("test"),
 						},
 					},
-					Status: svcapitypes.HTTPNamespaceStatus{
-						AtProvider: svcapitypes.HTTPNamespaceObservation{
-							OperationID: aws.String(validOpID),
-						},
-					},
 				},
 			},
 			want: want{
 				cr: &svcapitypes.HTTPNamespace{
+					ObjectMeta: v1.ObjectMeta{
+						Annotations: map[string]string{
+							"servicediscovery.aws.crossplane.io/operation-id": validOpID,
+						},
+					},
 					Spec: svcapitypes.HTTPNamespaceSpec{
 						ForProvider: svcapitypes.HTTPNamespaceParameters{
 							Region: "eu-central-1",
 							Name:   aws.String("test"),
 						},
 					},
-					Status: svcapitypes.HTTPNamespaceStatus{
-						AtProvider: svcapitypes.HTTPNamespaceObservation{
-							OperationID: aws.String(validOpID),
-						},
-					},
 				},
 				result: managed.ExternalObservation{
 					ResourceExists: true,
@@ -204,21 +204,26 @@ func TestObserve(t *testing.T) {
 				},
 				kube: nil,
 				cr: &svcapitypes.HTTPNamespace{
+					ObjectMeta: v1.ObjectMeta{
+						Annotations: map[string]string{
+							"servicediscovery.aws.crossplane.io/operation-id": validOpID,
+						},
+					},
 					Spec: svcapitypes.HTTPNamespaceSpec{
 						ForProvider: svcapitypes.HTTPNamespaceParameters{
 							Region: "eu-central-1",
 							Name:   aws.String("test"),
 						},
 					},
-					Status: svcapitypes.HTTPNamespaceStatus{
-						AtProvider: svcapitypes.HTTPNamespaceObservation{
-							OperationID: aws.String(validOpID),
-						},
-					},
 				},
 			},
 			want: want{
 				cr: &svcapitypes.HTTPNamespace{
+					ObjectMeta: v1.ObjectMeta{
+						Annotations: map[string]string{
+							"servicediscovery.aws.crossplane.io/operation-id": validOpID,
+						},
+					},
 					Spec: svcapitypes.HTTPNamespaceSpec{
 						ForProvider: svcapitypes.HTTPNamespaceParameters{
 							Region: "eu-central-1",
@@ -226,9 +231,6 @@ func TestObserve(t *testing.T) {
 						},
 					},
 					Status: svcapitypes.HTTPNamespaceStatus{
-						AtProvider: svcapitypes.HTTPNamespaceObservation{
-							OperationID: aws.String(validOpID),
-						},
 						ResourceStatus: xpv1.ResourceStatus{
 							ConditionedStatus: xpv1.ConditionedStatus{
 								Conditions: []xpv1.Condition{xpv1.Unavailable()},
@@ -258,6 +260,11 @@ func TestObserve(t *testing.T) {
 				},
 				kube: nil,
 				cr: &svcapitypes.HTTPNamespace{
+					ObjectMeta: v1.ObjectMeta{
+						Annotations: map[string]string{
+							"servicediscovery.aws.crossplane.io/operation-id": validOpID,
+						},
+					},
 					Spec: svcapitypes.HTTPNamespaceSpec{
 						ForProvider: svcapitypes.HTTPNamespaceParameters{
 							Region: "eu-central-1",
@@ -265,9 +272,6 @@ func TestObserve(t *testing.T) {
 						},
 					},
 					Status: svcapitypes.HTTPNamespaceStatus{
-						AtProvider: svcapitypes.HTTPNamespaceObservation{
-							OperationID: aws.String(validOpID),
-						},
 						ResourceStatus: xpv1.ResourceStatus{
 							ConditionedStatus: xpv1.ConditionedStatus{
 								Conditions: []xpv1.Condition{xpv1.Deleting()},
@@ -278,6 +282,11 @@ func TestObserve(t *testing.T) {
 			},
 			want: want{
 				cr: &svcapitypes.HTTPNamespace{
+					ObjectMeta: v1.ObjectMeta{
+						Annotations: map[string]string{
+							"servicediscovery.aws.crossplane.io/operation-id": validOpID,
+						},
+					},
 					Spec: svcapitypes.HTTPNamespaceSpec{
 						ForProvider: svcapitypes.HTTPNamespaceParameters{
 							Region: "eu-central-1",
@@ -285,9 +294,6 @@ func TestObserve(t *testing.T) {
 						},
 					},
 					Status: svcapitypes.HTTPNamespaceStatus{
-						AtProvider: svcapitypes.HTTPNamespaceObservation{
-							OperationID: aws.String(validOpID),
-						},
 						ResourceStatus: xpv1.ResourceStatus{
 							ConditionedStatus: xpv1.ConditionedStatus{
 								Conditions: []xpv1.Condition{xpv1.Unavailable()},
@@ -332,6 +338,11 @@ func TestObserve(t *testing.T) {
 					MockUpdate: test.NewMockUpdateFn(nil),
 				},
 				cr: &svcapitypes.HTTPNamespace{
+					ObjectMeta: v1.ObjectMeta{
+						Annotations: map[string]string{
+							"servicediscovery.aws.crossplane.io/operation-id": validOpID,
+						},
+					},
 					Spec: svcapitypes.HTTPNamespaceSpec{
 						ForProvider: svcapitypes.HTTPNamespaceParameters{
 							Region:      "eu-central-1",
@@ -339,17 +350,15 @@ func TestObserve(t *testing.T) {
 							Description: aws.String(validDescription),
 						},
 					},
-					Status: svcapitypes.HTTPNamespaceStatus{
-						AtProvider: svcapitypes.HTTPNamespaceObservation{
-							OperationID: aws.String(validOpID),
-						},
-					},
 				},
 			},
 			want: want{
 				cr: &svcapitypes.HTTPNamespace{
 					ObjectMeta: v1.ObjectMeta{
-						Annotations: map[string]string{"crossplane.io/external-name": validNSID},
+						Annotations: map[string]string{
+							"crossplane.io/external-name":                     validNSID,
+							"servicediscovery.aws.crossplane.io/operation-id": validOpID,
+						},
 					},
 					Spec: svcapitypes.HTTPNamespaceSpec{
 						ForProvider: svcapitypes.HTTPNamespaceParameters{
@@ -359,9 +368,6 @@ func TestObserve(t *testing.T) {
 						},
 					},
 					Status: svcapitypes.HTTPNamespaceStatus{
-						AtProvider: svcapitypes.HTTPNamespaceObservation{
-							OperationID: aws.String(validOpID),
-						},
 						ResourceStatus: xpv1.ResourceStatus{
 							ConditionedStatus: xpv1.ConditionedStatus{
 								Conditions: []xpv1.Condition{xpv1.Available()},
@@ -406,7 +412,10 @@ func TestObserve(t *testing.T) {
 				kube: nil,
 				cr: &svcapitypes.HTTPNamespace{
 					ObjectMeta: v1.ObjectMeta{
-						Annotations: map[string]string{"crossplane.io/external-name": validNSID},
+						Annotations: map[string]string{
+							"crossplane.io/external-name":                     validNSID,
+							"servicediscovery.aws.crossplane.io/operation-id": validOpID,
+						},
 					},
 					Spec: svcapitypes.HTTPNamespaceSpec{
 						ForProvider: svcapitypes.HTTPNamespaceParameters{
@@ -414,17 +423,15 @@ func TestObserve(t *testing.T) {
 							Name:   aws.String("test"),
 						},
 					},
-					Status: svcapitypes.HTTPNamespaceStatus{
-						AtProvider: svcapitypes.HTTPNamespaceObservation{
-							OperationID: aws.String(validOpID),
-						},
-					},
 				},
 			},
 			want: want{
 				cr: &svcapitypes.HTTPNamespace{
 					ObjectMeta: v1.ObjectMeta{
-						Annotations: map[string]string{"crossplane.io/external-name": validNSID},
+						Annotations: map[string]string{
+							"crossplane.io/external-name":                     validNSID,
+							"servicediscovery.aws.crossplane.io/operation-id": validOpID,
+						},
 					},
 					Spec: svcapitypes.HTTPNamespaceSpec{
 						ForProvider: svcapitypes.HTTPNamespaceParameters{
@@ -434,9 +441,6 @@ func TestObserve(t *testing.T) {
 						},
 					},
 					Status: svcapitypes.HTTPNamespaceStatus{
-						AtProvider: svcapitypes.HTTPNamespaceObservation{
-							OperationID: aws.String(validOpID),
-						},
 						ResourceStatus: xpv1.ResourceStatus{
 							ConditionedStatus: xpv1.ConditionedStatus{
 								Conditions: []xpv1.Condition{xpv1.Available()},
@@ -495,32 +499,32 @@ func TestDelete(t *testing.T) {
 				},
 				kube: nil,
 				cr: &svcapitypes.HTTPNamespace{
+					ObjectMeta: v1.ObjectMeta{
+						Annotations: map[string]string{
+							"servicediscovery.aws.crossplane.io/operation-id": validOpID,
+						},
+					},
 					Spec: svcapitypes.HTTPNamespaceSpec{
 						ForProvider: svcapitypes.HTTPNamespaceParameters{
 							Region: "eu-central-1",
 							Name:   aws.String("test"),
 						},
 					},
-					Status: svcapitypes.HTTPNamespaceStatus{
-						AtProvider: svcapitypes.HTTPNamespaceObservation{
-							OperationID: aws.String(validOpID),
-						},
-					},
 				},
 			},
 			want: want{
 				cr: &svcapitypes.HTTPNamespace{
+					ObjectMeta: v1.ObjectMeta{
+						Annotations: map[string]string{
+							"servicediscovery.aws.crossplane.io/operation-id": validOpID,
+						},
+					},
 					Spec: svcapitypes.HTTPNamespaceSpec{
 						ForProvider: svcapitypes.HTTPNamespaceParameters{
 							Region: "eu-central-1",
 							Name:   aws.String("test"),
 						},
 					},
-					Status: svcapitypes.HTTPNamespaceStatus{
-						AtProvider: svcapitypes.HTTPNamespaceObservation{
-							OperationID: aws.String(validOpID),
-						},
-					},
 				},
 			},
 		},