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

Support event subscriptions with glob wildcards #19205

Merged
merged 5 commits into from
Feb 16, 2023

Conversation

tomhjp
Copy link
Contributor

@tomhjp tomhjp commented Feb 15, 2023

Support event subscriptions with glob wildcards. Uses https://github.com/ryanuber/go-glob to implement * wildcards.

@tomhjp tomhjp added this to the 1.13.0 milestone Feb 15, 2023
@tomhjp tomhjp requested a review from swenson February 15, 2023 19:49
@tomhjp tomhjp changed the title Vault 9285/event wildcard subscriptions Support event subscriptions with glob wildcards Feb 15, 2023
Copy link
Contributor

@swenson swenson left a comment

Choose a reason for hiding this comment

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

Mostly looks good, but a question.

@@ -97,7 +99,7 @@ func (bus *EventBus) SendInternal(ctx context.Context, ns *namespace.Namespace,
// We can't easily know when the Send is complete, so we can't call the cancel function.
// But, it is called automatically after bus.timeout, so there won't be any leak as long as bus.timeout is not too long.
ctx, _ = context.WithTimeout(ctx, bus.timeout)
_, err := bus.broker.Send(ctx, eventlogger.EventType(eventType), eventReceived)
_, err := bus.broker.Send(ctx, eventTypeAll, eventReceived)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this right? We always send all events to all event types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I should have explained the approach better. This is kind of a halfway house towards your TODO comment TODO: should we have just one node per namespace, and handle all the routing ourselves?. There's one big firehose that everything flows through, and we use filter nodes to flexibly reduce that down to what each subscriber is interested in. It essentially splits eventType into two different meanings (which is why I introduced pattern to reduce our usage of eventType):

  • The subscriber's understanding of event types is the same as before - it's whatever the event producer defines.
  • But now the eventlogger library has a different internal-only representation of 'event type'. Its built-in routing is pretty inflexible, so the PR stops trying to use it for that, and instead just utilises it for dispatching events down pipelines (and the pipelines define their own routing along the way).

In Boundary, they have 3 different event types, sys, observation, and audit. I'm not sure what classifications we'd use yet, but if we go with this approach, we might want to follow a similar pattern and split eventTypeAll into some top-level broad categories. We should figure that out soon though, as it will probably mean some breaking changes to the Alpha's API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, sounds good!

Copy link
Contributor

@swenson swenson left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants