plugin: Handle panics from agent requests #785
Conversation
Closes #760. AFAIK this hasn't been an issue in the past, but as we're trying to improve reliability, it's good to get this out of the way before it becomes an issue. Note that this PR is quite minimal - expanding the existing tech debt we have around how the scheduler plugin handles HTTP requests. It's probably ok *enough* for now. I don't expect we'll be making too many changes to it in the near future. See also: #13. Tested locally by forcing it to panic on every request: diff --git a/pkg/plugin/run.go b/pkg/plugin/run.go index 007554a..6da7728 100644 --- a/pkg/plugin/run.go +++ b/pkg/plugin/run.go @@ -262,8 +262,10 @@ func (e *AutoscaleEnforcer) handleAgentRequest( } } - pod.vm.mostRecentComputeUnit = &e.state.conf.ComputeUnit - return &resp, 200, nil + panic(errors.New("test panic!")) + + // pod.vm.mostRecentComputeUnit = &e.state.conf.ComputeUnit + // return &resp, 200, nil } // getComputeUnitForResponse tries to return compute unit that the agent supports The change appears to work as intended.
| logger.Error(msg, zap.String("error", fmt.Sprint(err))) | ||
| finalStatus = 500 | ||
| w.WriteHeader(finalStatus) | ||
| _, _ = w.Write([]byte(msg)) |
There was a problem hiding this comment.
how about sending not just msg here, but msg: err?
There was a problem hiding this comment.
Hm, yeah. My impression was that it was better from a security perspective to not include the full error message (especially considering it's already visible in the logs), but tbh maybe it doesn't matter.
wdyt?
There was a problem hiding this comment.
Hmm, I see your concern. Although this is our internal API, so risk is not high.
But I guess, it can be left as is, it is easy to grep for logs. Maybe make the message more distinct, like "permit handler panicked" or else.
@areyou1or0 do you have an opinion on whether we should return the full error message, from catch-all exception handler?
There was a problem hiding this comment.
@Omrigan somehow I didn't get the notification for the mention, just seeing this now.
Yes, it's best practice to not reveal non-customised error message as this can disclose the details of underlying technology. Low severity but valid.
Closes #760.
AFAIK this hasn't been an issue in the past, but as we're trying to improve reliability, it's good to get this out of the way before it becomes an issue.
Note that this PR is quite minimal - expanding the existing tech debt we have around how the scheduler plugin handles HTTP requests. It's probably ok enough for now. I don't expect we'll be making too many changes to it in the near future. See also: #13.
Tested locally by forcing it to panic on every request:
The change appears to work as intended.