[TT-12323] fix panic when webhook handler is disabled by jeffy-mathew · Pull Request #6334 · TykTechnologies/tyk

jeffy-mathew · 2024-06-10T13:23:03Z

User description

Description

fix panic when webhook handler is disabled

Related Issue

https://tyktech.atlassian.net/browse/TT-12323

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

I ensured that the documentation is up to date
I explained why this PR updates go.mod in detail with reasoning why it's required
I would like a code coverage CI quality gate exception and have explained why

PR Type

Bug fix, Tests

Description

Added error handling for when the webhook handler is disabled by introducing a new error variable ErrEventHandlerDisabled.
Modified the Init method in WebHookHandler to return the new error when the webhook is disabled.
Updated the corresponding test to check for the ErrEventHandlerDisabled error instead of no error.

Changes walkthrough 📝

Relevant files

Bug fix

event_handler_webhooks.go `Add error handling for disabled webhook handler` gateway/event_handler_webhooks.go Added error handling for disabled webhook handler. Introduced `ErrEventHandlerDisabled` error variable. Modified `Init` method to return `ErrEventHandlerDisabled` when webhook is disabled.	+6/-1

Tests

event_handler_webhooks_test.go `Update test for disabled webhook handler error` gateway/event_handler_webhooks_test.go Updated test to check for `ErrEventHandlerDisabled` error.	+1/-1

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

github-actions · 2024-06-10T13:23:50Z

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	2
🧪 Relevant tests	Yes
🔒 Security concerns	No
⚡ Key issues to review	None

github-actions · 2024-06-10T13:23:54Z

API Changes

