alts: add ALTS config and integration test #4468
Conversation
Signed-off-by: Lizan Zhou <zlizan@google.com>
|
Just posting out PR here. This needs grpc/grpc#16648 to get build. cc @JimmyCYJ |
| namespace TransportSockets { | ||
| namespace Alts { | ||
|
|
||
| typedef CSmartPtr<grpc_alts_credentials_options, grpc_alts_credentials_options_destroy> |
There was a problem hiding this comment.
Can you add a comment on this CSmartPtr above? How does it behave, i.e. is it functionally a unique_ptr for C malloc/free?
|
|
||
| // Returns true if the peer's service account is found in peers, otherwise | ||
| // returns false and fills out err with an error message. | ||
| static bool doValidate(const tsi_peer& peer, const std::unordered_set<std::string>& peers, |
There was a problem hiding this comment.
Nit: prefer using anonymous namespace in .,cc to static functions in Envoy style.
| static bool doValidate(const tsi_peer& peer, const std::unordered_set<std::string>& peers, | ||
| std::string& err) { | ||
| for (size_t i = 0; i < peer.property_count; ++i) { | ||
| std::string name = std::string(peer.properties[i].name); |
| static bool doValidate(const tsi_peer& peer, const std::unordered_set<std::string>& peers, | ||
| std::string& err) { | ||
| for (size_t i = 0; i < peer.property_count; ++i) { | ||
| std::string name = std::string(peer.properties[i].name); |
There was a problem hiding this comment.
Do we have a length here to bound the string as with value below?
| Network::TransportSocketFactoryPtr | ||
| UpstreamAltsTransportSocketConfigFactory::createTransportSocketFactory( | ||
| const Protobuf::Message& message, Server::Configuration::TransportSocketFactoryContext&) { | ||
| auto config = |
There was a problem hiding this comment.
Should we have a templated typed wrapper here similar to https://github.com/envoyproxy/envoy/blob/master/source/extensions/filters/http/common/factory_base.h#L15 to eliminate boilerplate for transport socket filters?
| // in this use case. The validation will be performed by TsiSocket with the validator. | ||
| tsi_result status = alts_tsi_handshaker_create( | ||
| options.get(), "", handshaker_service.c_str(), true /* is_client */, &handshaker); | ||
| CHandshakerPtr handshaker_ptr{handshaker}; |
There was a problem hiding this comment.
Should this be moved to after the status check below (for clarity, rather than correctness)?
| return std::make_unique<TsiHandshaker>(std::move(handshaker_ptr), dispatcher); | ||
| }, | ||
| validator); | ||
| } |
There was a problem hiding this comment.
Upstream & downstream seem quite similar; any way to reasonably refactor?
|
|
||
| std::string server_address = Network::Test::getLoopbackAddressUrlString(version_) + ":0"; | ||
| grpc::ServerBuilder builder; | ||
| builder.AddListeningPort(server_address, grpc::InsecureServerCredentials(), |
There was a problem hiding this comment.
I'm guessing this is the most expedient way of setting up a fake server. The main downside is that when we hit races and other flakes, Envoy folks are going to have to be become knowledgable about Google gRPC internals, which is a steep cliff.
There was a problem hiding this comment.
Yes and No, this is almost the only way to setup integration test that can runs simultaneously. (Alternative is run another binary with Python and setup test with env or arg). Given we already have integration test for GoogleGrpc, which is subject same races or flakes from Google gRPC and they are dealing with more internal stuff, so I thought this is fine. Thoughts?
| return makeAltsConnection(); | ||
| }; | ||
| testRouterRequestAndResponseWithBody(1024, 512, false, &creator); | ||
| } |
There was a problem hiding this comment.
Are there more conditions to test, e.g. with/without peers etc?
|
|
||
| if (status != TSI_OK) { | ||
| ENVOY_LOG_MISC(warn, "Cannot create ATLS server handshaker, status: {}", status); | ||
| return nullptr; |
There was a problem hiding this comment.
If a null handshaker is returned, tsi_socket needs handle the failure below.
| void TsiSocket::onConnected() { ASSERT(!handshake_complete_); } | ||
| void TsiSocket::onConnected() { | ||
| ASSERT(!handshake_complete_); | ||
| doHandshakeNext(); |
There was a problem hiding this comment.
I understand that handshaker creation was moved to doHandshakeNext() because you called doHandhsakeNext() here.
But what's the purpose of making a call to doHandshakeNext() earlier at this point?
My worry is that localAddress() may not be initialized so far, can you check that?
There was a problem hiding this comment.
By doing this it is more efficient to not repeatedly call onHandshakerNext when the transport socket didn't received new data. The first time here we skip the empty check on read_buffer but the rest have. (The first next call for client need to be done when it is empty)
No worries about localAddress, since this is called at same stack that doWrite is called.
https://github.com/envoyproxy/envoy/blob/master/source/common/network/connection_impl.cc#L468
There was a problem hiding this comment.
Thanks for the explanation! To prevent future changes break this condition, can you add an assert before creating handshaker_ in line:52 to check local_address is initialized?
Signed-off-by: Lizan Zhou <zlizan@google.com>
Signed-off-by: Lizan Zhou <zlizan@google.com>
|
@JimmyCYJ can you ping me in this PR when you're ready for another round of review. I see there are a number of open comments still and we're waiting for the gRPC PR to merge for this to build under CI. |
@lizan please address comments for next round of review, and let me know if there are any missing parts that I can work on. |
|
Closing this, @JimmyCYJ will open other one from istio folk. |
|
I will address review comments in PR #4559, thanks. |
Signed-off-by: Lizan Zhou zlizan@google.com
Description:
Last one for #3429, introduce ALTS config and add integration test.
Risk Level: Low (extension)
Testing: manual test, integration test, CI
Docs Changes: Done
Release Notes:
Fixes #3429.