TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 105 Pull requests 12 Projects Releases 2 Activity Actions

tree: Use delete verb instead put for Remove #1443

Merged
fyrchik merged 2 commits from aarifullin/frostfs-node:fix/tree_remove_ape into master 2024-10-29 08:04:24 +00:00
Conversation 2 Commits 2 Files changed 2 +208 -1
aarifullin commented 2024-10-22 13:04:04 +00:00
Member
Copy link

Close #1406

We came to a point that resolving the issue #1406 by fixing policy-engine is not a good idea:
AWS specifies the behavior for negated operators and operators with ...IfExists... in very complicating manner. That may lead to the backward-combability problem and also we may get unexpected behavior.

So, here is the workaround. Let me remind: although we no longer check tree operations by eACL tables, we still use ACL "flavor" providing ACL operations instead of tree service verbs (for a while).
I believe, using acl.OpObjectDelete must be safe and the check for Remove won't hit inappropriate policy (see #1406)

Close #1406 We came to a point that resolving the issue #1406 by fixing [policy-engine](https://git.frostfs.info/TrueCloudLab/policy-engine/pulls/93) is not a good idea: AWS specifies the behavior for negated operators and operators with `...IfExists...` in very complicating manner. That may lead to the backward-combability problem and also we may get unexpected behavior. So, here is the workaround. Let me remind: although we no longer check tree operations by eACL tables, we still use ACL "flavor" providing ACL operations instead of tree service verbs (for a while). I believe, using `acl.OpObjectDelete` must be safe and the check for `Remove` won't hit inappropriate policy (see #1406)
aarifullin added the
bug
 label 2024-10-22 13:04:04 +00:00
aarifullin added 1 commit 2024-10-22 13:04:04 +00:00
[#1406] tree: Use delete verb instead put for Remove
All checks were successful
DCO action / DCO (pull_request) Successful in 1m5s
Details
Tests and linters / Run gofumpt (pull_request) Successful in 1m19s
Details
Vulncheck / Vulncheck (pull_request) Successful in 2m11s
Details
Pre-commit hooks / Pre-commit (pull_request) Successful in 2m20s
Details
Build / Build Components (pull_request) Successful in 2m28s
Details
Tests and linters / gopls check (pull_request) Successful in 2m49s
Details
Tests and linters / Staticcheck (pull_request) Successful in 3m5s
Details
Tests and linters / Lint (pull_request) Successful in 3m36s
Details
Tests and linters / Tests (pull_request) Successful in 4m17s
Details
Tests and linters / Tests with -race (pull_request) Successful in 5m45s
Details
37ebd45f3d 
Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>
aarifullin changed title from [#1406] tree: Use delete verb instead put for Remove to tree: Use delete verb instead put for Remove 2024-10-22 13:04:18 +00:00
requested reviews from alexvanin, dkirillov, storage-core-committers, storage-core-developers 2024-10-22 13:05:00 +00:00
aarifullin reviewed 2024-10-22 13:06:42 +00:00
pkg/services/tree/service.go
@ -229,3 +229,3 @@
}
err := s.verifyClient(ctx, req, cid, b.GetBearerToken(), acl.OpObjectPut)
err := s.verifyClient(ctx, req, cid, b.GetBearerToken(), acl.OpObjectDelete)
aarifullin commented 2024-10-22 13:06:42 +00:00
Author
Member
Copy link

An alternative: pass OpObjectPut as well but verifyClient must specify the context of invocation to set "object type" property to APE-resource that seems unnecessary

An alternative: pass `OpObjectPut` as well but `verifyClient` must specify the context of invocation to set "object type" property to APE-resource that seems unnecessary
dkirillov commented 2024-10-24 12:46:03 +00:00
Member
Copy link

I suppose it's ok to use just acl.OpObjectDelete here (in tree service). But for the same check in object service it seems we need add object type property. Otherwise, if put tombstone request goes to node that is not in container we will get the same problem (because check fails). Or do I miss something?

I suppose it's ok to use just `acl.OpObjectDelete` here (in tree service). But for the same check in object service it seems we need add object type property. Otherwise, if `put tombstone` request goes to node that is not in container we will get the same problem (because check fails). Or do I miss something?
aarifullin commented 2024-10-24 12:53:07 +00:00
Author
Member
Copy link

The object service correctly sets resource properties and checks are performed correctly. We've got no problems so far

The tree service should have set the property on its own - like the object service does. But that's unnecessary if we use Delete op instead Put

The object service correctly sets resource properties and checks are performed correctly. We've got no problems so far The tree service should have set the property on its own - like the object service does. But that's unnecessary if we use Delete op instead Put
dstepanov-yadro approved these changes 2024-10-23 06:54:54 +00:00
Dismissed
acid-ant approved these changes 2024-10-23 07:18:14 +00:00
Dismissed
elebedeva approved these changes 2024-10-23 08:14:05 +00:00
Dismissed
fyrchik commented 2024-10-24 12:33:04 +00:00
Owner
Copy link

@alexvanin @dkirillov could you approve this? We might've broken something.

@alexvanin @dkirillov could you approve this? We might've broken something.
dkirillov approved these changes 2024-10-25 13:06:51 +00:00
Dismissed
dkirillov left a comment
Member
Copy link

LGTM, but can we add some tests?

LGTM, but can we add some tests?
aarifullin commented 2024-10-28 09:22:25 +00:00
Author
Member
Copy link

LGTM, but can we add some tests?

Could you clarify then, what particularly you'd like to check with such tests? As we don't mock tree service, handlers won't be checked anyway. The checker itself is already tested and it works well (the same checker is used for object service and it's been already checked there)

