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

s3-notifications: Unable to update the s3 event notifications on an existing S3 bucket. #31303

Closed
1 task
Assignees
Labels
@aws-cdk/aws-s3-notifications @aws-cdk/aws-s3 Related to Amazon S3 bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@satanupa
Copy link

satanupa commented Sep 3, 2024

Describe the bug

I am facing an error when trying to update the s3 event notification configuration on an existing S3 bucket using CDK.
Created a stack to add s3 event notification on an existing s3 bucket. Create operation goes through successfully.
When I update the cdk stack to add event notifications on the same s3 bucket, the Update operation fails with below error.

Error:

Received response status [FAILED] from custom resource. Message returned: Error: An error occurred (InvalidArgument) when calling the PutBucketNotificationConfiguration operation: Configuration is ambiguously defined. Cannot have overlapping suffixes in two rules if the prefixes are overlapping for the same event type.. See the details in CloudWatch Log Stream:

Here the lambda code backing the BucketNotifications custom resource is unable to identify that the existing event notifications were also managed by the same cdk stack thereby creating duplicate notifications resulting in error.

Regression Issue

  • Select this option if this issue appears to be a regression.

Last Known Working CDK Version

No response

Expected Behavior

Should be able to add additional notification configuration on the existing s3 bucket.

Current Behavior

Issue
Created a cdk stack and added an event notification on an existing s3 bucket. Create operation is successful.
Updated the same stack to add a new event notification along with the existing ones. The update operation fails with below error.

Error:

Received response status [FAILED] from custom resource. Message returned: Error: An error occurred (InvalidArgument) when calling the PutBucketNotificationConfiguration operation: Configuration is ambiguously defined. Cannot have overlapping suffixes in two rules if the prefixes are overlapping for the same event type.. See the details in CloudWatch Log Stream:

Reproduction Steps

Steps:
1. Create a stack by commenting out the 2nd event notification (notificationfilter2)
2. once stack is created, update the stack after uncommenting the 2nd event notification.



 my_lambda = _lambda.Function(
          self, 'HelloHandler',
            runtime=_lambda.Runtime.PYTHON_3_8,
            code=_lambda.Code.from_asset('lambda'),
            handler='hello.handler',
        )  


        bucket = _s3.Bucket.from_bucket_name(self, "Bucket", "bucketname")
        notification = _s3_notify.LambdaDestination(my_lambda)

        notificationfilter1 = _s3.NotificationKeyFilter(prefix="foo/", suffix="bar/")
        bucket.add_event_notification(_s3.EventType.OBJECT_CREATED, notification, notificationfilter1
        )

       # notificationfilter2 = _s3.NotificationKeyFilter(prefix="fo1/",suffix="ba1/",)
      # bucket.add_event_notification(_s3.EventType.OBJECT_CREATED, notification, notificationfilter2
      #  )

Possible Solution

Analysis details:
Create a cdk stack to add below event notification to an existing s3 bucket. The Create operation is successful.

'Events': ['s3:ObjectCreated:*'], 'Filter': {'Key': {'FilterRules': [{'Name': 'Prefix', 'Value': 'bar/'}, {'Name': 'Suffix', 'Value': 'foo/'}]}}}

In the notifications-resource-handler code

For unmanaged buckets, there is a get_id function used to evaluate the hash for each event notification to confirm if this is created by the stack or is an existing external configuration:

def get_id(n):
    n['Id'] = ''
    strToHash=json.dumps(n, sort_keys=True).replace('"Name": "prefix"', '"Name": "Prefix"').replace('"Name": "suffix"', '"Name": "Suffix"')
    return f"{stack_id}-{hash(strToHash)}"

During creation, this goes fine as it treats all existing configurations as external.
Then it appends incoming + external and creates the final notification configuration as expected.

During an update operation, the lambda code will first get the existing event notifications on the s3 bucket.
Then it validates if the existing event notifications matches the notification in the incoming request from Cloudformation.
It does this by evaluating the hash for each existing event notification and validating if this matches with the hash of the incoming event notifications from Cloudformation.
When there is a match, it identifies this existing event configuration as managed by the stack.
This helps in eliminating the duplicates.

There is an issue with this hash evaluation, due to a change in order of the prefix and suffix within the filter rules between the existing notification and the notification coming from Cloudformation.

Sample event notification from Cloudformation request

