Add internal/net test #615

kevindiu · 2020-08-07T01:16:53Z

Author review required.

Description:

This PR implements test case for internal/net/net.go.
This PR also include the following changes:

Bug fix:

In ScanPorts(), when start > end, unexpected ports will be scanned
Fix: https://github.com/vdaas/vald/pull/615/files#diff-111853981bfa0f81d6c8945300daf97bR122-R124
In ScanPorts(), when end is 65535, the ScanPorts() will never return due to golang bug or behavior
Fix: https://github.com/vdaas/vald/pull/615/files#diff-111853981bfa0f81d6c8945300daf97bR134
Ref: Add internal/net test #615 (comment)
In ScanPorts(), scanning ports in range, with some ports are failed but some success, the error will be returned instead of returning succeed ports
Fix: https://github.com/vdaas/vald/pull/615/files#diff-111853981bfa0f81d6c8945300daf97bR143-R144
https://github.com/vdaas/vald/pull/615/files#diff-111853981bfa0f81d6c8945300daf97bR163-R165

Refactor:

In ScanPorts(), do not return connection close error.
Ref: https://github.com/vdaas/vald/pull/615/files#diff-111853981bfa0f81d6c8945300daf97bR151-R153
In SplitHostPort(), commented out the :: support due to invalid implementation, which always return error.
To support loopback IPv6 address please read the function comment.
Ref: https://github.com/vdaas/vald/pull/615/files#diff-111853981bfa0f81d6c8945300daf97bR98-R101

Related Issue:

How Has This Been Tested?:

Environment:

Go Version: 1.14.4
Docker Version: 19.03.8
Kubernetes Version: 1.18.2
NGT Version: 1.12.0

Types of changes:

Bug fix [type/bug]
New feature [type/feature]
Add tests [type/test]
Security related changes [type/security]
Add documents [type/documentation]
Refactoring [type/refactoring]
Update dependencies [type/dependency]
Update benchmarks and performances [type/bench]
Update CI [type/ci]

Changes to Core Features:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your core changes, as applicable?
Have you successfully ran tests with your changes locally?

Checklist:

I have read the CONTRIBUTING document.
I have checked open Pull Requests for the similar feature or fixes?
I have added tests and benchmarks to cover my changes.
I have ensured all new and existing tests passed.
I have commented my code, particularly in hard-to-understand areas
I have updated the documentation accordingly.

pull-assistant · 2020-08-07T01:17:00Z

Best reviewed: commit by commit

Optimal code review plan

refactor and add net test

Powered by Pull Assistant. Last update 72dc772 ... 77659e5. Read the comment docs.

vdaas-ci · 2020-08-07T01:17:18Z

[CHATOPS:HELP] ChatOps commands.

🙆‍♀️ /approve - approve
💌 /changelog - add changelog comment
🍱 /format - format codes and add licenses
✅ /gen-test - generate test codes
🏷️ /label - add labels
⏪ /rebase - rebase master

kevindiu · 2020-08-07T02:41:02Z

/rebase

vdaas-ci · 2020-08-07T02:41:36Z

[REBASE] Rebase triggered by kevindiu for branch: test/internal/net

codecov · 2020-08-07T02:47:50Z

Codecov Report

Merging #615 into master will increase coverage by 0.25%.
The diff coverage is 69.23%.

@@            Coverage Diff             @@
##           master     #615      +/-   ##
==========================================
+ Coverage   13.73%   13.99%   +0.25%     
==========================================
  Files         409      409              
  Lines       21516    21519       +3     
==========================================
+ Hits         2956     3011      +55     
+ Misses      18285    18229      -56     
- Partials      275      279       +4

