[#143] Fix NoSuchKey error on get/head #156

Merged

dkirillov merged 1 commit from dkirillov/frostfs-s3-gw:bugix/fix_not_found_error into master

2023-07-26 21:08:01 +00:00

dkirillov commented

2023-06-29 13:05:00 +00:00

Member

Add forgotten check for sdk not found error.

There are some questions related to this topic:

Should we wrap error in cases like this to see full info in logs:

compare:
{"level":"error","msg":"reqeust failed","status":404,"request_id":"","method":"","bucket":"bucket","object":"obj","description":"could not find object","error":"NoSuchKey: 404 => The specified key does not exist.: status: code = 2049 message = object not found"}
and
{"level":"error","msg":"reqeust failed","status":404,"request_id":"","method":"","bucket":"bucket","object":"obj","description":"could not find object","error":"NoSuchVersion: 404 => Indicates that the version ID specified in the request does not match an existing version."}

It would be nice if the following test will pass:


func TestGetObject(t *testing.T) {
	hc := prepareHandlerContext(t)
	bktName, objName := "bucket", "obj"
	bktInfo, objInfo := createVersionedBucketAndObject(hc.t, hc, bktName, objName)

	putObject(hc.t, hc, bktName, objName)

	checkFound(hc.t, hc, bktName, objName, objInfo.VersionID())
	checkFound(hc.t, hc, bktName, objName, emptyVersion)

	addr := getAddressOfLastVersion(hc, bktInfo, objName)
	hc.tp.SetObjectError(addr, apistatus.ObjectNotFound{})
	hc.tp.SetObjectError(objInfo.Address(), apistatus.ObjectNotFound{})

	getObjectAssertS3Error(hc, bktName, objName, objInfo.VersionID(), s3errors.ErrNoSuchVersion)
	getObjectAssertS3Error(hc, bktName, objName, emptyVersion, s3errors.ErrNoSuchKey)
}

Actually we get 200 http code because of writing header before reading object from FrostFS (if object id exists in the cache but not in storage we get 200 code and error in body, probably we can write status code header right after init reading)

Add forgotten check for sdk not found error. There are some questions related to this topic: 1. Should we wrap error in cases like [this](https://git.frostfs.info/dkirillov/frostfs-s3-gw/src/commit/1587180b9e2f013aad98d17ebeffb895887dcc03/api/layer/object.go#L373-L378) to see full info in logs: ``` compare: {"level":"error","msg":"reqeust failed","status":404,"request_id":"","method":"","bucket":"bucket","object":"obj","description":"could not find object","error":"NoSuchKey: 404 => The specified key does not exist.: status: code = 2049 message = object not found"} and {"level":"error","msg":"reqeust failed","status":404,"request_id":"","method":"","bucket":"bucket","object":"obj","description":"could not find object","error":"NoSuchVersion: 404 => Indicates that the version ID specified in the request does not match an existing version."} ``` 2. It would be nice if the following test will pass: ```golang func TestGetObject(t *testing.T) { hc := prepareHandlerContext(t) bktName, objName := "bucket", "obj" bktInfo, objInfo := createVersionedBucketAndObject(hc.t, hc, bktName, objName) putObject(hc.t, hc, bktName, objName) checkFound(hc.t, hc, bktName, objName, objInfo.VersionID()) checkFound(hc.t, hc, bktName, objName, emptyVersion) addr := getAddressOfLastVersion(hc, bktInfo, objName) hc.tp.SetObjectError(addr, apistatus.ObjectNotFound{}) hc.tp.SetObjectError(objInfo.Address(), apistatus.ObjectNotFound{}) getObjectAssertS3Error(hc, bktName, objName, objInfo.VersionID(), s3errors.ErrNoSuchVersion) getObjectAssertS3Error(hc, bktName, objName, emptyVersion, s3errors.ErrNoSuchKey) } ``` Actually we get `200` http code because of [writing header before reading object from](https://git.frostfs.info/dkirillov/frostfs-s3-gw/src/commit/1587180b9e2f013aad98d17ebeffb895887dcc03/api/handler/get.go#L204-L209) FrostFS (if object id exists in the cache but not in storage we get 200 code and error in body, probably we can write status code header right after [init reading](https://git.frostfs.info/dkirillov/frostfs-s3-gw/src/commit/1587180b9e2f013aad98d17ebeffb895887dcc03/api/layer/layer.go#L422-L425))

dkirillov self-assigned this 2023-06-29 13:05:00 +00:00

alexvanin commented

2023-06-30 06:50:29 +00:00

Owner

Should we wrap error in cases like this to see full info in logs:

I think gateway will benefit from this unless it takes major refactor to make it happen.

It would be nice if the following test will pass:

Agree. There are cases when S3 docs tell the client that status code might not be relevant, e.g. with CompleteMultipartUpload. However, if it is possible to avoid it in this case, we should try to do it.

https://docs.aws.amazon.com/AmazonS3/latest/API/API_CompleteMultipartUpload.html
A request could fail after the initial 200 OK response has been sent. This means that a 200 OK response can contain either a success or an error. If you call the S3 API directly, make sure to design your application to parse the contents of the response and handle it appropriately.

> Should we wrap error in cases like this to see full info in logs: I think gateway will benefit from this unless it takes major refactor to make it happen. > It would be nice if the following test will pass: Agree. There are cases when S3 docs tell the client that status code might not be relevant, e.g. with CompleteMultipartUpload. However, if it is possible to avoid it in this case, we should try to do it. > https://docs.aws.amazon.com/AmazonS3/latest/API/API_CompleteMultipartUpload.html > A request could fail after the initial 200 OK response has been sent. This means that a 200 OK response can contain either a success or an error. If you call the S3 API directly, make sure to design your application to parse the contents of the response and handle it appropriately.

dkirillov commented

2023-06-30 07:24:37 +00:00

Author

Member

However, if it is possible to avoid it in this case, we should try to do it.

Ok, I'll create a new issue for that since it requires more changes than I'd like to add in this PR.

> However, if it is possible to avoid it in this case, we should try to do it. Ok, I'll create a new issue for that since it requires more changes than I'd like to add in this PR.

dkirillov referenced this pull request

2023-06-30 07:33:40 +00:00

Don't return 200 code when object body is not found #158

dkirillov force-pushed bugix/fix_not_found_error from 89463d711f to 0caa5dd3c5

2023-06-30 09:05:13 +00:00

Compare

dkirillov force-pushed bugix/fix_not_found_error from 0caa5dd3c5 to d531b13866

2023-06-30 09:11:50 +00:00

Compare

dkirillov changed title from ~~WIP: [#143] Fix NoSuchKey error on get/head~~ to [#143] Fix NoSuchKey error on get/head

2023-06-30 09:15:46 +00:00

alexvanin approved these changes 2023-07-04 08:30:59 +00:00