[#872] object: Use APE instead EACL for object service handlers #888
No reviewers
TrueCloudLab/storage-core-committers
TrueCloudLab/storage-core-developers
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#888
Loading…
Reference in a new issue
No description provided.
Delete branch "feature/872-object_svc_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?
from object SDK
APE instead EACL
Close #872
Close #844
b275f23369
to41a3fd0fce
@ -28,0 +75,4 @@
errMissingOID = fmt.Errorf("object ID is not set")
)
func (c *apeCheckerImpl) objectSDKFromRequest(ctx context.Context, request any, reqInfo v2.RequestInfo) (*objectSDK.Object, error) {
See readObjectHeadersFromRequestXHeaderSource
@ -28,0 +111,4 @@
}
}
func (c *apeCheckerImpl) objectSDKFromResponse(ctx context.Context, response any, reqInfo v2.RequestInfo) (*objectSDK.Object, error) {
See readObjectHeadersResponseXHeaderSource
@ -53,0 +90,4 @@
return
}
func nativeSchemaRole(reqInfo v2.RequestInfo) string {
I intended to re-use
RequestInfo
structure. Its definition isThis is quite usable for this purpose. However, these SDK-fields have been remained:
acl.Role
,acl.Op
.How do you think should we to refactor these fields to
string
-s to make them set as APE-constants?Each op is defined in a native schema, so old
acl.Op
for object service can be translated.The role is less useful now, because request initiator for
ingress
chain is by definition not an innerring or storage node.Not actual
@ -28,0 +131,4 @@
hdr.SetCreationEpoch(headerPart.GetCreationEpoch())
hdr.SetOwnerID(headerPart.GetOwnerID())
hdr.SetObjectType(headerPart.GetObjectType())
hdr.SetPayloadLength(headerPart.GetPayloadLength())
Why
SetPayloadHash
andSetHomomorphicHash
skipped?Also, don't we have any helper in SDK/api?
I've added these setters.
I didn't find anything. But I can move this to some util
@ -53,0 +87,4 @@
res[nativeschema.PropertyKeyObjectHomomorphicHash] = hcs.String()
}
return
Object can have other user-defined attributes, i think they should be checked too.
Added
@ -155,3 +153,2 @@
return basicACLErr(reqInfo)
} else if err := b.checker.CheckEACL(request, reqInfo); err != nil {
return eACLErr(reqInfo, err)
} else if err := b.apeChecker.CheckIfAPEPermitsRequest(context.TODO(), request, reqInfo); err != nil {
stream
has context.@ -41,2 +181,4 @@
}
func (c *apeCheckerImpl) CheckIfAPEPermitsRequest(ctx context.Context, request any, reqInfo v2.RequestInfo) error {
if !util.IsObjectServiceRequest(request) {
Please, take a look at #881/
Can we do it in a similar way? In particular, this check seems superfluous.
Also, what about forming headers inside each service? So instead of
objectSDKFromRequest
with a switch we have a function inside each ofobject/...
, possibly with some reusable helpers here.I've made
apeSvc
. Please, check it out@ -0,0 +4,4 @@
objectV2 "git.frostfs.info/TrueCloudLab/frostfs-api-go/v2/object"
)
func IsObjectServiceRequest(msg any) bool {
IMO we are doing sth wrong if we need this: APE application doesn't seem do depend on particular structs in any way, so why not provide exactly the needed interface?
It is true. And the access policy engine does not depend on these request/response structs itself. But APE-checker does, because it some kind of adapter for object service to perform checks in the engine.
My point is that this is correct because
APEChainChecker
is part of object service and not standalone checker.Another option is to make the interface like that:
Not actual
41a3fd0fce
to8019b7cee4
8019b7cee4
to4762d81c14
4762d81c14
tof5b860d722
@ -0,0 +357,4 @@
return nil, err
}
// nothing to check with ape
Don't understand: line 356 is APE check.
Sorry, that was left after previous workaround :). It'll be removed!
I think some unit-tests are required.
f5b860d722
to5a75f6dd52
@ -0,0 +2,4 @@
import "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/acl"
type RequestContextKeyT string
It is usually better to use
RequestContextKey struct{}
and empty value as a key: this is the approach used in golangnet
stdlib.Thank you, fixed
5a75f6dd52
to29debad95f
29debad95f
to4c81991888
4c81991888
toe528c9f966
e528c9f966
tod7e7610756
I have implemented unit-tests and tried to cover cases as more as possible
@ -0,0 +91,4 @@
func requestContext(ctx context.Context) (*objectSvc.RequestContext, error) {
untyped := ctx.Value(objectSvc.RequestContextKey)
if untyped == nil {
return nil, fmt.Errorf("context does not contain this key %s", objectSvc.RequestContextKey)
Maybe just
no key %s in context
?Fixed
@ -0,0 +289,4 @@
return nil, err
}
err = c.apeChecker.CheckAPE(ctx, Prm{
Looks like redundant check.
I think you are right. I will remove this check
Nah, let's keep it like that. Please, check this comment out
I agree for
get
like requests, now it is clear, but for delete it looks strange. Even if we reject the wholedelete
request, we perform physical deletion, isn't it?This sounds really reasonable, okay. I'll remove it
Fixed
Just for my information, in this case when we will get attributes of the object to do some checks via
ape
?#888/files
objectProperties
method@ -0,0 +355,4 @@
return nil, err
}
if err = c.apeChecker.CheckAPE(ctx, prm); err != nil {
Why we need to perform this check twice?
This idea is not mine, it is already used in object service.
That doesn't look obvious but first check
c.apeChecker.CheckAPE
is kind of trial invocation: if an object is placed locally, then an object's header can be recieved immediately and thus we are able to collect all object properties like system and user attributes . If it cannot be found locally, then only CID, OID attributes passed to APE: at least this can be used for checks like "does APE allow to put more object to the container?" - we can reduce response time because we don't need to get entire object to check if the request is denined by <CID, OID> in APE.If it is passed after first check, we get the requested object, we get it with filled header (-> properties) and we need to perform the check second time to these cases when APE check can be performed only on object attributes like
payloadHash < 1000?
orobjectType != regular
. That is fair for all get-like methods but you was rightdelete
barely needs this second check (however, we still want to check if an object with some attributes is allowed to get deleted)Now it is pretty much clear, thanks.
d7e7610756
toe19109a3bb
@ -0,0 +134,4 @@
header = prm.Header
} else if prm.Object != nil {
headerObjSDK, err := c.headerProvider.GetHeader(ctx, prm.Container, *prm.Object)
if err == nil {
Is it ok to skip getHeader error?
Yes. If
err != nil
, then it means the object is not found locally (check this out).Do you suggest to check it for
NotFoundError
?e19109a3bb
to3d62af38e9
3d62af38e9
toaccad44c40
accad44c40
tod2b9a262d1
Sorry, the source branch has been accidentally created in origin stream. The PR is moved to here
Pull request closed