Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(s3): Extract QueueARN instead of external name #1161

Merged

Conversation

MisterMX
Copy link
Collaborator

@MisterMX MisterMX commented Feb 21, 2022

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:

  • 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

Manually using the following 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

* Make QueueARN field optional
* Update put bucket notification configuration error message to give a
hint about missing IAM credentials.

Signed-off-by: Maximilian Blatt <maximilian.blatt-extern@deutschebahn.com>
(external expert on behalf of DB Netz AG)
@MisterMX MisterMX force-pushed the s3-bucket-notification-config-fix branch from 0dbf961 to 8d596e9 Compare February 21, 2022 15:29
Copy link
Member

@haarchri haarchri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for implementing this

@haarchri haarchri merged commit 20f8471 into crossplane-contrib:master Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

S3 Bucket NotificationConfiguration Queue referencers should extract ARN instead of external name
2 participants