object: Implement Patch
method #1307
Labels
No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
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
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#1307
Loading…
Reference in a new issue
No description provided.
Delete branch "aarifullin/frostfs-node:feat/patch/1"
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?
go.mod
: Bumpfrostfs-sdk-go/frostfs-api-go/v2
versionsPatch
methodPatch
methodobject patch
commandfe007035c6
to9880d508d5
9880d508d5
todfda548449
dfda548449
to7a6e386e15
@ -249,6 +249,26 @@ func (b Service) Put() (object.PutObjectStream, error) {
}, err
}
type patchStreamBasicChecker struct {
Same for this wrapper.
Fixed
@ -204,6 +204,26 @@ func (c *Service) Put() (objectSvc.PutObjectStream, error) {
}, err
}
type patchStreamBasicChecker struct {
This seems like a useless wrapper currently.
Why do we need this?
Removed
Now other methods have some APE checks defined here.
Where are
Patch()
APE checks processed?I supposed to support
APE
checks in next PR as I need to introduce the method inpolicy-engine
package. Currently, this is the stub but I can leaveTODO
for a whileIf they will be eventually introduced, OK, let's remove this wrapper.
I just do not see any reason not to do it now.
@ -173,0 +211,4 @@
// Send implements PatchObjectStream.
func (a *auditPatchStream) Send(ctx context.Context, req *object.PatchRequest) error {
a.containerID = req.GetBody().GetAddress().GetContainerID()
This should be similar to
put
,GetAddress
can return nil on the non-first send, right?True. However,
Patch
has noinit
message but that's can be fixed anywayFixed
@ -0,0 +72,4 @@
commonPrm: commonPrm,
}
putstm, err := s.putSvc.Put()
Will this stream will perform additional APE checks on everything?
Fixed
@ -0,0 +97,4 @@
RangeProvider: rangeProvider,
ObjectWriter: putstm.Target(),
It seems you initialize
putstm
just to violate incapsulation and get the target.Please, let's make this target construction explicit instead.
If it is too much trouble now, please, create a task to fix this.
The reason why I have violated the encapsulation - to make the
Put
logic the same. So, this requires a code refactor to move the logic to common package. Otherwise, we would get two put approaches and each requires a support that will lead to errors for sure because any change inPutSvc
must be reflected toPatchSvc
Common code reuse is exactly what I was looking at. Not necessary in a different package.
Could you create a task for this?
#1310
@ -0,0 +161,4 @@
patch := new(objectSDK.Patch)
patch.FromV2(req.GetBody())
if patch.ReplaceAttributes || len(patch.NewAttributes) > 0 {
I expected that no such logic was needed in node, why do we have this
if
?As we have no
init
message - we need to somehow track the first request message that can set attribute patch. I've replaced this withnonFirstSend
flag@ -0,0 +168,4 @@
}
}
if patch.PayloadPatch != nil {
If both fields are nil (attributes and payload), this should be an error, is it?
The check has been added
7a6e386e15
to735078c72c
735078c72c
tof1dfd749f2
f1dfd749f2
toa28941f3bc
a28941f3bc
to8a14260b18
Put
operation to common package #1310@ -872,0 +903,4 @@
}
slices.SortFunc(prm.PayloadPatches, func(a, b PayloadPatch) int {
if a.Range.GetOffset() == b.Range.GetOffset() {
So, basically
cmp.Compare
? https://pkg.go.dev/cmp#CompareFixed
@ -191,1 +215,4 @@
}
func (s patchStreamMetric) Send(ctx context.Context, req *object.PatchRequest) error {
if req.GetBody().GetPatch() != nil {
s.metrics.AddPayloadSize("Patch", len(req.GetBody().GetPatch().Chunk))
GetPatch().GetChunk()
?It seems this way
len(nil)
will get us zero and we have no need to branch withif
.Sorry, I need to fix
frostfs-api-go/v2
because I forgot to add the getterFixed
@ -0,0 +40,4 @@
})
rngPrm.SetRange(rng)
go func() {
What is the easiest way to see why this goroutine won't leak?
If I get your question correctly, then the easiest way to check this - to cancel
ctx
.When I was implementing
GetRange
I relied on this fact thatr.getSvc.GetRange(ctx, rngPrm)
returns an error by the cancelled job. But if it doesn't, the goroutine is screwed up.This way is way better:
@ -0,0 +47,4 @@
switch {
case errors.As(err, &splitErr):
pipeWriter.Close()
What is so special about this error?
You're right. From the patcher's POV it is the inability to get the original object's payloads
This error is only returned on raw=true GET of a big object.
Do we currently handle big objects correctly?
@ -0,0 +51,4 @@
case errors.As(err, &ecErr):
pipeWriter.Close()
default:
pipeWriter.CloseWithError(err)
Where will we handle this error in code?
This is done implicitly.
pipeWriter
closes the pipe with the error and the reader withinpatcher
will get an error by doingRead
. So, streamer'sSend
gets this error and returnsI already checked this trying to send incorrect object address to the handler and reading the range failed with error
@ -0,0 +168,4 @@
if !s.nonFirstSend {
err := s.patcher.ApplyAttributesPatch(ctx, patch.NewAttributes, patch.ReplaceAttributes)
if err != nil {
return fmt.Errorf("patch attributes error: %w", err)
remove
error
? it will occur in each level of wrapping (and in log message most likely)True
Fixed
06848e88d5
toe6c52f511c
e6c52f511c
to4e24aeae5c
4e24aeae5c
tofb93c26083
fb93c26083
to8a38f15c08
8a38f15c08
to8256622a9f
8256622a9f
toa1f0e9bc6e
a1f0e9bc6e
toc3825e2a27
c3825e2a27
to985bebaba5
@ -163,6 +167,8 @@ func (c *checkerImpl) CheckAPE(ctx context.Context, prm Prm) error {
}
}
prm.Method = replaceBySessionTokenVerb(prm.Method, prm.SessionToken)
We MUST NOT do things like this, not even think about it, it will bite us multiple times later.
CheckAPE
applies ape rules to the method it recieves, end of story.985bebaba5
tof4b706506b
@ -162,6 +166,13 @@ func (c *checkerImpl) CheckAPE(ctx context.Context, prm Prm) error {
default:
}
}
signedByStorageNode, err := c.signedByStorageNode(prm.SignatureKey)
This check should not be inside
CheckAPE
, it should be outside, right where we get the rules. We get the rules foringress
, thus if the request is from container node, nothing should be done.There is
prm.Role == nativeschema.PropertyValueContainerRoleContainer
in the beginning of this function, why is this not enough?Because a put request (by a patch operation, i.e.
ChunkedObjectWriter
) can be performed on non-container node. In this case, the role is notRoleContainer
@ -176,6 +180,7 @@ func (p *putStreamBasicChecker) Send(ctx context.Context, request *objectV2.PutR
Header: partInit.GetHeader(),
Method: nativeschema.MethodPutObject,
SenderKey: hex.EncodeToString(reqCtx.SenderKey),
SignatureKey: originalSig.GetKey(),
We shouldn't touch PUT while implementing PATCH.
Sorry, but I have no better idea. I sign
head/range/put
request (while performing a patch) by a local node key.sender_key ≠ signature
, because sender key is defined from session token etc.So,
initTrustedTarget
doesn't touch the session token but it signs a request by localNode key for patch case.It can pass APE check (APE just ignores it as
head/range/put
performed by patch), but a node doesn't become a new owner of the objectf4b706506b
to09a305a418
09a305a418
toc723c3d091
c723c3d091
to400fc81d71
400fc81d71
toaf5ad50b5a
@ -737,0 +768,4 @@
return errors.New("missing oid")
}
obj := new(oid.ID)
err = obj.ReadFromV2(*request.GetBody().GetAddress().GetObjectID())
*objV2
?Sure! Fixed
af5ad50b5a
to8fc9bf0254
8fc9bf0254
toc086e266ce
c086e266ce
to973e766044
@ -70,2 +70,4 @@
SenderKey string
// Original message signature key.
SignatureKey []byte
Still needed?
Fixed!
973e766044
tod168ba9beb