TrueCloudLab
/
policy-engine
generated from TrueCloudLab/basic
20
0
Fork 9
Code Issues 2 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

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
aarifullin commented 2023-12-01 13:11:35 +00:00
Collaborator
  • 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
Collaborator

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
Poster
Collaborator

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
Collaborator

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
Poster
Collaborator

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
Collaborator

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
Poster
Collaborator

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
Collaborator

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
Poster
Collaborator

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
Collaborator

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 Label
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 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
There is no content yet.
Delete Branch "aarifullin/policy-engine:feature/revise_local_override_storage_iface"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?