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

Make unhandled webhook events return 200 instead of 501 #257

Merged
merged 2 commits into from
Apr 13, 2022

Conversation

nickmisasi
Copy link
Contributor

Summary

Zoom sends webhooks for various reasons. The Mattermost Zoom Plugin only handles meeting.ended events. For all others, a 501 - Not Implemented status code is returned. Zoom expects a 200 or 204 response, otherwise it assumes the delivery failed and it will try again at a future time. This means that by returning a 501, Zoom will re-queue the webhook to be sent again - exasperating the issue.

This has been causing us to go over alert thresholds in the Community mattermost server due to it being a 5xx error. My proposal here is to simply return a 200 in the event we don't care about a specific webhook, so that Zoom marks the delivery as successful and doesn't attempt further.

Ticket Link

n/a

@codecov
Copy link

codecov bot commented Apr 11, 2022

Codecov Report

Merging #257 (b8735c3) into master (8a5a54d) will not change coverage.
The diff coverage is 0.00%.

@@          Coverage Diff           @@
##           master    #257   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           8       8           
  Lines         900     900           
======================================
  Misses        900     900           
Impacted Files Coverage Δ
server/http.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a5a54d...b8735c3. Read the comment docs.

@nickmisasi nickmisasi changed the title Make webhook event types that are unhandled return 200 instaed of 501 Make webhook event types that are unhandled return 200 instead of 501 Apr 11, 2022
@nickmisasi nickmisasi changed the title Make webhook event types that are unhandled return 200 instead of 501 Make unhandled webhook events return 200 instead of 501 Apr 11, 2022
@nickmisasi nickmisasi added the 2: Dev Review Requires review by a core committer label Apr 11, 2022
@hanzei hanzei requested review from mickmister and removed request for hanzei April 11, 2022 19:36
@hanzei hanzei added the 3: QA Review Requires review by a QA tester label Apr 11, 2022
Copy link

@stylianosrigas stylianosrigas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mickmister mickmister removed the 2: Dev Review Requires review by a core committer label Apr 13, 2022
@mickmister mickmister merged commit a986bb4 into master Apr 13, 2022
@mickmister mickmister deleted the unhandled-webhooks-return-200 branch April 13, 2022 15:45
@mickmister mickmister added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Apr 13, 2022
@mickmister
Copy link
Contributor

@nickmisasi Looks like we have an extra event configured for our Zoom instance Participant/Host joined meeting. I've removed this event just now, so we shouldn't be seeing this occur anymore in the meantime.

Thanks for implementing this fix 👍

image

@dipak-demansol
Copy link

@mickmister You removed Qa review label but still my name is there in the review list so could you pls tell me, should i test it or not?

@mickmister
Copy link
Contributor

@dipak-demansol The change was trivial enough that I deemed it good to merge without QA testing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants