TrueCloudLab/policy-engine
21
0
Fork 12
generated from TrueCloudLab/basic
Code Issues 5 Pull requests 2 Projects Releases Packages Wiki Activity Actions

[#7] engine: Revise storage interface #15

Merged
fyrchik merged 3 commits from aarifullin/policy-engine:feature/7-revise_iface into master 2023-11-15 09:22:43 +00:00
Conversation 1 Commits 3 Files changed 21 +1002 -498
aarifullin commented 2023-11-07 11:04:19 +00:00
Member
Copy link
  • Nuke out CachedChainStorage interface
  • Introduce LocalOverrideStorage interface to manage
    local overrides
  • Introduce MorphRuleChainStorage interface to manage
    chains in the policy contract
  • Extend Engine interface

Signed-off-by: aarifullin aarifullin@yadro.com

* Nuke out CachedChainStorage interface * Introduce LocalOverrideStorage interface to manage local overrides * Introduce MorphRuleChainStorage interface to manage chains in the policy contract * Extend Engine interface Signed-off-by: aarifullin <aarifullin@yadro.com>
aarifullin added the
refactoring
 label 2023-11-07 11:04:19 +00:00
aarifullin requested review from storage-core-committers 2023-11-07 11:04:30 +00:00
aarifullin requested review from storage-core-developers 2023-11-07 11:04:33 +00:00
aarifullin requested review from storage-services-committers 2023-11-07 11:04:33 +00:00
aarifullin requested review from storage-services-developers 2023-11-07 11:05:28 +00:00
aarifullin force-pushed feature/7-revise_iface from aa76ef86a6 to d3e9d0a52d 2023-11-07 11:06:09 +00:00 Compare
aarifullin force-pushed feature/7-revise_iface from d3e9d0a52d to 5f103a7a5f 2023-11-07 11:53:13 +00:00 Compare
fyrchik commented 2023-11-07 12:14:07 +00:00
Owner
Copy link

There conflicts and linters fail, please, take a look.

There conflicts and linters fail, please, take a look.
dstepanov-yadro reviewed 2023-11-07 13:15:20 +00:00
chain.go Outdated
@ -11,3 +10,3 @@
// IsAllowed returns status for the operation after all checks.
// The second return value signifies whether a matching rule was found.
IsAllowed(name Name, namespace string, r Request) (Status, bool)
IsAllowed(name Name, target string, r Request) (status Status, found bool, err error)
dstepanov-yadro commented 2023-11-07 13:03:26 +00:00
Member
Copy link

At first glance, the question arises: why is found not the Status field?

At first glance, the question arises: why is `found` not the `Status` field?
aarifullin commented 2023-11-07 18:38:08 +00:00
Author
Member
Copy link

... why is found not the Status field?

Probably you are right. I suppose that's was first implementation's flavour and we can use just NoRuleFound without flag but since we need the error because we will list rules either from a local db or from the contract

> ... why is `found` not the `Status` field? Probably you are right. I suppose that's was first implementation's flavour and we can use just `NoRuleFound` without flag but since we need the error because we will list rules either from a local db or from the contract
dstepanov-yadro marked this conversation as resolved
inmemory/errors.go Outdated
@ -0,0 +3,4 @@
import "errors"
var (
errUnknownTarget = errors.New("unknown target type")
dstepanov-yadro commented 2023-11-07 13:14:25 +00:00
Member
Copy link

The same as in inmemory.go. Maybe unify?

The same as in `inmemory.go`. Maybe unify?
aarifullin commented 2023-11-07 18:32:06 +00:00
Author
Member
Copy link

Sorry. That's was after incorrect rebase. This problem is not actual

Sorry. That's was after incorrect rebase. This problem is not actual
dstepanov-yadro marked this conversation as resolved
inmemory/inmemory.go Outdated
@ -0,0 +12,4 @@
morph ape.MorphRuleChainStorage
}
func NewInMemory() ape.Engine {
dstepanov-yadro commented 2023-11-07 13:15:12 +00:00
Member
Copy link

Looks like inmemory.go duplicate

Looks like `inmemory.go` duplicate
aarifullin commented 2023-11-07 18:32:12 +00:00
Author
Member
Copy link

Sorry. That's was after incorrect rebase. This problem is not actual

Sorry. That's was after incorrect rebase. This problem is not actual
dstepanov-yadro marked this conversation as resolved
aarifullin force-pushed feature/7-revise_iface from 5f103a7a5f to c2dc96931a 2023-11-07 18:30:53 +00:00 Compare
aarifullin force-pushed feature/7-revise_iface from c2dc96931a to 48bb4a110c 2023-11-07 18:44:32 +00:00 Compare
aarifullin reviewed 2023-11-07 18:49:46 +00:00
pkg/engine/inmemory/inmemory_test.go Outdated
@ -0,0 +9,4 @@
"github.com/stretchr/testify/require"
)
func TestInmemory(t *testing.T) {
aarifullin commented 2023-11-07 18:49:45 +00:00
Author
Member
Copy link

@TrueCloudLab/storage-core-developers

Even I tried to move to this file with git mv it's showed as the new created file because I changed the file in next commits. But I want to emphasize I have not changed the logical structure of the unittest and changed only signatures

@TrueCloudLab/storage-core-developers Even I tried to move to this file with `git mv` it's showed as the new created file because I changed the file in next commits. But I want to **emphasize** I have **not** changed the logical structure of the unittest and changed only signatures
aarifullin marked this conversation as resolved
aarifullin reviewed 2023-11-07 18:49:47 +00:00
aarifullin force-pushed feature/7-revise_iface from 48bb4a110c to 0f427b496f 2023-11-07 18:55:38 +00:00 Compare
dstepanov-yadro approved these changes 2023-11-08 06:41:57 +00:00
acid-ant approved these changes 2023-11-08 06:50:22 +00:00
aarifullin force-pushed feature/7-revise_iface from 0f427b496f to 63aa1e4708 2023-11-08 09:04:59 +00:00 Compare
aarifullin force-pushed feature/7-revise_iface from 63aa1e4708 to 45b524d8db 2023-11-08 09:22:41 +00:00 Compare
aarifullin force-pushed feature/7-revise_iface from 45b524d8db to 97fe723559 2023-11-08 13:44:48 +00:00 Compare
aarifullin force-pushed feature/7-revise_iface from 97fe723559 to 99994dcc4c 2023-11-08 14:28:32 +00:00 Compare
aarifullin requested review from dstepanov-yadro 2023-11-09 11:08:08 +00:00
aarifullin requested review from acid-ant 2023-11-09 11:08:09 +00:00
dkirillov approved these changes 2023-11-09 12:14:29 +00:00
fyrchik reviewed 2023-11-10 05:59:35 +00:00
iam/converter_test.go Outdated
@ -4,3 +4,3 @@
"testing"
policyengine "git.frostfs.info/TrueCloudLab/policy-engine"
chainpkg "git.frostfs.info/TrueCloudLab/policy-engine/pkg/chain"
fyrchik commented 2023-11-09 13:00:30 +00:00
Owner
Copy link

Why do we use alias here? In the previous version it was because of the hyphen.

Why do we use alias here? In the previous version it was because of the hyphen.
aarifullin commented 2023-11-10 07:50:20 +00:00
Author
Member
Copy link

Because tests use

var chain chain.Chain

So, either we need to make either an alias for the package or rename chain. Would we prefer the second option?

Because tests use ```golang var chain chain.Chain ``` So, either we need to make either an alias for the package or rename `chain`. Would we prefer the second option?
aarifullin commented 2023-11-10 08:02:39 +00:00
Author
Member
Copy link

I think the second option is better. I will fix

I think the second option is better. I will fix
👍 1
fyrchik marked this conversation as resolved
pkg/chain/chain.go Outdated
@ -0,0 +13,4 @@
type ID string
type Chain struct {
ID *ID
fyrchik commented 2023-11-09 13:00:54 +00:00
Owner
Copy link

Why is it a pointer? Empty value can equal to missing ID is is not a valid name.

Why is it a pointer? Empty value can equal to missing ID is is not a valid name.
aarifullin commented 2023-11-10 09:28:26 +00:00
Author
Member
Copy link

Fixed

Fixed
fyrchik marked this conversation as resolved
pkg/engine/inmemory/local_storage.go Outdated
@ -0,0 +24,4 @@
}
}
func (s *inmemoryLocalStorage) generateChainID(name chain.Name, resource string) *chain.ID {
fyrchik commented 2023-11-10 05:59:28 +00:00
Owner
Copy link

Does chain id have a specific structure?

Does chain id have a specific structure?
aarifullin commented 2023-11-10 07:58:11 +00:00
Author
Member
Copy link

No, it does not.

I glimpsed at the chain names in policy contract unit-tests and found the idea to name a chain like ingress:resource:name more useful than a numeric ID. Anyway, inmemory implementation will only be used for a while and will be kept only for unit-tests. I think such names are fine and readable

No, it does not. I glimpsed at the chain names in policy contract unit-tests and found the idea to name a chain like `ingress:resource:name` more useful than a numeric ID. Anyway, `inmemory` implementation will only be used for a while and will be kept only for unit-tests. I think such names are fine and readable
dkirillov commented 2023-11-14 08:09:54 +00:00
Member
Copy link

Anyway, inmemory implementation will only be used for a while and will be kept only for unit-tests.

Will we have some in-memory implementation for LocalOverrideStorage that can be used in s3-gw/node? Or it's expected that s3-gw and node must implement their own LocalOverrideStorage ?

> Anyway, inmemory implementation will only be used for a while and will be kept only for unit-tests. Will we have some in-memory implementation for `LocalOverrideStorage` that can be used in s3-gw/node? Or it's expected that s3-gw and node must implement their own `LocalOverrideStorage` ?
aarifullin commented 2023-11-14 08:51:41 +00:00
Author
Member
Copy link

Good question.
It assumed that LocalOverrideStorage should be implemented by each service separately. For example, frostfs-node will use DB to keep and retrieve local overrides that doesn't seem applicable for s3-gw.

But I am wondering was it implied to define local overrides for s3-gw? If it was not, then probably it makes sense to separate the interface:

type Engine interface {
   ChainRouter

   MorphRuleChainStorage() MorphRuleChainStorage
}

type EngineWithLocalOverrides interface {
    Engine
    
    LocalStorage() LocalOverrideStorage
}
Good question. It assumed that `LocalOverrideStorage` should be implemented by each service separately. For example, frostfs-node will use DB to keep and retrieve local overrides that doesn't seem applicable for s3-gw. But I am wondering was it implied to define local overrides for `s3-gw`? If it was not, then probably it makes sense to separate the interface: ```golang type Engine interface { ChainRouter MorphRuleChainStorage() MorphRuleChainStorage } type EngineWithLocalOverrides interface { Engine LocalStorage() LocalOverrideStorage } ```
dkirillov commented 2023-11-14 09:11:40 +00:00
Member
Copy link

But I am wondering was it implied to define local overrides for s3-gw?

I'm not sure. It seems we will have only some cache layer, but it be a part of MorphRuleChainStorage implementation I think

@alexvanin

> But I am wondering was it implied to define local overrides for s3-gw? I'm not sure. It seems we will have only some cache layer, but it be a part of `MorphRuleChainStorage` implementation I think @alexvanin
aarifullin commented 2023-11-14 10:40:39 +00:00
Author
Member
Copy link

I have splitted the interface into two: LocalOverrideEngine and Engine (the first one contains the second). Please, check

I have splitted the interface into two: `LocalOverrideEngine` and `Engine` (the first one contains the second). Please, check
pkg/resource/testutil/resource.go Outdated
@ -0,0 +17,4 @@
return r.properties[name]
}
func NewResource(name string, properties map[string]string) *Resource {
fyrchik commented 2023-11-10 05:58:18 +00:00
Owner
Copy link

Why do both request and resource accept a map now?

Why do both request and resource accept a map now?
aarifullin commented 2023-11-10 09:36:41 +00:00
Author
Member
Copy link

I don't know. I just moved the file and made constructors importable :) (new.. -> New...). What is your concern about these maps?

Also, this structs are usable only for unit-tests witihn policy-engine

I don't know. I just moved the file and made constructors importable :) (`new..` -> `New...`). What is your concern about these maps? Also, this structs are usable only for unit-tests witihn policy-engine
fyrchik commented 2023-11-13 07:10:59 +00:00
Owner
Copy link

Oh, I see, missed testutil package

Oh, I see, missed testutil package
fyrchik marked this conversation as resolved
aarifullin force-pushed feature/7-revise_iface from 99994dcc4c to 89a1896cb1 2023-11-10 08:32:37 +00:00 Compare
aarifullin force-pushed feature/7-revise_iface from 89a1896cb1 to 0b6526fad0 2023-11-10 09:20:16 +00:00 Compare
aarifullin force-pushed feature/7-revise_iface from 0b6526fad0 to 342551d2c0 2023-11-10 09:27:54 +00:00 Compare
aarifullin force-pushed feature/7-revise_iface from 342551d2c0 to 4455219384 2023-11-10 12:04:13 +00:00 Compare
aarifullin requested review from dkirillov 2023-11-10 12:06:25 +00:00
dstepanov-yadro approved these changes 2023-11-13 12:56:13 +00:00
aarifullin force-pushed feature/7-revise_iface from 4455219384 to 73959e4747 2023-11-14 07:56:45 +00:00 Compare
aarifullin requested review from dstepanov-yadro 2023-11-14 08:40:03 +00:00
aarifullin force-pushed feature/7-revise_iface from 73959e4747 to 5a306794c7 2023-11-14 10:34:31 +00:00 Compare
aarifullin force-pushed feature/7-revise_iface from 5a306794c7 to e47af4b111 2023-11-14 10:39:46 +00:00 Compare
dkirillov approved these changes 2023-11-14 11:08:40 +00:00
dstepanov-yadro approved these changes 2023-11-15 08:28:50 +00:00
fyrchik approved these changes 2023-11-15 09:22:21 +00:00
fyrchik merged commit 17453d3cda into master 2023-11-15 09:22:43 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
acid-ant
dkirillov
dstepanov-yadro
fyrchik
TrueCloudLab/storage-core-developers
TrueCloudLab/storage-services-developers
Labels
Clear labels
  
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
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
5 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/policy-engine#15
No description provided.
Delete branch "aarifullin/policy-engine:feature/7-revise_iface"

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?