From 15461183b1c2570fce587736f3a332ed541ed707 Mon Sep 17 00:00:00 2001 From: Mike Mason Date: Wed, 21 Jun 2023 21:53:49 +0000 Subject: [PATCH 1/6] correct events.nats.token path Signed-off-by: Mike Mason --- chart/permissions-api/templates/deployment-worker.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/chart/permissions-api/templates/deployment-worker.yaml b/chart/permissions-api/templates/deployment-worker.yaml index 8e4f293d..c3ca9e91 100644 --- a/chart/permissions-api/templates/deployment-worker.yaml +++ b/chart/permissions-api/templates/deployment-worker.yaml @@ -76,7 +76,7 @@ spec: {{- end }} {{- if .Values.config.events.nats.token }} - name: PERMISSIONSAPI_EVENTS_SUBSCRIBER_NATS_TOKEN - value: "{{ .Values.config.events.token }}" + value: "{{ .Values.config.events.nats.token }}" {{- end }} {{- if .Values.config.oidc.issuer }} {{- with .Values.config.oidc.audience }} From 887726a2f94fb27e2b37994f4aa79098e393ea45 Mon Sep 17 00:00:00 2001 From: Mike Mason Date: Wed, 21 Jun 2023 21:54:41 +0000 Subject: [PATCH 2/6] add missing loggers Signed-off-by: Mike Mason --- cmd/worker.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/worker.go b/cmd/worker.go index 5dc747df..59faf194 100644 --- a/cmd/worker.go +++ b/cmd/worker.go @@ -62,9 +62,9 @@ func worker(ctx context.Context, cfg *config.AppConfig) { logger.Fatalw("invalid spicedb policy", "error", err) } - engine := query.NewEngine("infratographer", spiceClient, query.WithPolicy(policy)) + engine := query.NewEngine("infratographer", spiceClient, query.WithPolicy(policy), query.WithLogger(logger)) - subscriber, err := pubsub.NewSubscriber(ctx, cfg.Events.Subscriber, engine) + subscriber, err := pubsub.NewSubscriber(ctx, cfg.Events.Subscriber, engine, pubsub.WithLogger(logger)) if err != nil { logger.Fatalw("unable to initialize subscriber", "error", err) } From 1ed811f5539a3a6e9ad1efe6819466760f0d9ce3 Mon Sep 17 00:00:00 2001 From: Mike Mason Date: Wed, 21 Jun 2023 21:55:31 +0000 Subject: [PATCH 3/6] subscriber.Listen is blocking so we never capture and exit with a signal Signed-off-by: Mike Mason --- cmd/worker.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/cmd/worker.go b/cmd/worker.go index 59faf194..a05e58d9 100644 --- a/cmd/worker.go +++ b/cmd/worker.go @@ -75,9 +75,13 @@ func worker(ctx context.Context, cfg *config.AppConfig) { } } - if err := subscriber.Listen(); err != nil { - logger.Fatalw("error listening for events", "error", err) - } + logger.Info("Listening for events") + + go func() { + if err := subscriber.Listen(); err != nil { + logger.Fatalw("error listening for events", "error", err) + } + }() // Wait until we're told to stop sig := <-sigCh From 9bc834b5f3269a245ea434677968aab64c5350cf Mon Sep 17 00:00:00 2001 From: Mike Mason Date: Wed, 21 Jun 2023 21:55:53 +0000 Subject: [PATCH 4/6] correct policy example Signed-off-by: Mike Mason --- policy.example.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/policy.example.yaml b/policy.example.yaml index 0ed480d1..4a6e46f2 100644 --- a/policy.example.yaml +++ b/policy.example.yaml @@ -71,13 +71,13 @@ actionbindings: - relationshipaction: relation: parent actionname: loadbalancer_delete - - actionname: loadbalancer_create + - actionname: loadbalancer_get typename: loadbalancer conditions: - rolebinding: {} - relationshipaction: relation: owner - actionname: loadbalancer_create + actionname: loadbalancer_get - actionname: loadbalancer_update typename: loadbalancer conditions: From 5f8c6e98877366c54a8fee7dd749bae4d7de1084 Mon Sep 17 00:00:00 2001 From: Mike Mason Date: Wed, 21 Jun 2023 21:56:51 +0000 Subject: [PATCH 5/6] skip nacking for errors We need to dig into the errors that may be produced and create a list of errors which should be reprocessable. Otherwise we might reprocess a message which will never be succeed, wasting resources. Signed-off-by: Mike Mason --- internal/pubsub/subscriber.go | 10 ++++------ internal/pubsub/subscriber_test.go | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/internal/pubsub/subscriber.go b/internal/pubsub/subscriber.go index 27f50baf..1f50778e 100644 --- a/internal/pubsub/subscriber.go +++ b/internal/pubsub/subscriber.go @@ -81,6 +81,8 @@ func (s *Subscriber) Subscribe(topic string) error { s.changeChannels = append(s.changeChannels, msgChan) + s.logger.Infof("Subscribing to topic %s", topic) + return nil } @@ -190,9 +192,7 @@ func (s *Subscriber) createRelationships(ctx context.Context, msg *message.Messa // Attempt to create the relationships in SpiceDB. If this fails, nak the message for reprocessing _, err := s.qe.CreateRelationships(ctx, relationships) if err != nil { - s.logger.Errorw("error creating relationships - will reprocess", "error", err.Error()) - - return err + s.logger.Errorw("error creating relationships - will not reprocess", "error", err.Error()) } return nil @@ -201,9 +201,7 @@ func (s *Subscriber) createRelationships(ctx context.Context, msg *message.Messa func (s *Subscriber) deleteRelationships(ctx context.Context, msg *message.Message, resource types.Resource) error { _, err := s.qe.DeleteRelationships(ctx, resource) if err != nil { - s.logger.Errorw("error deleting relationships - will reprocess", "error", err.Error()) - - return err + s.logger.Errorw("error deleting relationships - will not reprocess", "error", err.Error()) } return nil diff --git a/internal/pubsub/subscriber_test.go b/internal/pubsub/subscriber_test.go index 03448c73..c26abe08 100644 --- a/internal/pubsub/subscriber_test.go +++ b/internal/pubsub/subscriber_test.go @@ -151,7 +151,7 @@ func TestNATS(t *testing.T) { return context.WithValue(ctx, contextKeyEngine, &engine) }, CheckFn: func(ctx context.Context, t *testing.T, result testingx.TestResult[*Subscriber]) { - require.ErrorIs(t, result.Err, eventtools.ErrNack) + require.NoError(t, result.Err) }, }, { From e7cdd255ad9142d551098e5012d79b19370feb1e Mon Sep 17 00:00:00 2001 From: Mike Mason Date: Wed, 21 Jun 2023 21:59:38 +0000 Subject: [PATCH 6/6] create relationships for all matched relations This allows for relations to be determined based on their type. All matched relations will be created. Signed-off-by: Mike Mason --- internal/pubsub/subscriber.go | 33 ++++++++++++++++++++++++++------ internal/query/errors.go | 9 +++++++++ internal/query/mock/mock.go | 15 +++++++++++++++ internal/query/relations.go | 25 ++++++++++++++---------- internal/query/relations_test.go | 2 +- internal/query/service.go | 10 +++++++--- 6 files changed, 74 insertions(+), 20 deletions(-) diff --git a/internal/pubsub/subscriber.go b/internal/pubsub/subscriber.go index 1f50778e..28c1e3a8 100644 --- a/internal/pubsub/subscriber.go +++ b/internal/pubsub/subscriber.go @@ -165,6 +165,13 @@ func (s *Subscriber) processEvent(msg *message.Message) error { func (s *Subscriber) createRelationships(ctx context.Context, msg *message.Message, resource types.Resource, additionalSubjectIDs []gidx.PrefixedID) error { var relationships []types.Relationship + rType := s.qe.GetResourceType(resource.Type) + if rType == nil { + s.logger.Warnw("no resource type found for", "resource_type", resource.Type) + + return nil + } + // Attempt to create relationships from the message fields. If this fails, reject the message for _, id := range additionalSubjectIDs { subjResource, err := s.qe.NewResourceFromID(id) @@ -174,13 +181,27 @@ func (s *Subscriber) createRelationships(ctx context.Context, msg *message.Messa continue } - relationship := types.Relationship{ - Resource: resource, - Relation: subjResource.Type, - Subject: subjResource, - } + for _, rel := range rType.Relationships { + var relation string - relationships = append(relationships, relationship) + for _, tName := range rel.Types { + if tName == subjResource.Type { + relation = rel.Relation + + break + } + } + + if relation != "" { + relationship := types.Relationship{ + Resource: resource, + Relation: relation, + Subject: subjResource, + } + + relationships = append(relationships, relationship) + } + } } if len(relationships) == 0 { diff --git a/internal/query/errors.go b/internal/query/errors.go index e887d914..c118e660 100644 --- a/internal/query/errors.go +++ b/internal/query/errors.go @@ -9,4 +9,13 @@ var ( // ErrInvalidReference represents an error condition where a given SpiceDB object reference is for some reason invalid. ErrInvalidReference = errors.New("invalid reference") + + // ErrInvalidNamespace represents an error when the id prefix is not found in the resource schema + ErrInvalidNamespace = errors.New("invalid namespace") + + // ErrInvalidType represents an error when a resource type is not found in the resource schema + ErrInvalidType = errors.New("invalid type") + + // ErrInvalidRelationship represents an error when no matching relationship was found + ErrInvalidRelationship = errors.New("invalid relationship") ) diff --git a/internal/query/mock/mock.go b/internal/query/mock/mock.go index 306e8a48..6c458875 100644 --- a/internal/query/mock/mock.go +++ b/internal/query/mock/mock.go @@ -104,6 +104,21 @@ func (e *Engine) NewResourceFromID(id gidx.PrefixedID) (types.Resource, error) { return out, nil } +// GetResourceType returns the resource type by name +func (e *Engine) GetResourceType(name string) *types.ResourceType { + if e.schema == nil { + e.schema = iapl.DefaultPolicy().Schema() + } + + for _, resourceType := range e.schema { + if resourceType.Name == name { + return &resourceType + } + } + + return nil +} + // SubjectHasPermission returns nil to satisfy the Engine interface. func (e *Engine) SubjectHasPermission(ctx context.Context, subject types.Resource, action string, resource types.Resource, queryToken string) error { e.Called() diff --git a/internal/query/relations.go b/internal/query/relations.go index b29a94ce..9eee8aaa 100644 --- a/internal/query/relations.go +++ b/internal/query/relations.go @@ -2,7 +2,6 @@ package query import ( "context" - "errors" "io" "strings" @@ -13,12 +12,6 @@ import ( var roleSubjectRelation = "subject" -var ( - errorInvalidNamespace = errors.New("invalid namespace") - errorInvalidType = errors.New("invalid type") - errorInvalidRelationship = errors.New("invalid relationship") -) - func (e *engine) getTypeForResource(res types.Resource) (types.ResourceType, error) { for _, resType := range e.schema { if res.Type == resType.Name { @@ -26,7 +19,7 @@ func (e *engine) getTypeForResource(res types.Resource) (types.ResourceType, err } } - return types.ResourceType{}, errorInvalidType + return types.ResourceType{}, ErrInvalidType } func (e *engine) validateRelationship(rel types.Relationship) error { @@ -40,6 +33,8 @@ func (e *engine) validateRelationship(rel types.Relationship) error { return err } + e.logger.Infow("validation relationship", "sub", subjType.Name, "rel", rel.Relation, "res", resType.Name) + for _, typeRel := range resType.Relationships { // If we find a relation with a name and type that matches our relationship, // return @@ -53,7 +48,7 @@ func (e *engine) validateRelationship(rel types.Relationship) error { } // No matching relationship was found, so we should return an error - return errorInvalidRelationship + return ErrInvalidRelationship } func resourceToSpiceDBRef(namespace string, r types.Resource) *pb.ObjectReference { @@ -441,7 +436,7 @@ func (e *engine) NewResourceFromID(id gidx.PrefixedID) (types.Resource, error) { rType, ok := e.schemaPrefixMap[prefix] if !ok { - return types.Resource{}, errorInvalidNamespace + return types.Resource{}, ErrInvalidNamespace } out := types.Resource{ @@ -451,3 +446,13 @@ func (e *engine) NewResourceFromID(id gidx.PrefixedID) (types.Resource, error) { return out, nil } + +// GetResourceType returns the resource type by name +func (e *engine) GetResourceType(name string) *types.ResourceType { + rType, ok := e.schemaTypeMap[name] + if !ok { + return nil + } + + return &rType +} diff --git a/internal/query/relations_test.go b/internal/query/relations_test.go index 66ef3ca8..b5106dda 100644 --- a/internal/query/relations_test.go +++ b/internal/query/relations_test.go @@ -224,7 +224,7 @@ func TestRelationships(t *testing.T) { Subject: parentRes, }, CheckFn: func(ctx context.Context, t *testing.T, res testingx.TestResult[[]types.Relationship]) { - assert.ErrorIs(t, res.Err, errorInvalidRelationship) + assert.ErrorIs(t, res.Err, ErrInvalidRelationship) }, }, { diff --git a/internal/query/service.go b/internal/query/service.go index 4cd5c1b3..459ddb3e 100644 --- a/internal/query/service.go +++ b/internal/query/service.go @@ -21,6 +21,7 @@ type Engine interface { ListRoles(ctx context.Context, resource types.Resource, queryToken string) ([]types.Role, error) DeleteRelationships(ctx context.Context, resource types.Resource) (string, error) NewResourceFromID(id gidx.PrefixedID) (types.Resource, error) + GetResourceType(name string) *types.ResourceType SubjectHasPermission(ctx context.Context, subject types.Resource, action string, resource types.Resource, queryToken string) error } @@ -30,13 +31,16 @@ type engine struct { client *authzed.Client schema []types.ResourceType schemaPrefixMap map[string]types.ResourceType + schemaTypeMap map[string]types.ResourceType } -func (e *engine) cacheSchemaPrefixes() { +func (e *engine) cacheSchemaResources() { e.schemaPrefixMap = make(map[string]types.ResourceType, len(e.schema)) + e.schemaTypeMap = make(map[string]types.ResourceType, len(e.schema)) for _, res := range e.schema { e.schemaPrefixMap[res.IDPrefix] = res + e.schemaTypeMap[res.Name] = res } } @@ -55,7 +59,7 @@ func NewEngine(namespace string, client *authzed.Client, options ...Option) Engi if e.schema == nil { e.schema = iapl.DefaultPolicy().Schema() - e.cacheSchemaPrefixes() + e.cacheSchemaResources() } return e @@ -76,6 +80,6 @@ func WithPolicy(policy iapl.Policy) Option { return func(e *engine) { e.schema = policy.Schema() - e.cacheSchemaPrefixes() + e.cacheSchemaResources() } }