{
    "Events": [
        "s3:ObjectCreated:*"
    ],
    "Filter": {
        "Key": {
            "FilterRules": [
                {
                    "Name": "Suffix",
                    "Value": "foo/"
                },
                {
                    "Name": "Prefix",
                    "Value": "bar/"
                }
            ]
        }
    },
    "Id": "",
    "LambdaFunctionArn": "arn:aws:lambda:<aws-region>:<aws-account-id>:function:<FunctionName>"
}

Sample existing S3 event notification:

    "Events": [
        "s3:ObjectCreated:*"
    ],
    "Filter": {
        "Key": {
            "FilterRules": [
                {
                    "Name": "Prefix",
                    "Value": "bar/"
                },
                {
                    "Name": "Suffix",
                    "Value": "foo/"
                }
            ]
        }
    },
    "Id": "",
    "LambdaFunctionArn": "arn:aws:lambda:<aws-region>:<aws-account-id>:function:<FunctionName>"
}

Note: Even though the content of the above jsons are the same, the order of the filter rules (prefix and suffix) are different.
Due to this, the hash evaluated will also be different. So, it is unable to identify that the existing configuration was also added by the stack itself.
This ends up in a configuration which includes the duplicates (existing configurations on s3 + incoming event configuration from Cloudformation stack)
resulting in below error:

Received response status [FAILED] from custom resource. Message returned: Error: An error occurred (InvalidArgument) when calling the PutBucketNotificationConfiguration operation: Configuration is ambiguously defined. Cannot have overlapping suffixes in two rules if the prefixes are overlapping for the same event type.

This can be workaround by changing the order of filters in the cdk synthesized stack template, by passing the prefix first and then suffix within the filter rules array.

Additional Information/Context

No response

CDK CLI Version

2.152.0

Framework Version

No response

Node.js Version

v20.16.0

OS

Linux

Language

Python

Language Version

No response

Other information

No response

@satanupa satanupa added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 3, 2024
@github-actions github-actions bot added the @aws-cdk/aws-s3 Related to Amazon S3 label Sep 3, 2024
@pahud pahud self-assigned this Sep 3, 2024
@pahud pahud added the investigating This issue is being investigated and/or work is in progress to resolve the issue. label Sep 3, 2024
@pahud
Copy link
Contributor

pahud commented Sep 4, 2024

Yes this is reproducible.

class IssueTriagePyStack(Stack):
  def __init__(self, scope: Construct, construct_id: str, **kwargs) -> None:
    super().__init__(scope, construct_id, **kwargs)
  
    my_lambda = _lambda.Function(
          self, 'HelloHandler',
            runtime=_lambda.Runtime.PYTHON_3_8,
            code=_lambda.Code.from_asset('lambda'),
            handler='hello.handler',
    )    
  
    bucket = s3.Bucket.from_bucket_name(self, "Bucket", "my-existing-bucket-name")
    notification = s3_notify.LambdaDestination(my_lambda)

    notificationfilter1 = s3.NotificationKeyFilter(prefix="foo/", suffix="bar/")
    bucket.add_event_notification(s3.EventType.OBJECT_CREATED, notification, notificationfilter1)

    notificationfilter2 = s3.NotificationKeyFilter(prefix="fo1/",suffix="ba1/",)
    bucket.add_event_notification(s3.EventType.OBJECT_CREATED, notification, notificationfilter2)

On cdk deploy to update the stack:

issue-triage: creating CloudFormation changeset...
10:35:14 PM | UPDATE_FAILED        | Custom::S3BucketNotifications | BucketNotifications8F2E257D
Received response status [FAILED] from custom resource. Message returned: Error: An error occurred (InvalidArgument) when calling the PutBucketNotificati
onConfiguration operation: Configuration is ambiguously defined. Cannot have overlapping suffixes in two rules if the prefixes are overlapping for the sa
me event type.. See the details in CloudWatch Log Stream: 2024/09/04/[$LATEST]ff56425363ce4d8db0b594f6d560320a (RequestId: 9b4cb9c7-2141-4e7b-ba02-b4a880
70d580)

10:35:18 PM | UPDATE_FAILED        | Custom::S3BucketNotifications | BucketNotifications8F2E257D
Received response status [FAILED] from custom resource. Message returned: Error: An error occurred (InvalidArgument) when calling the PutBucketNotificati
onConfiguration operation: Configuration is ambiguously defined. Cannot have overlapping suffixes in two rules if the prefixes are overlapping for the sa
me event type.. See the details in CloudWatch Log Stream: 2024/09/04/[$LATEST]ff56425363ce4d8db0b594f6d560320a (RequestId: dc73fb2a-5abc-48d6-b0dd-da0625
2e55bc)

