Skip to content

add check for FakeDeleteMarker in case of versioned bucket#7948

Merged
thrau merged 3 commits intomasterfrom
fix-s3-versioned
Mar 24, 2023
Merged

add check for FakeDeleteMarker in case of versioned bucket#7948
thrau merged 3 commits intomasterfrom
fix-s3-versioned

Conversation

@bentsku
Copy link
Contributor

@bentsku bentsku commented Mar 24, 2023 •
edited
Loading

While looking at the tests run, I realised we were encountering issue when doing test cleanup.

Trace:

2023-03-23T21:22:27.7599773Z Traceback (most recent call last):
2023-03-23T21:22:27.7600672Z   File "/home/runner/work/localstack-ext/localstack-ext/localstack/localstack/aws/chain.py", line 90, in handle
2023-03-23T21:22:27.7601377Z     handler(self, self.context, response)
2023-03-23T21:22:27.7602178Z   File "/home/runner/work/localstack-ext/localstack-ext/localstack/localstack/aws/handlers/service.py", line 122, in __call__
2023-03-23T21:22:27.7602712Z     handler(chain, context, response)
2023-03-23T21:22:27.7603397Z   File "/home/runner/work/localstack-ext/localstack-ext/localstack/localstack/aws/handlers/service.py", line 92, in __call__
2023-03-23T21:22:27.7603934Z     skeleton_response = self.skeleton.invoke(context)
2023-03-23T21:22:27.7604622Z   File "/home/runner/work/localstack-ext/localstack-ext/localstack/localstack/aws/skeleton.py", line 153, in invoke
2023-03-23T21:22:27.7605130Z     return self.dispatch_request(context, instance)
2023-03-23T21:22:27.7605835Z   File "/home/runner/work/localstack-ext/localstack-ext/localstack/localstack/aws/skeleton.py", line 165, in dispatch_request
2023-03-23T21:22:27.7606339Z     result = handler(context, instance) or {}
2023-03-23T21:22:27.7607016Z   File "/home/runner/work/localstack-ext/localstack-ext/localstack/localstack/aws/forwarder.py", line 60, in _call
2023-03-23T21:22:27.7607483Z     return handler(context, req)
2023-03-23T21:22:27.7608151Z   File "/home/runner/work/localstack-ext/localstack-ext/localstack/localstack/aws/skeleton.py", line 117, in __call__
2023-03-23T21:22:27.7608616Z     return self.fn(*args, **kwargs)
2023-03-23T21:22:27.7609473Z   File "/home/runner/work/localstack-ext/localstack-ext/localstack/localstack/services/s3/provider.py", line 485, in delete_objects
2023-03-23T21:22:27.7610083Z     s3_notification_ctx = S3EventNotificationContext.from_request_context(
2023-03-23T21:22:27.7610903Z   File "/home/runner/work/localstack-ext/localstack-ext/localstack/localstack/services/s3/notifications.py", line 127, in from_request_context
2023-03-23T21:22:27.7611457Z     key_etag=key.etag.strip('"'),
2023-03-23T21:22:27.7612135Z AttributeError: 'FakeDeleteMarker' object has no attribute 'etag'
2023-03-23T21:22:27.7613013Z 2023-03-23T21:22:27.760  INFO --- [   asgi_gw_2] localstack.request.aws     : AWS s3.DeleteObjects => 500 (InternalError)

After some reproductions, I could reproduce the issue when the bucket is versioned, which we, it turns out, don't support very well. While migrating the S3 provider, we did not account very much for the concept of VersionId, and we did not have tests covering it (not a single operation was done with VersionId parameter).

So I've added some checks and fixed some direct usages of moto FakeKey object which can be FakeDeleteMarker, a case we didn't account for. We can fix a lot more when using versioned bucket, but at least now we shouldn't raise direct AttributeError.

Concerning notifications, we do not support s3:ObjectRemoved:DeleteMarkerCreated at the moment and we will send a s3:ObjectRemoved:Delete instead, but this will need a bit of refactoring in the notification handling (in the map determining the service operation, it would depend if the bucket is versioned or not and if we delete a key or delete a key version).

I guess we can see this as a bandaid, we don't implement versioned buckets in a better way for now (except for PutObjectAcl), but at least we don't raise exceptions linked to that. We will need to come back to this to better support the use case with some usage numbers.

Sources:
https://docs.aws.amazon.com/AmazonS3/latest/userguide/DeleteMarker.html
https://docs.aws.amazon.com/AmazonS3/latest/userguide/DeletingObjectVersions.html
https://docs.aws.amazon.com/AmazonS3/latest/userguide/notification-how-to-event-types-and-destinations.html#supported-notification-event-types

bentsku
bentsku commented Mar 24, 2023
View reviewed changes
localstack/services/s3/utils.py
moto_bucket: FakeBucket,
key: ObjectKey,
version_id: str = None,
raise_if_delete_marker_method: Literal["GET", "PUT"] = None,
Copy link
Contributor Author

@bentsku bentsku Mar 24, 2023

Choose a reason for hiding this comment

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

I really dislike this, but this is a quick fix for now and would need to be revisited to better implement versioning.

@coveralls
Copy link

coveralls commented Mar 24, 2023

Coverage Status

Coverage: 81.81% (+0.01%) from 81.796% when pulling 8d0836b on fix-s3-versioned into b5427bb on master.

thrau
thrau approved these changes Mar 24, 2023
View reviewed changes
Copy link
Member

@thrau thrau left a comment

Choose a reason for hiding this comment

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

great find! and looks like a pretty reasonable fix for now! 🏅 for all the snapshot tests.

@thrau thrau merged commit be5080c into master Mar 24, 2023
@thrau thrau deleted the fix-s3-versioned branch March 24, 2023 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thrau thrau thrau approved these changes

@macnev2013 macnev2013 Awaiting requested review from macnev2013

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bentsku @coveralls @thrau