Impacted Files	Coverage Δ
internal/errors/net.go	`0.00% <ø> (ø)`
internal/net/net.go	`87.30% <69.23%> (+87.30%)`	⬆️
internal/worker/queue.go	`95.65% <0.00%> (-2.90%)`	⬇️
internal/worker/worker.go	`86.36% <0.00%> (+1.81%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13c5365...77659e5. Read the comment docs.

kevindiu · 2020-08-07T06:26:01Z

internal/net/net.go

 func SplitHostPort(hostport string) (host string, port uint16, err error) {
 	switch {
 	case strings.HasPrefix(hostport, "::"):
-		hostport = localIPv6 + hostport
+		hostport = localIPv6 + hostport[2:]


The format of IPv6 address with port number should be [addr]:port, the logic of line 96 is incorrect, should I fix it? @kpango @rinx

or if the hostport do not contains port number, there are no [] to wrap the address, instead the host should use the hostport with port defaultPort

I suggest:

switch { case hostport == "::1": hostport = "[" + localIPv6 + "]:" + defaultPort ........

or we can return directly.

switch { case hostport == "::1": return localIPv6, defaultPort, nil

I think we have to make 2 changes, this case was implemented for detecting loopback address, if there is no port definition on hostport string, we can use default port but in this case we have to check there is a port in hostport string or not, and if exists we have to parse it.
one more thing as you mentioned, loopback addr is ::1 but it also can be written as :: or 0:0:0:0:0:0:0:1. so we have to check them

Could you please read RFC more carefully, In RFC5952 (https://tools.ietf.org/html/rfc5952#section-6) there are 6 types of ipv6 w/ port format we should support them.
Your code seems support only 1 format

@kpango
May I confirm that this switch case statement is checking if hostport contains hostname or port number only and use the default value if not exists?

For example, line 98-99 use the default local IPv4 address if the host is missing.
I think IPv6 do not have empty host case so we don't need to support it.

But I think IPv6 have empty port, so I wrote the logic above.

BTW net.SplitHostPort doesn't support all RFC format.
https://golang.org/src/net/ipsock.go?s=4766:4832#L146

how should we work on this?

Suggested change

hostport = localIPv6 + hostport[2:]

// hostport = localIPv6 + hostport[2:]

https://github.com/vdaas/vald/projects/3#card-43504189

kevindiu · 2020-08-11T09:31:26Z

/rebase

vdaas-ci · 2020-08-11T09:32:04Z

[REBASE] Rebase triggered by kevindiu for branch: test/internal/net

internal/errors/net.go

internal/net/net_test.go

internal/net/net.go

internal/net/net_test.go

kevindiu · 2020-08-12T06:28:21Z

/rebase

vdaas-ci · 2020-08-12T06:28:56Z

[REBASE] Rebase triggered by kevindiu for branch: test/internal/net

internal/net/net_test.go

kevindiu · 2020-08-12T08:07:04Z

/rebase

vdaas-ci · 2020-08-12T08:07:39Z

[REBASE] Rebase triggered by kevindiu for branch: test/internal/net

internal/net/net.go

kpango · 2020-08-13T07:46:06Z

/rebase
/format
/approve

vdaas-ci · 2020-08-13T07:46:50Z

[REBASE] Rebase triggered by kpango for branch: test/internal/net

vdaas-ci · 2020-08-13T07:47:51Z

[FORMAT] Updating license headers and formatting go codes triggered by kpango.

vdaas-ci

[APPROVED] This PR is approved by kpango.

internal/net/net.go

github-actions bot added area/internal team/set SET team size/XL type/test Adding tests priority/low labels Aug 7, 2020

kevindiu changed the title ~~Create net test~~ internal/net test Aug 7, 2020

vdaas-ci force-pushed the test/internal/net branch from ff38768 to b792c14 Compare August 7, 2020 02:41

kevindiu commented Aug 7, 2020

View reviewed changes

vdaas-ci force-pushed the test/internal/net branch from 3d43512 to 1c943d0 Compare August 11, 2020 09:32

github-actions bot reviewed Aug 11, 2020

View reviewed changes

kevindiu changed the title ~~internal/net test~~ [WIP] internal/net test Aug 12, 2020

github-actions bot reviewed Aug 12, 2020

View reviewed changes

internal/net/net.go Outdated Show resolved Hide resolved

github-actions bot reviewed Aug 12, 2020

View reviewed changes

internal/net/net_test.go Show resolved Hide resolved

vdaas-ci force-pushed the test/internal/net branch from 33c62ed to bcfb14e Compare August 12, 2020 06:29

kevindiu changed the title ~~[WIP] internal/net test~~ Add internal/net test Aug 12, 2020

github-actions bot reviewed Aug 12, 2020

View reviewed changes

internal/net/net_test.go Outdated Show resolved Hide resolved

internal/net/net_test.go Outdated Show resolved Hide resolved

internal/net/net_test.go Outdated Show resolved Hide resolved

kevindiu requested a review from hlts2 August 12, 2020 06:52

vdaas-ci force-pushed the test/internal/net branch from bcfb14e to eb9810f Compare August 12, 2020 08:07

kevindiu marked this pull request as ready for review August 12, 2020 08:24

vankichi reviewed Aug 13, 2020

View reviewed changes

internal/net/net.go Outdated Show resolved Hide resolved

internal/net/net.go Show resolved Hide resolved

kevindiu commented Aug 13, 2020

View reviewed changes

internal/net/net.go Outdated Show resolved Hide resolved

kevindiu dismissed hlts2’s stale review via b39d571 August 13, 2020 06:43

vankichi previously approved these changes Aug 13, 2020

View reviewed changes

kevindiu added 8 commits August 13, 2020 07:46

refactor and add net test

72dc772

fix

fd30fa8

revert changes

7334323

add comments

49c8e6c

fix type

ce48e73

fix

eecf14b

fix deepsource

22efdf0

add comment

229127e

vdaas-ci dismissed vankichi’s stale review via 229127e August 13, 2020 07:47

vdaas-ci force-pushed the test/internal/net branch from b39d571 to 229127e Compare August 13, 2020 07:47

vdaas-ci previously approved these changes Aug 13, 2020

View reviewed changes

github-actions bot reviewed Aug 13, 2020

View reviewed changes

internal/net/net.go Show resolved Hide resolved

fix CI

77659e5

kevindiu dismissed vdaas-ci’s stale review via 77659e5 August 13, 2020 09:43

vankichi approved these changes Aug 13, 2020

View reviewed changes

kevindiu requested review from hlts2 and vankichi and removed request for vankichi August 13, 2020 09:58

hlts2 approved these changes Aug 13, 2020

View reviewed changes

kpango approved these changes Aug 13, 2020

View reviewed changes

kpango merged commit aca45ea into master Aug 13, 2020

kpango deleted the test/internal/net branch August 13, 2020 10:04

vdaas-ci mentioned this pull request Sep 2, 2020

[patch] update deps #660

Merged

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add internal/net test #615

Add internal/net test #615

kevindiu commented Aug 7, 2020 •

edited

Loading

pull-assistant bot commented Aug 7, 2020 •

edited

Loading

vdaas-ci commented Aug 7, 2020

kevindiu commented Aug 7, 2020

vdaas-ci commented Aug 7, 2020

codecov bot commented Aug 7, 2020 •

edited

Loading

kevindiu Aug 7, 2020

kevindiu Aug 7, 2020

kevindiu Aug 11, 2020

kevindiu Aug 11, 2020

kpango Aug 11, 2020

kpango Aug 11, 2020 •

edited

Loading

kevindiu Aug 11, 2020

kevindiu Aug 11, 2020

vankichi Aug 12, 2020

vankichi Aug 12, 2020

kevindiu commented Aug 11, 2020

vdaas-ci commented Aug 11, 2020

kevindiu commented Aug 12, 2020

vdaas-ci commented Aug 12, 2020

kevindiu commented Aug 12, 2020

vdaas-ci commented Aug 12, 2020

kpango commented Aug 13, 2020

vdaas-ci commented Aug 13, 2020

vdaas-ci commented Aug 13, 2020

vdaas-ci left a comment

	hostport = localIPv6 + hostport[2:]
	// hostport = localIPv6 + hostport[2:]

Add internal/net test #615

Add internal/net test #615

Conversation

kevindiu commented Aug 7, 2020 • edited Loading

Description:

Related Issue:

How Has This Been Tested?:

Environment:

Types of changes:

Changes to Core Features:

Checklist:

pull-assistant bot commented Aug 7, 2020 • edited Loading

vdaas-ci commented Aug 7, 2020

kevindiu commented Aug 7, 2020

vdaas-ci commented Aug 7, 2020

codecov bot commented Aug 7, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpango Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevindiu commented Aug 11, 2020

vdaas-ci commented Aug 11, 2020

kevindiu commented Aug 12, 2020

vdaas-ci commented Aug 12, 2020

kevindiu commented Aug 12, 2020

vdaas-ci commented Aug 12, 2020

kpango commented Aug 13, 2020

vdaas-ci commented Aug 13, 2020

vdaas-ci commented Aug 13, 2020

vdaas-ci left a comment

Choose a reason for hiding this comment

kevindiu commented Aug 7, 2020 •

edited

Loading

pull-assistant bot commented Aug 7, 2020 •

edited

Loading

codecov bot commented Aug 7, 2020 •

edited

Loading

kpango Aug 11, 2020 •

edited

Loading