cdk synth

  BucketNotifications8F2E257D:
    Type: Custom::S3BucketNotifications
    Properties:
      ServiceToken:
        Fn::GetAtt:
          - BucketNotificationsHandler050a0587b7544547bf325f094a3db8347ECC3691
          - Arn
      BucketName: pahud-foo-bar
      NotificationConfiguration:
        LambdaFunctionConfigurations:
          - Events:
              - s3:ObjectCreated:*
            Filter:
              Key:
                FilterRules:
                  - Name: suffix
                    Value: bar/
                  - Name: prefix
                    Value: foo/
            LambdaFunctionArn:
              Fn::GetAtt:
                - HelloHandler2E4FBA4D
                - Arn
          - Events:
              - s3:ObjectCreated:*
            Filter:
              Key:
                FilterRules:
                  - Name: suffix
                    Value: ba1/
                  - Name: prefix
                    Value: fo1/
            LambdaFunctionArn:
              Fn::GetAtt:
                - HelloHandler2E4FBA4D
                - Arn
      Managed: false
      SkipDestinationValidation: false

As I can't figure out any workaround. Making this a p1 bug.

@pahud pahud added the p1 label Sep 4, 2024
@pahud pahud removed their assignment Sep 4, 2024
@pahud pahud added effort/medium Medium work item – several days of effort and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. needs-triage This issue or PR still needs to be triaged. labels Sep 4, 2024
@pahud pahud changed the title @aws-cdk/aws-s3: Unable to update the s3 event notifications on an existing S3 bucket. s3: Unable to update the s3 event notifications on an existing S3 bucket. Sep 4, 2024
@pahud pahud changed the title s3: Unable to update the s3 event notifications on an existing S3 bucket. s3-notifications: Unable to update the s3 event notifications on an existing S3 bucket. Sep 4, 2024
@stefanopallicca-imagicle

Facing the same issue: I previously defined two notifications, one with
prefix: person/upload
suffix: .csv

another with
prefix: car/upload
suffix: .csv

This configuration worked with 2.146.0, results in same issue by simply updating cdk to 2.155.0

@migurski
Copy link

migurski commented Sep 5, 2024

I’ve been tracking down this same issue, and can confirm that I see an identical error when attempting to update to a new Lambda function ARN on an existing notification configured for an externally-defined bucket.

Abbreviated version of my reproduction, first deployment succeeds:

log_function = aws_lambda.Function(self, "Function1", …)
bucket.add_event_notification(
    aws_s3.EventType.OBJECT_CREATED,
    aws_s3_notifications.LambdaDestination(log_function),
    aws_s3.NotificationKeyFilter(prefix="logs/resource-cdn/", suffix=".gz"),
)

Second deployment fails:

log_function = aws_lambda.Function(self, "Function2", …)
bucket.add_event_notification(
    aws_s3.EventType.OBJECT_CREATED,
    aws_s3_notifications.LambdaDestination(log_function),
    aws_s3.NotificationKeyFilter(prefix="logs/resource-cdn/", suffix=".gz"),
)

@xazhao xazhao self-assigned this Sep 6, 2024
@xazhao
Copy link
Contributor

xazhao commented Sep 6, 2024

Looks like the the same issue #28915.

I was able to reproduce in v2.147+ and it works fine in v2.146.0. However I don't see any related change in the diff. Need to look further into it.

@ashishdhingra
Copy link
Contributor

ashishdhingra commented Sep 12, 2024

@xazhao Related issue #31413. It might give some clue about the root cause.

@xenonlcs
Copy link

xenonlcs commented Sep 30, 2024

Hello everyone, here are my 2 cents. THIS IS NOT A SORTING PROBLEM.

In CDK v2.3 the method used to verify if a notification configuration was external was based on the following check (this is from the Custom Resource behind aws_s3_notifications.LambdaDestination resource):
external_notifications[t] = [n for n in existing_notifications.get(t, []) if not n['Id'].startswith(f"{stack_id}-")]

In CDK v2.136 the method is this one (not checked when this change was introduced):

  ids = [with_id(n) for n in old.get(t, [])]
  old_incoming_ids = [n['Id'] for n in ids]
  external_notifications[t] = [n for n in existing_notifications.get(t, []) if not n['Id'] in old_incoming_ids]

The function with_id(n) is this one:

def with_id(n):
    n['Id'] = f"{stack_id}-{hash(json.dumps(n, sort_keys=True))}"
    return n

