alts: Perform full handshake in ALTS tests. #6177
Conversation
|
|
||
| // Close closes all open connections to the handshaker service. | ||
| // | ||
| // For testing purposes only. |
There was a problem hiding this comment.
This seems like a generic close function. Why is it expected to be used in tests only?
There was a problem hiding this comment.
In practice, the connection to the handshaker service is kept open until the application is killed, so this Close function is never needed/called.
However, in the e2e test, we need to do call this Close function so that we don't get a goroutine leak.
There was a problem hiding this comment.
Why can't you explicitly close the connection from the test?
There was a problem hiding this comment.
Disclaimer: I'm not a Go expert so apologies if I'm missing something obvious. :)
The hsConnMap, which holds the connection, is not exported. So we have 2 options:
- Add a test-only
CloseAPI to close any connections in the map, which we call inTestFullHandshake. - Call
Dialto get the same connection and then manually close it. This feels hacky, because we never want users to callDial(it should only be called by the internals of the ALTS libraries) and so I didn't want to set this precedent).
WDYT?
There was a problem hiding this comment.
Thanks for the exaplaination. How about calling the method CloseForTesting or something which makes is very obvious that the method is only for testing purposes. We do that regularly in our codebase. Thanks.
There was a problem hiding this comment.
Sure thing, done.
|
|
||
| // Close closes all open connections to the handshaker service. | ||
| // | ||
| // For testing purposes only. |
There was a problem hiding this comment.
Why can't you explicitly close the connection from the test?
credentials/alts/alts_test.go
Outdated
| t.Fatalf("LocalTCPListener() failed: %v", err) | ||
| } | ||
| s := grpc.NewServer() | ||
| altspb.RegisterHandshakerServiceServer(s, &testutil.FakeHandshaker{}) |
There was a problem hiding this comment.
This should be altsgrpc with a separate import.
There was a problem hiding this comment.
Done, sorry I missed this one. :)
|
|
||
| // Close closes all open connections to the handshaker service. | ||
| // | ||
| // For testing purposes only. |
There was a problem hiding this comment.
Thanks for the exaplaination. How about calling the method CloseForTesting or something which makes is very obvious that the method is only for testing purposes. We do that regularly in our codebase. Thanks.
|
One of the tests is failing because of a flake which we recently fixed. I've kicked off another run of the tests. If it fails again, maybe rebase your branch to master. |
The ALTS tests in this repo do not perform full ALTS handshakes against a fake handshaker service. This leads to changes in this repo breaking e2e/interop tests once the change is imported internally. This PR will (hopefully) reduce the frequency with which that happens.
RELEASE NOTES: none