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

Initial implementation #2

Merged
fyrchik merged 1 commit from fyrchik/policy-engine:init into master 2023-10-23 10:18:07 +00:00
Conversation 1 Commits 1 Files changed 13 +664
fyrchik commented 2023-10-04 08:33:26 +00:00
Owner
Copy link
No description provided.
fyrchik force-pushed init from f95aac7b36 to 839b694a89 2023-10-17 14:11:25 +00:00 Compare
fyrchik force-pushed init from 839b694a89 to 3970569602 2023-10-17 14:33:29 +00:00 Compare
fyrchik changed title from [#xx] Initial implementation to Initial implementation 2023-10-19 14:51:56 +00:00
fyrchik requested review from storage-core-committers 2023-10-19 14:52:04 +00:00
fyrchik requested review from storage-core-developers 2023-10-19 14:52:05 +00:00
fyrchik requested review from storage-services-committers 2023-10-19 14:52:05 +00:00
fyrchik requested review from storage-services-developers 2023-10-19 14:52:05 +00:00
dkirillov approved these changes 2023-10-20 06:26:37 +00:00
dkirillov reviewed 2023-10-20 08:03:19 +00:00
chain.go Outdated
@ -0,0 +151,4 @@
return NoRuleFound, false
}
for i := range r.Action {
if globMatch(r.Action[i], req.Operation()) {
dkirillov commented 2023-10-20 08:03:19 +00:00
Member
Copy link

Should the args be in reverse order?
if globMatch(req.Operation(), r.Action[i]) {

Should the args be in reverse order? `if globMatch(req.Operation(), r.Action[i]) {`
fyrchik commented 2023-10-20 10:17:55 +00:00
Author
Owner
Copy link

Fixed

Fixed
dkirillov commented 2023-10-20 10:45:12 +00:00
Member
Copy link

Did you push the fix into init branch in the main repo

Line 154 in c1ac4ad
if globMatch(req.Operation(), r.Action[i]) {

and not to the branch from which this PR is opened https://git.frostfs.info/fyrchik/policy-engine/src/commit/3970569602d100fc47b9b0e51f55586820652f8b/chain.go#L154 ?

Did you push the fix into `init` branch in the main repo https://git.frostfs.info/TrueCloudLab/policy-engine/src/commit/c1ac4ad957262ac4da2dc3bbfcb3b31028d71128/chain.go#L154 and not to the branch from which this PR is opened https://git.frostfs.info/fyrchik/policy-engine/src/commit/3970569602d100fc47b9b0e51f55586820652f8b/chain.go#L154 ?
fyrchik commented 2023-10-20 11:04:19 +00:00
Author
Owner
Copy link

Yes, probably, fixed

Yes, probably, fixed
dkirillov marked this conversation as resolved
dstepanov-yadro reviewed 2023-10-20 10:29:22 +00:00
chain.go Outdated
@ -0,0 +20,4 @@
func init() {
// FIXME #1 (@fyrchik): Introduce more optimal serialization format.
gob.Register(Chain{})
dstepanov-yadro commented 2023-10-20 10:19:13 +00:00
Member
Copy link

As far as i remember gob requires to register every type, but Chain contains interfaces. Is simple json not good enough?

As far as i remember `gob` requires to register every type, but Chain contains interfaces. Is simple json not good enough?
fyrchik commented 2023-10-20 11:04:47 +00:00
Author
Owner
Copy link

true, replaced

true, replaced
dstepanov-yadro marked this conversation as resolved
chain.go Outdated
@ -0,0 +110,4 @@
CondArnNotLike ConditionType = "ArnNotLike"
)
func (c *Condition) Match(obj Request) bool {
dstepanov-yadro commented 2023-10-20 10:25:55 +00:00
Member
Copy link

obj Request -> req Request

obj Request -> req Request
fyrchik commented 2023-10-20 11:35:35 +00:00
Author
Owner
Copy link

fixed

fixed
dstepanov-yadro marked this conversation as resolved
chain.go Outdated
@ -0,0 +118,4 @@
case ObjectRequest:
val = obj.Property(c.Key)
default:
return false
dstepanov-yadro commented 2023-10-20 10:23:04 +00:00
Member
Copy link

Explain please why unknown Object leads to false, but unknown Op leads to panic?

Explain please why unknown Object leads to false, but unknown Op leads to panic?
fyrchik commented 2023-10-20 11:36:28 +00:00
Author
Owner
Copy link

Made panic for both. Panic is for the development stage, to avoid long debugging sessions after silent misbehaviour.

Made panic for both. Panic is for the development stage, to avoid long debugging sessions after silent misbehaviour.
dstepanov-yadro marked this conversation as resolved
dstepanov-yadro reviewed 2023-10-20 10:32:48 +00:00
resource.go Outdated
@ -0,0 +3,4 @@
// Request represents generic named resource (bucket, container etc.).
// Name is resource depenent but should be globally unique for any given
// type of resource.
type Request interface {
dstepanov-yadro commented 2023-10-20 10:32:48 +00:00
Member
Copy link

I think interface can be replaced with struct, because Request looks like data-holder, not behavior.
But it is up to you.

I think interface can be replaced with struct, because Request looks like data-holder, not behavior. But it is up to you.
fyrchik commented 2023-10-20 11:38:32 +00:00
Author
Owner
Copy link

It is an interfarce because of 2 reasons:

  1. Multiple implementations are possible (S3 and gRPC protocol).
  2. We do not want to depend on frostfs-api here (object) and we do not want to copy all objects attributes just to create this struct.

Maybe I don't understand, what is the exact struct you have in mind?

It is an interfarce because of 2 reasons: 1. Multiple implementations are possible (S3 and gRPC protocol). 2. We do not want to depend on frostfs-api here (object) and we do not want to copy all objects attributes just to create this struct. Maybe I don't understand, what is the exact struct you have in mind?
dstepanov-yadro commented 2023-10-20 12:35:40 +00:00
Member
Copy link

Something like that:

type Request struct {
	Operation string
	Properties map[string]string
	Resource Resource
}

Multiple implementations are possible (S3 and gRPC protocol).

I belive that both of s3-gw and node will go to the same implementation of this interface.

we do not want to copy all objects attributes

It is not required to copy everything

Something like that: ``` type Request struct { Operation string Properties map[string]string Resource Resource } ``` > Multiple implementations are possible (S3 and gRPC protocol). I belive that both of s3-gw and node will go to the same implementation of this interface. > we do not want to copy all objects attributes It is not required to copy everything
fyrchik commented 2023-10-20 12:45:28 +00:00
Author
Owner
Copy link

I wanted to avoid allocating Properties map as much as possible. Let's see the prototypes and figure out whether is is better.

I wanted to avoid allocating `Properties` map as much as possible. Let's see the prototypes and figure out whether is is better.
fyrchik commented 2023-10-20 12:45:40 +00:00
Author
Owner
Copy link

This is not a final implementation.

This is not a final implementation.
dstepanov-yadro marked this conversation as resolved
dstepanov-yadro reviewed 2023-10-20 10:49:23 +00:00
chain.go Outdated
@ -0,0 +176,4 @@
func (r *Rule) matchAll(obj Request) (status Status, matched bool) {
for i := range r.Condition {
if !r.Condition[i].Match(obj) {
return NoRuleFound, false
dstepanov-yadro commented 2023-10-20 10:49:23 +00:00
Member
Copy link

NoRuleFound -> AccessDenied:

NoRuleFound -> AccessDenied:
fyrchik commented 2023-10-20 11:39:30 +00:00
Author
Owner
Copy link

There are multiple rules in the chain, AccessDenied implies one of the conditions has matched.

There are multiple rules in the chain, `AccessDenied` implies one of the conditions has matched.
dstepanov-yadro commented 2023-10-20 12:36:11 +00:00
Member
Copy link

Ah, ok

Ah, ok
dstepanov-yadro marked this conversation as resolved
dstepanov-yadro reviewed 2023-10-20 10:50:43 +00:00
chain.go Outdated
@ -0,0 +171,4 @@
return r.Status, true
}
}
return NoRuleFound, false
dstepanov-yadro commented 2023-10-20 10:50:43 +00:00
Member
Copy link

NoRuleFound -> AccessDenied

NoRuleFound -> AccessDenied
fyrchik commented 2023-10-20 11:40:16 +00:00
Author
Owner
Copy link

Agree here, I have postponed having default DENY values for future, to make the result more explicit.

Agree here, I have postponed having default DENY values for future, to make the result more explicit.
dstepanov-yadro marked this conversation as resolved
fyrchik force-pushed init from 3970569602 to c1ac4ad957 2023-10-20 11:04:18 +00:00 Compare
fyrchik force-pushed init from c1ac4ad957 to 9fe3e55d6f 2023-10-20 11:40:47 +00:00 Compare
dkirillov approved these changes 2023-10-20 13:19:20 +00:00
dstepanov-yadro approved these changes 2023-10-23 06:50:32 +00:00
dkirillov commented 2023-10-23 07:37:32 +00:00
Member
Copy link

It seems we need fix link to the PR in commit message

It seems we need fix link to the PR in commit message
👍 1
fyrchik force-pushed init from 9fe3e55d6f to 5ebb2e694c 2023-10-23 07:45:28 +00:00 Compare
fyrchik merged commit 5ebb2e694c into master 2023-10-23 10:18:07 +00:00
fyrchik referenced this pull request from a commit 2023-10-23 10:18:09 +00:00
[#2] Initial implementation
fyrchik deleted branch init 2023-10-23 10:18:14 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
dkirillov
dstepanov-yadro
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
4 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#2
No description provided.
Delete branch "fyrchik/policy-engine:init"

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?