Allow consul debug on non-ACL consul servers #15155
Conversation
github-actions
bot
added
theme/api
| // If enableDebug or ACL enabled, register wrapped pprof handlers | ||
| if enableDebug || !s.checkACLDisabled() { | ||
| handlePProf("/debug/pprof/", pprof.Index) | ||
| handlePProf("/debug/pprof/cmdline", pprof.Cmdline) | ||
| handlePProf("/debug/pprof/profile", pprof.Profile) | ||
| handlePProf("/debug/pprof/symbol", pprof.Symbol) | ||
| handlePProf("/debug/pprof/trace", pprof.Trace) | ||
| } |
There was a problem hiding this comment.
I thought it would make more sense for the handler registration to be guarded by the if condition here rather than allowing the endpoints to be exposed but returning a 401 every time.
Would this cause any side effects?
There was a problem hiding this comment.
The only one I can think of is that it will alter the HTTP status code send back when enable debug is false and acls are not on. I think it will be a 404 instead of a 403/401. I think that should be fine though.
| @@ -209,13 +209,6 @@ func (s *HTTPHandlers) handler(enableDebug bool) http.Handler { | |||
| var token string | |||
| s.parseToken(req, &token) | |||
|
|
|||
| // If enableDebug is not set, and ACLs are disabled, write | |||
| // an unauthorized response | |||
| if !enableDebug && s.checkACLDisabled() { | |||
There was a problem hiding this comment.
Instead of checking these conditions per request, I've moved the check below to conditionally register pprof handlers instead.
| if c.captureTarget(targetProfiles) { | ||
| dir, err := makeIntervalDir(c.output, c.timeNow()) |
There was a problem hiding this comment.
avoids making empty directories if this branch is never entered
| @@ -274,6 +273,22 @@ func (c *cmd) prepare() (version string, err error) { | |||
| c.capture = defaultTargets | |||
| } | |||
|
|
|||
| // If EnableDebug is not true, skip collecting pprof | |||
| enableDebug, ok := self["DebugConfig"]["EnableDebug"].(bool) | |||
There was a problem hiding this comment.
kisunji
added
backport/1.12
| // If enableDebug or ACL enabled, register wrapped pprof handlers | ||
| if enableDebug || !s.checkACLDisabled() { | ||
| handlePProf("/debug/pprof/", pprof.Index) | ||
| handlePProf("/debug/pprof/cmdline", pprof.Cmdline) | ||
| handlePProf("/debug/pprof/profile", pprof.Profile) | ||
| handlePProf("/debug/pprof/symbol", pprof.Symbol) | ||
| handlePProf("/debug/pprof/trace", pprof.Trace) | ||
| } |
There was a problem hiding this comment.
The only one I can think of is that it will alter the HTTP status code send back when enable debug is false and acls are not on. I think it will be a 404 instead of a 403/401. I think that should be fine though.
kisunji
force-pushed
the
NET-1174-74937-consul-debug-cli-broken-on-non-acl-clusters
branch
from
7163f33
to
86972fa
Compare
kisunji
deleted the
NET-1174-74937-consul-debug-cli-broken-on-non-acl-clusters
branch
Description
#10273 had an unintended side-effect of crashing
consul debugCLI command if ACLs were disabled anddebug_enabledwas false.The correct behavior would be to disable the HTTP endpoints for profiling information but still allowing other debug profile targets like
logsandmetricsto run without issues.Testing & Reproduction steps
PR Checklist