UPD: I've add a couple of unit-tests to check that put and delete rules are not intersected and status is returned correctly

> LGTM, but can we add some tests? Could you clarify then, what particularly you'd like to check with such tests? As we don't mock tree service, handlers won't be checked anyway. The checker itself is already tested and it works well (the same checker is used for object service and it's been already checked there) **UPD:** I've add a couple of unit-tests to check that put and delete rules are not intersected and status is returned correctly
👍 1
aarifullin added 1 commit 2024-10-28 10:09:19 +00:00
[#1406] tree: Add unit-tests for ape check
All checks were successful
Tests and linters / Run gofumpt (pull_request) Successful in 1m27s
Details
DCO action / DCO (pull_request) Successful in 1m38s
Details
Pre-commit hooks / Pre-commit (pull_request) Successful in 2m23s
Details
Vulncheck / Vulncheck (pull_request) Successful in 2m19s
Details
Build / Build Components (pull_request) Successful in 2m33s
Details
Tests and linters / gopls check (pull_request) Successful in 2m53s
Details
Tests and linters / Staticcheck (pull_request) Successful in 3m1s
Details
Tests and linters / Lint (pull_request) Successful in 3m45s
Details
Tests and linters / Tests (pull_request) Successful in 4m20s
Details
Tests and linters / Tests with -race (pull_request) Successful in 5m59s
Details
6919c7c17f 
Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>
aarifullin dismissed dstepanov-yadro's review 2024-10-28 10:09:19 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

aarifullin dismissed acid-ant's review 2024-10-28 10:09:19 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

aarifullin dismissed elebedeva's review 2024-10-28 10:09:19 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

aarifullin dismissed dkirillov's review 2024-10-28 10:09:19 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

requested reviews from dkirillov, storage-core-committers, storage-core-developers 2024-10-28 10:27:48 +00:00
acid-ant approved these changes 2024-10-28 10:35:31 +00:00
dkirillov approved these changes 2024-10-28 14:25:39 +00:00
fyrchik merged commit 012af5cc38 into master 2024-10-29 08:04:24 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
alexvanin
acid-ant
dkirillov
TrueCloudLab/storage-core-developers
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
badger

Badger DB related issues

  
frostfs-adm

   
frostfs-cli

   
frostfs-ir

   
frostfs-lens

   
frostfs-node

   
good first issue

Good for newcomers

  
triage
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

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
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
6 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#1443
No description provided.
Delete branch "aarifullin/frostfs-node:fix/tree_remove_ape"

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?