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
8 changed files with 150 additions and 91 deletions

View file

@ -833,7 +833,7 @@ func TestComplexNativeConditions(t *testing.T) {
} {
t.Run(tc.name, func(t *testing.T) {
req := testutil.NewRequest(tc.action, testutil.NewResource(tc.resource, tc.resourceMap), tc.requestMap)
status, _, err := s.IsAllowed("name", "ns", req)
status, _, err := s.IsAllowed("name", engine.NewRequestTargetWithNamespace("ns"), req)
require.NoError(t, err)
require.Equal(t, tc.status.String(), status.String())
})
@ -1064,7 +1064,7 @@ func TestComplexS3Conditions(t *testing.T) {
} {
t.Run(tc.name, func(t *testing.T) {
req := testutil.NewRequest(tc.action, testutil.NewResource(tc.resource, tc.resourceMap), tc.requestMap)
status, _, err := s.IsAllowed("name", "ns", req)
status, _, err := s.IsAllowed("name", engine.NewRequestTargetWithNamespace("ns"), req)
require.NoError(t, err)
require.Equal(t, tc.status.String(), status.String())
})

View file

@ -24,45 +24,65 @@ func NewDefaultChainRouterWithLocalOverrides(morph MorphRuleChainStorage, local
}
}
func (dr *defaultChainRouter) IsAllowed(name chain.Name, namespace string, r resource.Request) (status chain.Status, ruleFound bool, err error) {
if dr.local != nil {
var localRuleFound bool
status, localRuleFound, err = dr.checkLocalOverrides(name, r)
if err != nil {
return chain.NoRuleFound, false, err
} else if localRuleFound {
ruleFound = true
func (dr *defaultChainRouter) IsAllowed(name chain.Name, rt RequestTarget, r resource.Request) (status chain.Status, ruleFound bool, err error) {

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

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
status, ruleFound, err = dr.checkLocal(name, rt, r)
if err != nil {
return chain.NoRuleFound, false, err
} else if ruleFound {
dkirillov marked this conversation as resolved Outdated

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.

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
// The local overrides have the highest priority and thus
// morph rules are not considered if a local one is found.
return
}
status, ruleFound, err = dr.checkMorph(name, rt, r)
return
}
func (dr *defaultChainRouter) checkLocal(name chain.Name, rt RequestTarget, r resource.Request) (status chain.Status, ruleFound bool, err error) {
if dr.local == nil {
return
}
var ruleFounds []bool
for _, target := range rt.Targets() {
status, ruleFound, err = dr.matchLocalOverrides(name, target, r)
if err != nil || ruleFound && status != chain.Allow {
return
}
}
var namespaceRuleFound bool
status, namespaceRuleFound, err = dr.checkNamespaceChains(name, namespace, r)
if err != nil {
return
} else if namespaceRuleFound && status != chain.Allow {
ruleFound = true
return
}
var cnrRuleFound bool
status, cnrRuleFound, err = dr.checkContainerChains(name, r.Resource().Name(), r)
if err != nil {
return
} else if cnrRuleFound && status != chain.Allow {
ruleFound = true
return
ruleFounds = append(ruleFounds, ruleFound)
}
status = chain.NoRuleFound
if ruleFound = namespaceRuleFound || cnrRuleFound; ruleFound {
status = chain.Allow
for _, ruleFound = range ruleFounds {
if ruleFound {
dkirillov marked this conversation as resolved Outdated

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

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

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
status = chain.Allow
break
}
}
return
}
func (dr *defaultChainRouter) checkLocalOverrides(name chain.Name, r resource.Request) (status chain.Status, ruleFound bool, err error) {
localOverrides, err := dr.local.ListOverrides(name, r.Resource().Name())
func (dr *defaultChainRouter) checkMorph(name chain.Name, rt RequestTarget, r resource.Request) (status chain.Status, ruleFound bool, err error) {
var ruleFounds []bool
for _, target := range rt.Targets() {
status, ruleFound, err = dr.matchMorphRuleChains(name, target, r)
if err != nil || ruleFound && status != chain.Allow {
return
}
ruleFounds = append(ruleFounds, ruleFound)
}
status = chain.NoRuleFound
for _, ruleFound = range ruleFounds {
if ruleFound {
status = chain.Allow
break
}
}
return
}
func (dr *defaultChainRouter) matchLocalOverrides(name chain.Name, target Target, r resource.Request) (status chain.Status, ruleFound bool, err error) {
localOverrides, err := dr.local.ListOverrides(name, target)
if err != nil {
return
}
@ -74,8 +94,8 @@ func (dr *defaultChainRouter) checkLocalOverrides(name chain.Name, r resource.Re
return
}
func (dr *defaultChainRouter) checkNamespaceChains(name chain.Name, namespace string, r resource.Request) (status chain.Status, ruleFound bool, err error) {
namespaceChains, err := dr.morph.ListMorphRuleChains(name, NamespaceTarget(namespace))
func (dr *defaultChainRouter) matchMorphRuleChains(name chain.Name, target Target, r resource.Request) (status chain.Status, ruleFound bool, err error) {
namespaceChains, err := dr.morph.ListMorphRuleChains(name, target)
if err != nil {
return
}
@ -86,16 +106,3 @@ func (dr *defaultChainRouter) checkNamespaceChains(name chain.Name, namespace st
}
return
}
func (dr *defaultChainRouter) checkContainerChains(name chain.Name, container string, r resource.Request) (status chain.Status, ruleFound bool, err error) {
containerChains, err := dr.morph.ListMorphRuleChains(name, ContainerTarget(container))
if err != nil {
return
}
for _, c := range containerChains {
if status, ruleFound = c.Match(r); ruleFound {
return
}
}
return
}

View file

@ -43,6 +43,6 @@ func (im *inmemory) MorphRuleChainStorage() engine.MorphRuleChainStorage {
return im.morph
}
func (im *inmemory) IsAllowed(name chain.Name, namespace string, r resource.Request) (status chain.Status, ruleFound bool, err error) {
return im.router.IsAllowed(name, namespace, r)
func (im *inmemory) IsAllowed(name chain.Name, rt engine.RequestTarget, r resource.Request) (status chain.Status, ruleFound bool, err error) {
return im.router.IsAllowed(name, rt, r)
}

View file

@ -29,7 +29,7 @@ func TestInmemory(t *testing.T) {
"Actor": actor1,
})
status, ok, _ := s.IsAllowed(chain.Ingress, namespace, reqGood)
status, ok, _ := s.IsAllowed(chain.Ingress, engine.NewRequestTargetWithNamespace(namespace), reqGood)
require.Equal(t, chain.NoRuleFound, status)
require.False(t, ok)
@ -103,7 +103,7 @@ func TestInmemory(t *testing.T) {
"SourceIP": "10.122.1.20",
"Actor": actor1,
})
status, ok, _ := s.IsAllowed(chain.Ingress, namespace, reqBadIP)
status, ok, _ := s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqBadIP)
require.Equal(t, chain.AccessDenied, status)
require.True(t, ok)
})
@ -113,7 +113,7 @@ func TestInmemory(t *testing.T) {
"SourceIP": "10.1.1.13",
"Actor": actor2,
})
status, ok, _ := s.IsAllowed(chain.Ingress, namespace, reqBadActor)
status, ok, _ := s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqBadActor)
require.Equal(t, chain.AccessDenied, status)
require.True(t, ok)
})
@ -121,14 +121,14 @@ func TestInmemory(t *testing.T) {
objGood := resourcetest.NewResource("native::object::abc/id1", map[string]string{"Department": "HR"})
objBadAttr := resourcetest.NewResource("native::object::abc/id2", map[string]string{"Department": "Support"})
status, ok, _ := s.IsAllowed(chain.Ingress, namespace, resourcetest.NewRequest("native::object::get", objGood, map[string]string{
status, ok, _ := s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), resourcetest.NewRequest("native::object::get", objGood, map[string]string{
"SourceIP": "10.1.1.14",
"Actor": actor2,
}))
require.Equal(t, chain.Allow, status)
require.True(t, ok)
status, ok, _ = s.IsAllowed(chain.Ingress, namespace, resourcetest.NewRequest("native::object::get", objBadAttr, map[string]string{
status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), resourcetest.NewRequest("native::object::get", objBadAttr, map[string]string{
"SourceIP": "10.1.1.14",
"Actor": actor2,
}))
@ -141,33 +141,33 @@ func TestInmemory(t *testing.T) {
"SourceIP": "10.1.1.12",
"Actor": actor1,
})
status, ok, _ := s.IsAllowed(chain.Ingress, namespace, reqBadOperation)
status, ok, _ := s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqBadOperation)
require.Equal(t, chain.AccessDenied, status)
require.True(t, ok)
})
t.Run("inverted rules", func(t *testing.T) {
req := resourcetest.NewRequest("native::object::put", resourcetest.NewResource(object, nil), nil)
status, ok, _ = s.IsAllowed(chain.Ingress, namespace2, req)
status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace2, container), req)
require.Equal(t, chain.NoRuleFound, status)
require.False(t, ok)
req = resourcetest.NewRequest("native::object::put", resourcetest.NewResource("native::object::cba/def", nil), nil)
status, ok, _ = s.IsAllowed(chain.Ingress, namespace2, req)
status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace2, container), req)
require.Equal(t, chain.AccessDenied, status)
require.True(t, ok)
req = resourcetest.NewRequest("native::object::get", resourcetest.NewResource("native::object::cba/def", nil), nil)
status, ok, _ = s.IsAllowed(chain.Ingress, namespace2, req)
status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace2, container), req)
require.Equal(t, chain.NoRuleFound, status)
require.False(t, ok)
})
t.Run("good", func(t *testing.T) {
status, ok, _ = s.IsAllowed(chain.Ingress, namespace, reqGood)
status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqGood)
require.Equal(t, chain.NoRuleFound, status)
require.False(t, ok)
t.Run("quota on a different container", func(t *testing.T) {
s.LocalStorage().AddOverride(chain.Ingress, container, &chain.Chain{
s.LocalStorage().AddOverride(chain.Ingress, engine.ContainerTarget(container), &chain.Chain{
Rules: []chain.Rule{{
Status: chain.QuotaLimitReached,
Actions: chain.Actions{Names: []string{"native::object::put"}},
@ -175,14 +175,14 @@ func TestInmemory(t *testing.T) {
}},
})
status, ok, _ = s.IsAllowed(chain.Ingress, namespace, reqGood)
status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqGood)
require.Equal(t, chain.NoRuleFound, status)
require.False(t, ok)
})
var quotaRuleChainID chain.ID
t.Run("quota on the request container", func(t *testing.T) {
quotaRuleChainID, _ = s.LocalStorage().AddOverride(chain.Ingress, container, &chain.Chain{
quotaRuleChainID, _ = s.LocalStorage().AddOverride(chain.Ingress, engine.ContainerTarget(container), &chain.Chain{
Rules: []chain.Rule{{
Status: chain.QuotaLimitReached,
Actions: chain.Actions{Names: []string{"native::object::put"}},
@ -190,15 +190,15 @@ func TestInmemory(t *testing.T) {
}},
})
status, ok, _ = s.IsAllowed(chain.Ingress, namespace, reqGood)
status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqGood)
require.Equal(t, chain.QuotaLimitReached, status)
require.True(t, ok)
})
t.Run("removed quota on the request container", func(t *testing.T) {
err := s.LocalStorage().RemoveOverride(chain.Ingress, container, quotaRuleChainID)
err := s.LocalStorage().RemoveOverride(chain.Ingress, engine.ContainerTarget(container), quotaRuleChainID)
require.NoError(t, err)
status, ok, _ = s.IsAllowed(chain.Ingress, namespace, reqGood)
status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqGood)
require.Equal(t, chain.NoRuleFound, status)
require.False(t, ok)
})

View file

@ -10,7 +10,7 @@ import (
"git.frostfs.info/TrueCloudLab/policy-engine/util"
)
type targetToChain map[string][]*chain.Chain
type targetToChain map[engine.Target][]*chain.Chain
type inmemoryLocalStorage struct {
usedChainID map[chain.ID]struct{}
@ -24,11 +24,11 @@ func NewInmemoryLocalStorage() engine.LocalOverrideStorage {
}
}
func (s *inmemoryLocalStorage) generateChainID(name chain.Name, resource string) chain.ID {
func (s *inmemoryLocalStorage) generateChainID(name chain.Name, target engine.Target) chain.ID {
var id chain.ID
for {
suffix := rand.Uint32() % 100
sid := fmt.Sprintf("%s:%s/%d", name, resource, suffix)
sid := fmt.Sprintf("%s:%s/%d", name, target.Name, suffix)
sid = strings.ReplaceAll(sid, "*", "")
sid = strings.ReplaceAll(sid, "/", ":")
sid = strings.ReplaceAll(sid, "::", ":")
@ -43,24 +43,30 @@ func (s *inmemoryLocalStorage) generateChainID(name chain.Name, resource string)
return id
}
func (s *inmemoryLocalStorage) AddOverride(name chain.Name, resource string, c *chain.Chain) (chain.ID, error) {
func (s *inmemoryLocalStorage) AddOverride(name chain.Name, target engine.Target, c *chain.Chain) (chain.ID, error) {
// AddOverride assigns generated chain ID if it has not been assigned.
if c.ID == "" {
c.ID = s.generateChainID(name, resource)
c.ID = s.generateChainID(name, target)
}
if s.nameToResourceChains[name] == nil {
s.nameToResourceChains[name] = make(targetToChain)
}
rc := s.nameToResourceChains[name]
rc[resource] = append(rc[resource], c)
for i := range rc[target] {
dkirillov marked this conversation as resolved Outdated

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?

Since it is replaced if IDs are equal

Since it is replaced if IDs are equal
if rc[target][i].ID == c.ID {
rc[target][i] = c
return c.ID, nil
}
}
rc[target] = append(rc[target], c)
return c.ID, nil
}
func (s *inmemoryLocalStorage) GetOverride(name chain.Name, resource string, chainID chain.ID) (*chain.Chain, error) {
func (s *inmemoryLocalStorage) GetOverride(name chain.Name, target engine.Target, chainID chain.ID) (*chain.Chain, error) {
if _, ok := s.nameToResourceChains[name]; !ok {
return nil, engine.ErrChainNameNotFound
}
chains, ok := s.nameToResourceChains[name][resource]
chains, ok := s.nameToResourceChains[name][target]
if !ok {
return nil, engine.ErrResourceNotFound
}
@ -72,30 +78,33 @@ func (s *inmemoryLocalStorage) GetOverride(name chain.Name, resource string, cha
return nil, engine.ErrChainNotFound
}
func (s *inmemoryLocalStorage) RemoveOverride(name chain.Name, resource string, chainID chain.ID) error {
func (s *inmemoryLocalStorage) RemoveOverride(name chain.Name, target engine.Target, chainID chain.ID) error {
if _, ok := s.nameToResourceChains[name]; !ok {
return engine.ErrChainNameNotFound
}
chains, ok := s.nameToResourceChains[name][resource]
chains, ok := s.nameToResourceChains[name][target]
if !ok {
return engine.ErrResourceNotFound
}
for i, c := range chains {
if c.ID == chainID {
s.nameToResourceChains[name][resource] = append(chains[:i], chains[i+1:]...)
s.nameToResourceChains[name][target] = append(chains[:i], chains[i+1:]...)
return nil
}
}
return engine.ErrChainNotFound
}
func (s *inmemoryLocalStorage) ListOverrides(name chain.Name, resource string) ([]*chain.Chain, error) {
func (s *inmemoryLocalStorage) ListOverrides(name chain.Name, target engine.Target) ([]*chain.Chain, error) {
rcs, ok := s.nameToResourceChains[name]
if !ok {
return []*chain.Chain{}, nil
}
for container, chains := range rcs {
if !util.GlobMatch(resource, container) {
for t, chains := range rcs {
if t.Type != target.Type {
continue
}
if !util.GlobMatch(target.Name, t.Name) {
continue
}
return chains, nil

View file

@ -9,11 +9,15 @@ import (
)
const (
resrc = "native:::object/ExYw/*"
container = "native:::object/ExYw/*"
chainID = "ingress:ExYw"
nonExistChainId = "ingress:LxGyWyL"
)
var (
resrc = engine.ContainerTarget(container)
)
func testInmemLocalStorage() *inmemoryLocalStorage {
return NewInmemoryLocalStorage().(*inmemoryLocalStorage)
}

View file

@ -20,9 +20,9 @@ func NewInmemoryMorphRuleChainStorage() engine.MorphRuleChainStorage {
func (s *inmemoryMorphRuleChainStorage) AddMorphRuleChain(name chain.Name, target engine.Target, c *chain.Chain) (err error) {
switch target.Type {
case engine.Namespace:
_, err = s.nameToNamespaceChains.AddOverride(name, target.Name, c)
_, err = s.nameToNamespaceChains.AddOverride(name, target, c)
case engine.Container:
_, err = s.nameToContainerChains.AddOverride(name, target.Name, c)
_, err = s.nameToContainerChains.AddOverride(name, target, c)
default:
err = engine.ErrUnknownTarget
}
@ -32,9 +32,9 @@ func (s *inmemoryMorphRuleChainStorage) AddMorphRuleChain(name chain.Name, targe
func (s *inmemoryMorphRuleChainStorage) RemoveMorphRuleChain(name chain.Name, target engine.Target, chainID chain.ID) error {
switch target.Type {
case engine.Namespace:
return s.nameToNamespaceChains.RemoveOverride(name, target.Name, chainID)
return s.nameToNamespaceChains.RemoveOverride(name, target, chainID)
case engine.Container:
return s.nameToContainerChains.RemoveOverride(name, target.Name, chainID)
return s.nameToContainerChains.RemoveOverride(name, target, chainID)
default:
return engine.ErrUnknownTarget
}
@ -43,9 +43,9 @@ func (s *inmemoryMorphRuleChainStorage) RemoveMorphRuleChain(name chain.Name, ta
func (s *inmemoryMorphRuleChainStorage) ListMorphRuleChains(name chain.Name, target engine.Target) ([]*chain.Chain, error) {
switch target.Type {
case engine.Namespace:
return s.nameToNamespaceChains.ListOverrides(name, target.Name)
return s.nameToNamespaceChains.ListOverrides(name, target)
case engine.Container:
return s.nameToContainerChains.ListOverrides(name, target.Name)
return s.nameToContainerChains.ListOverrides(name, target)
default:
}
return nil, engine.ErrUnknownTarget

View file

@ -8,19 +8,19 @@ import (
type ChainRouter interface {
// IsAllowed returns status for the operation after all checks.
// The second return value signifies whether a matching rule was found.
IsAllowed(name chain.Name, target string, r resource.Request) (status chain.Status, found bool, err error)
IsAllowed(name chain.Name, reqTarget RequestTarget, r resource.Request) (status chain.Status, found bool, err error)
}
// LocalOverrideStorage is the interface to manage local overrides defined
// for a node. Local overrides have a higher priority than chains got from morph storage.
type LocalOverrideStorage interface {
AddOverride(name chain.Name, resource string, c *chain.Chain) (chain.ID, error)
AddOverride(name chain.Name, target Target, c *chain.Chain) (chain.ID, error)
GetOverride(name chain.Name, resource string, chainID chain.ID) (*chain.Chain, error)
GetOverride(name chain.Name, target Target, chainID chain.ID) (*chain.Chain, error)
RemoveOverride(name chain.Name, resource string, chainID chain.ID) error
RemoveOverride(name chain.Name, target Target, chainID chain.ID) error
ListOverrides(name chain.Name, resource string) ([]*chain.Chain, error)
ListOverrides(name chain.Name, target Target) ([]*chain.Chain, error)
DropAllOverrides(name chain.Name) error
}
@ -37,6 +37,45 @@ type Target struct {
Name string
}
// RequestTarget combines several targets on which the request is performed.
type RequestTarget struct {
Namespace *Target
Container *Target
}
func NewRequestTargetWithNamespace(namespace string) RequestTarget {
nt := NamespaceTarget(namespace)
return RequestTarget{
Namespace: &nt,
}
}
func NewRequestTargetWithContainer(container string) RequestTarget {
ct := ContainerTarget(container)
return RequestTarget{
Container: &ct,
}
}
func NewRequestTarget(namespace, container string) RequestTarget {
nt := NamespaceTarget(namespace)
ct := ContainerTarget(container)
return RequestTarget{
Namespace: &nt,
Container: &ct,
}
}
func (rt *RequestTarget) Targets() (targets []Target) {
if rt.Container != nil {
targets = append(targets, *rt.Container)
}
if rt.Namespace != nil {
targets = append(targets, *rt.Namespace)
}
return
}
func NamespaceTarget(namespace string) Target {
return Target{
Type: Namespace,