--- prev.txt	2024-06-10 14:00:03.588701478 +0000
+++ current.txt	2024-06-10 14:00:00.288711149 +0000
@@ -7286,6 +7286,10 @@
     a future version.
 
 var (
+	// ErrEventHandlerDisabled is returned when the event handler is disabled.
+	ErrEventHandlerDisabled = errors.New("event handler disabled")
+)
+var (
 	ErrPoliciesFetchFailed = errors.New("fetch policies request login failure")
 )
 var (

github-actions · 2024-06-10T13:24:22Z

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add nil check for configuration to prevent potential panics It's recommended to add a check for the configuration being nil before accessing its properties. This can prevent potential panics due to nil dereference. gateway/event_handler_webhooks.go [65] -if w.conf.Disabled { +if w.conf != nil && w.conf.Disabled { Suggestion importance[1-10]: 9 Why: Adding a nil check for the configuration is important to prevent potential runtime panics, which can lead to more robust and stable code.	9
Enhancement	Add logging for errors to enhance debugging and error tracking Consider logging the error before returning it to provide more debugging information about why the event handler was disabled. gateway/event_handler_webhooks.go [68] +log.Error("Event handler disabled due to configuration: ", w.conf.Name) return ErrEventHandlerDisabled Suggestion importance[1-10]: 8 Why: Adding logging before returning the error can provide valuable debugging information and improve error tracking, making it a useful enhancement.	8
	Improve error handling by adding more context or using a custom error type Consider using a more specific error type or wrapping the error with additional context. Using `errors.New` directly with a generic message can make it harder to handle errors programmatically elsewhere in the code. You might want to create a custom error type or use `fmt.Errorf` to add more context. gateway/event_handler_webhooks.go [40] -ErrEventHandlerDisabled = errors.New("event handler disabled") +ErrEventHandlerDisabled = fmt.Errorf("event handler disabled: %w", someContextVariable) Suggestion importance[1-10]: 7 Why: The suggestion to use a more specific error type or add context to the error message can improve error handling and debugging. However, it is not crucial and the current implementation is functional.	7
	Enhance test to check for unintended modifications in handler configuration To ensure the test is robust, consider adding a check to confirm that the handler's configuration is not modified when the handler is disabled. This can help catch unintended side effects in the configuration logic. gateway/event_handler_webhooks_test.go [55] assert.True(t, h.conf.Disabled) +assert.Equal(t, expectedConfig, h.conf, "Handler configuration should not be modified when disabled") Suggestion importance[1-10]: 6 Why: The suggestion enhances the test by ensuring that the configuration remains unchanged, which can help catch unintended side effects. However, it is a minor improvement and not critical.	6

sonarqubecloud · 2024-06-10T14:12:48Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

jeffy-mathew · 2024-06-11T09:12:37Z

/release to release-5.4

jeffy-mathew · 2024-06-11T09:13:16Z

/release to release-5.4.0

tykbot · 2024-06-11T09:13:34Z

Working on it! Note that it can take a few minutes.

### **User description**  ## Description fix panic when webhook handler is disabled ## Related Issue https://tyktech.atlassian.net/browse/TT-12323 ## Motivation and Context  ## How This Has Been Tested     ## Screenshots (if appropriate) ## Types of changes  - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Refactoring or add test (improvements in base code or adds test coverage to functionality) ## Checklist    - [ ] I ensured that the documentation is up to date - [ ] I explained why this PR updates go.mod in detail with reasoning why it's required - [ ] I would like a code coverage CI quality gate exception and have explained why ___ ### **PR Type** Bug fix, Tests ___ ### **Description** - Added error handling for when the webhook handler is disabled by introducing a new error variable `ErrEventHandlerDisabled`. - Modified the `Init` method in `WebHookHandler` to return the new error when the webhook is disabled. - Updated the corresponding test to check for the `ErrEventHandlerDisabled` error instead of no error. ___ ### **Changes walkthrough** 📝 <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix </strong></td><td><table> <tr> <td> <details> <summary><strong>event_handler_webhooks.go</strong><dd><code>Add error handling for disabled webhook handler</code>                    </dd></summary> <hr> gateway/event_handler_webhooks.go <li>Added error handling for disabled webhook handler.<br> <li> Introduced <code>ErrEventHandlerDisabled</code> error variable.<br> <li> Modified <code>Init</code> method to return <code>ErrEventHandlerDisabled</code> when webhook is <br>disabled.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6334/files#diff-6587ad3f2629cfa6c84a71144127acd6cc7824e5141f0b4961848945a87e0198">+6/-1</a>      </td> </tr> </table></td></tr><tr><td><strong>Tests </strong></td><td><table> <tr> <td> <details> <summary><strong>event_handler_webhooks_test.go</strong><dd><code>Update test for disabled webhook handler error</code>                      </dd></summary> <hr> gateway/event_handler_webhooks_test.go - Updated test to check for `ErrEventHandlerDisabled` error. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6334/files#diff-1a2f8d6ab4443031b0f8486809453f6389291a6f625745fe8c65ac5cdb0e9621">+1/-1</a>      </td> </tr> </table></td></tr></tr></tbody></table> ___ > 💡 **PR-Agent usage**: >Comment `/help` on the PR to get a list of all available PR-Agent tools and their descriptions (cherry picked from commit 3e5963c)

tykbot · 2024-06-11T09:13:53Z

@jeffy-mathew Succesfully merged PR

tykbot · 2024-06-11T09:13:55Z

Working on it! Note that it can take a few minutes.

### **User description**  ## Description fix panic when webhook handler is disabled ## Related Issue https://tyktech.atlassian.net/browse/TT-12323 ## Motivation and Context  ## How This Has Been Tested     ## Screenshots (if appropriate) ## Types of changes  - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Refactoring or add test (improvements in base code or adds test coverage to functionality) ## Checklist    - [ ] I ensured that the documentation is up to date - [ ] I explained why this PR updates go.mod in detail with reasoning why it's required - [ ] I would like a code coverage CI quality gate exception and have explained why ___ ### **PR Type** Bug fix, Tests ___ ### **Description** - Added error handling for when the webhook handler is disabled by introducing a new error variable `ErrEventHandlerDisabled`. - Modified the `Init` method in `WebHookHandler` to return the new error when the webhook is disabled. - Updated the corresponding test to check for the `ErrEventHandlerDisabled` error instead of no error. ___ ### **Changes walkthrough** 📝 <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix </strong></td><td><table> <tr> <td> <details> <summary><strong>event_handler_webhooks.go</strong><dd><code>Add error handling for disabled webhook handler</code>                    </dd></summary> <hr> gateway/event_handler_webhooks.go <li>Added error handling for disabled webhook handler.<br> <li> Introduced <code>ErrEventHandlerDisabled</code> error variable.<br> <li> Modified <code>Init</code> method to return <code>ErrEventHandlerDisabled</code> when webhook is <br>disabled.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6334/files#diff-6587ad3f2629cfa6c84a71144127acd6cc7824e5141f0b4961848945a87e0198">+6/-1</a>      </td> </tr> </table></td></tr><tr><td><strong>Tests </strong></td><td><table> <tr> <td> <details> <summary><strong>event_handler_webhooks_test.go</strong><dd><code>Update test for disabled webhook handler error</code>                      </dd></summary> <hr> gateway/event_handler_webhooks_test.go - Updated test to check for `ErrEventHandlerDisabled` error. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6334/files#diff-1a2f8d6ab4443031b0f8486809453f6389291a6f625745fe8c65ac5cdb0e9621">+1/-1</a>      </td> </tr> </table></td></tr></tr></tbody></table> ___ > 💡 **PR-Agent usage**: >Comment `/help` on the PR to get a list of all available PR-Agent tools and their descriptions (cherry picked from commit 3e5963c)

tykbot · 2024-06-11T09:14:14Z

@jeffy-mathew Succesfully merged PR

…s disabled (#6334) [TT-12323] fix panic when webhook handler is disabled (#6334) ### **User description**  ## Description fix panic when webhook handler is disabled ## Related Issue https://tyktech.atlassian.net/browse/TT-12323 ## Motivation and Context  ## How This Has Been Tested     ## Screenshots (if appropriate) ## Types of changes  - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Refactoring or add test (improvements in base code or adds test coverage to functionality) ## Checklist    - [ ] I ensured that the documentation is up to date - [ ] I explained why this PR updates go.mod in detail with reasoning why it's required - [ ] I would like a code coverage CI quality gate exception and have explained why ___ ### **PR Type** Bug fix, Tests ___ ### **Description** - Added error handling for when the webhook handler is disabled by introducing a new error variable `ErrEventHandlerDisabled`. - Modified the `Init` method in `WebHookHandler` to return the new error when the webhook is disabled. - Updated the corresponding test to check for the `ErrEventHandlerDisabled` error instead of no error. ___ ### **Changes walkthrough** 📝 <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix </strong></td><td><table> <tr> <td> <details> <summary><strong>event_handler_webhooks.go</strong><dd><code>Add error handling for disabled webhook handler</code>                    </dd></summary> <hr> gateway/event_handler_webhooks.go <li>Added error handling for disabled webhook handler.<br> <li> Introduced <code>ErrEventHandlerDisabled</code> error variable.<br> <li> Modified <code>Init</code> method to return <code>ErrEventHandlerDisabled</code> when webhook is <br>disabled.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6334/files#diff-6587ad3f2629cfa6c84a71144127acd6cc7824e5141f0b4961848945a87e0198">+6/-1</a>      </td> </tr> </table></td></tr><tr><td><strong>Tests </strong></td><td><table> <tr> <td> <details> <summary><strong>event_handler_webhooks_test.go</strong><dd><code>Update test for disabled webhook handler error</code>                      </dd></summary> <hr> gateway/event_handler_webhooks_test.go - Updated test to check for `ErrEventHandlerDisabled` error. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6334/files#diff-1a2f8d6ab4443031b0f8486809453f6389291a6f625745fe8c65ac5cdb0e9621">+1/-1</a>      </td> </tr> </table></td></tr></tr></tbody></table> ___ > 💡 **PR-Agent usage**: >Comment `/help` on the PR to get a list of all available PR-Agent tools and their descriptions

…disabled (#6334) [TT-12323] fix panic when webhook handler is disabled (#6334) ### **User description**  ## Description fix panic when webhook handler is disabled ## Related Issue https://tyktech.atlassian.net/browse/TT-12323 ## Motivation and Context  ## How This Has Been Tested     ## Screenshots (if appropriate) ## Types of changes  - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Refactoring or add test (improvements in base code or adds test coverage to functionality) ## Checklist    - [ ] I ensured that the documentation is up to date - [ ] I explained why this PR updates go.mod in detail with reasoning why it's required - [ ] I would like a code coverage CI quality gate exception and have explained why ___ ### **PR Type** Bug fix, Tests ___ ### **Description** - Added error handling for when the webhook handler is disabled by introducing a new error variable `ErrEventHandlerDisabled`. - Modified the `Init` method in `WebHookHandler` to return the new error when the webhook is disabled. - Updated the corresponding test to check for the `ErrEventHandlerDisabled` error instead of no error. ___ ### **Changes walkthrough** 📝 <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix </strong></td><td><table> <tr> <td> <details> <summary><strong>event_handler_webhooks.go</strong><dd><code>Add error handling for disabled webhook handler</code>                    </dd></summary> <hr> gateway/event_handler_webhooks.go <li>Added error handling for disabled webhook handler.<br> <li> Introduced <code>ErrEventHandlerDisabled</code> error variable.<br> <li> Modified <code>Init</code> method to return <code>ErrEventHandlerDisabled</code> when webhook is <br>disabled.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6334/files#diff-6587ad3f2629cfa6c84a71144127acd6cc7824e5141f0b4961848945a87e0198">+6/-1</a>      </td> </tr> </table></td></tr><tr><td><strong>Tests </strong></td><td><table> <tr> <td> <details> <summary><strong>event_handler_webhooks_test.go</strong><dd><code>Update test for disabled webhook handler error</code>                      </dd></summary> <hr> gateway/event_handler_webhooks_test.go - Updated test to check for `ErrEventHandlerDisabled` error. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6334/files#diff-1a2f8d6ab4443031b0f8486809453f6389291a6f625745fe8c65ac5cdb0e9621">+1/-1</a>      </td> </tr> </table></td></tr></tr></tbody></table> ___ > 💡 **PR-Agent usage**: >Comment `/help` on the PR to get a list of all available PR-Agent tools and their descriptions

### **User description**  ## Description fix panic when webhook handler is disabled ## Related Issue https://tyktech.atlassian.net/browse/TT-12323 ## Motivation and Context  ## How This Has Been Tested     ## Screenshots (if appropriate) ## Types of changes  - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Refactoring or add test (improvements in base code or adds test coverage to functionality) ## Checklist    - [ ] I ensured that the documentation is up to date - [ ] I explained why this PR updates go.mod in detail with reasoning why it's required - [ ] I would like a code coverage CI quality gate exception and have explained why ___ ### **PR Type** Bug fix, Tests ___ ### **Description** - Added error handling for when the webhook handler is disabled by introducing a new error variable `ErrEventHandlerDisabled`. - Modified the `Init` method in `WebHookHandler` to return the new error when the webhook is disabled. - Updated the corresponding test to check for the `ErrEventHandlerDisabled` error instead of no error. ___ ### **Changes walkthrough** 📝 <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix </strong></td><td><table> <tr> <td> <details> <summary><strong>event_handler_webhooks.go</strong><dd><code>Add error handling for disabled webhook handler</code>                    </dd></summary> <hr> gateway/event_handler_webhooks.go <li>Added error handling for disabled webhook handler.<br> <li> Introduced <code>ErrEventHandlerDisabled</code> error variable.<br> <li> Modified <code>Init</code> method to return <code>ErrEventHandlerDisabled</code> when webhook is <br>disabled.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6334/files#diff-6587ad3f2629cfa6c84a71144127acd6cc7824e5141f0b4961848945a87e0198">+6/-1</a>      </td> </tr> </table></td></tr><tr><td><strong>Tests </strong></td><td><table> <tr> <td> <details> <summary><strong>event_handler_webhooks_test.go</strong><dd><code>Update test for disabled webhook handler error</code>                      </dd></summary> <hr> gateway/event_handler_webhooks_test.go - Updated test to check for `ErrEventHandlerDisabled` error. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6334/files#diff-1a2f8d6ab4443031b0f8486809453f6389291a6f625745fe8c65ac5cdb0e9621">+1/-1</a>      </td> </tr> </table></td></tr></tr></tbody></table> ___ > 💡 **PR-Agent usage**: >Comment `/help` on the PR to get a list of all available PR-Agent tools and their descriptions

github-actions Bot added Tests Bug fix Review effort [1-5]: 2 labels Jun 10, 2024

fix panic when webhook handler is disabled

cad78ca

jeffy-mathew force-pushed the fix/TT-12323/panic-on-disabled-webhook-trigger branch from 963c85e to cad78ca Compare June 10, 2024 13:59

jeffy-mathew requested review from andrei-tyk and titpetric June 10, 2024 13:59

titpetric approved these changes Jun 10, 2024

View reviewed changes

andrei-tyk approved these changes Jun 11, 2024

View reviewed changes

jeffy-mathew merged commit 3e5963c into master Jun 11, 2024

jeffy-mathew deleted the fix/TT-12323/panic-on-disabled-webhook-trigger branch June 11, 2024 09:12

buger mentioned this pull request Jun 11, 2024

Merging to release-5.4: [TT-12323] fix panic when webhook handler is disabled (#6334) #6340

Merged

7 tasks

buger mentioned this pull request Jun 11, 2024

Merging to release-5.4.0: [TT-12323] fix panic when webhook handler is disabled (#6334) #6341

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[TT-12323] fix panic when webhook handler is disabled#6334

[TT-12323] fix panic when webhook handler is disabled#6334
jeffy-mathew merged 1 commit into
masterfrom
fix/TT-12323/panic-on-disabled-webhook-trigger

jeffy-mathew commented Jun 10, 2024 •

edited by github-actions Bot

Loading

Uh oh!

github-actions Bot commented Jun 10, 2024

Uh oh!

github-actions Bot commented Jun 10, 2024 •

edited

Loading

Uh oh!

github-actions Bot commented Jun 10, 2024

Uh oh!

sonarqubecloud Bot commented Jun 10, 2024

Uh oh!

jeffy-mathew commented Jun 11, 2024

Uh oh!

jeffy-mathew commented Jun 11, 2024

Uh oh!

tykbot Bot commented Jun 11, 2024

Uh oh!

tykbot Bot commented Jun 11, 2024

Uh oh!

tykbot Bot commented Jun 11, 2024

Uh oh!

tykbot Bot commented Jun 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

jeffy-mathew commented Jun 10, 2024 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Description

Related Issue

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

Checklist

PR Type

Description

Changes walkthrough 📝

Uh oh!

github-actions Bot commented Jun 10, 2024

PR Reviewer Guide 🔍

Uh oh!

github-actions Bot commented Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jun 10, 2024

PR Code Suggestions ✨

Uh oh!

sonarqubecloud Bot commented Jun 10, 2024

Quality Gate passed

Uh oh!

jeffy-mathew commented Jun 11, 2024

Uh oh!

jeffy-mathew commented Jun 11, 2024

Uh oh!

tykbot Bot commented Jun 11, 2024

Uh oh!

tykbot Bot commented Jun 11, 2024

Uh oh!

tykbot Bot commented Jun 11, 2024

Uh oh!

tykbot Bot commented Jun 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jeffy-mathew commented Jun 10, 2024 •

edited by github-actions Bot

Loading

github-actions Bot commented Jun 10, 2024 •

edited

Loading