Repository: crossplane/provider-aws (go)

Issue:
S3 Bucket NotificationConfiguration Queue referencers should extract ARN instead of external name
### What happened?

The current referencer implementation for `spec.forProvider.noficationConfiguration.queueConfigurations[].queueArn` in `Bucket` does extract the value of the `crossplane.io/external-name` annotation but the field requires the ARN from `status.atProvider.queueArn`.


### How can we reproduce it?

1. Create a queue from `examples`
2. Create a bucket from `examples` with `spec.forProvider.noficationConfiguration` set to the following:
```yaml
        queueArnRef:
          name: test-queue3
```
3. The bucket creation fails because the field does not contain a valid ARN.

### What environment did it happen in?

`provider-aws: v0.24.1`




Pull request:
fix(s3): Extract QueueARN instead of external name
### Description of your changes

This changes the referencer implementation for `spec.forProvider.noficationConfiguration.queueConfigurations[].queueArn` for `Bucket` so it will extract the ARN from the `Queue` resource instead of the default external name annotation.

Fixes #1160 

It will also make the field optional, because it can be filled using referencers.

Additionally, I updated the put bucket notification configuration error message to give a
hint about missing IAM credentials. (Because the AWS error message is incomplete and might be misleading).

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

Manually using the following YAML:

```yaml
---
apiVersion: s3.aws.crossplane.io/v1beta1
kind: Bucket
metadata:
  name: repl-dest
  annotations:
    # This will be the actual bucket name. It must be globally unique, so you
    # probably want to change it before trying to apply this example.
    crossplane.io/external-name: crossplane-example-repl-dest-123455
spec:
  deletionPolicy: Delete
  forProvider:
    acl: private
    locationConstraint: eu-central-1
    notificationConfiguration:
      queueConfigurations:
      - events:
        - "s3:ObjectCreated:*"
        queueArnRef:
          name: test-queue
    paymentConfiguration:
      payer: BucketOwner
    serverSideEncryptionConfiguration:
      rules:
        - applyServerSideEncryptionByDefault:
            sseAlgorithm: AES256
    versioningConfiguration:
      status: Enabled
  providerConfigRef:
    name: example
---
apiVersion: sqs.aws.crossplane.io/v1beta1
kind: Queue
metadata:
  name: test-queue
spec:
  forProvider:
    region: eu-central-1
    delaySeconds: 4
    policy: |
      {
        "Statement": [
          {
            "Sid":"allow-s3",
            "Effect":"Allow",
            "Principal": {
              "Service": "s3.amazonaws.com"
            },
            "Action":"sqs:SendMessage",
            "Resource":"*",
            "Condition":{
              "ArnEquals":{
                "aws:SourceArn":"arn:aws:s3:::*"
              }
            }
          }
        ]
      }
  providerConfigRef:
    name: example
```

[contribution process]: https://git.io/fj2m9


