Fix flaky tests and fix #69 #67
Conversation
joao-r-reis
requested review from
alicel,
grighetto,
absurdfarce and
weideng1
as code owners
…to add-tls-test-debug
integration-tests/tls_test.go
Outdated
| @@ -1056,16 +1057,36 @@ func testProxyClientTls(t *testing.T, ccmSetup *setup.CcmTestSetup, | |||
|
|
|||
| logMessages := buffer.String() | |||
|
|
|||
| warnCheckFail := false | |||
| warnCheckDone := false | |||
There was a problem hiding this comment.
The Done suffix here confused me for a second, I thought it would be set when the result of some async processing was done, but in reality this value is simply set when a warning is expected. My suggestion would be to name this warningExpected instead. The same goes for errCheckDone below, errExpected.
integration-tests/tls_test.go
Outdated
| } | ||
| } | ||
| if errCheckDone && warnCheckDone { | ||
| require.True(t, !errCheckFail || !warnCheckFail) // only 1 check needs to pass in this scenario |
There was a problem hiding this comment.
Nit: I think this would be clearer given the other requires are False too:
require.False(t, errCheckFail && warnCheckFail)
There was a problem hiding this comment.
I actually set this to require.True because to me it felt easier to understand but since I added a comment anyway I can use require.False to be consistent with the others
| @@ -1340,6 +1361,14 @@ func getClientSideVerifyConnectionCallback(rootCAs *x509.CertPool) func(cs tls.C | |||
| } | |||
| } | |||
| _, err := cs.PeerCertificates[0].Verify(opts) | |||
| if err != nil { | |||
| // use zerolog to avoid interacting with proxy's log messages that are used for assertions in the test | |||
There was a problem hiding this comment.
Just for my understanding, are we suppressing the log messages from a lib to avoid throwing our log assertions off?
There was a problem hiding this comment.
The goal is to avoid throwing our log assertions off but we aren't suppressing any log messages. We are capturing the log output from the logrus library which is what the proxy uses and the test client uses zerolog. To avoid messing with the log assertions I decided to use zerolog here on this test code in particular instead of logrus
| if !protocolErrOccurred { | ||
| protocolErrOccurred = true | ||
| cc.sendResponseToClient(protocolErrResponseFrame) | ||
| cc.clientHandlerShutdownRequestCancelFn() |
There was a problem hiding this comment.
It looks like clientHandlerShutdownRequestCancelFn is not called anywhere anymore, we should probably remove it from the struct.
There was a problem hiding this comment.
We want to call it once #68 is addressed, maybe I can add a comment about this
| setDrainModeNowFunc() | ||
| continue | ||
| } | ||
| alreadySentProtocolErr = protocolErrResponseFrame |
There was a problem hiding this comment.
should this assignment happen after cc.sendResponseToClient(protocolErrResponseFrame)?
There was a problem hiding this comment.
Doesn't really matter because there is no concurrency here, it's a single goroutine with a for loop
There was a problem hiding this comment.
yep it just looked a bit more intuitive but it doesn't really matter.
| protocolErrOccurred = true | ||
| cc.sendResponseToClient(protocolErrResponseFrame) | ||
| continue | ||
| } else if alreadySentProtocolErr != nil { |
There was a problem hiding this comment.
Just to check that I understand correctly. The intended behaviour with this change is: if a protocol error is sent on the connection, then we want to ensure that all other potentially pending responses that may still be returned are replaced by that protocol error (unless they were themselves protocol errors in the first place). So we're keeping the already returned protocol error to replace the frame of any subsequent non-protocol error response with it.
The idea is to ensure that we do not send seemingly valid responses on a connection on which a protocol error was returned while this connection is being drained.
Is this correct?
There was a problem hiding this comment.
Yes, but also the main change is that we no longer trigger the connection shutdown mechanism so the connection will be left in an unusable state (constant protocol errors) and it will be up to the driver to close it (at least until we address #68 )
Fix #69
Fix more flaky tests
┆Issue is synchronized with this Jira Task by Unito
┆friendlyId: ZDM-412