object: Implement Patch method #1307

Merged
fyrchik merged 5 commits from aarifullin/frostfs-node:feat/patch/1 into master 2024-08-16 14:13:12 +00:00
Member
  • go.mod: Bump frostfs-sdk-go/frostfs-api-go/v2 versions
  • Also, resolve dependencies and conflicts for object service by creating stub for Patch method
  • object service: Implement Patch method
  • cli: Introduce object patch command
* `go.mod`: Bump `frostfs-sdk-go/frostfs-api-go/v2` versions * Also, resolve dependencies and conflicts for object service by creating stub for `Patch` method * object service: Implement `Patch` method * cli: Introduce `object patch` command
aarifullin force-pushed feat/patch/1 from fe007035c6 to 9880d508d5 2024-08-12 14:17:35 +00:00 Compare
aarifullin requested review from storage-core-committers 2024-08-12 14:21:34 +00:00
aarifullin requested review from storage-core-developers 2024-08-12 14:21:37 +00:00
aarifullin force-pushed feat/patch/1 from 9880d508d5 to dfda548449 2024-08-12 14:49:44 +00:00 Compare
aarifullin force-pushed feat/patch/1 from dfda548449 to 7a6e386e15 2024-08-13 07:37:42 +00:00 Compare
fyrchik reviewed 2024-08-13 08:55:12 +00:00
@ -249,6 +249,26 @@ func (b Service) Put() (object.PutObjectStream, error) {
}, err
}
type patchStreamBasicChecker struct {
Owner

Same for this wrapper.

Same for this wrapper.
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -204,6 +204,26 @@ func (c *Service) Put() (objectSvc.PutObjectStream, error) {
}, err
}
type patchStreamBasicChecker struct {
Owner

This seems like a useless wrapper currently.
Why do we need this?

This seems like a useless wrapper currently. Why do we need this?
Author
Member

Removed

Removed
Owner

Now other methods have some APE checks defined here.
Where are Patch() APE checks processed?

Now other methods have some APE checks defined here. Where are `Patch()` APE checks processed?
Author
Member

I supposed to support APE checks in next PR as I need to introduce the method in policy-engine package. Currently, this is the stub but I can leave TODO for a while

I supposed to support `APE` checks in next PR as I need to introduce the method in `policy-engine` package. Currently, this is the stub but I can leave `TODO` for a while
Owner

If they will be eventually introduced, OK, let's remove this wrapper.
I just do not see any reason not to do it now.

If they will be eventually introduced, OK, let's remove this wrapper. I just do not see any reason not to do it now.
fyrchik marked this conversation as resolved
@ -173,0 +211,4 @@
// Send implements PatchObjectStream.
func (a *auditPatchStream) Send(ctx context.Context, req *object.PatchRequest) error {
a.containerID = req.GetBody().GetAddress().GetContainerID()
Owner

This should be similar to put, GetAddress can return nil on the non-first send, right?

This should be similar to `put`, `GetAddress` can return nil on the non-first send, right?
Author
Member

This should be similar to put

True. However, Patch has no init message but that's can be fixed anyway

> This should be similar to put True. However, `Patch` has no `init` message but that's can be fixed anyway
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -0,0 +72,4 @@
commonPrm: commonPrm,
}
putstm, err := s.putSvc.Put()
Owner

Will this stream will perform additional APE checks on everything?

Will this stream will perform additional APE checks on everything?
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -0,0 +97,4 @@
RangeProvider: rangeProvider,
ObjectWriter: putstm.Target(),
Owner

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.

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.
Author
Member

If it is too much trouble now

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 in PutSvc must be reflected to PatchSvc

> If it is too much trouble now 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 in `PutSvc` must be reflected to `PatchSvc`
Owner

Common code reuse is exactly what I was looking at. Not necessary in a different package.
Could you create a task for this?

Common code reuse is exactly what I was looking at. Not necessary in a different package. Could you create a task for this?
Author
Member
https://git.frostfs.info/TrueCloudLab/frostfs-node/issues/1310
fyrchik marked this conversation as resolved
@ -0,0 +161,4 @@
patch := new(objectSDK.Patch)
patch.FromV2(req.GetBody())
if patch.ReplaceAttributes || len(patch.NewAttributes) > 0 {
Owner

I expected that no such logic was needed in node, why do we have this if?

I expected that no such logic was needed in node, why do we have this `if`?
Author
Member

As we have no init message - we need to somehow track the first request message that can set attribute patch. I've replaced this with nonFirstSend flag

As we have no `init` message - we need to somehow track the first request message that can set attribute patch. I've replaced this with `nonFirstSend` flag
fyrchik marked this conversation as resolved
@ -0,0 +168,4 @@
}
}
if patch.PayloadPatch != nil {
Owner

If both fields are nil (attributes and payload), this should be an error, is it?

If both fields are nil (attributes and payload), this should be an error, is it?
Author
Member

The check has been added

The check has been added
fyrchik marked this conversation as resolved
aarifullin force-pushed feat/patch/1 from 7a6e386e15 to 735078c72c 2024-08-13 10:23:24 +00:00 Compare
aarifullin force-pushed feat/patch/1 from 735078c72c to f1dfd749f2 2024-08-13 10:48:46 +00:00 Compare
aarifullin force-pushed feat/patch/1 from f1dfd749f2 to a28941f3bc 2024-08-13 11:46:46 +00:00 Compare
aarifullin force-pushed feat/patch/1 from a28941f3bc to 8a14260b18 2024-08-13 11:48:07 +00:00 Compare
fyrchik requested changes 2024-08-13 13:51:34 +00:00
@ -872,0 +903,4 @@
}
slices.SortFunc(prm.PayloadPatches, func(a, b PayloadPatch) int {
if a.Range.GetOffset() == b.Range.GetOffset() {
Owner

So, basically cmp.Compare? https://pkg.go.dev/cmp#Compare

So, basically `cmp.Compare`? https://pkg.go.dev/cmp#Compare
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -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))
Owner

GetPatch().GetChunk() ?
It seems this way len(nil) will get us zero and we have no need to branch with if.

`GetPatch().GetChunk()` ? It seems this way `len(nil)` will get us zero and we have no need to branch with `if`.
Author
Member

Sorry, I need to fix frostfs-api-go/v2 because I forgot to add the getter

Sorry, I need to fix `frostfs-api-go/v2` because I forgot to add the getter
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -0,0 +40,4 @@
})
rngPrm.SetRange(rng)
go func() {
Owner

What is the easiest way to see why this goroutine won't leak?

What is the easiest way to see why this goroutine won't leak?
Author
Member

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 that r.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:

func (r *rangeProvider) GetRange(ctx context.Context, rng *objectSDK.Range) io.Reader {
	pipeReader, pipeWriter := io.Pipe()

	var rngPrm getsvc.RangePrm
	rngPrm.SetCommonParameters(r.commonPrm)

	rngPrm.WithAddress(r.addr)
	rngPrm.WithRawFlag(true)
	rngPrm.SetChunkWriter(&pipeChunkWriter{
		wr: pipeWriter,
	})
	rngPrm.SetRange(rng)

	getRangeErr := make(chan error)

	go func() {
		defer pipeWriter.Close()

		select {
		case <-ctx.Done():
			pipeWriter.CloseWithError(ctx.Err())
		case err := <-getRangeErr:
			pipeWriter.CloseWithError(err) // pipeWriter.CloseWithError(nil) -> pipeWriter.Close()
		}
	}()

	go func() {
		getRangeErr <- r.getSvc.GetRange(ctx, rngPrm)
	}()

	return pipeReader
}
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 that `r.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: ```go func (r *rangeProvider) GetRange(ctx context.Context, rng *objectSDK.Range) io.Reader { pipeReader, pipeWriter := io.Pipe() var rngPrm getsvc.RangePrm rngPrm.SetCommonParameters(r.commonPrm) rngPrm.WithAddress(r.addr) rngPrm.WithRawFlag(true) rngPrm.SetChunkWriter(&pipeChunkWriter{ wr: pipeWriter, }) rngPrm.SetRange(rng) getRangeErr := make(chan error) go func() { defer pipeWriter.Close() select { case <-ctx.Done(): pipeWriter.CloseWithError(ctx.Err()) case err := <-getRangeErr: pipeWriter.CloseWithError(err) // pipeWriter.CloseWithError(nil) -> pipeWriter.Close() } }() go func() { getRangeErr <- r.getSvc.GetRange(ctx, rngPrm) }() return pipeReader } ```
@ -0,0 +47,4 @@
switch {
case errors.As(err, &splitErr):
pipeWriter.Close()
Owner

What is so special about this error?

What is so special about this error?
Author
Member

You're right. From the patcher's POV it is the inability to get the original object's payloads

You're right. From the patcher's POV it is the inability to get the original object's payloads
Owner

This error is only returned on raw=true GET of a big object.
Do we currently handle big objects correctly?

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)
Owner

Where will we handle this error in code?

Where will we handle this error in code?
Author
Member

This is done implicitly. pipeWriter closes the pipe with the error and the reader within patcher will get an error by doing Read. So, streamer's Send gets this error and returns

func (p *patcher) copyRange(ctx context.Context, rng *objectSDK.Range) error {
	rdr := p.rangeProvider.GetRange(ctx, rng)
	for {
		buffOrigPayload := make([]byte, p.readerBuffSize)
		n, readErr := rdr.Read(buffOrigPayload)
		if readErr != nil {
			if readErr != io.EOF {
				return fmt.Errorf("read: %w", readErr)
			}
		}
		_, wrErr := p.objectWriter.Write(ctx, buffOrigPayload[:n])
		if wrErr != nil {
			return fmt.Errorf("write: %w", wrErr)
		}
		if readErr == io.EOF {
			break
		}
	}
	return nil
}

I already checked this trying to send incorrect object address to the handler and reading the range failed with error

This is done implicitly. `pipeWriter` closes the pipe with the error and the reader within `patcher` will get an error by doing `Read`. So, streamer's `Send` gets this error and returns ```go func (p *patcher) copyRange(ctx context.Context, rng *objectSDK.Range) error { rdr := p.rangeProvider.GetRange(ctx, rng) for { buffOrigPayload := make([]byte, p.readerBuffSize) n, readErr := rdr.Read(buffOrigPayload) if readErr != nil { if readErr != io.EOF { return fmt.Errorf("read: %w", readErr) } } _, wrErr := p.objectWriter.Write(ctx, buffOrigPayload[:n]) if wrErr != nil { return fmt.Errorf("write: %w", wrErr) } if readErr == io.EOF { break } } return nil } ``` I already checked this trying to send incorrect object address to the handler and reading the range failed with error
fyrchik marked this conversation as resolved
@ -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)
Owner

remove error? it will occur in each level of wrapping (and in log message most likely)

remove `error`? it will occur in each level of wrapping (and in log message most likely)
Author
Member

True

True
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
aarifullin force-pushed feat/patch/1 from 06848e88d5 to e6c52f511c 2024-08-13 15:02:47 +00:00 Compare
aarifullin force-pushed feat/patch/1 from e6c52f511c to 4e24aeae5c 2024-08-13 16:34:44 +00:00 Compare
aarifullin force-pushed feat/patch/1 from 4e24aeae5c to fb93c26083 2024-08-13 16:45:58 +00:00 Compare
aarifullin force-pushed feat/patch/1 from fb93c26083 to 8a38f15c08 2024-08-13 16:59:33 +00:00 Compare
aarifullin force-pushed feat/patch/1 from 8a38f15c08 to 8256622a9f 2024-08-13 21:07:24 +00:00 Compare
aarifullin force-pushed feat/patch/1 from 8256622a9f to a1f0e9bc6e 2024-08-13 21:11:51 +00:00 Compare
aarifullin force-pushed feat/patch/1 from a1f0e9bc6e to c3825e2a27 2024-08-14 11:40:13 +00:00 Compare
aarifullin force-pushed feat/patch/1 from c3825e2a27 to 985bebaba5 2024-08-14 11:48:42 +00:00 Compare
fyrchik requested changes 2024-08-14 11:51:51 +00:00
@ -163,6 +167,8 @@ func (c *checkerImpl) CheckAPE(ctx context.Context, prm Prm) error {
}
}
prm.Method = replaceBySessionTokenVerb(prm.Method, prm.SessionToken)
Owner

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.

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.
aarifullin marked this conversation as resolved
aarifullin force-pushed feat/patch/1 from 985bebaba5 to f4b706506b 2024-08-15 09:28:42 +00:00 Compare
fyrchik requested changes 2024-08-15 10:45:22 +00:00
@ -162,6 +166,13 @@ func (c *checkerImpl) CheckAPE(ctx context.Context, prm Prm) error {
default:
}
}
signedByStorageNode, err := c.signedByStorageNode(prm.SignatureKey)
Owner

This check should not be inside CheckAPE, it should be outside, right where we get the rules. We get the rules for ingress, thus if the request is from container node, nothing should be done.

This check should not be inside `CheckAPE`, it should be outside, right where we get the rules. We get the rules for `ingress`, thus if the request is from container node, nothing should be done.
Owner

There is prm.Role == nativeschema.PropertyValueContainerRoleContainer in the beginning of this function, why is this not enough?

There is `prm.Role == nativeschema.PropertyValueContainerRoleContainer` in the beginning of this function, why is this not enough?
Author
Member

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 not RoleContainer

> 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 not `RoleContainer`
@ -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(),
Owner

We shouldn't touch PUT while implementing PATCH.

We shouldn't touch PUT while implementing PATCH.
Author
Member

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 object

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 object
aarifullin force-pushed feat/patch/1 from f4b706506b to 09a305a418 2024-08-15 13:40:24 +00:00 Compare
aarifullin force-pushed feat/patch/1 from 09a305a418 to c723c3d091 2024-08-15 17:53:48 +00:00 Compare
aarifullin force-pushed feat/patch/1 from c723c3d091 to 400fc81d71 2024-08-15 18:06:31 +00:00 Compare
aarifullin force-pushed feat/patch/1 from 400fc81d71 to af5ad50b5a 2024-08-15 18:33:01 +00:00 Compare
fyrchik reviewed 2024-08-16 07:59:18 +00:00
@ -737,0 +768,4 @@
return errors.New("missing oid")
}
obj := new(oid.ID)
err = obj.ReadFromV2(*request.GetBody().GetAddress().GetObjectID())
Owner

*objV2?

`*objV2`?
Author
Member

Sure! Fixed

Sure! Fixed
fyrchik marked this conversation as resolved
aarifullin force-pushed feat/patch/1 from af5ad50b5a to 8fc9bf0254 2024-08-16 11:12:20 +00:00 Compare
aarifullin force-pushed feat/patch/1 from 8fc9bf0254 to c086e266ce 2024-08-16 11:45:40 +00:00 Compare
aarifullin force-pushed feat/patch/1 from c086e266ce to 973e766044 2024-08-16 12:36:40 +00:00 Compare
fyrchik approved these changes 2024-08-16 13:02:21 +00:00
@ -70,2 +70,4 @@
SenderKey string
// Original message signature key.
SignatureKey []byte
Owner

Still needed?

Still needed?
Author
Member

Fixed!

Fixed!
fyrchik marked this conversation as resolved
aarifullin force-pushed feat/patch/1 from 973e766044 to d168ba9beb 2024-08-16 13:07:13 +00:00 Compare
fyrchik approved these changes 2024-08-16 13:13:43 +00:00
acid-ant approved these changes 2024-08-16 14:01:51 +00:00
fyrchik merged commit 41104f2383 into master 2024-08-16 14:13:12 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-developers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#1307
No description provided.