The built-in function hash does not return the same value across different instances of python even if the input is the same.

The Custom Resource will treat as external_notifications even the ones that are not, leading to duplicates

EDIT: for versions older than Python 3.2 (included) the hash function generates the same value for the same input across different python instances. Starting from Python 3.3, the hash function starts with a random seed for each python instance.

@xazhao
Copy link
Contributor

xazhao commented Oct 3, 2024

@xenonlcs Thanks for adding more context here. From the description, it looks like a different issue from the original issue. Do you mean hash() here return different values? Is there a way for me to reproduce the issue?

@xenonlcs
Copy link

xenonlcs commented Oct 3, 2024

Hi @xazhao, I will attach this screenshot to explain better what I mean:
image

As you can see, stopping and restarting a new process of Python leads to the re-initialization of the built-in hash function with another seed. This behavior is introduced starting from version 3.3 of Python. I think that this is not a different issue cause as far as I can see in the code of the custom resource, no code could break due to some sorting problems (the checks are performed with not in). EDIT: I checked again the issue description and it seems that it's also a sorting problem of the returned information by the s3.get_bucket_notification_configuration, or a sorting problem about how the cdk synth is performed, but due to the behaviour of the built-in hash function I think that just sorting the properties will not solve the issue (I have a situation in which there is the same error but only with a prefix).

Since the fact that s3.put_bucket_notification_configuration wants the final configuration for the bucket and not only the items to add, the lambda function tries to understand which notification configurations are not managed by the CloudFormation stack that is being deployed.

So, in the Lambda function we can see that there are 3 different kinds of notification configurations:

  • existing_notifications: these are the notification configurations that are currently attached to the s3 bucket (retrieved with s3.get_bucket_notification_configuration)
  • incoming: these are the notification configurations that comes from the custom resource (both new and previously deployed)
  • external_notifications: these are the notification configurations that could be created manually or by other CloudFormation stacks. These ones must be preserved too.

The check to identify external_notifications is performed comparing the hashes of existing_notifications and the hashes calculated by the function get_id on the old list.

The old list is populated through the custom resource event that details the configuration of the custom resource before the CloudFormation Update event.

Since the get_id function uses the built-in hash function, the hashes calculated on the old list will never match the ones on the existing_notifications list. This leads to treating all the existing_notifications as external_notifications.

The final notification configuration is created merging the external with the incoming ones. All the old notification configurations will be present in both the external and incoming lists.

You can also verify the different hashes on the CloudTrail event for the event named PutBucketNotification (ye, this is one of the rare cases where the SDK function name does not match the API name) originated by the Lambda.

Let me know if I can further explain my point,
Best Regards

@xazhao
Copy link
Contributor

xazhao commented Oct 3, 2024

Thanks for the explanation. The sorting I added is not for old list but for a property in notification object. For old list, it will calculate hash value for each notification object. Then it calculated each notifications in existing_notifications list to check if hash values match.

This is why sorting matters -- objects will have different hash values:

  "FilterRules": [
      {
          "Name": "Suffix",
          "Value": "foo/"
      },
      {
          "Name": "Prefix",
          "Value": "bar/"
      }
  ]

and

  "FilterRules": [
      {
          "Name": "Prefix",
          "Value": "bar/"
      },
      {
          "Name": "Suffix",
          "Value": "foo/"
      }
  ]

When the lambda function is invoked, it will re-run hashing on those 2 lists (because it will not save the hashing value). No matter what seed it's using, the seed should be the same for calculating hashes for those 2 lists within one single lambda execution.

@xenonlcs
Copy link

xenonlcs commented Oct 3, 2024

Yep I agree, i got misled by a previous version of cdk (v2.136.0) in which there was no get_id function.
Sorry for the misunderstanding

@mergify mergify bot closed this as completed in #31431 Oct 8, 2024
mergify bot pushed a commit that referenced this issue Oct 8, 2024
…3 bucket (#31431)

### Issue # (if applicable)

Closes #31303.

### Reason for this change

During the update S3 bucket notifications, if there are multiple filter rules involved, the order of filter rules is different from CFN event notifications and S3 get notifications call. This will result different hash value which makes CDK unable to recognize s3 notifications created by the stack itself.

### Description of changes
Sort the notifications filter rules by key when calculating the hash value.

### Description of how you validated changes

Manually tested and integration tests.

### Checklist
- [ ] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Copy link

github-actions bot commented Oct 8, 2024

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.