[#1249] object: Remove all APE pre-checks in handlers #1252
No reviewers
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
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#1252
Loading…
Reference in a new issue
No description provided.
Delete branch "aarifullin/frostfs-node:fix/ape_attr"
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?
Head
,Get
,GetRangeHash
should no longer use APE pre-checksas that leads only to incorrect rule chain processing for requests:
NoRuleFound
may be unexpected as someAllow
rule is actually defined but can't be matched yet as it gets no object
attributes;
Allow
may be incorrect as someDeny
ruleis actually defined but can't bet matched yet as it gets no object
attirbutes;
Close #1249
@ -251,2 +261,3 @@
return nil, toStatusErr(err)
}
}
unrelated to the commit
Fixed
@ -248,3 +254,3 @@
})
if err != nil {
return nil, toStatusErr(err)
// For `Head`` case `NoRuleFound` status doesn't mean yet that the request is not allowed.
Where in code will the DENY rule be found afterwise?
288 line:
err = c.apeChecker.CheckAPE(ctx, Prm{...})
on backward check@ -248,3 +254,3 @@
})
if err != nil {
return nil, toStatusErr(err)
// For `Head`` case `NoRuleFound` status doesn't mean yet that the request is not allowed.
Where in code will the DENY rule be found afterwise?
db20381f63
tocc03658e69
Just FYI, not very important.
In PR's description:
In the commit header:
In the commit message:
It should be
even IF
, not justeven
.@ -204,0 +206,4 @@
}
}
// nonAllowStatusError is returned by CheckAPE in the case if non-allow status returned by the chain router.
I suggest rephrasing the comment:
nonAllowStatusError is returned by CheckAPE if the chain router returns non-allow status.
Fixed
@ -157,3 +157,3 @@
})
if err != nil {
return toStatusErr(err)
// For Get case `NoRuleFound` status doesn't mean yet that the request is not allowed.
For Get request `NoRuleFound` status doesn't mean that the request is not allowed.
@ -158,2 +158,3 @@
if err != nil {
return toStatusErr(err)
// For Get case `NoRuleFound` status doesn't mean yet that the request is not allowed.
// Probably, the request may be allowed by specific attributes that coudln't be read on forward
This comment is hard to understand. Can you please rephrase it using simpler wording?
Sure!
Rephrased:
What do you think?
Much better!
Only one question:
Filled with what?
Slightly fixed and changed the text. Check it out! :)
Slightly fixed and changed the text. Check it out! :)
cc03658e69
to47d8eb92c6
47d8eb92c6
to4ad28b8831
[#1249] object: Go on handling even APE returnsto [#1249] object: Go on handling even if APE returnsNoRuleFound
NoRuleFound
@ -157,3 +157,3 @@
})
if err != nil {
return toStatusErr(err)
// `Get` is checked by APE in both direction. The forward check is performed without the object header
directionS
@ -248,3 +256,3 @@
})
if err != nil {
return nil, toStatusErr(err)
// `Head` is checked by APE in both direction. The forward check is performed without the object header
directionS
fixed!
4ad28b8831
to639c96a4aa
@ -160,0 +162,4 @@
// specific object attributes that aren't retrieved yet. But still they can be retrieved
// on the backward check and the rule may match on the fully filled APE-request.
var statError nonAllowStatusError
if errors.As(err, &statError) && !statError.IsNoRuleFoundStatus() {
What about EACL-compatible tables? They are processed in order and we can encounter DENY because we skipped some valid ALLOW.
Do we need this check at all if we also check everything afterwards?
What about EACL-compatible tables? They are processed in order and we can encounter DENY because we skipped some valid ALLOW.
Do we need this check at all if we also check everything afterwards?
It works out when either local overrides or Policy contract defines general denying rule like
deny all object operation on this container
. So, we've got the deny status immediatly.I can say - yes, we need as it makes this handling faster
Probably, I didn't get your point. Could you clarify, please.
For now I am wondering if going on checking if we get
NoRuleFound
is correct way. It seems immediate return makes sense only for denying statutes likeAccessDenied
andQuotaLimitReached
EACL-compatible tables go over rules in order, so we may skip applying ALLOW rule (because of the missing attributes) and hit DENY, even though it should be ALLOW, had we have all attributes.
Returning DENY faster is certainly beneficial in some cases. However, if we know that the data is incomplete and we will check it again, does it worth the effort?
The proper solution would probably include lazy attribute fetching and sth like NotEnoughData error from the
IsAllowed
method.We had an agreement that any
DENY
has the highest priority over anyALLOW
-ing rule from the point of access policy engine. It's okay if the data is incomplete because it'll be declined byDENY
anyway afterwards. That's not the same way like withEACL
tablesOkay, @fyrchik meant if a chain sets
MatchTypeFirstMatch
flag that means it works like with EACL-table.So, the conclusion is to remove pre-check as it leads to errors only
639c96a4aa
to33857a6372
[#1249] object: Go on handling even if APE returnsto [#1249] object: Remove all APE pre-checks in handlersNoRuleFound
33857a6372
to732851dd8a