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.

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

Slack chat.postMessage errors are not caught #1048

Open
ordovicia opened this issue Feb 19, 2025 · 3 comments · May be fixed by #1086
Open

Slack chat.postMessage errors are not caught #1048

ordovicia opened this issue Feb 19, 2025 · 3 comments · May be fixed by #1086

Comments

@ordovicia
Copy link

postMessage function assumes that HTTP responses have a meaningful status code (2xx for success, 4xx or 5xx for error).

if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusAccepted && resp.StatusCode != http.StatusCreated {

Slack chat.postMessage API does not satisfy this assumption.
https://api.slack.com/methods/chat.postMessage
Seems that it always responds with 200 OK.
ok and error fields in response body JSON indicate an error, if any.

Thus, Slack errors are not caught by postMessage function, and will not appear in notification-controller logs nor Alert resource Events.

@matheuscscp
Copy link
Member

matheuscscp commented Feb 19, 2025

@ordovicia are you willing to contribute this fix?

The way I see it, we should create an option for that fuction that we can pass at the end of the args through reqOpts ...requestOptFunc. By passing this option only in the slack provider we would modify the behavior of this function only for slack. We may need to improve the definition of an option as it only accepts a pointer to retryablehttp.Request:

type requestOptFunc func(*retryablehttp.Request)

So to pass a boolean or something we would probably need an options struct and modify the function type above to accept both things, something along those lines...

@ordovicia
Copy link
Author

I will try to contribute to a fix in a few months.
If anyone is interested in this, please feel free to take over.

@ordovicia ordovicia linked a pull request Mar 25, 2025 that will close this issue
@ordovicia
Copy link
Author

Hi, I am preparing a fix in #1086
It's still in draft, but could you take a look?
I would like to make sure that we agree on the implementation direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants