Code deficiencies of various kinds and severities

[dominikh]

Hi,

I ran staticcheck on weave at commit bec2470, filtered out benign issues and false positives and assembled the following report, grouped by category and sorted by severity in descending order. I hope this report is useful to you.

Subtle bugs/potential for wrong behaviour

Location	Issue
prog/plugin/main.go:97:39	os.Kill cannot be trapped (did you mean syscall.SIGTERM?) (SA1016)
prog/weaveutil/docker_tls_args.go:48:5	ineffective break statement. Did you mean to break out of the outer loop? (SA4011)
prog/weaveutil/unique_id.go:19:13	printf-style function with dynamic first argument and no further arguments should use print-style function instead (SA1006)
ipam/allocator.go:215:6	identical expressions on the left and right side of the '==' operator (SA4000)
router/sleeve.go:1041:5	this value of err is never used (SA4006)
router/sleeve.go:1062:41	use net.IP.Equal to compare net.IPs, not bytes.Equal (SA1021)
router/sleeve.go:1114:20	use net.IP.Equal to compare net.IPs, not bytes.Equal (SA1021)

Resource leaks

Location	Issue
nameserver/nameserver.go:60:22	using time.Tick leaks the underlying ticker, consider using it only in endless functions, tests and the main package, and use time.NewTicker here (SA1015)
testing/gossip/mocks.go:118:26	using time.Tick leaks the underlying ticker, consider using it only in endless functions, tests and the main package, and use time.NewTicker here (SA1015)

Potentially buggy tests

Location	Issue
ipam/allocator_test.go:188:9	this value of err is never used (SA4006)
ipam/space/space_test.go:55:6	this value of got is never used (SA4006)
ipam/space/space_test.go:70:5	this value of ok is never used (SA4006)
ipam/space/space_test.go:80:5	this value of ok is never used (SA4006)

Usage of deprecated code

Location	Issue
proxy/proxy_intercept.go:36:12	httputil.NewClientConn is deprecated: Use the Client or Transport in package net/http instead. (SA1019)
proxy/proxy_intercept.go:40:26	httputil.ErrPersistEOF is deprecated: No longer used. (SA1019)
proxy/proxy_intercept.go:72:70	httputil.ClientConn is deprecated: Use Client or Transport in package net/http instead. (SA1019)
proxy/proxy_intercept.go:131:76	httputil.ClientConn is deprecated: Use Client or Transport in package net/http instead. (SA1019)
proxy/proxy_intercept.go:158:44	httputil.ClientConn is deprecated: Use Client or Transport in package net/http instead. (SA1019)

[bboreham]

Thanks! Some of those are bugs; some not. We'll take a good look through all of them.

[bboreham]

The main ones that are not worth fixing are of this form:

using time.Tick leaks the underlying ticker, consider using it only in endless functions, tests and the main package, and use time.NewTicker here (SA1015)

where the usage is in endless functions and tests.

[dominikh]

Regarding the time.Tick leaks: staticcheck actively filters those calls to time.Tick that exist in endless functions or in package main. In the two instances that I reported here, I believe that at least the first one is an actual leak. The function can terminate (by calling Nameserver.Stop) and the API suggests that multiple instances might be started and stopped.

I couldn't determine if the second match was a valid issue, but if it's only used from test code, then it is indeed not.

[bboreham]

The nameserver isn't stopped in ordinary running, just in tests.

[bboreham]

#2857 fixed most of this, with the exception of the obsolete http client used in the Docker proxy, which needs further thought.

[bboreham]

I ran staticcheck again; I filtered out warnings ST1005 and SA1015:

ipam/allocator.go:215:25: func (*Allocator).cancelOp is unused (U1000)
ipam/allocator_test.go:20:2: const testStart2 is unused (U1000)
ipam/allocator_test.go:21:2: const testStart3 is unused (U1000)
ipam/allocator_test.go:105:3: const donateSize is unused (U1000)
ipam/allocator_test.go:106:3: const donateStart is unused (U1000)
ipam/allocator_test.go:339:5: identical expressions on the left and right side of the '||' operator (SA4000)
ipam/http.go:68:34: http.CloseNotifier is deprecated: the CloseNotifier interface predates Go's context package. New code should use Request.Context instead.  (SA1019)
ipam/http.go:80:34: http.CloseNotifier is deprecated: the CloseNotifier interface predates Go's context package. New code should use Request.Context instead.  (SA1019)
ipam/http_test.go:165:3: http.DefaultTransport.(*http.Transport).CancelRequest is deprecated: Use Request.WithContext to create a request with a cancelable context instead. CancelRequest cannot cancel HTTP/2 requests.  (SA1019)
ipam/paxos/paxos_test.go:59:17: func (*Model).linkExists is unused (U1000)
ipam/space/space_test.go:91:3: const testAddr2 is unused (U1000)
ipam/space/space_test.go:94:3: const containerID is unused (U1000)
ipam/space/space_test.go:133:3: const testAddrx is unused (U1000)
ipam/space/space_test.go:134:3: const testAddry is unused (U1000)
ipam/space/space_test.go:135:3: const containerID is unused (U1000)
ipam/space/space_test.go:194:3: const testAddr2 is unused (U1000)
ipam/testutils_test.go:27:6: func toStringArray is unused (U1000)
ipam/testutils_test.go:100:6: func ExpectMessage is unused (U1000)
ipam/testutils_test.go:208:6: func AssertNothingSentErr is unused (U1000)
ipam/tracker/awsvpc.go:47:13: session.New is deprecated: Use NewSession functions to create sessions instead. NewSession has the same functionality as New except an error can be returned when the func is called instead of waiting to receive an error until a request is made.  (SA1019)
nameserver/dns_test.go:46:17: this result of append is never used, except maybe in other appends (SA4010)
net/ipsec/ipsec.go:427:21: func (*IPSec).removeDropNonEncryptedInbound is unused (U1000)
npc/controller_test.go:517:3: const fooPodIP is unused (U1000)
npc/namespace.go:158:15: func (*ns).checkLocalPod is unused (U1000)
npc/namespace.go:159:2: should use 'return <expr>' instead of 'if <expr> { return <bool> }; return <bool>' (S1008)
plugin/skel/listener.go:204:27: func (*listener).discoverNew is unused (U1000)
plugin/skel/listener.go:213:27: func (*listener).discoverDelete is unused (U1000)
prog/kube-utils/main.go:346:16: the channel used with signal.Notify should be buffered (SA1017)
prog/kube-utils/peerlist_test.go:19:2: const testIters is unused (U1000)
prog/kube-utils/peerlist_test.go:143:3: this value of storedPeerList is never used (SA4006)
prog/weave-npc/main.go:260:33: client.Core is deprecated: Core retrieves the default version of CoreClient. Please explicitly pick a version.  (SA1019)
prog/weave-npc/main.go:280:34: client.Core is deprecated: Core retrieves the default version of CoreClient. Please explicitly pick a version.  (SA1019)
prog/weaveutil/plugin_network.go:55:12: client.NewEnvClient is deprecated: use NewClientWithOpts(FromEnv)  (SA1019)
proxy/common.go:18:24: should use raw string (`...`) with regexp.MustCompile to avoid having to escape twice (S1007)
proxy/common.go:20:2: var weaveEntrypoint is unused (U1000)
proxy/common.go:21:2: var weaveContainerName is unused (U1000)
proxy/proxy_intercept.go:36:12: httputil.NewClientConn is deprecated: Use the Client or Transport in package net/http instead.  (SA1019)
proxy/proxy_intercept.go:40:26: httputil.ErrPersistEOF is deprecated: No longer used.  (SA1019)
proxy/proxy_intercept.go:72:70: httputil.ClientConn is deprecated: Use Client or Transport in package net/http instead.  (SA1019)
proxy/proxy_intercept.go:131:76: httputil.ClientConn is deprecated: Use Client or Transport in package net/http instead.  (SA1019)
proxy/proxy_intercept.go:158:44: httputil.ClientConn is deprecated: Use Client or Transport in package net/http instead.  (SA1019)
router/frame_crypto.go:271:2: default case should be first or last in switch statement (ST1015)
router/overlay_switch.go:337:5: should omit comparison to bool constant, can be simplified to healthy (S1002)
router/overlay_switch.go:343:5: should omit comparison to bool constant, can be simplified to !healthy (S1002)
router/pcap.go:146:28: should use make([]byte, pktLen) instead (S1019)
test/tls/generate_certs.go:18:2: const serverKeyPath is unused (U1000)
test/tls/generate_certs.go:19:2: const serverCertPath is unused (U1000)
testing/util.go:32:6: func stackTrace is unused (U1000)

So the one remaining issue from before is still there. Some of the other warnings sound interesting.

I believe Go's http proxy now supports WebSockets directly, which may simplify upgrading.