Patch:
diff --git a/apis/s3/v1beta1/notificationConfiguration_types.go b/apis/s3/v1beta1/notificationConfiguration_types.go
index 1ab6fe8ba..832125bf7 100644
--- a/apis/s3/v1beta1/notificationConfiguration_types.go
+++ b/apis/s3/v1beta1/notificationConfiguration_types.go
@@ -136,7 +136,8 @@ type QueueConfiguration struct {
 	//
 	// QueueArn is a required field
 	// +crossplane:generate:reference:type=github.com/crossplane/provider-aws/apis/sqs/v1beta1.Queue
-	QueueArn string `json:"queueArn"`
+	// +crossplane:generate:reference:extractor=github.com/crossplane/provider-aws/apis/sqs/v1beta1.QueueARN()
+	QueueArn *string `json:"queueArn,omitempty"`
 
 	// QueueArnRef references an Queue to retrieve its ARN
 	// +optional
diff --git a/apis/s3/v1beta1/zz_generated.deepcopy.go b/apis/s3/v1beta1/zz_generated.deepcopy.go
index dc02a6827..c17266847 100644
--- a/apis/s3/v1beta1/zz_generated.deepcopy.go
+++ b/apis/s3/v1beta1/zz_generated.deepcopy.go
@@ -926,6 +926,11 @@ func (in *QueueConfiguration) DeepCopyInto(out *QueueConfiguration) {
 		*out = new(string)
 		**out = **in
 	}
+	if in.QueueArn != nil {
+		in, out := &in.QueueArn, &out.QueueArn
+		*out = new(string)
+		**out = **in
+	}
 	if in.QueueArnRef != nil {
 		in, out := &in.QueueArnRef, &out.QueueArnRef
 		*out = new(v1.Reference)
diff --git a/apis/s3/v1beta1/zz_generated.resolvers.go b/apis/s3/v1beta1/zz_generated.resolvers.go
index 70c9aa132..f4b4d2f88 100644
--- a/apis/s3/v1beta1/zz_generated.resolvers.go
+++ b/apis/s3/v1beta1/zz_generated.resolvers.go
@@ -137,8 +137,8 @@ func (mg *Bucket) ResolveReferences(ctx context.Context, c client.Reader) error
 	if mg.Spec.ForProvider.NotificationConfiguration != nil {
 		for i4 := 0; i4 < len(mg.Spec.ForProvider.NotificationConfiguration.QueueConfigurations); i4++ {
 			rsp, err = r.Resolve(ctx, reference.ResolutionRequest{
-				CurrentValue: mg.Spec.ForProvider.NotificationConfiguration.QueueConfigurations[i4].QueueArn,
-				Extract:      reference.ExternalName(),
+				CurrentValue: reference.FromPtrValue(mg.Spec.ForProvider.NotificationConfiguration.QueueConfigurations[i4].QueueArn),
+				Extract:      v1beta11.QueueARN(),
 				Reference:    mg.Spec.ForProvider.NotificationConfiguration.QueueConfigurations[i4].QueueArnRef,
 				Selector:     mg.Spec.ForProvider.NotificationConfiguration.QueueConfigurations[i4].QueueArnSelector,
 				To: reference.To{
@@ -149,7 +149,7 @@ func (mg *Bucket) ResolveReferences(ctx context.Context, c client.Reader) error
 			if err != nil {
 				return errors.Wrap(err, "mg.Spec.ForProvider.NotificationConfiguration.QueueConfigurations[i4].QueueArn")
 			}
-			mg.Spec.ForProvider.NotificationConfiguration.QueueConfigurations[i4].QueueArn = rsp.ResolvedValue
+			mg.Spec.ForProvider.NotificationConfiguration.QueueConfigurations[i4].QueueArn = reference.ToPtrValue(rsp.ResolvedValue)
 			mg.Spec.ForProvider.NotificationConfiguration.QueueConfigurations[i4].QueueArnRef = rsp.ResolvedReference
 
 		}
diff --git a/package/crds/s3.aws.crossplane.io_buckets.yaml b/package/crds/s3.aws.crossplane.io_buckets.yaml
index daecd158b..57e02b39c 100644
--- a/package/crds/s3.aws.crossplane.io_buckets.yaml
+++ b/package/crds/s3.aws.crossplane.io_buckets.yaml
@@ -701,7 +701,6 @@ spec:
                               type: object
                           required:
                           - events
-                          - queueArn
                           type: object
                         type: array
                       topicConfigurations:
diff --git a/pkg/controller/s3/bucket/notificationConfig.go b/pkg/controller/s3/bucket/notificationConfig.go
index 8a081b4bf..3bf834c37 100644
--- a/pkg/controller/s3/bucket/notificationConfig.go
+++ b/pkg/controller/s3/bucket/notificationConfig.go
@@ -36,7 +36,7 @@ import (
 
 const (
 	notificationGetFailed = "cannot get Bucket notification"
-	notificationPutFailed = "cannot put Bucket notification"
+	notificationPutFailed = "cannot put Bucket notification (may be caused by insuffifienct permissions on the target)"
 )
 
 // NotificationConfigurationClient is the client for API methods and reconciling the LifecycleConfiguration
@@ -180,7 +180,7 @@ func GenerateQueueConfigurations(config *v1beta1.NotificationConfiguration) []ty
 	for _, v := range config.QueueConfigurations {
 		conf := types.QueueConfiguration{
 			Id:       v.ID,
-			QueueArn: awsclient.String(v.QueueArn),
+			QueueArn: v.QueueArn,
 		}
 		if v.Events != nil {
 			conf.Events = copyEvents(v.Events)
@@ -359,7 +359,7 @@ func LateInitializeQueue(external []types.QueueConfiguration, local []v1beta1.Qu
 			Events:   LateInitializeEvents(local[i].Events, v.Events),
 			Filter:   LateInitializeFilter(local[i].Filter, v.Filter),
 			ID:       awsclient.LateInitializeStringPtr(local[i].ID, v.Id),
-			QueueArn: awsclient.LateInitializeString(local[i].QueueArn, v.QueueArn),
+			QueueArn: awsclient.LateInitializeStringPtr(local[i].QueueArn, v.QueueArn),
 		}
 	}
 	return local


Test patch:
diff --git a/pkg/controller/s3/bucket/notificationConfig_test.go b/pkg/controller/s3/bucket/notificationConfig_test.go
index 378a00da6..f24e6dab8 100644
--- a/pkg/controller/s3/bucket/notificationConfig_test.go
+++ b/pkg/controller/s3/bucket/notificationConfig_test.go
@@ -83,7 +83,7 @@ func generateNotificationConfig() *v1beta1.NotificationConfiguration {
 			Events:   generateNotificationEvents(),
 			Filter:   generateNotificationFilter(),
 			ID:       &id,
-			QueueArn: queueArn,
+			QueueArn: awsclient.String(queueArn),
 		}},
 		TopicConfigurations: []v1beta1.TopicConfiguration{{
 			Events:   generateNotificationEvents(),
@@ -406,7 +406,7 @@ func TestIsNotificationConfigurationUpToDate(t *testing.T) {
 						Events:   generateNotificationEvents(),
 						Filter:   generateNotificationFilter(),
 						ID:       &id,
-						QueueArn: queueArn,
+						QueueArn: awsclient.String(queueArn),
 					}},
 					TopicConfigurations: []v1beta1.TopicConfiguration{{
 						Events:   generateNotificationEvents(),
@@ -453,7 +453,7 @@ func TestIsNotificationConfigurationUpToDate(t *testing.T) {
 						Events:   generateNotificationEvents(),
 						Filter:   generateNotificationFilter(),
 						ID:       awsclient.String("queue-id-1"),
-						QueueArn: queueArn,
+						QueueArn: awsclient.String(queueArn),
 					}},
 					TopicConfigurations: []v1beta1.TopicConfiguration{{
 						Events:   generateNotificationEvents(),