Random thoughts on middleware

[binarymash]

Hey @TomPallister,

Just been looking through the code trying to understand how it all works. I've got a few thoughts about the middleware implementation...

1. Is there any reason for ExceptionHandlingMiddleware to not implement OcelotMiddleware?

2. Any reason for not defining abstract Task Invoke(HttpContext context) on OcelotMiddleware? Adding this would give extra compile-time check that middleware implementors are on the correct path.

3. It took me a while to understand what parts of the code were middleware, and what parts of the code were infrastructure stuff. Given that the middleware are effectively the feature slices of the project, and are where all the magic happens, I feel it could do with being a bit more distinct from the rest of the supporting code. Any opinion on grouping all the middleware together as individual folders under the an Ocelot.Middleware namespace? Eg,

+-Ocelot
  +-Middleware
  | +-Authentication
  | | +- ...
  | |
  | +-Authorisation
  | | +- ...
  | | 
  | +-...
  |
  +-other stuff

This structure would also provide a path to providing a more modular package-based approach to middleware in the future, with a common naming convention: optional middleware could be published in packages; the common Ocelot.Middleware.Whatever convention would make it easy to discover these packages; and say someone didn't want to use identityserver then they could choose to avoid pulling this package in.

If you think any of this is worth doing I'm happy to pick up the work on it.

[TomPallister]

@binarymash makes sense, I tried to stuff all the classes below a relevant feature such as Authentication has all the code for authentication and anything cross cutting I just dumped in a catch all..probably called Infrastructure.

I find the code quite hard to follow myself because it is not directly procedural in the sense that you can go this method calls that and that calls this. Due to the fact that anyone can add middleware or do what they want basically it could all fall apart quite quickly!!

More than happy for you to make those changes! You shouldn't have any nightmare merges.

The only feature Im thinking of doing is storing the config in Consul which gets me away from having to implement that raft consensus algorithm any time soon.

edit..the code is pretty ropey at times you will have to forgive :(

[binarymash]

Cool bananas. That's my Easter Sunday afternoon sorted out then :)

I noticed a few naming inconsistencies between the folders and the middleware classes. I'll have a look at making everything consistent.

Oh yeah there's definitely still a need for the cross cutting stuff, no problem with that.

[TomPallister]

@binarymash awesome thanks! I'm painting and decorating. It's harder than I thought to select a colour.

[binarymash]

My advice is just go with white ;)

[TomPallister]

Thats what Im trying to tell the Mrs! Nevermind.

[binarymash]

Ok, I think you might finish the decorating before I finish this... it's a bit more complex than I first thought, because I naively assumed that each of the middleware implementations was isolated from the others, but now I see that there's shared code between some of them, and some of them rely on each other. Some of this might be solved by extracting code out into a common model, and other parts might be fixed by removing leaky abstractions, and some might be solved by consolidation. Or maybe I'm wasting my time and it's never going to be squeezed into the shape I imagine :)

Basically, I need to think about this a bit more. Whatever, any code change here would be a breaking change to everyone anyway.

I've pushed up the refactoring so far - see https://github.com/binarymash/Ocelot/tree/feature/RestructureMiddleware

btw are you seeing unstable tests? Saw quite a lot of apparently random failures when building on the command line.

[binarymash]

Oops. Forgot to say, for future reference here's the dependencies I see between the middleware...

DownstreamRouteFinder is referenced by:

• DownstreamUrlCreator (UrlMatcher.IDownstreamPathPlaceholderReplacer, UrlMatcher.UrlPathPlaceholderNameAndValue)
• Authentication (DownstreamRoute.ReRoute())
• Authorisation (DownstreamRoute.ReRoute())
• Headers (DownstreamRoute, UrlMatcher.UrlPathPlaceholderNameAndValue)
• OcelotMiddleware (DownstreamRoute)

Request is referenced by:

• Requester (Request)

Requester is referenced by:

• Request (QoS.IQoSProvider, QoS.IQosProviderHouse)

RequestId is referenced by:

• Request (RequestId)

Headers is referenced by:

• Responder (IRemoveOutputHeaders)

[TomPallister]

@binarymash sorry! Yeh some of the middleware are dependent on others I think I mentioned above it might better to just refactor them into one procedural style stack! Not sure.

Obviously they cannot be pulled apart without some refactoring. Also I'm not sure you get any value from them unless they are a together so no point being middleware. They all need that scoped data repository which is a crap dependency imo and that would go away if they were refactored into one stack. There is quite a lot that annoys me.

Sorry the code is a bit crap! I just cracked on trying to get something working.

[binarymash]

Just been looking through the code a bit more to understand what each of the middleware does. So, in general, they do one of the following...

1. Modifying the request that we've received (downstream route finder, request id, claims builder, request headers, query string, load balancing, downstream url creator)

2. Applying some cross-cutting functionality (exception handler, authentication, authorisation, output cache, requester)

3. Modifying the response we return (responder)

Actually, a middleware could potentially do a combination of any of these things.

I think the middleware approach is fine, but I think what makes things a bit complex at the moment is that for all those middleware that modify the request, we store their changes individually, and then try to gather them all together just before we send the request. Maybe there's a good reason for doing that - I didn't go through all the hard work of writing it all, so you're in a better position than me to answer :)

It feels like things might be easier if we turned things round the other way... rather than trying to assemble a request at the very end of the pipeline, how about, when the request first came in, we immediately use this to create an initial version of the downstream request, and then each of the middleware directly mutate this request? This means that each middleware would not need to know at all about the implementation details of the other middleware - they just do whatever they need to do directly on the downstream request. This would prevent the need for leaking middleware implementation into the OcelotMiddleware base class. At the end of the pipeline, the requester just sends whatever the current downstream request is.

In effect, this is kind of what you're doing with the response to the downstream request... the requester immediately stores the response in the IRequestScopedDataRepository, and then other middleware (for example, the output cache) use this to do whatever they want with it.

[binarymash]

I've hacked together a poc of this which decouples each bit of middleware... work in progress, not fully wired up yet so no idea if it works, and I've broken lots of tests, but it compiles ;) My initial thoughts are that it reduces the amount of code and complexity. I'll try to spend a bit more time on this in the next day or two to see if it is worth pursuing.

https://github.com/binarymash/Ocelot/commits/feature/RequestMutation

[TomPallister]

@binarymash I'll have a look tomorrow! Thanks man. Went with jasmine white in the end.

[TomPallister]

@binarymash we keeping this open? just doing some house cleaning?

[binarymash]

Yeah now that #88 has made the middleware pretty much independent of each other I want to try this again. Job for the bank holiday weekend :)

[TomPallister]

Gonna close this one as no longer using asp.net middleware and passing an Ocelot specific context tho its name might not be correct right now "DownstreamContext" doesnt seem right anymore haha.