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
Member
  • 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
@ -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) {
Member

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` ?
Author
Member

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
@ -53,3 +53,3 @@
}
rc := s.nameToResourceChains[name]
rc[resource] = append(rc[resource], c)
rc[target] = append(rc[target], c)
Member

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?
Author
Member

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
@ -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 {
Member

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.
Author
Member

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
@ -34,2 +54,3 @@
ruleFound = true
return
} else if ruleFound {
break
Member

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

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

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

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.
No description provided.