Backport PR #2634 to release/v1.7 for Add option to disable dns resolve #2641
Conversation
* fix: add option to disable dns resolve Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: add mock function Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: unimplemented error Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: change ForwardedContext method to private Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> --------- Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
Deploying vald with
|
| Latest commit: |
232a2d1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://af81e067.vald.pages.dev |
| Branch Preview URL: | https://backport-release-v1-7-fix-mi-dmyq.vald.pages.dev |
WalkthroughWalkthroughThe changes introduce a new method, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[CHATOPS:HELP] ChatOps commands.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (5)
internal/test/mock/grpc/grpc_client_mock.go (2)
37-37: LGTM! Consider adding a comment for clarity.The addition of
SetDisableResolveDNSAddrFuncis consistent with the existing structure and naming conventions.Consider adding a brief comment explaining the purpose of this function, similar to other function pointers in the struct. For example:
// SetDisableResolveDNSAddrFunc is used to mock the behavior of setting DNS resolution disable flag for a given address. SetDisableResolveDNSAddrFunc func(addr string, disabled bool)
75-78: LGTM! Consider adding a nil check for robustness.The implementation of
SetDisableResolveDNSAddris consistent with other methods in the struct and correctly delegates to the function pointer.For improved robustness, consider adding a nil check before calling the function pointer. This prevents potential panics if the mock is not fully initialized. For example:
func (gc *GRPCClientMock) SetDisableResolveDNSAddr(addr string, disabled bool) { if gc.SetDisableResolveDNSAddrFunc != nil { gc.SetDisableResolveDNSAddrFunc(addr, disabled) } }internal/test/mock/grpc_testify_mock.go (1)
216-217: LGTM. Consider adding documentation for clarity.The addition of the
SetDisableResolveDNSAddrmethod to theClientInternalstruct is consistent with the PR objectives. The empty implementation is appropriate for a mock object, allowing test cases to override the behavior as needed.Consider adding a comment to explain the purpose of this method and the
distributedparameter, which would improve clarity for developers working with this mock in the future. For example:// SetDisableResolveDNSAddr mocks the behavior of disabling DNS resolution for a specific address. // The 'distributed' parameter indicates whether the setting applies to a distributed environment (e.g., mirror gateway). func (c *ClientInternal) SetDisableResolveDNSAddr(addr string, distributed bool) {}pkg/gateway/mirror/service/mirror.go (1)
322-322: LGTM! Consider adding a comment for clarity.The addition of
SetDisableResolveDNSAddraligns well with the PR objective to introduce an option for disabling DNS resolution. This change ensures that DNS resolution is disabled for specific addresses before attempting to connect, which should resolve the connection issues mentioned in the PR description.Consider adding a brief comment explaining why DNS resolution is being disabled for these connections. This would improve code readability and maintainability. For example:
+// Disable DNS resolution for this address to avoid connection issues m.gateway.GRPCClient().SetDisableResolveDNSAddr(addr, true)internal/net/grpc/client.go (1)
951-955: Improve code comments for clarity and professionalismThe comments within the
SetDisableResolveDNSAddrmethod contain personal notes and informal language. To enhance maintainability and readability, consider updating the comments to clearly explain the purpose and usage of the method.Apply this diff to refine the comments:
- // NOTE: When connecting to multiple locations, it was necessary to switch dynamically, so implementation was added. - // There is no setting for disable on the helm chart side, so I used this implementation. + // Allows dynamic control over DNS resolution for specific addresses. + // Useful when connecting to multiple locations where DNS resolution needs to be toggled.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
apis/grpc/v1/payload/payload.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/rpc/errdetails/error_details.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (6)
- internal/net/grpc/client.go (3 hunks)
- internal/test/mock/grpc/grpc_client_mock.go (2 hunks)
- internal/test/mock/grpc_testify_mock.go (1 hunks)
- pkg/gateway/mirror/service/gateway.go (5 hunks)
- pkg/gateway/mirror/service/mirror.go (1 hunks)
- pkg/gateway/mirror/service/mirror_test.go (6 hunks)
Additional comments not posted (12)
internal/test/mock/grpc/grpc_client_mock.go (1)
Line range hint
1-78: Overall, the changes look good and align with the PR objectives.The additions to the
GRPCClientMockstruct successfully introduce the functionality to disable DNS resolution for specific addresses. This aligns with the PR objective of adding an option to disable DNS resolution when connecting to multiple locations via the mirror gateway.The changes are minimal, focused, and well-integrated with the existing code structure. They provide the necessary mock implementations to support testing of the new functionality in the main codebase.
pkg/gateway/mirror/service/mirror_test.go (5)
90-90: LGTM: New mock method added correctly.The addition of
SetDisableResolveDNSAddrFuncto thegrpcmock.GRPCClientMockstruct is correct and aligns with the PR objective. The empty implementation is appropriate for a mock object.
122-122: LGTM: Consistent implementation of new mock method.The addition of
SetDisableResolveDNSAddrFuncis consistent with the previous test case. The empty implementation remains appropriate for a mock object.
226-226: LGTM: Consistent implementation across test cases.The addition of
SetDisableResolveDNSAddrFuncin theTest_mirr_Disconnectfunction maintains consistency with previous test cases. The empty implementation and method signature remain appropriate and consistent.
258-258: LGTM: Consistent implementation maintained throughout.The addition of
SetDisableResolveDNSAddrFuncin this test case of theTest_mirr_Disconnectfunction maintains perfect consistency with all previous instances. The empty implementation and method signature remain appropriate and consistent throughout the file.
90-90: Summary: Consistent and appropriate implementation of new mock method.The new method
SetDisableResolveDNSAddrFunchas been consistently added to all relevant test cases in themirror_test.gofile. The implementation (empty function) and signature (two parameters:addrstring anddisabledbool) are uniform across all instances. These changes appropriately support the PR objective of introducing an option to disable DNS resolution without altering existing test logic.Great job on maintaining consistency and adhering to the project's testing standards!
Also applies to: 122-122, 226-226, 258-258
pkg/gateway/mirror/service/gateway.go (4)
152-160: Verify error handling logic in 'Do' methodIn the
Domethod, when an error occurs, the code attempts a fallback usingRoundRobin. The conditionstrings.EqualFold(conn.Target(), target)skips over connections matching the original target. This might lead to unintended behavior if multiple connections have the same target with different cases or formats. Please verify if this logic is intended or if a more precise comparison is needed.
Line range hint
116-121: Consistent use of the new 'forwardedContext' methodThe implementation of
g.forwardedContext(ctx, g.podName)in theBroadCastmethod ensures that the forwarding context is consistently applied across all requests. This enhances encapsulation and maintains code clarity.
Line range hint
181-188: Updated 'DoMulti' method to use 'forwardedContext'The
DoMultimethod now utilizesg.forwardedContext(ctx, g.podName), aligning it with other methods for consistent context handling. This promotes code consistency and reduces the risk of context-related issues.
76-78:⚠️ Potential issueChanging 'ForwardedContext' to private may introduce breaking changes
The method
ForwardedContexthas been renamed toforwardedContext, altering its visibility from public to private. If this method is used outside of this package, this change could cause breaking issues for external packages or tests relying on it. Please verify ifForwardedContextis being used externally.Run the following script to identify external usages of
ForwardedContext:Verification successful
External usages of
ForwardedContextnot foundThe
ForwardedContextmethod is only utilized within internal test and mock files. There are no external package dependencies relying on it. Therefore, changing its visibility from public to private will not introduce breaking changes for external consumers.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for external usages of 'ForwardedContext' method. # Test: Search the codebase for 'ForwardedContext' method calls outside 'gateway.go'. # Expectation: No external usages found. rg --type go 'ForwardedContext\(' | grep -v 'pkg/gateway/mirror/service/gateway.go'Length of output: 1101
internal/net/grpc/client.go (2)
986-992: Ensure correct logic in DNS resolution decisionThe logic within the
WithResolveDNSclosure correctly checks thedisableResolveDNSAddrsmap to determine if DNS resolution should be disabled for the given address. This allows for dynamic control based on specific addresses.
104-104: Verify the use ofsync.Mapwith genericsThe
disableResolveDNSAddrsfield is declared assync.Map[string, bool]. Ensure that the customsync.Mapimplementation supports generics and that this usage is compatible with the intended Go version and your internal packages.If using Go's standard library
sync.Map, which doesn't support generics, consider using a type-safe wrapper or alternative concurrency-safe data structures.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/v1.7 #2641 +/- ##
===============================================
Coverage ? 24.35%
===============================================
Files ? 536
Lines ? 46459
Branches ? 0
===============================================
Hits ? 11317
Misses ? 34375
Partials ? 767 ☔ View full report in Codecov by Sentry. |
hlts2
deleted the
backport/release/v1.7/fix/mirror/add-disable-resolve-dns
branch
Description
When we verified the mirror gateway to connect to multiple locations, we could not connect when dns resolve was enabled. Therefore, I added an interface to disable it.
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
New Features
Bug Fixes