[#488] middleware/policy: frostfs to S3 error transformation #488
No reviewers
Labels
No labels
P0
P1
P2
P3
good first issue
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-s3-gw#488
Loading…
Reference in a new issue
No description provided.
Delete branch "nzinkevich/frostfs-s3-gw:access_bug"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Moved layer public errors to api/layer/errors to avoid circular dependencies
Made transformToS3Error public and moved to
api/errors
packageSigned-off-by: Nikita Zinkevich n.zinkevich@yadro.com
7f425712c1
to6d002d9164
[#XXX] Add transformation to S3 error in policy checkto [#488] Add transformation to S3 error in policy check6d002d9164
to41dc579744
[#488] Add transformation to S3 error in policy checkto [#488] policy: add transformation to S3 error41dc579744
todcdc22a87c
[#488] policy: add transformation to S3 errorto [#488] policy: add error transformation to S3[#488] policy: add error transformation to S3to [#488] policy: add error transformation to S3 error[#488] policy: add error transformation to S3 errorto [#488] middleware/policy: frostfs to S3 error transformationLooks good to me, but I would wait for @dkirillov review.
@ -6,2 +7,3 @@
frosterrors "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/internal/frostfs/errors"
layerErr "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/layer/errors"
frostErr "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/internal/frostfs/errors"
Package name should have lower case name
@ -31,3 +30,3 @@
func (h *handler) logAndSendError(w http.ResponseWriter, logText string, reqInfo *middleware.ReqInfo, err error, additional ...zap.Field) {
err = handleDeleteMarker(w, err)
if code, wrErr := middleware.WriteErrorResponse(w, reqInfo, transformToS3Error(err)); wrErr != nil {
if code, wrErr := middleware.WriteErrorResponse(w, reqInfo, s3errors.TransformToS3Error(err)); wrErr != nil {
I would prefer to use for example
apierr
orapierrors
instead ofs3errors
@ -2,3 +2,3 @@
import (
"errors"
stdErrors "errors"
Let's keep standard error package as
errors
@ -226,4 +223,0 @@
ErrAccessDenied = errors.New("access denied")
// ErrGatewayTimeout is returned from FrostFS in case of timeout, deadline exceeded etc.
ErrGatewayTimeout = errors.New("gateway timeout")
Actually, I would try to keep errors near to interface that is related to it. Can we move interface to the same directory
layer/error
(it should be renamed then)?@ -86,3 +85,3 @@
if err := policyCheck(r, cfg); err != nil {
reqLogOrDefault(ctx, cfg.Log).Error(logs.PolicyValidationFailed, zap.Error(err))
err = frostfsErrors.UnwrapErr(err)
err = apiErr.TransformToS3Error(err)
It would be nice to have tests for such change
Also consider using the
TransformToS3Error
inAuth
middlewareAuth
already receives S3 errors here. Should we change the type of error returned in 'center.Authenticate()'?box, err := center.Authenticate(r)
Yes but if some error returned from
center.Authenticate
isn't "s3 api error". We returnaccess denied
to client. But maybe sometimereturn nil, fmt.Errorf("get box '%s': %w", addr, err)
res, err := x.frostFS.GetObject(ctx, layer.PrmObjectGet{
It would be nice to get original gateway timeout
return fmt.Errorf("%s: %w: %s", msg, layer.ErrGatewayTimeout, err.Error())
Please, add tests
Added
dcdc22a87c
toff02c86dbf
New commits pushed, approval review dismissed automatically according to repository settings
New commits pushed, approval review dismissed automatically according to repository settings
Why don't we just move whole file (without any change)? The same for
api/layer/tree_service.go
Moved entire frostfs.go and tree_service.go to packages layer/frostfs and layer/tree respectively
@ -4,2 +4,2 @@
access_key: CAtUxDSSFtuVyVCjHTMhwx3eP3YSPo5ffwbPcnKfcdrD06WwUSn72T5EBNe3jcgjL54rmxFM6u3nUAoNBps8qJ1PD
secret_key: 560027d81c277de7378f71cbf12a32e4f7f541de724be59bcfdbfdc925425f30
access_key: CjvKZyyVWA3au4s14SzPuaxqKW7c5EBRThwt4aaWPxeu0E1CQYfJ91yxtGZ62F7rJMBf2dQXnUDsmfeRnRx6huHNt
secret_key: 2c318a7e56e2b69335176e7ae12f44ba15a34941e452f715e93a933514d20381
Why do we change this?
Fixed
@ -108,4 +108,2 @@
m.requestDuration.WithLabelValues(node.Address(), methodListContainer).Set(float64(node.AverageListContainer().Milliseconds()))
m.requestDuration.WithLabelValues(node.Address(), methodDeleteContainer).Set(float64(node.AverageDeleteContainer().Milliseconds()))
m.requestDuration.WithLabelValues(node.Address(), methodGetContainerEacl).Set(float64(node.AverageGetContainerEACL().Milliseconds()))
m.requestDuration.WithLabelValues(node.Address(), methodSetContainerEacl).Set(float64(node.AverageSetContainerEACL().Milliseconds()))
Why do we drop it? At least it should be done in separate commit (and
methodGetContainerEacl
constant should be deleted as well).Undo frostfs-sdk-go version, fixed by #496
@ -6,3 +6,3 @@
"encoding/base64"
"encoding/xml"
"errors"
stderrors "errors"
Why? Keep it
"errors"
. (The same for other similar places)ff02c86dbf
toff4a57d686
New commits pushed, approval review dismissed automatically according to repository settings
afbf50b0c7
toec1114466a
ec1114466a
tob2df6d3a67
b2df6d3a67
toc2adbd758a