Merging to release-5.6: TT-13130 updated version of gorpc library and prevent panic on start edge (#6629)

buger commented 1 month ago

User description

TT-13130 updated version of gorpc library and prevent panic on start edge (#6629)

User description

Summary	Tyk Cloud: Panic appears when a user tried to deploy GW before Control Plane is in deployed state
Type	Bug
Status	In Dev
Points	N/A
Labels	Re_open

Description

Moved the logic of waitgroup to be handled internally in the gorpc library. GW only have to wait until done()

Related Issue

TT-13130

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, Enhancement

Description

Refactored the connection dialing logic in rpc_client.go to remove manual sync.WaitGroup handling, leveraging the internal wait group management provided by the gorpc library.
Updated the gorpc library to a newer version in go.mod and go.sum, ensuring compatibility and leveraging improvements.

Changes walkthrough 📝

Relevant files

Enhancement

rpc_client.go

Refactor connection
dialing wait group handling

rpc/rpc_client.go

Removed manual handling of sync.WaitGroup for connection dialing.

Utilized clientSingleton.ConnectionDialingWG for managing connection
readiness.

+1/-7

Dependencies

go.mod `Update gorpc library version in go.mod` go.mod - Updated `gorpc` library version to latest.	+1/-1
go.sum `Update go.sum for new gorpc version` go.sum - Updated checksums for new `gorpc` library version.	+8/-2

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

Co-authored-by: sredny buitrago sredny@srednys-MacBook-Pro.local

PR Type

Bug fix, Enhancement

Description

Refactored the connection dialing logic in rpc_client.go to remove manual sync.WaitGroup handling, leveraging the internal wait group management provided by the gorpc library.
Updated the gorpc library to a newer version in go.mod and go.sum, ensuring compatibility and leveraging improvements.

Changes walkthrough 📝

Relevant files

Enhancement

rpc_client.go

Refactor connection dialing wait group handling

rpc/rpc_client.go

Removed manual handling of sync.WaitGroup for connection dialing.

Utilized clientSingleton.ConnectionDialingWG for managing connection
readiness.

+1/-8

Dependencies

go.mod `Update gorpc library version in go.mod` go.mod - Updated `gorpc` library version to the latest.	+1/-1
go.sum `Update go.sum for new gorpc version` go.sum - Updated checksums for new `gorpc` library version.	+2/-2

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

github-actions[bot] commented 1 month ago

API Changes

no api changes detected

github-actions[bot] commented 1 month ago

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

**🎫 Ticket compliance analysis 🔶** **[6629](https://github.com/TykTechnologies/tyk/issues/6629) - Partially compliant** Fully compliant requirements: - Update the gorpc library to a newer version. - Refactor the connection dialing logic to prevent panic when deploying GW before Control Plane is deployed. Not compliant requirements: - Ensure the changes do not introduce new issues and are compatible with existing systems.

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review

Connection Handling
The removal of manual `sync.WaitGroup` handling and reliance on `clientSingleton.ConnectionDialingWG.Wait()` needs thorough testing to ensure no race conditions or deadlocks occur.

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible bug	Add error handling for `conn.Write` operations to ensure robustness ___ Ensure that the `clientSingleton.Dial` function handles potential errors from the `conn.Write` operations to prevent runtime panics or unhandled errors. [rpc/rpc_client.go [294-296]](https://github.com/TykTechnologies/tyk/pull/6638/files#diff-3b88914c99bb9418e44e6389ce73579843562e8900730b380d7fff2e95c51033R294-R296) ```diff -conn.Write([]byte("proto2")) -conn.Write([]byte{byte(len(connID))}) -conn.Write([]byte(connID)) +if _, err := conn.Write([]byte("proto2")); err != nil { + return nil, err +} +if _, err := conn.Write([]byte{byte(len(connID))}); err != nil { + return nil, err +} +if _, err := conn.Write([]byte(connID)); err != nil { + return nil, err +} ``` Suggestion importance[1-10]: 9 Why: The suggestion correctly identifies a potential issue where errors from `conn.Write` operations are not handled, which could lead to runtime panics or unhandled errors. Adding error handling significantly improves the robustness and reliability of the code.	9

sonarcloud[bot] commented 1 month ago

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

TykTechnologies / tyk