notify/webhook: Fix crash on errors when url_file is used (#3798)

[zecke]

When using url_file the conf.URL will be nil and when an error occurs we will panic. Given that the URL is considered a secret, let's just remove the custom details func.

[grobinson-grafana]

Good find that URL is not redacted. I understand that this change removes both the response and URL when the server returns HTTP 500. Something to keep in mind is that <redacted> is still shown for errors:

ts=2024-04-10T09:03:42.464Z caller=notify.go:851 level=warn component=dispatcher receiver=webhook integration=webhook[0] aggrGroup={}:{} msg="Notify attempt failed, will retry later" attempts=1 err="Post \"<redacted>\": dial tcp 127.0.0.1:8080: connect: connection refused"

It would be nice if we could either show <redacted> for both 5xx responses and errors or remove it from both entirely. What do you think?

[zecke]

My understanding was that we continue to include the response body (if present):

alertmanager/notify/util.go

Line 241 in 14cbe63

details = readAll(body)

I think the remaining comes from the notify.RedactURL code that handles net/url.Error to remove the URL. What is the benefit of further modifying this?

[grobinson-grafana]

I think the remaining comes from the notify.RedactURL code that handles net/url.Error to remove the URL. What is the benefit of further modifying this?

I thought it would be nice if we can make the error message consistent. For example, how it works right now is if the server returns a 5xx error the URL is logged. But if the error is a url.Error (for example, the server cannot be reached) the URL is <redacted>.

Your fix removes the URL in the case of 5xx error – but should we also remove <redacted> in the case the server cannot be reached? It seems to serve no purpose and then the error message would be consistent for both cases. We could also add <redacted> back to this error message.

[zecke]

Is there any indication of someone expecting the error to follow the url.Error.Error() format (link)

I am torn on this. The formats are not the same today (one uses %s and the other %q) and then we don't have the URL readily available in the CustomDetailsFunc. Just printing "<redacted>" seems misleading as we claim to have information that we don't, changing the API to pass the URL through and then just print "<redacted>" seems odd too.

At the same time modifying notify.RedactURL to change "<redact>" to nil has impact on every other receiver.

Am I missing something?

[grobinson-grafana]

Is there any indication of someone expecting the error to follow the url.Error.Error() format (link)

I am torn on this. The formats are not the same today (one uses %s and the other %q) and then we don't have the URL readily available in the CustomDetailsFunc. Just printing "<redacted>" seems misleading as we claim to have information that we don't, changing the API to pass the URL through and then just print "<redacted>" seems odd too.

At the same time modifying notify.RedactURL to change "<redact>" to nil has impact on every other receiver.

Am I missing something?

It's OK – I realized I'm distracting the actual contribution which is fixing a panic. I'm going to test this now and let @gotjosh know.

[grobinson-grafana]

I've tested it and looks good to me! Thank you for your contribution! 👍

Here is the log output for the different cases I tested:

HTTP 5xx

ts=2024-04-12T09:22:50.198Z caller=notify.go:851 level=warn component=dispatcher receiver=test integration=webhook[0] aggrGroup={}:{} msg="Notify attempt failed, will retry later" attempts=1 err="unexpected status code 500"

Connection refused

ts=2024-04-12T09:23:07.309Z caller=notify.go:851 level=warn component=dispatcher receiver=test integration=webhook[0] aggrGroup={}:{} msg="Notify attempt failed, will retry later" attempts=9 err="Post \"<redacted>\": dial tcp 127.0.0.1:8080: connect: connection refused"

HTTP 5xx with "Hello, world!" in the response

ts=2024-04-12T09:23:49.522Z caller=notify.go:851 level=warn component=dispatcher receiver=test integration=webhook[0] aggrGroup={}:{} msg="Notify attempt failed, will retry later" attempts=11 err="unexpected status code 500: Hello, world!"

[gotjosh]

LGTM

[ryan-dyer-sp]

@zecke We just hit this issue and see its merged but not released yet. Is there a workaround for this configuration wise until the next version of alertmanager is released?