TrueCloudLab/policy-engine
20
0
Fork 11
generated from TrueCloudLab/basic
Code Issues 5 Pull requests 1 Projects Releases Packages Wiki Activity Actions

engine: Refactor LocalOverrideStorage #25

Merged
fyrchik merged 2 commits from aarifullin/policy-engine:feature/revise_local_override_storage_iface into master 2023-12-05 09:21:00 +00:00
Conversation 0 Commits 2 Files changed 8 +150 -91
aarifullin commented 2023-12-01 13:11:35 +00:00
Member
Copy link
  • Make LocalOverrideStorage methods to receive Target type
    instead resource
  • Refactor unit-tests and dependencies
  • Make default chain router check local overrides not
    only for container but also for namespace
* Make LocalOverrideStorage methods to receive Target type instead resource * Refactor unit-tests and dependencies * Make default chain router check local overrides not only for container but also for namespace
aarifullin added the
refactoring
 label 2023-12-01 13:11:35 +00:00
aarifullin force-pushed feature/revise_local_override_storage_iface from 0257f20056 to ea4d41a973 2023-12-01 13:12:07 +00:00 Compare
aarifullin changed title from [#XX] engine: Refactor LocalOverrideStorage to engine: Refactor LocalOverrideStorage 2023-12-01 13:12:18 +00:00
aarifullin requested review from storage-core-committers 2023-12-01 13:12:35 +00:00
aarifullin requested review from storage-core-developers 2023-12-01 13:12:52 +00:00
aarifullin requested review from storage-services-committers 2023-12-01 13:12:56 +00:00
aarifullin requested review from storage-services-developers 2023-12-01 13:13:07 +00:00
dstepanov-yadro approved these changes 2023-12-01 13:39:05 +00:00
dkirillov reviewed 2023-12-01 14:02:27 +00:00
pkg/engine/chain_router.go Outdated
@ -25,19 +25,36 @@ func NewDefaultChainRouterWithLocalOverrides(morph MorphRuleChainStorage, local
}
func (dr *defaultChainRouter) IsAllowed(name chain.Name, namespace string, r resource.Request) (status chain.Status, ruleFound bool, err error) {
dkirillov commented 2023-12-01 14:02:05 +00:00
Member
Copy link

By the way, why don't we use target Target instead namespace string ?

By the way, why don't we use `target Target` instead `namespace string` ?
aarifullin commented 2023-12-01 15:18:57 +00:00
Author
Member
Copy link

I think I've got better idea: pass targets ...Target instead the only namespace

I found the router works correctly for PutObject cases because we can retrieve ContainerTarget from resource (native:object//LxGyWyL/*) that allows to succesfully retrieve rule chains from local/morph storage, but it will stop working for other cases.

We should check if the request is allowed against passed targets like:

  • namespace
  • namespace and container
  • namespace1, namespace2, container

other combinations

That works

WDYT?

cc @fyrchik @alexvanin @dstepanov-yadro

I think I've got better idea: pass `targets ...Target` instead the only `namespace` I found the router works correctly for `PutObject` cases because we can retrieve `ContainerTarget` from resource (`native:object//LxGyWyL/*`) that allows to succesfully retrieve rule chains from local/morph storage, but it will stop working for other cases. We should check if the request is allowed **against** passed `targets` like: - namespace - namespace and container - namespace1, namespace2, container other combinations That works WDYT? cc @fyrchik @alexvanin @dstepanov-yadro
👍 1
pkg/engine/inmemory/local_storage.go Outdated
@ -53,3 +53,3 @@
}
rc := s.nameToResourceChains[name]
rc[resource] = append(rc[resource], c)
rc[target] = append(rc[target], c)
dkirillov commented 2023-12-01 13:59:26 +00:00
Member
Copy link

Can we check that current list doesn't contain chain with the same ID?

Can we check that current list doesn't contain chain with the same ID?
aarifullin commented 2023-12-04 08:22:59 +00:00
Author
Member
Copy link

Since it is replaced if IDs are equal

Since it is replaced if IDs are equal
dkirillov marked this conversation as resolved
acid-ant approved these changes 2023-12-04 07:33:55 +00:00
dkirillov reviewed 2023-12-04 08:15:17 +00:00
pkg/engine/chain_router.go Outdated
@ -30,1 +29,3 @@
status, localRuleFound, err = dr.checkLocalOverrides(name, r)
func (dr *defaultChainRouter) IsAllowed(name chain.Name, r resource.Request, targets ...Target) (status chain.Status, ruleFound bool, err error) {
// Container rule chains have higher priority.
sort.Slice(targets, func(i, j int) bool {
dkirillov commented 2023-12-04 08:15:17 +00:00
Member
Copy link

Shouldn't we use sort.SliceStable or considering names in sort in addition? Otherwise it seems we can get different result when providing two container targets.

Shouldn't we use `sort.SliceStable` or considering names in sort in addition? Otherwise it seems we can get different result when providing two container targets.
aarifullin commented 2023-12-04 08:58:11 +00:00
Author
Member
Copy link

You're correct about using sort.SliceStable - it is exactly what I meant here

considering names in sort in addition

I suppose we do not need so keen about this - we just need to consider containers first, then - namespaces

You're correct about using `sort.SliceStable` - it is exactly what I meant here > considering names in sort in addition I suppose we do not need so keen about this - we just need to consider containers first, then - namespaces
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-12-04 08:17:35 +00:00
pkg/engine/chain_router.go Outdated
@ -34,2 +54,3 @@
ruleFound = true
return
} else if ruleFound {
break
dkirillov commented 2023-12-04 08:17:35 +00:00
Member
Copy link

Why logic for handling local targets differ from handling morph targets?

Why logic for handling local targets differ from handling morph targets?
aarifullin commented 2023-12-04 08:44:49 +00:00
Author
Member
Copy link

This is a good point. In the first implementation local overrides were target-less and returned the result immediatly.
For now local overrides operate with several target types and I think you are right. The logic here (not above) should be the same with morph rules when checkLocal iterates all target rules

This is a good point. In the first implementation local overrides were target-**less** and returned the result immediatly. For now local overrides operate with several target types and I think you are right. The logic here (not above) should be the same with morph rules when `checkLocal` iterates all target rules
dkirillov marked this conversation as resolved
aarifullin force-pushed feature/revise_local_override_storage_iface from c8c0b0d95f to 67626c0019 2023-12-04 08:22:05 +00:00 Compare
aarifullin force-pushed feature/revise_local_override_storage_iface from 67626c0019 to 17bcde6b24 2023-12-04 08:57:54 +00:00 Compare
aarifullin force-pushed feature/revise_local_override_storage_iface from 17bcde6b24 to ddd305e867 2023-12-04 09:01:16 +00:00 Compare
dkirillov approved these changes 2023-12-04 09:48:24 +00:00
dkirillov left a comment
Member
Copy link

LGTM

LGTM
aarifullin force-pushed feature/revise_local_override_storage_iface from ddd305e867 to e78ae34bbd 2023-12-04 11:15:29 +00:00 Compare
dkirillov approved these changes 2023-12-04 11:31:20 +00:00
aarifullin requested review from acid-ant 2023-12-04 11:32:41 +00:00
aarifullin requested review from dstepanov-yadro 2023-12-04 11:32:42 +00:00
acid-ant approved these changes 2023-12-04 11:40:13 +00:00
dstepanov-yadro approved these changes 2023-12-04 15:35:57 +00:00
fyrchik approved these changes 2023-12-05 09:20:00 +00:00
fyrchik merged commit 2d4a9fc6dc into master 2023-12-05 09:21:00 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
dkirillov
acid-ant
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#25
No description provided.
Delete branch "aarifullin/policy-engine:feature/revise_local_override_storage_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?