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

buger commented 2 weeks 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 2 weeks ago

API Changes

no api changes detected

github-actions[bot] commented 2 weeks 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 that the new implementation is tested and does not introduce new issues.

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review

Refactoring Impact
The refactoring removes manual sync.WaitGroup handling which could impact how connections are managed. Ensure that this change does not introduce connection handling issues or race conditions.

github-actions[bot] commented 2 weeks ago

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible bug	Improve error handling for connection writes to enhance data integrity and error management ___ Consider handling potential errors from `conn.Write` to ensure data integrity and proper error management. [rpc/rpc_client.go [294-296]](https://github.com/TykTechnologies/tyk/pull/6637/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: This suggestion addresses a critical issue by adding error handling for `conn.Write` operations, which is essential for ensuring data integrity and preventing potential runtime errors.	9
Possible bug	Verify synchronization of WaitGroup to prevent deadlocks or runtime panics ___ Ensure that the `Wait` on `clientSingleton.ConnectionDialingWG` is correctly synchronized with `Add` calls, which seem to be removed in this PR, potentially leading to deadlocks or panics. [rpc/rpc_client.go [309]](https://github.com/TykTechnologies/tyk/pull/6637/files#diff-3b88914c99bb9418e44e6389ce73579843562e8900730b380d7fff2e95c51033R309-R309) ```diff +// Ensure corresponding Add() calls are made before this Wait() clientSingleton.ConnectionDialingWG.Wait() ``` Suggestion importance[1-10]: 8 Why: The suggestion highlights a potential synchronization issue with the WaitGroup, which could lead to deadlocks or panics. Ensuring proper synchronization is crucial for the stability of the application.	8
Maintainability	Enhance modularity and reusability by refactoring dialer configuration into a separate function ___ Refactor the dialer configuration to a separate function or configuration struct to enhance modularity and reusability. [rpc/rpc_client.go [262-264]](https://github.com/TykTechnologies/tyk/pull/6637/files#diff-3b88914c99bb9418e44e6389ce73579843562e8900730b380d7fff2e95c51033R262-R264) ```diff -dialer := &net.Dialer{ - Timeout: 10 * time.Second, - KeepAlive: 30 * time.Second, +dialer := getDefaultDialer() +... +func getDefaultDialer() net.Dialer { + return &net.Dialer{ + Timeout: 10 time.Second, + KeepAlive: 30 * time.Second, + } } ``` Suggestion importance[1-10]: 6 Why: Refactoring the dialer configuration into a separate function improves code modularity and reusability, which is beneficial for maintainability, though it does not address any immediate functional issues.	6
Enhancement	Simplify initialization of connection pool size with direct assignment ___ Initialize `clientSingleton.Conns` directly with a default value if the configuration returns zero to simplify the conditional logic. [rpc/rpc_client.go [256-258]](https://github.com/TykTechnologies/tyk/pull/6637/files#diff-3b88914c99bb9418e44e6389ce73579843562e8900730b380d7fff2e95c51033R256-R258) ```diff clientSingleton.Conns = values.Config().RPCPoolSize if clientSingleton.Conns == 0 { - clientSingleton.Conns = 5 + clientSingleton.Conns = 5 // Consider initializing directly to 5 if Config().RPCPoolSize is zero } ``` Suggestion importance[1-10]: 3 Why: While the suggestion offers a minor improvement in code readability by suggesting a direct assignment, it does not significantly impact the functionality or performance of the code.	3

sonarcloud[bot] commented 2 weeks ago

Quality Gate failed

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

See analysis details on SonarCloud

TykTechnologies / tyk