Enable extensible middleware for admin API #1327
Conversation
We need to inject in the gin middleware the function which allows to load and extend the middleware with custom ones. Fixes: #1326
| @@ -1090,6 +1090,8 @@ func (s *Server) bindHandlers(ctx context.Context) { | |||
| admin := s.AdminRouter | |||
| // now for extensible middleware | |||
| engine.Use(s.rootMiddlewareWrapper()) | |||
| // extensible middleware for the admin API | |||
| admin.Use(s.apiMiddlewareWrapper()) | |||
There was a problem hiding this comment.
this is used for all /v2 endpoints, should we make an adminMiddlewareWrapper ? I admit it's a bit unfortunate to have 3 different layers of middleware but they all seem like separate concerns. it seems like something worth discussing at least, it's just a bit of a hair ball I think we can sort out.
enumerating for purpose of discussion. there's currently 2 kinds of middleware:
- rootMiddleware - runs over any endpoint except the admin endpoints (notably,
/and runner endpoints such as hybrid (the living dead api) and triggers and invoke, not found / no method and any user added routes) - apiMiddleware - runs over any endpoint with /v2, just the CRUD for resources, not the runner endpoints or anything else in the root list
it seems logical at least to have a separate type of middleware for just handling the admin endpoints since this doesn't exist, otherwise we're expecting users to add facilities to wrap api or root middleware with checking the http route itself to make sure it's an admin endpoint in order to run admin logic against it - presumably, it's different than the API middleware logic that would normally run in that path. at least, it seems a little hairy to use api middleware as the admin middleware as well, as users will have to manually handle the set of admin routes which aren't rooted with any kind of pattern to make that easy (/metrics, /debug, etc).
one thing i'm not extremely keen on i guess is that root middleware is basically dual purpose for being runner middleware (invoke, http triggers) as well as root middleware. I'm also not incredibly keen on the way we've done middleware as it's really cumbersome at present (we could just allow defining a handler func that takes a handler on server creation and let users define middleware this way instead of all our boilerplate for adding middlewares to a server) - maybe need to digress.
I could see various configurations that 'make sense' to me, would be nice to get some thoughts, and I know we could short cut to get something working if it's really urgent but wrapping admin with api doesn't seem very wise I don't think it's expected behavior or a very clear contract for api middleware itself. additionally, maybe or maybe not a can of worms, open to making middleware a bit easier to munge around which would make adding levels of middleware less daunting perhaps - open to discuss. anyway, i see some blend of the following making sense:
middleware levels:
- root - covers everything except admin
- admin - covers only admin endpoints
- runner - covers invoke and http triggers
- api - covers resource CRUD (/v2)
We need to inject in the gin middleware the function which allows to
load and extend the middleware with custom ones.
Fixes: #1326