Repository: localstack/localstack
Pull request: Bugfix: Fix S3 internalError when try to get an object
Fix https://github.com/localstack/localstack/issues/6553


Problem statement:
bug: S3 internalError when try to get an object which was deleted before instead of error code NoSuchKey
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current Behavior

We have some tests which are creating and deleting objects at s3. This tests are failing with localstack version 0.14.4 and newer.

Because of an internalError we get as error null instead of NoSuchKey as error code.

Logs of localstack:

```
2022-07-29T14:44:55.319 ERROR --- [   asgi_gw_0] l.aws.handlers.logging     : exception during call chain: 'FakeDeleteMarker' object has no attribute '_expiry'
2022-07-29T14:44:55.325  INFO --- [   asgi_gw_0] localstack.request.aws     : AWS s3.GetObject => 500 (InternalError)
2022-07-29T14:44:55.463 ERROR --- [   asgi_gw_1] l.aws.handlers.logging     : exception during call chain: 'FakeDeleteMarker' object has no attribute '_expiry'
2022-07-29T14:44:55.464  INFO --- [   asgi_gw_1] localstack.request.aws     : AWS s3.GetObject => 500 (InternalError)
2022-07-29T14:44:57.444 ERROR --- [   asgi_gw_1] l.aws.handlers.logging     : exception during call chain: 'FakeDeleteMarker' object has no attribute '_expiry'
2022-07-29T14:44:57.445  INFO --- [   asgi_gw_1] localstack.request.aws     : AWS s3.GetObject => 500 (InternalError)
2022-07-29T14:45:00.403 ERROR --- [   asgi_gw_0] l.aws.handlers.logging     : exception during call chain: 'FakeDeleteMarker' object has no attribute '_expiry'
2022-07-29T14:45:00.404  INFO --- [   asgi_gw_0] localstack.request.aws     : AWS s3.GetObject => 500 (InternalError)
2022-07-29T14:45:06.976 ERROR --- [   asgi_gw_1] l.aws.handlers.logging     : exception during call chain: 'FakeDeleteMarker' object has no attribute '_expiry'
2022-07-29T14:45:06.977  INFO --- [   asgi_gw_1] localstack.request.aws     : AWS s3.GetObject => 500 (InternalError)

An error occurred (InternalError) when calling the GetObject operation (reached max retries: 4): exception while calling s3.GetObject: 'FakeDeleteMarker' object has no attribute '_expiry'
```

### Expected Behavior

We would expect to get an error code NoSuchKey instead of null.

### How are you starting LocalStack?

With a docker-compose file

### Steps To Reproduce

#### How are you starting localstack (e.g., `bin/localstack` command, arguments, or `docker-compose.yml`)

docker-conpose.yml:
> version: '3.5'
> services:
>   localstack:
>     image: localstack/localstack:1.0.2
>     volumes:
>       - ./localstack/init-localstack.sh:/docker-entrypoint-initaws.d/init-localstack.sh
>     environment:
>       - DEFAULT_REGION=eu-central-1
>       - SERVICES=s3

#### Client commands (e.g., AWS SDK code snippet, or sequence of "awslocal" commands)

init-localstack.sh: 
```
awslocal s3api create-bucket --bucket test-bucket --create-bucket-configuration LocationConstraint=eu-central-1
awslocal s3api put-object --bucket test-bucket --key test.json
awslocal s3api get-object --bucket test-bucket --key test.json test.json
awslocal s3api delete-object --bucket test-bucket --key test.json
awslocal s3api get-object --bucket test-bucket --key test.json test.json
```


### Environment

```markdown
- OS: Ubuntu 20.04
- LocalStack: 1.0.2
```


### Anything else?

_No response_

