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

Enable extensible middleware for admin API #1327

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

reclaro
Copy link
Contributor

@reclaro reclaro commented Nov 27, 2018

We need to inject in the gin middleware the function which allows to
load and extend the middleware with custom ones.

Fixes: #1326

We need to inject in the gin middleware the function which allows to
load and extend the middleware with custom ones.

Fixes: #1326
Copy link
Contributor

@rdallman rdallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see line comment ^ - thanks for adding, this was a big omission when adding admin endpoints.

@@ -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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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.

2 participants