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.

Sign up for GitHub

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

Jump to bottom

Slack Plugin - server side exception handling for invalid webhook url #634

Closed
wants to merge 1 commit into from
Closed

Slack Plugin - server side exception handling for invalid webhook url #634

wants to merge 1 commit into from

Conversation

lingpri
Copy link
Contributor

@lingpri lingpri commented Nov 4, 2020 •
edited
Loading

Given : the user creates slack notification with a semi invalid URL like below https://hooks.slack.com/services/xxxxx/xxxxxx/xxxxxxx,
When : A event definition is configured with the above mentioned slack event notification.
Then : A PermanentNotificationException is seen in the graylog server logs and a notification is signalled in the UI.

addresses #632, with a server side solution.

Screenshots:
slack_PEN

2020-11-04 11:00:29,822 ERROR: org.graylog.scheduler.JobExecutionEngine - Job execution error - trigger=5fa2de2dd6679506fa1ea969 job=5fa2d048929df006d1be9092
org.graylog.scheduler.JobExecutionException: Failed permanently to execute notification, giving up - <5fa2d048929df006d1be9091/slack notification/slack-notification-v1>
	at org.graylog.events.notifications.EventNotificationExecutionJob.execute(EventNotificationExecutionJob.java:156) ~[classes/:?]
	at org.graylog.scheduler.JobExecutionEngine.executeJob(JobExecutionEngine.java:166) ~[classes/:?]
	at org.graylog.scheduler.JobExecutionEngine.lambda$handleTrigger$2(JobExecutionEngine.java:144) ~[classes/:?]
	at com.codahale.metrics.Timer.time(Timer.java:151) ~[metrics-core-4.1.9.jar:4.1.9]
	at org.graylog.scheduler.JobExecutionEngine.handleTrigger(JobExecutionEngine.java:144) ~[classes/:?]
	at org.graylog.scheduler.JobExecutionEngine.lambda$execute$0(JobExecutionEngine.java:119) ~[classes/:?]
	at org.graylog.scheduler.worker.JobWorkerPool.lambda$execute$0(JobWorkerPool.java:110) ~[classes/:?]
	at com.codahale.metrics.InstrumentedExecutorService$InstrumentedRunnable.run(InstrumentedExecutorService.java:180) [metrics-core-4.1.9.jar:4.1.9]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [?:1.8.0_272]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [?:1.8.0_272]
	at com.codahale.metrics.InstrumentedThreadFactory$InstrumentedRunnable.run(InstrumentedThreadFactory.java:66) [metrics-core-4.1.9.jar:4.1.9]
	at java.lang.Thread.run(Thread.java:748) [?:1.8.0_272]
Caused by: org.graylog.events.notifications.PermanentEventNotificationException: Please verify your Slack Event Configuration (webhooks and channel) :: Expected successful HTTP response [2xx] but got [404]. https://hooks.slack.com/services/xxxxx/xxxxx/xxxxx
	at org.graylog.integrations.notifications.types.SlackEventNotification.execute(SlackEventNotification.java:101) ~[classes/:?]
	at org.graylog.events.notifications.EventNotificationExecutionJob.execute(EventNotificationExecutionJob.java:135) ~[classes/:?]
	... 11 more
Caused by: org.graylog.events.notifications.PermanentEventNotificationException: Expected successful HTTP response [2xx] but got [404]. https://hooks.slack.com/services/xxxxx/xxxxx/xxxxx
	at org.graylog.integrations.notifications.types.SlackClient.send(SlackClient.java:75) ~[classes/:?]
	at org.graylog.integrations.notifications.types.SlackEventNotification.execute(SlackEventNotification.java:88) ~[classes/:?]
	at org.graylog.events.notifications.EventNotificationExecutionJob.execute(EventNotificationExecutionJob.java:135) ~[classes/:?]
	... 11 more

@lingpri lingpri changed the title exception handling for invalid webhook url Slack Plugin - server side exception handling for invalid webhook url Nov 4, 2020
@lingpri lingpri requested a review from waab76 November 4, 2020 17:06
@lingpri lingpri self-assigned this Nov 4, 2020
@waab76
Copy link
Contributor

waab76 commented Nov 4, 2020

First: I am unsure why there is urgency to change the error handling in Slack notifications and I don't think this should be our highest priority at the moment.

Second: I agree that the Slack plugin needs some cleanup of error handling. However, I don't think this PR covers what's needed. The SlackClient should have its own exceptions rather than using TemporaryEventNotificationException and PermanentEventNotificationException and SlackEventNotification should handle the exceptions coming from SlackClient appropriately.

Third: Outside of emergency situations where a broken build needs to get fixed, I'm not going to approve PRs that add @Ignore annotations to JUnit tests instead of fixing those JUnit tests.

My recommendation is to close this PR, add a descriptive issue to our backlog, and proceed with other - more urgent - tasks.

@lingpri
Copy link
Contributor Author

lingpri commented Nov 4, 2020

First: I am unsure why there is urgency to change the error handling in Slack notifications and I don't think this should be our highest priority at the moment.

Second: I agree that the Slack plugin needs some cleanup of error handling. However, I don't think this PR covers what's needed. The SlackClient should have its own exceptions rather than using TemporaryEventNotificationException and PermanentEventNotificationException and SlackEventNotification should handle the exceptions coming from SlackClient appropriately.

Third: Outside of emergency situations where a broken build needs to get fixed, I'm not going to approve PRs that add @Ignore annotations to JUnit tests instead of fixing those JUnit tests.

My recommendation is to close this PR, add a descriptive issue to our backlog, and proceed with other - more urgent - tasks.

Thanks for taking your time to look into this. I will add them in the backlog.

@lingpri lingpri linked an issue Nov 4, 2020 that may be closed by this pull request
slack plugin - validate webhook url while creation and update #632
Open
@lingpri lingpri closed this Nov 4, 2020
@lingpri lingpri linked an issue Nov 4, 2020 that may be closed by this pull request
Slack Plugin - Code Refactoring #611
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@waab76 waab76 Awaiting requested review from waab76

Assignees

@lingpri lingpri

Labels
ready-for-review slack-plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

slack plugin - validate webhook url while creation and update Slack Plugin - Code Refactoring
2 participants
@lingpri @waab76