common: validate endpoint fields in parseEndpoint#586
common: validate endpoint fields in parseEndpoint#586wenzaee wants to merge 1 commit intokubeedge:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wenzaee The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @wenzaee! It looks like this is your first PR to kubeedge/edgemesh 🎉 |
There was a problem hiding this comment.
Code Review
This pull request implements IP and port validation in the parseEndpoint function within pkg/loadbalancer/util.go, replacing a previous TODO. The review feedback suggests refining the validation logic by incorporating a port range check and removing redundant checks, while also highlighting that the current implementation is limited to IPv4 support.
| port, err := strconv.Atoi(info[portIndex]) | ||
| if err != nil { | ||
| return "", "", "", "", false | ||
| } | ||
| if net.ParseIP(info[ipIndex]) == nil || !isValidEndpoint(info[ipIndex], port) { | ||
| return "", "", "", "", false | ||
| } |
There was a problem hiding this comment.
The validation logic can be improved by checking the full valid port range (1-65535) and removing redundant checks. Since net.ParseIP already validates that the string is a valid IP address, the host != "" check inside isValidEndpoint is unnecessary here. Combining the port conversion and range check also makes the code more concise.
Additionally, note that the current parsing logic using strings.Split(endpoint, ":") is incompatible with IPv6 addresses as they contain colons. While fixing the parsing logic might be out of scope for this PR, it's worth noting that this function currently only supports IPv4.
| port, err := strconv.Atoi(info[portIndex]) | |
| if err != nil { | |
| return "", "", "", "", false | |
| } | |
| if net.ParseIP(info[ipIndex]) == nil || !isValidEndpoint(info[ipIndex], port) { | |
| return "", "", "", "", false | |
| } | |
| port, err := strconv.Atoi(info[portIndex]) | |
| if err != nil || port <= 0 || port > 65535 { | |
| return "", "", "", "", false | |
| } | |
| if net.ParseIP(info[ipIndex]) == nil { | |
| return "", "", "", "", false | |
| } |
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR implements the TODO in
parseEndpointby adding IP and port validation.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
NONE