security,jwtauthccl: Fix test failures due to leaked goroutines #129802
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @BabuSrithar and @pritesh-lahoti)
pkg/util/leaktest/leaktest.go line 67 at r1 (raw file):
// Ignore pq goroutine that watches for context cancellation. strings.Contains(stack, "github.com/lib/pq.(*conn).watchCancel") || // Ignore TLS handshake related goroutine.
There was an issue with net/http.addTLS referenced here: golang/go#43966
The fix for this went in on March 8th and the fix is on go1.23(13th August). If think this won't be needed if we are updating go version although the error signature is slightly different
- crypto/tls.generateECDHEParameters(0x91a580, 0xc00007e570, 0xc00f870019, 0x1, 0x58, 0x0, 0x0)
- /usr/lib/go/src/crypto/tls/key_schedule.go:132 +0x330 fp=0xc35abd1b10 sp=0xc35abd1a50 pc=0x5f9210
- crypto/tls.(*clientHandshakeStateTLS13).processHelloRetryRequest(0xc35abd1df0, 0x0, 0x0)
- /usr/lib/go/src/crypto/tls/handshake_client_tls13.go:227 +0x29d fp=0xc35abd1c00 sp=0xc35abd1b10 pc=0x5e8cdd
- crypto/tls.(*clientHandshakeStateTLS13).handshake(0xc35abd1df0, 0xc2d94eed50, 0x4)
- /usr/lib/go/src/crypto/tls/handshake_client_tls13.go:65 +0x38d fp=0xc35abd1c50 sp=0xc35abd1c00 pc=0x5e848d
+ crypto/tls.(*Conn).verifyServerCertificate(0xc008412a88, {0xc009aa5ce0, 0x1, 0x1})
+ GOROOT/src/crypto/tls/handshake_client.go:997 +0x819
+ crypto/tls.(*clientHandshakeStateTLS13).readServerCertificate(0xc002663bd0)
+ GOROOT/src/crypto/tls/handshake_client_tls13.go:531 +0x273
+ crypto/tls.(*clientHandshakeStateTLS13).handshake(0xc002663bd0)
+ GOROOT/src/crypto/tls/handshake_client_tls13.go:96 +0x29aThe error seems to be from the persistenConn reference and may fix this.
To be sure, I feel we should remove the rule and check once we update to 1.23.
There was a problem hiding this comment.
Reviewable status:
pkg/util/leaktest/leaktest.go line 67 at r1 (raw file):
Previously, souravcrl wrote…
There was an issue with
net/http.addTLSreferenced here: golang/go#43966
The fix for this went in on March 8th and the fix is on go1.23(13th August). If think this won't be needed if we are updating go version although the error signature is slightly different- crypto/tls.generateECDHEParameters(0x91a580, 0xc00007e570, 0xc00f870019, 0x1, 0x58, 0x0, 0x0) - /usr/lib/go/src/crypto/tls/key_schedule.go:132 +0x330 fp=0xc35abd1b10 sp=0xc35abd1a50 pc=0x5f9210 - crypto/tls.(*clientHandshakeStateTLS13).processHelloRetryRequest(0xc35abd1df0, 0x0, 0x0) - /usr/lib/go/src/crypto/tls/handshake_client_tls13.go:227 +0x29d fp=0xc35abd1c00 sp=0xc35abd1b10 pc=0x5e8cdd - crypto/tls.(*clientHandshakeStateTLS13).handshake(0xc35abd1df0, 0xc2d94eed50, 0x4) - /usr/lib/go/src/crypto/tls/handshake_client_tls13.go:65 +0x38d fp=0xc35abd1c50 sp=0xc35abd1c00 pc=0x5e848d + crypto/tls.(*Conn).verifyServerCertificate(0xc008412a88, {0xc009aa5ce0, 0x1, 0x1}) + GOROOT/src/crypto/tls/handshake_client.go:997 +0x819 + crypto/tls.(*clientHandshakeStateTLS13).readServerCertificate(0xc002663bd0) + GOROOT/src/crypto/tls/handshake_client_tls13.go:531 +0x273 + crypto/tls.(*clientHandshakeStateTLS13).handshake(0xc002663bd0) + GOROOT/src/crypto/tls/handshake_client_tls13.go:96 +0x29aThe error seems to be from the
persistenConnreference and may fix this.
To be sure, I feel we should remove the rule and check once we update to 1.23.
The fix for the issue: golang/go@8590f0a
There was a problem hiding this comment.
Reviewable status:
We have been seeing intermittent test failures for `TestUseCerts` and `TestJWTAuthWithCustomCACert`. These failures have been due to a leaked goroutine that establishes a TLS handshake. The change is to ignore this goroutine while checking for leaked goroutines. Added a TODO to revisit this once we update Go to 1.23, as this seems to have been fixed: golang/go#62227. Epic: CRDB-36214, CRDB-40867 Fixes: cockroachdb#119052, cockroachdb#128214 Release note: None
pritesh-lahoti
force-pushed
the
skip-leaked-goroutine
branch
from
54fd947
to
155e2d3
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/util/leaktest/leaktest.go line 67 at r1 (raw file):
Previously, souravcrl wrote…
The fix for the issue: golang/go@8590f0a
Thank you! Added a TODO for the same.
|
bors r=souravcrl |
pritesh-lahoti
added
backport-24.2.1-rc
backport-24.2.x
|
blathers backport 24.2 24.2.1-rc |
![blathers[crl]](https://avatars.github.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches. Issue #119052: branch-release-24.2, branch-release-24.2.1-rc. Issue #128214: branch-release-24.2.1-rc. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
We have been seeing intermittent test failures for
TestUseCertsandTestJWTAuthWithCustomCACert.These failures have been due to a leaked goroutine that establishes a TLS handshake.
The change is to ignore this goroutine while checking for leaked goroutines.
Added a TODO to revisit this once we update Go to 1.23, as this seems to
have been fixed: golang/go#62227.
Epic: CRDB-36214, CRDB-40867
Fixes: #119052, #128214
Release note: None