Add unsecured /health endpoint, remove auth checks from isLeader#5087
Add unsecured /health endpoint, remove auth checks from isLeader#5087himanshug merged 2 commits intoapache:masterfrom
Conversation
| "/overlord/false" | ||
| "/overlord/false", | ||
| "/health", | ||
| "/druid/coordinator/v1/isLeader" |
There was a problem hiding this comment.
I think you need to include the overlord isLeader URL too. oops, no you don't, that's in a different file.
There was a problem hiding this comment.
Instead of putting /health and /druid/coordinator/v1/isLeader here (& similar in other CLI files) I suggest adding something in the resource itself that just sets the "yeah, I checked myself" flag. Probably using a new helper in AuthorizationUtils. It's better because it's more local -- it's really easy for someone reviewing the http code to miss that the security check is skipped here.
There was a problem hiding this comment.
I could move the authorization check into the resource, but to skip authentication I'd still need to setup the UnsecuredResourceFilter with the unsecured paths (since the authentication checks happen before the request hits the resource handler methods).
Maybe another approach for authentication could be to have the resources with unsecured endpoints register with some singleton injected registry object that UnsecuredResourceFilter checks (and have UnsecuredResourceFilter apply to all paths), but I'd rather handle that in a separate PR.
There was a problem hiding this comment.
Ok, that makes sense. In that case I'd suggest leaving things as you have them here, but adding a comment in the resource file saying that this file has it listed as unsecured.
Really, I'm just concerned that a reader of the http code is going to be confused about where the security check is. I was suggesting a programmatic approach but a comment works too.
There was a problem hiding this comment.
Added comments on the resources with these unsecured paths, pointing to the service initialization code
|
|
||
| /** | ||
| */ | ||
| @Path("/health") |
There was a problem hiding this comment.
Could you put this at /status/health? It would simplify the top level url structure a bit. It could be part of StatusResource.
There was a problem hiding this comment.
Moved this to /status/health and StatusResource
jon-wei
force-pushed
the
status_endpoints
branch
from
1cf4af2 to
636b831
Compare
…che#5087) * Add unsecured /health endpoint, remove auth checks from isLeader * PR comments
…che#5087) * Add unsecured /health endpoint, remove auth checks from isLeader * PR comments
3 participants
This PR adds a new /health endpoint on all services, intended for external health checks issued to Druid services.
This endpoint is unsecured, as external health checks do not necessarily support attaching authentication credentials (e.g., those issued by Amazon ELB).
This PR also removes auth checks from the "/isLeader" endpoints on coordinators and overlords, for similar external compatibility reasons.