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

Add experimental NATS messaging channel #4075

Merged
merged 4 commits into from
Aug 28, 2024

Conversation

evankanderson
Copy link
Member

@evankanderson evankanderson commented Aug 2, 2024

Summary

This provides an experimental implementation of our messaging interfaces using CloudEvents + NATS.

This is somewhat a proof-of-concept: we'd need to do some additional work and testing (for example, with multiple consumers, and testing of the NATS channel code) before deploying it, but it works for at least some cases and has a few advantages:

  1. JetStream seems to have at least a bit of a competing-consumers model that shouldn't bottleneck us to a single outstanding delivery at a time. This might allow us to simplify some of the other code that tries to work around Watermill issues.

  2. CloudEvents has a richer metadata model, and at least some support (not currently wired) for tracing and OTel instrumentation.

  3. With an explicit messaging service, you can locally test both receiving and sending events using the nats CLI to send or watch events being handled by Minder. For example:

    nats consumer create
    < answer a bunch of questions >
    nats consumer next minder manual
    < read a message that was sent earlier >
    

    This seems like it could be helpful for debugging and isolating interactions (e.g. replay processing a set of repo events without needing to generate GitHub events and wire a webhook), as well as helping the communication between Minder and Reminder.

Change Type

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

Manual testing with make run-docker, then registering a repo and checking the Minder logs + nats messages. I would not say this has been heavily tested.

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

Copy link

Minder Vulnerability Report ✅

Minder analyzed this PR and found no vulnerable dependencies.

Vulnerability scan of d891bdf5:

  • 🐞 vulnerable packages: 0
  • 🛠 fixes available for: 0

@evankanderson evankanderson requested a review from JAORMX August 2, 2024 23:06
@evankanderson
Copy link
Member Author

I recall Ozz was interested in this earlier, and maybe Don

@evankanderson evankanderson requested a review from dmjb August 2, 2024 23:07
@coveralls
Copy link

coveralls commented Aug 2, 2024

Coverage Status

coverage: 54.014% (+0.1%) from 53.87%
when pulling c5048e5 on evankanderson:nats-testing
into 9f27d61 on stacklok:main.

@evankanderson evankanderson marked this pull request as ready for review August 26, 2024 18:13
@evankanderson
Copy link
Member Author

Okay, I've added a test which uncovered at least one bug which led to double-base64-encoding the payload.

@evankanderson
Copy link
Member Author

Before we roll this out anywhere, I want to:

  1. Add the ability to subscribe on multiple channels, and choose which channel to publish on. This will allow listen-on-DB, publish-to-NATS for a smooth message transition.
  2. Add some driver-level stats (or see if CloudEvents-NATS already has them) for publishing / receiving throughput, and possibly also message delay (we should get the time attribute which we can use for this computation).

We'll also need to start running a NATS server, and work out authentication using the Kubernetes JWT before too long.

@evankanderson evankanderson merged commit 42df741 into mindersec:main Aug 28, 2024
21 checks passed
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