Hints from issue discussion:
Welcome to LocalStack! Thanks for reporting your first issue and our team will be working towards fixing the issue for you or reach out for more background information. We recommend joining our [Slack Community](https://localstack.cloud/contact/) for real-time help and drop a message to LocalStack Pro Support if you are a Pro user! If you are willing to contribute towards fixing this issue, please have a look at our [contributing guidelines](https://github.com/localstack/.github/blob/main/CONTRIBUTING.md) and our [developer guide](https://docs.localstack.cloud/developer-guide/).
Running into this as well. Full stack here:

```
2022-09-13T17:21:33.625 ERROR --- [   asgi_gw_1] l.aws.handlers.logging     : exception during call chain
Traceback (most recent call last):
File "/opt/code/localstack/localstack/aws/chain.py", line 57, in handle
handler(self, self.context, response)
File "/opt/code/localstack/localstack/aws/handlers/service.py", line 121, in __call__
handler(chain, context, response)
File "/opt/code/localstack/localstack/aws/handlers/legacy.py", line 81, in __call__
result = self.forward_request(
File "/opt/code/localstack/localstack/aws/handlers/legacy.py", line 141, in forward_request
return do_forward_request(
File "/opt/code/localstack/localstack/services/edge.py", line 225, in do_forward_request
result = do_forward_request_inmem(api, method, path, data, headers, port=port)
File "/opt/code/localstack/localstack/services/edge.py", line 249, in do_forward_request_inmem
response = modify_and_forward(
File "/opt/code/localstack/localstack/services/generic_proxy.py", line 604, in wrapper
value = func(*args, **kwargs)
File "/opt/code/localstack/localstack/services/generic_proxy.py", line 777, in modify_and_forward
updated_response = listener.return_response(
File "/opt/code/localstack/localstack/services/s3/s3_listener.py", line 1643, in return_response
if key_name and is_object_expired(bucket_name, key_name):
File "/opt/code/localstack/localstack/services/s3/s3_listener.py", line 626, in is_object_expired
if not key_obj or not key_obj._expiry:
AttributeError: 'FakeDeleteMarker' object has no attribute '_expiry'
2022-09-13T17:21:33.626  INFO --- [   asgi_gw_1] localstack.request.aws     : AWS s3.GetObject => 500 (InternalError)
```

Any ideas for a workaround aside from disabling versioning on the bucket?

Commit message: Fix s3 GetObject method error

Fix https://github.com/localstack/localstack/issues/6553

Patch to review:
diff --git a/localstack/services/s3/s3_listener.py b/localstack/services/s3/s3_listener.py
index bfcb920345253..f8db9447afe6c 100644
--- a/localstack/services/s3/s3_listener.py
+++ b/localstack/services/s3/s3_listener.py
@@ -688,7 +688,7 @@ def add_accept_range_header(response):
 def is_object_expired(bucket_name: str, key: str) -> bool:
     bucket = BackendState.get_bucket(bucket_name)
     key_obj = bucket.keys.get(key)
-    if not key_obj or not key_obj._expiry:
+    if not key_obj or not getattr(key_obj, "_expiry", False):
         return False
     tzname = key_obj._expiry.tzname()
     if not tzname:


Code review comments:
diff_hunk: @@ -688,7 +688,7 @@ def add_accept_range_header(response):
 def is_object_expired(bucket_name: str, key: str) -> bool:
     bucket = BackendState.get_bucket(bucket_name)
     key_obj = bucket.keys.get(key)
-    if not key_obj or not key_obj._expiry:
+    if not key_obj or not getattr(key_obj, "_expiry", False):
path: localstack/services/s3/s3_listener.py
text: @user1:
if the attribute doesn't exist anymore, then probably it doesn't make sense to try and access it in the first place, even with `getattr`. maybe the attribute moved somewhere else in moto?

@author:
Looks like you already have similar logic here https://github.com/localstack/localstack/blob/1bbc07609287f0df1310b66e73efb42524ed9d65/localstack/services/s3/utils.py#L118-L121
I guess that it is needed only for `FakeDeleteMarker` instance.

@user2:
Right. The issue sits right there: when getting an object in a versioned bucket, a deleted key would not be deleted but be marked as a `FakeDeleteMarker`, which cannot expire and does not have the `_expiry` attribute.
You can either use the `is_key_expired` helper function here to replace the part you modified, or implement the same logic in your PR.
We are in the process of overhauling the whole S3 service in LocalStack, and it will be soon available to be tested. 
Thanks a lot for tackling this issue! 

@user2:
Also, once fixed, you could test the change with the following test:
`tests.integration.s3.test_s3.TestS3.test_get_object_after_deleted_in_versioned_bucket`
You can remove the `xfail` as it should be fixed in both S3 providers. 

@author:
@user2 could you please help me with understanding why `test_get_object_after_deleted_in_versioned_bucket` test [failed](https://app.circleci.com/pipelines/github/localstack/localstack/10947/workflows/73abe8ef-82a5-4461-b3b8-d2e31ed00e48/jobs/74695/tests) and how I can fix this?

@user2:
Oh sorry, I overlooked that part. The current S3 provider has some parity issues with AWS. I suggested a change, you might need to copy/paste it and run the linter again to be sure.
This change allows the snapshot verification to skip those 2 values that are returned by LocalStack but should not be there. Again, sorry for overlooking that part!
And thanks a lot again!
(If you're interested in the snapshot mechanism and how we use it in LocalStack, you can read more here: https://localstack.cloud/blog/2022-08-04-parity-explained/)

@author:
Thank you!