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.

Sign up for GitHub

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
Open

Enable extensible middleware for admin API #1327

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions api/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)


engine.GET("/", handlePing)
admin.GET("/version", handleVersion)
Expand Down