Plugins RFC

[deontologician]

Rendered version of RFC

This change is

[Tryneus]

It sounds like commands should be an object or array of objects.

[deontologician]

I thought of that, but then I thought if they wanted to implement multiple commands, they could just do them as subcommands. So instead of hz crom-input and hz crom-output they'd do hz crom input and hz crom output

[mlucy]

One piece of feedback: I think we should require people to specify a namespace for their plugin, and then namespace things like environment variables and endpoints and whatnot. (So if I was making a GraphQL plugin, I might register gql as my namespace, and then all my endpoints would be /horizon/gql/foo and all my environment variables would be GQL_BAR and so on.) Otherwise plugins are inevitably going to conflict with each other, and it's going to be very annoying for users to deal with.

[Tryneus]

Maybe we should just use Joi for this, since it's how we're doing schema validation elsewhere. We won't have to reimplement a bunch of stuff.

[deontologician]

That's a good idea, I'll change it

[deontologician]

@mlucy While I didn't mention an explicit namespacing scheme, the plugin system should verify at plugin activation time whether anything is in conflict. This would allow people to use nicer names for things. Essentially the idea is that the plugins are somewhat less paranoid about name conflicts since they declare everything they use. So there might be multiple plugins that provide a /graphQL endpoint, but you wouldn't ever have both of them installed at the same time.

[mlucy]

I think we should omit processConfig (and maybe simplify addArguments to be more restrictive). I think we should try to force plugins to follow certain standards for what arguments they accept, specifically that all arguments can be specified with the same name in the config file, in environment variables, or on the command line (in increasing order of precedence). In practice every good plugin will end up doing that anyway, so why allow for bad plugins with confusing argument semantics like giving the config file precedence over the command line or using different names for the environment variable and the command line option?

[deontologician]

addArguments is for specifying the command line syntax, whereas processConfig runs after the command has been run and the options are parsed out successfully. I don't have a strong opinion, but it seems like environment variables may need more namespacing than config options or commands. Command options can be part of subcommands and the config has sections which are both like namespaces.

It'd be nice if we didn't enforce a namespacing scheme on the environment variables, or ensure that they have a specific precedence order, but maybe it's not a useful kind of flexibility.

[mlucy]

Ah, OK.

Personally I would:

• Only let plugins claim environment variables prefixed with their name (or a shortname or something). So an Elasticsearch plugin could register ELASTICSEARCH_PORT but not just plain old PORT. I think that would be clearer to users and less prone to conflicts. (And arguable more consistent since the config sections are acting basically like namespacing.)
• Enforce the command line beats environment beats config ordering. I think it would be extremely confusing if different plugins were using different precedence schemes.

[deontologician]

These seem reasonable suggestions

[mlucy]

Is the idea that Horizon will look in a certain directory on startup to figure out what plugins to load? Is it OK for different Horizon nodes to be running different sets of plugins? (Also, am I correct in assuming that plugins live on disk rather than inside RethinkDB?)

[mlucy]

@deontologician -- I'm more worried about e.g. a GraphQL plugin and a performance graphing plugin both providing a graphFOO endpoint. Or two other plugins with totally unrelated behavior that happen to pick the same name for an endpoint.

I think we should have namespacing for the same reason people who write packages for programming languages don't get to just dump their functions into the global namespace: if people put things into the global namespace, conflicts are literally inevitable once there are a certain number of plugins, and it's extremely frustrating to not be able to use two totally unrelated plugins together just because they happened to pick the same name for something. (You could argue that a responsible plugin writer would namespace their endpoints anyway, which is 100% true, but if that's what a responsible plugin writer would do, why not just force everyone to be responsible?)

[deontologician]

@mlucy in the case where two plugins pick the same name for an endpoint, the plugins should cooperate and pick different names. I think this kind of conflict will be pretty rare, and as long as we don't allow them both to run (by checking that endpoints don't overlap) it won't be a problem. It's much better to have clean looking paths and command names than to namespace everything in case two plugins happen to have a name clash.

I'm not arguing a responsible plugin writer should namespace their endpoints, I think a responsible plugin writer should name the endpoint whatever is most natural, and if/when a conflict occurs, decide if the newer project should rename its endpoints or namespace them (and they'll probably always just rename them). Or they could decide the conflict is ok since usage of the two plugins should be mutually exclusive (like two graphQL plugins)

[deontologician]

Is the idea that Horizon will look in a certain directory on startup to figure out what plugins to load?

