Proposal: introduce context provider command support

[adnanh]

Sometimes it might be necessary to extend webhook with custom logic before the hook gets invoked.

In order to support that in modular case I propose implementing a "context provider command" support in webhook.

The context provider command would be executed whenever the hook gets matched. It would receive via STDIN a JSON representation of an object with following structure:

{
  "hookID": "<ID OF THE HOOK THAT GOT MATCHED>",
  "method": "<METHOD USED TO TRIGGER THE WEBHOOK (i.e. GET, POST, PATCH, PUT...)",
  "body": "<RAW BODY>",
  "headers": { "<HEADER>": ["<VALUE>", "<VALUE>", ... ], ... },
  "query": { "<PARAM>": "<VALUE>", ... },
  "host": "<HOST THAT CLIENT REQUESTED>",
  "URI": "<ROUTE THAT CLIENT REQUESTED>",
  "remoteAddress": "<IP AND PORT OF THE CLIENT>"
}

The context provider command is expected to output a valid JSON representation of an object which would be then stored in a special source named "context".

The special source would be usable in all places where one is able to use "payload" source.

This would allow users to implement custom payload parsers to extract required fields, or inject custom values that can be computed in runtime (i.e. custom signature verification, etc...) and pass them along to webhook's trigger rules etc...

The working implementation is available at the feature/context-provider-command branch. What's missing is documentation and tests.

Any suggestions or comments?

@moorereason

[moorereason]

I like the idea that users can solve complex requirements without having to extend webhook in some weird way or at least not have to wait on us to extend webhook to solve their problem.

I do have one suggestion. You're proposing a kind of pre-processor feature. Perhaps call it prehook instead of context? In my mind the "context" is largely what you're proposing to pass to this command (namely, contextProviderCommandStdin). This proposal is adding to the "context" for the trigger rules to evaluate later. Saying that we're adding context to the context sounds odd to my ear.

[AndrewSav]

Not all character codes are allowed as a character in a string according to json specification. I think passing byte array (I'm talking about body) as a string is not a very good idea. But if you insist on it, I'd like an options that would pass base64 encoding of the body instead.

[adnanh]

@AndrewSav correct, we should default to base64 encoded string.

[adnanh]

@moorereason Implemented the suggestions, does it look better now? :-)

[moorereason]

Changes look good, but I've got more feedback. :smiley:

• [x] I'd use RemoteAddr instead of RemoteAddress to stay in line with the http package. Everyone knows what that means, right?
• [x] The Request.RemoteAddr is the "IP:port", not just the IP. Docs are incorrect.
• [ ] Add tests.
• [ ] An example in the docs would be helpful.
• [ ] A mention of the feature in the README would be nice.

[adnanh]

Merged latest development to the feature branch. I'm hoping to write some examples during this week. Any help on tests and docs would be appreciated :-)

I'm thinking this could be a good way forward to support esoteric scenarios and custom stuff by maintaining a standard library of functions and helpers for payload handling and processing in the pre-hook commands...

[moorereason]

Is a successful prehook required for successful hook execution? I would assume so in most scenarios. Why define a prehook if you don't care if it was successful? If we can assume they're required to be successful, several error-handling steps in hookHandler could write errors to the http.ResponseWriter and return, which would eliminate several else-statements and unnecessary nesting.

One thing that often bugs me is how complex hookHandler is. We don't have to fix that here, but at some point I'd like to clean that up by moving the different sections of that function into separate functions.

Overall, it looks good. I look forward to seeing your examples. If I have time, I'll try to help out with tests & docs. 👍

[adnanh]

That is a good question! Come to think about it, it makes much more sense to make it required.

You're also absolutely right about the hookHandler complexity. We could however get this in first, and then clean it all up.

[adnanh]

Also, I'm thinking about including "response-headers", "response-http-status-code" and "response-message" in trigger rules, so when a rule node fails, it would override the default response message.

This would allow implementing stuff like rate-limiting with pre-hook commands where the pre-hook command checks for currently running processes, and then injects a field in the context, which the hook would have in a trigger rule.

[adnanh]

Updated existing hook tests with the new context source.

[adnanh]

@moorereason Rebased the branch with latest changes from development. Still missing:

• [x] implement early abort if prehook command fails
• [x] maybe rename Context to PreHook? would that make more sense instead of the rather arbitrary and ambiguous "Context"? or to something else, not sure...
• [ ] documentation
• [ ] examples

[moorereason]

maybe rename Context to PreHook? would that make more sense instead of the rather arbitrary and ambiguous "Context"? or to something else, not sure...

I agree. I would prefer that the only "context" usage we keep is the name of the hook.PreHookContext type since it really does act as the pre-hook context internally. I would use const SourcePreHook string = "pre-hook" (which will go naturally with the pre-hook-command option). Request.Context would be Request.PreHook. And so on...

[adnanh]

Yep, exactly what I had in mind

[adnanh]

Merged latest development changes, and imeplemented early abort if prehook command fails for any reason. Also renamed Context to PreHook where applicable.

Need docs & examples.

[adnanh]

Added some docs and an example to get the request IP :-)