Revise storage interface for policy engine #7

New Issue

Closed

opened 2023-10-26 14:09:29 +00:00 by fyrchik · 3 comments

fyrchik commented

2023-10-26 14:09:29 +00:00

Owner

There are 2 problems with it:

AddNamespace and AddResource could really combined in a single method (see policy contract for an example).
Allow to remove local override rules.

The following scheme must be easy to implement:

Local overrides via control service (possibly, chain could be identified with a name, so support removal of a single rule)
All other methods will be called with chains from the policy contract.

There are 2 problems with it: 1. `AddNamespace` and `AddResource` could really combined in a single method (see policy contract for an example). 2. Allow to remove local override rules. The following scheme must be easy to implement: 1. Local overrides via control service (possibly, chain could be identified with a name, so support removal of a single rule) 2. All other methods will be called with chains from the policy contract.

fyrchik added the

refactoring

label 2023-10-26 14:13:00 +00:00

aarifullin commented

2023-10-30 10:06:03 +00:00

Collaborator

I think we need to split CachedChainStorage into several interfaces. Each interface would adhere to SRP. That nukes out CachedChainStorage at all:

Engine should be kept standalone
```
 IsAllowed(...)
```

LocalOverrides/LocalRuleChainSource/, LocalRuleChains with methods

type LocalOverrides interface {
    AddChain(name Name) (ChainID, error)
    RemoveChain(name Name, chainID ChainID) error
    GetChain(name Name, chainID ChainID) (*Chain, error)
    ListChains(name Name)  ([]Chain, error)
}

MorphRuleChainSource with methods

type MorphRuleChainSource interface {
   AddChain(name Name, entity string) error
   RemoveChain(name Name, entity string) error
   GetChain(name Name, entity string) (*Chain, error)
   ListChains(name Name)  ([]Chain, error)
}

Also, I would consider to rename Engine to ChainRouter name with method:

type ChainRouter interface {
   CheckPermission(name Name, namespace string, r Request) (status Status, ruleFound bool)
}

WDYT?

I think we need to split [CachedChainStorage](https://git.frostfs.info/TrueCloudLab/policy-engine/src/commit/31a308ea61fb9b478773c988548f352ccf453913/interface.go#L4) into several interfaces. Each interface would adhere to SRP. That nukes out `CachedChainStorage` at all: 1. `Engine` should be kept standalone ```golang IsAllowed(...) ``` 2. `LocalOverrides`/`LocalRuleChainSource`/, `LocalRuleChains` with methods ```golang type LocalOverrides interface { AddChain(name Name) (ChainID, error) RemoveChain(name Name, chainID ChainID) error GetChain(name Name, chainID ChainID) (*Chain, error) ListChains(name Name) ([]Chain, error) } ``` 3. `MorphRuleChainSource` with methods ```golang type MorphRuleChainSource interface { AddChain(name Name, entity string) error RemoveChain(name Name, entity string) error GetChain(name Name, entity string) (*Chain, error) ListChains(name Name) ([]Chain, error) } ``` Also, I would consider to rename `Engine` to `ChainRouter` name with method: ```golang type ChainRouter interface { CheckPermission(name Name, namespace string, r Request) (status Status, ruleFound bool) } ``` WDYT?

fyrchik referenced this issue

2023-10-31 10:24:00 +00:00

Introduce ChainID type and several methods for interface #13

aarifullin referenced this issue

2023-10-31 14:20:47 +00:00

Introduce ChainID type and several methods for interface #13

aarifullin referenced this issue

2023-11-02 10:49:28 +00:00

Add a smart-contract and a neo-go client wrapper #3

aarifullin self-assigned this 2023-11-02 11:28:50 +00:00

aarifullin referenced this issue

2023-11-07 11:04:19 +00:00

[#7] engine: Revise storage interface #15

dkirillov referenced this issue from TrueCloudLab/frostfs-s3-gw

2023-11-07 12:29:25 +00:00

Add basic support of policy engine #257

fyrchik referenced this issue from a commit

2023-11-15 09:22:45 +00:00

[#7] engine: Move globMatch to common util package

fyrchik referenced this issue from a commit

2023-11-15 09:22:45 +00:00

[#7] engine: Set project structure pattern for files

fyrchik referenced this issue from a commit

2023-11-15 09:22:45 +00:00

[#7] engine: Revise CachedChainStorage interface

dkirillov commented

2023-11-29 09:59:15 +00:00

Collaborator

Why does the LocalOverrideStorage require resource instead of namespace unlike MorphRuleChainStorage ?

Probably the namespace be part of resource anyway. But it seems this is right only for storage-node (not for s3).
Furthermore when we want to add local overrides using control api (in s3/node) we have to know resource name from chain rules. But what if there are more than just one resource in chain (do we have to split rules into different chains then)?

Why does the [LocalOverrideStorage](https://git.frostfs.info/TrueCloudLab/policy-engine/src/commit/5fa9d91903bae3d0e29f5078a5d21f7f66de17b5/pkg/engine/interface.go#L17) require `resource` instead of namespace unlike [MorphRuleChainStorage](https://git.frostfs.info/TrueCloudLab/policy-engine/src/commit/5fa9d91903bae3d0e29f5078a5d21f7f66de17b5/pkg/engine/interface.go#L57) ? Probably the namespace be part of resource anyway. But it seems this is right only for storage-node (not for s3). Furthermore when we want to add local overrides using control api (in s3/node) we have to know resource name from chain rules. But what if there are more than just one resource in chain (do we have to split rules into different chains then)?

aarifullin commented

2024-01-11 14:34:04 +00:00

Collaborator

The interface has been revised in PRs mentioned above

aarifullin closed this issue

2024-01-11 14:34:05 +00:00