Node modules have a field in their package.json that allows them to specify keywords. So we'll assume any modules currently installed that have the correct keyword are horizon plugins and try to load them. This is how another module project, Yeoman, detects plugins and it seems reasonable to me.

Is it OK for different Horizon nodes to be running different sets of plugins?

My gut feeling is no for simplicity's sake, but it might be worth allowing so people can add worker plugins only on certain servers, things like that.

am I correct in assuming that plugins live on disk rather than inside RethinkDB?

yes, they'll live in the node_modules directory

[marshall007]

Just at first glance, I think this RFC is trying to squeeze too much under the "plugin" umbrella. There are more or less four basic ways one might want to extend the Horizon server:

1. Extending the WebSocket protocol.
2. Hooking into life-cycle events (i.e. for auth, request validation, document validation, etc).
3. Adding custom HTTP endpoints.
4. Workers/Scheduled Tasks

I'm not sure it makes sense for a single "plugin" to be responsible for configuring all these things at once. Primus has done really well in this regard and is a pretty solid example for many of the goals in this RFC. Several core components in Primus are in fact implemented as plugins/middleware, which I think is along the same lines as how you guys are thinking about this. Notice how they have different mechanisms for hooking into the server (authorization, middleware, plugins, events) and each is tailored to a particular set of use-cases.

Likewise, I don't think "command plugins" should be handled here since that seems to imply a hard dependency on the CLI. You should be able to write plugins that can be used by any @horizon/server instance regardless of how you spin it up. Why not have the Horizon CLI be independently extensible?

[deontologician]

Likewise, I don't think "command plugins" should be handled here since that seems to imply a hard dependency on the CLI.

We haven't totally fleshed out how plugins should be integrated if you're embedding the server, but I think it's reasonable to just have the cli aspect be a no-op in that case. It's kind of nice if you have a server plugin to be able to add related convenience commands to the cli (like create an example config), and it's probably not worth having multiple plugin types.

If you're doing manual backend and a plugin only has a cli component, you just wouldn't use that plugin, so it's not a huge burden.

The idea of linking them together is to make it clear they're intended to be significant fully featured extensions to Horizon, rather than disconnected addons. In addition, the plugin will be able to share its own state that is isolated from other plugins, so if several of these pieces need to work together, they can. For example, if a GraphQL plugin wanted to define both an http endpoint and a new Horizon request type it could share configuration and state between the two.

[marshall007]

The idea of linking them together is to make it clear they're intended to be significant fully featured extensions to Horizon, rather than disconnected addons.

@deontologician right, this makes sense but there are two cases to consider:

1. Extending with one-off "addons".
2. Installing a full-fledged third-party plugin.

I think (1) is going to be the more common case and I wouldn't want to create a full plugin definition just to add a couple HTTP endpoints. What I'm getting at is that I would prefer a solution that provides specialized "addon" APIs for integrating into the server in various ways and then a "plugin definition" just exports some metadata and a handler function that receives a server instance to configure.

Rough example:

const plugin = function (server, options, next) {
  // add HTTP endpoint
  server.http.endpoint('/test', (req, res) => {
    res.send(...);
  });

  // extend WS protocol
  server.ws.command('test', ...);

  // more configuration here...

  next();
};

plugin.meta = {
  name: 'my-plugin',
  version: 'v0.0.1',
  config: Joi.object() // schema for `options`, validated before handler is called
};

module.exports = plugin;

[deontologician]

a "plugin definition" just exports some metadata and a handler function that receives a server instance to configure.

I think fundamentally we should avoid any kind of free-form configuration like this. It's more flexible, but I think it's better to make it hard for plugins to step on each other's toes than it is to make them completely free to add any functionality to the server. Specifically, I think we should avoid the middleware paradigm with plugins, leaving middleware kinds of setups to users who are embedding horizon

[bopjesvla]

I think fundamentally we should avoid any kind of free-form configuration like this. It's more flexible, but I think it's better to make it hard for plugins to step on each other's toes than it is to make them completely free to add any functionality to the server. Specifically, I think we should avoid the middleware paradigm with plugins, leaving middleware kinds of setups to users who are embedding horizon

