Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update to use new event handling #109

Merged
merged 9 commits into from
Jun 19, 2023
Merged

Conversation

mikemrm
Copy link
Contributor

@mikemrm mikemrm commented Jun 16, 2023

This adds the new x/events handling.

Note:

There are some issues with detecting Ack/Nack's on messages. Which has resulted in some tests not being able to be done that check for Nacked messages.

@mikemrm mikemrm requested review from a team as code owners June 16, 2023 16:49
@mikemrm mikemrm force-pushed the update-events branch 2 times, most recently from ba0f621 to 0dea11d Compare June 16, 2023 17:06
This adds the new x/events handling.

Note:

There are some issues with detecting Ack/Nack's on messages.
Which has resulted in some tests not being able to be done that check
for Nacked messages.

Signed-off-by: Mike Mason <[email protected]>
@mikemrm mikemrm force-pushed the update-events branch 3 times, most recently from 66cd0da to d010173 Compare June 16, 2023 17:46
Copy link
Contributor

@jnschaeffer jnschaeffer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks decent so far - from what I understand we can't reintroduce the testing logic to check that a message was acked because the NATS server isn't directly exposed in x/testing/eventtools. Thus, most of my input is around the code itself.

internal/pubsub/subscriber.go Outdated Show resolved Hide resolved
internal/pubsub/subscriber.go Outdated Show resolved Hide resolved
}

func (c *Client) createRelationships(ctx context.Context, msg *nats.Msg, resource types.Resource, fields map[string]string) error {
func (s *Subscriber) createRelationships(ctx context.Context, msg *message.Message, resource types.Resource, additionalSubjectIDs []gidx.PrefixedID) error {
Copy link
Contributor

@jnschaeffer jnschaeffer Jun 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So additionalSbjectIDs isn't the abstraction we want here - that's more of a generic "things that are related to this thing" list. I'm pretty sure what we actually want is a map[string]gidx.PrefixedID or something similar, where that map is built from (events.ChangeMessage).SubjectFields or (events.ChangeMessage).Changes as follows:

  • For the given resource, look up its type in the engine schema
  • For each field in the map, check to see if a relation exists with that name
  • If it does, look up the target types for the relation
  • If it does, ensure the prefix in the ID matches at least one prefix in the target types

As a concrete example, say we have a payload that look like so:

{
    "subjectID": "loadbal-abc123",
    "subjectFields": {
        "name": "my-cool-loadbalancer",
        "owner": "tnntten-abc123"
    }
}

Then, assume a policy exists (in YAML) like so:

resourceTypes:
  - name: tenant
    idPrefix: tnntten
  - name: loadbalancer
    idPrefix: loadbal
    relationships:
      - relation: owner
        targetTypeNames:
          - tenant

When we process the event, we should load the resource type definition from the schema. From there, we can see owner is a relation on the authorization policy for a loadbalancer resource, but name is not. Thus, we should persist anything called owner as a relationship in SpiceDB, and check that its ID prefix is tnntten before we do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense. Right now the services aren't using the SubjectFields attribute and is something I think we need to discuss further. I believe once we've figured that out, this will be an easy transition. For now I've published a few changes to handle the AdditionalSubjectIDs better.

Signed-off-by: Mike Mason <[email protected]>
This enables us to run all tests which check for specific acks.

Signed-off-by: Mike Mason <[email protected]>
This ignores unrelated ids instead of failing if an id is found that
isn't part of the schema.

Signed-off-by: Mike Mason <[email protected]>
Signed-off-by: Mike Mason <[email protected]>
fishnix
fishnix previously approved these changes Jun 19, 2023
Copy link
Contributor

@fishnix fishnix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple of minor comments, otherwise lgtm 👍

case events.DeleteChangeType:
err = s.handleDeleteEvent(ctx, msg, changeMsg)
default:
s.logger.Warnw("ignoring msg, not a create, update or delete event", zap.String("event-type", changeMsg.EventType))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should be structuring these with . notation instead of -, although I haven't looked around to see what's being done elsewhere so feel free to ignore if there's an established pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe in most places we actually use _ which I can update this to use.

var rType *types.ResourceType

if e.schema == nil {
e.schema = iapl.DefaultPolicy().Schema()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like the schema should be preloaded and this should be an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just for the mock. Most tests just define an empty mock Engine. So this simplifies to use the default schema if we aren't testing something specific.

fishnix
fishnix previously approved these changes Jun 19, 2023
@mikemrm mikemrm merged commit 4651abd into infratographer:main Jun 19, 2023
@mikemrm mikemrm deleted the update-events branch June 19, 2023 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants