[#25] engine: Refactor ChainRouter interface

* Pass RequestTarget instead only namespace
* Refactor unit-tests and dependencies

Signed-off-by: Airat Arifullin <aarifullin@yadro.com>

iam/converter_test.go

@@ -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())
 		})

pkg/engine/chain_router.go

@@ -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) {
+	status, ruleFound, err = dr.checkLocal(name, rt, r)
+	if err != nil {
+		return chain.NoRuleFound, false, err
+	} else if ruleFound {
+		// 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 {
+			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
-}

pkg/engine/inmemory/inmemory.go

@@ -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)
 }

pkg/engine/inmemory/inmemory_test.go

@@ -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)
 		})

pkg/engine/inmemory/local_storage.go

@@ -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] {
+		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

pkg/engine/inmemory/local_storage_test.go

@@ -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)
 }

pkg/engine/inmemory/morph_storage.go

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

pkg/engine/interface.go

@@ -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,