How about exposing a wrapper around the horizon server with limited functionality? (It'd be great if this wrapper was isomorphic to the horizon client, but I don't know if that's doable.)

Also, as I noted in #373, I really think authentication and validation should be implemented through plugins hooking into onconnect and oninsert/onupdate events, respectively:

// plugin

module.exports = function(hz) { // taking a wrapped hz server
    hz.on("connect", creds => {
        if (!hz.find({user: creds.user, pw: creds.pw}))
            // A special error that is magically propagated to the client, like Meteor's Meteor.Error   
            throw new hz.Error("invalid-creds", "Incorrect username or password.")
    })
}

// client

hz.connect({user: "erdogan", pw: "merkel1234"})

[marshall007]

Specifically, I think we should avoid the middleware paradigm with plugins, leaving middleware kinds of setups to users who are embedding horizon

@deontologician I actually completely disagree. For one thing I think Node developers are used to this paradigm and it is adopted by virtually all popular server frameworks. More importantly, though, I think the flexibility it provides generally leads to a much richer plugin ecosystem. You can imagine plugins that simply provide additional abstractions for things like configuring validation or a plugin that would fully integrate PassportJS authentication.

These kinds of things would be nearly impossible to do under a structured and isolated plugin interface.

[mlucy]

I'm not sure doing argument validation in the client is necessarily a good idea. We'll have to add a special parameter for every type of validation we want to let people do (as compared to validating on the server, where they can just write whatever JS code they want), and the only benefit is that it reduces the time to first error by like 50ms.

(Also, I would guess that people will be doing validation on the server anyway, especially if their semantics are at all complicated.)

I would at least strongly consider making these arguments optional.

[deontologician]

I think it's reasonable that they be optional, but these client side restrictions are just the "general shape" restrictions we enforce on ast methods currently.

[mlucy]

How are we going to handle errors in plugins? Specifically:

• What happens if an exception is thrown from e.g. a request handler?
• Is there a way for plugins to report that they're experiencing internal problems and should be deactivated?
• Is there a way for plugins to log warnings and errors?
• If a plugin fails, should we automatically try to reload it, or just leave it disabled? (Should we give plugins a way to request that they be reloaded to reset e.g. any connection state they might have that's messed up?)

[mlucy]

Is it safe to call this multiple times (e.g. if we're briefly disconnected from RethinkDB and the connection they got the first time is invalidated)?

[deontologician]

This is in general a hard thing, since we don't provide resumable changefeeds. I'd say we should expect dispose() to be called before activate is called a second time

[deontologician]

What happens if an exception is thrown from e.g. a request handler?

• Horizon should remain stable and return an error response from a request handler, a 500 response from a route handler, and print an error message on the command line

Is there a way for plugins to report that they're experiencing internal problems and should be deactivated?

Open to suggestions on this one

Is there a way for plugins to log warnings and errors?

• we should explicitly expose the logger (possibly with a log prefix) to plugins

If a plugin fails, should we automatically try to reload it, or just leave it disabled? (Should we give plugins a way to request that they be reloaded to reset e.g. any connection state they might have that's messed up?)

This is interesting. It seems like it probably shouldn't be a plugin setting, but some general policy. Given that the plugin in question is buggy, it sort of implies the author of the plugin doesn't know where the bug is, so it's not clear whether it's ok to restart it or not. (Say it crashes before or after some system resource is allocated, it might or might not be a good idea to restart it)

[deontologician]

Ok, at this point, the RFC is pretty big, if you're going to comment, please do it in Reviewable, where I can keep track of things better.

[abastardi]

Review status: 0 of 1 files reviewed at latest revision, 15 unresolved discussions.

---

rfcs/plugins.md, line 5 [r6] (raw file):

The plugin system I'm proposing isn't for node developers, it's for people using horizon in serverless apps who would like to get additional functionality. ...I think it would be a nice bonus to make it easy for node developers to be able to use horizon plugins to save some work, but it's a secondary or tertiary goal here.

It might help to make the above more explicit in the RFC, as that was not entirely clear (particularly as there is currently no documentation on how one could go about writing a custom backend that does the things listed in this RFC).

It would also be helpful, perhaps as a separate RFC, to articulate proposed changes to the backend API that will (a) be needed to enable the plugin functionality, and (b) make it easier to develop custom backend functionality (via embedding Horizon rather than via plugins). For example, will any hooks be added to the request-response cycle? Will it be possible to create or add to rulesets programmatically as well as incorporate custom validation methods? Right now there is the add_request_handler method, but there doesn't appear to be much that enables more fine-grained control over any of the standard request handlers. The roadmap includes, "Backend -- an API/protocol to integrate custom backend code with Horizon server/client-libraries" -- it would be great to see this fleshed out further.

---

rfcs/plugins.md, line 16 [r6] (raw file):

 - Worker processes ([#368](https://github.com/rethinkdb/horizon/issues/368))
 - Scheduled tasks ([#311](https://github.com/rethinkdb/horizon/issues/311))

It might help to explain a bit how plugins can be used to achieve some of the above, particularly things like custom authentication methods, server-side rendering, and scheduled tasks.

---

Comments from Reviewable

[deontologician]

Review status: 0 of 1 files reviewed at latest revision, 15 unresolved discussions.

---

rfcs/plugins.md, line 5 [r6] (raw file):

It might help to make the above more explicit in the RFC

I agree, sorry for the confusion

It would also be helpful, perhaps as a separate RFC, to articulate proposed changes to the backend API [...]

I think we should do this once this plugin api is implemented, as well as propose a proper solution for middleware in embedded horizon ala #24, since it'll entail refactoring that @Tryneus will be in the best position to make decisions on as he's writing it. Once plugins are in, it'll be easier to do some minor changes to the details to create an api that will be nice to use.

---

rfcs/plugins.md, line 16 [r6] (raw file):

Previously, abastardi wrote…

It might help to explain a bit how plugins can be used to achieve some of the above, particularly things like custom authentication methods, server-side rendering, and scheduled tasks.

This is a good idea too, but not sure if I'll have time to flesh these out before we start writing the plugin api. It seems like it's converging pretty quickly

---

Comments from Reviewable

[deontologician]

@mlucy @Tryneus see further updates in 599f5ea

[deontologician]

I still don't think we've addressed logging or any mechanism for a plugin to declare it needs to be restarted (deactivated then reactivated)

[marshall007]

Another thing I just thought of is that plugins exposing requests or httpRoutes might need a mechanism for making authenticated requests using OAuth credentials from the current user's session. For example, if users authenticate using GitHub, it's likely that the HTTP routes and WS requests you want to expose need to make authenticated calls to the GitHub API on behalf of the user.

[deontologician]

@bopjesvla made some relevant comments on this proposal in #607 #607 (comment).

Also, I think we should perhaps provide a plugins metadata table where plugins can preserve their state. Thoughts @Tryneus ?

[deontologician]

@marshall007 What kind of credentials would be needed? I don't think we store anything other than "this user was authenticated". Would we need #216 ?

[marshall007]

@deontologician I just mean the credentials obtained from the OAuth handshake. Not the user account/profile info, but it's the same credentials you'd use to make that request too.

[bopjesvla]

oh whoops

Plugins should also be able to alter inserted documents. That'd give way for plugins to add stuff like:

Random API keys to every created user
Document creation and modification metadata to documents in specified collections
Estimated location based on IPs
Right now, all data in a horizon app has to come from the untrusted client. That's simply not workable.

I agree, but I think this should be done by defining new terminals, like hz.createUserWithApiKey(...) or hz('foo').storeSpecial({...}), so that clients are clear which plugin is responsible for the behavior.

What if I want to use the API-key-adding capabilities of one plugin and the location-adding capabilities of another?

The plugin responsible for the behaviour is either the one named horizon-plugin-add-api-key or the one named horizon-plugin-random that required the user to put something like

[plugins.random.users.apikey]

length = 32

in config.toml. I don't think plugins surprising users by adding untraceable data will be the issue you're making it out to be.

[bopjesvla]

What if I want to use the API-key-adding capabilities of one plugin and the location-adding capabilities of another?

To be clear, this for the same document at the same insert.

[deontologician]

We also need to allow plugins to add an entry to horizon/auth_methods which this RFC doesn't currently allow. @Tryneus might have an opinion on what we need for that

[dalanmiller]

"the"

[deontologician]

Review status: 0 of 1 files reviewed at latest revision, 17 unresolved discussions.

---

a discussion (no related file):
Things that need to be passed to activate in some way:

• metadata.userTable, a reql Table Ast object pointing to the user table
• metadata.projectDb, a reql Db Ast object pointing to the horizon project database

---

Comments from Reviewable

[deontologician]

Review status: 0 of 1 files reviewed at latest revision, 17 unresolved discussions.

---

rfcs/plugins.md, line 156 [r8] (raw file):

Previously, dalanmiller (Daniel Alan Miller) wrote…

"the"

This is supposed to be the server metadata object. I think that may be a bad idea though, it should probably be a plugin-specific object.

---

Comments from Reviewable