alts: add ALTS config and integration test #4559

JimmyCYJ · 2018-09-28T20:24:52Z

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.

Signed-off-by: JimmyCYJ jimmychen.0102@gmail.com

Signed-off-by: Lizan Zhou <zlizan@google.com>

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

danzh2010 · 2018-10-01T14:50:27Z

source/extensions/transport_sockets/alts/config.cc

+  HandshakerFactory factory =
+      [handshaker_service](Event::Dispatcher& dispatcher,
+                           const Network::Address::InstanceConstSharedPtr&,
+                           const Network::Address::InstanceConstSharedPtr&) -> TsiHandshakerPtr {


Can you add a check that |local_address| passed-in should be initialized?

Per offline discussion with @lizan checking nullptr is enough here.

danzh2010 · 2018-10-01T14:53:26Z

test/extensions/transport_sockets/alts/alts_integration_test.cc

+                      std::to_string(fake_handshaker_server_port_));
+}
+
+void AltsIntegrationTest::initialize() {


Is it Envoy code style that member definition has to be outside of class declaration?

Moved all definitions into declarations, just like other integration test files.

danzh2010 · 2018-10-01T15:03:16Z

test/extensions/transport_sockets/alts/alts_integration_test.cc

+  };
+  testRouterRequestAndResponseWithBody(1024, 512, false, &creator);
+}
+


I don't know how possible it is with grpc fake handshake service, but it'll be great if you can add a handshake failure test and peer validation failure test.

Added tests to cover peer validation failure at both sides. Handshake failure case is not easy to add, which might involve changes to fake handshake server.

One thing you might be able to do is point handshaker_service to non-exist handshaker server, that will fail handshake.

I have added one test which sets a wrong address to client's config, so that client fails to do handshake. For the server side, it is weird that Envoy always connects to the fake handshaker server even I set an arbitrary string as handshaker server's address. I am still debugging it.

After discussing with @jiangtaoli2016 and @yihuazhang, we figure out why Envoy can still talk to fake handshaker server even the handshaker url is " ". This is because in integration test, client and Envoy are sharing one gRPC binary. When client starts talking to fake handshaker server, init_shared_resources() is called and kSharedResource->channel is created. Later when Envoy talks to fake handshaker server, it reuses the kSharedResource->channel and continues handshake process.

In real application use, two handshake clients (e.g. two Envoys) are depending on separate gRPC library, so this issue will not happen.

I have removed TEST_P(AltsIntegrationTestServerWrongHandshaker, ConnectToWrongHandshakerAddress), as TEST_P(AltsIntegrationTestClientWrongHandshaker, ConnectToWrongHandshakerAddress) is sufficient to cover the error case.

stale · 2018-10-08T15:07:38Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

htuch · 2018-10-09T01:18:52Z

@JimmyCYJ can you ping when this is ready for review? Thanks.

JimmyCYJ · 2018-10-09T01:41:47Z

can you ping when this is ready for review? Thanks.

Sure, I am working on it this week.

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

lizan · 2018-10-10T02:39:04Z

can you merge master? coverage fails due to that.

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

JimmyCYJ · 2018-10-15T17:03:25Z

bazel/repository_locations.bzl

+        # TODO(lizan): Use release once new version released.
+        sha256 = "dcd972e32e39e41f6248c7f8252676de2993d93289da842b72e271d0a89ed150",
+        strip_prefix = "grpc-1.16.0-pre1",
+        urls = ["https://github.com/grpc/grpc/archive/v1.16.0-pre1.tar.gz"],


grpc-1.16.0 is going to be released on 10/22/2018

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

htuch · 2018-10-23T03:31:22Z

@JimmyCYJ is this ready for review yet?

JimmyCYJ · 2018-10-23T04:02:31Z

is this ready for review yet?

@htuch Yes, this is ready for review. gRPC 1.16.0 will be released this week. I will update gRPC dependency when 1.16.0 is released.

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

JimmyCYJ · 2018-10-23T16:59:57Z

gRPC version is updated.

JimmyCYJ · 2018-10-24T01:18:55Z

@htuch This is ready for review. Please take a look. Thanks.

htuch · 2018-10-24T21:35:17Z

api/envoy/config/transport_socket/alts/v2alpha/alts.proto

+// https://cloud.google.com/security/encryption-in-transit/application-layer-transport-security/
+message Alts {
+  // The location of a handshaker service, this is usually 169.254.169.254:8080
+  // on GCE


Nit: end sentences with .

htuch · 2018-10-24T21:37:18Z

source/extensions/transport_sockets/alts/config.cc

+      MessageUtil::downcastAndValidate<const envoy::config::transport_socket::alts::v2alpha::Alts&>(
+          message);
+
+  std::string handshaker_service = config.handshaker_service();


Nit: constify the rest of these.

htuch · 2018-10-24T21:42:18Z

source/extensions/transport_sockets/alts/config.cc

+  }
+
+  HandshakerFactory factory =
+      [handshaker_service](Event::Dispatcher& dispatcher,


Can we refactor this and the upstream case to something that has most of the common logic?

htuch · 2018-10-24T21:42:57Z

test/extensions/transport_sockets/alts/alts_integration_test.cc

+                                  ->mutable_listeners(0)
+                                  ->mutable_filter_chains(0)
+                                  ->mutable_transport_socket();
+      std::string yaml = R"EOF(


htuch · 2018-10-24T21:43:15Z

test/extensions/transport_sockets/alts/alts_integration_test.cc

+
+  void initialize() override {
+    config_helper_.addConfigModifier([this](envoy::config::bootstrap::v2::Bootstrap& bootstrap) {
+      auto transport_socket = bootstrap.mutable_static_resources()


Nit: auto transport_socket*

htuch · 2018-10-24T22:20:35Z

test/extensions/transport_sockets/alts/alts_integration_test.cc

+      handshaker_service: ")EOF" +
+                         fakeHandshakerServerAddress(server_connect_handshaker_) + "\"";
+      if (!server_peer_identity_.empty()) {
+        yaml.replace(yaml.find("[]"), std::string::size_type(2), server_peer_identity_);


See next comment..

htuch · 2018-10-24T22:21:29Z

test/extensions/transport_sockets/alts/alts_integration_test.cc

+      handshaker_service: ")EOF" +
+                              fakeHandshakerServerAddress(client_connect_handshaker_) + "\"";
+    if (!client_peer_identity_.empty()) {
+      client_yaml.replace(client_yaml.find("[]"), std::string::size_type(2), client_peer_identity_);


I think it'd be just cleaner to explicitly work with the protobuf typed object than this kind of sed-like replacement.. or at least use fmt::format..

htuch · 2018-10-24T22:27:19Z

source/extensions/transport_sockets/alts/tsi_socket.cc

@@ -54,6 +46,20 @@ Network::PostIoAction TsiSocket::doHandshake() {
 void TsiSocket::doHandshakeNext() {
  ENVOY_CONN_LOG(debug, "TSI: doHandshake next: received: {}", callbacks_->connection(),
                 raw_read_buffer_.length());
+
+  if (!handshaker_) {
+    handshaker_ = handshaker_factory_(callbacks_->connection().dispatcher(),


Missing coverage.

@JimmyCYJ LGTM, but were you planning on adding coverage here?

Hmm this shouldn't be missing coverage if integration test ran in coverage CI...

@JimmyCYJ for the coverage missing in L54, I think you can easily add it in tsi_socket unit test.

@htuch @lizan Sorry for the slow PR update. Yes, I was planning to add unit test into tsi_socket_test.cc but don't get a chance to do so. I will add test soon and ping you to review. Thanks for asking.

@htuch Please take a look. Thanks.

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

htuch

Awesome, thanks!

lizan and others added 8 commits September 19, 2018 11:56

alts: add ALTS config and integration test

9c14ac2

Signed-off-by: Lizan Zhou <zlizan@google.com>

build fix, review comments

6eee37a

Signed-off-by: Lizan Zhou <zlizan@google.com>

Merge remote-tracking branch 'upstream/master' into alts_config

be8ce3d

const

f02c977

Signed-off-by: Lizan Zhou <zlizan@google.com>

fix format, handle nullptr

3018c8e

Signed-off-by: Lizan Zhou <zlizan@google.com>

Merge remote-tracking branch 'upstream/master' into alts_config

dd504fb

fix merge

15229f1

Signed-off-by: Lizan Zhou <zlizan@google.com>

Merge remote-tracking branch 'envoyupstream/master' into alts_config

e27d14f

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

JimmyCYJ requested a review from htuch as a code owner September 28, 2018 20:24

JimmyCYJ mentioned this pull request Sep 28, 2018

alts: add ALTS config and integration test #4468

Closed

mattklein123 assigned lizan Sep 30, 2018

danzh2010 suggested changes Oct 1, 2018

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 8, 2018

Merge remote-tracking branch 'envoyupstream/master' into alts_config

33bc8f9

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Oct 9, 2018

JimmyCYJ added 3 commits October 9, 2018 14:51

Revise per comments.

d54e8a5

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Add tests

04a4ce4

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Fix format.

a2b9f48

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

JimmyCYJ added 7 commits October 10, 2018 10:14

Merge remote-tracking branch 'envoyupstream/master' into alts_config

f223e03

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Add test

fad2108

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

fix format.

c90c507

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Modify port.

c92aecb

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Add test.

1a0b703

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

debug

d54038a

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

fix format

805ed96

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

JimmyCYJ commented Oct 15, 2018

View reviewed changes

JimmyCYJ added 5 commits October 15, 2018 10:09

Remove debug string

3ddabb2

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Merge remote-tracking branch 'envoyupstream/master' into alts_config

3d84bbb

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Merge remote-tracking branch 'envoyupstream/master' into alts_config

303bfc6

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Merge remote-tracking branch 'envoyupstream/master' into alts_config

c861126

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Remove test.

2373635

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Update gRPC version

1974883

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

JimmyCYJ changed the title ~~[WIP] alts: add ALTS config and integration test~~ alts: add ALTS config and integration test Oct 24, 2018

htuch suggested changes Oct 24, 2018

View reviewed changes

JimmyCYJ added 6 commits October 25, 2018 18:07

Revise per comments.

73e485a

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

fix format.

26f84b9

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Revise per comment.

2d4bff4

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Fix format.

47a9746

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Add test.

8fc06e0

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Merge remote-tracking branch 'envoyupstream/master' into alts_config

877e23f

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

lizan approved these changes Oct 28, 2018

View reviewed changes

htuch approved these changes Oct 29, 2018

View reviewed changes

htuch merged commit f9edb52 into envoyproxy:master Oct 29, 2018

JimmyCYJ deleted the alts_config branch October 29, 2018 04:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

alts: add ALTS config and integration test #4559

alts: add ALTS config and integration test #4559

JimmyCYJ commented Sep 28, 2018

danzh2010 Oct 1, 2018

JimmyCYJ Oct 10, 2018

danzh2010 Oct 1, 2018

JimmyCYJ Oct 10, 2018

danzh2010 Oct 1, 2018

JimmyCYJ Oct 10, 2018

lizan Oct 10, 2018

JimmyCYJ Oct 15, 2018 •

edited

Loading

JimmyCYJ Oct 22, 2018 •

edited

Loading

stale bot commented Oct 8, 2018

htuch commented Oct 9, 2018

JimmyCYJ commented Oct 9, 2018

lizan commented Oct 10, 2018

JimmyCYJ Oct 15, 2018

htuch commented Oct 23, 2018

JimmyCYJ commented Oct 23, 2018 •

edited

Loading

JimmyCYJ commented Oct 23, 2018

JimmyCYJ commented Oct 24, 2018

htuch Oct 24, 2018

htuch Oct 24, 2018

htuch Oct 24, 2018

htuch Oct 24, 2018

htuch Oct 24, 2018

htuch Oct 24, 2018

htuch Oct 24, 2018

htuch Oct 24, 2018

htuch Oct 26, 2018

lizan Oct 26, 2018

lizan Oct 26, 2018

JimmyCYJ Oct 26, 2018

JimmyCYJ Oct 28, 2018

htuch left a comment

alts: add ALTS config and integration test #4559

alts: add ALTS config and integration test #4559

Conversation

JimmyCYJ commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimmyCYJ Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

JimmyCYJ Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

stale bot commented Oct 8, 2018

htuch commented Oct 9, 2018

JimmyCYJ commented Oct 9, 2018

lizan commented Oct 10, 2018

Choose a reason for hiding this comment

htuch commented Oct 23, 2018

JimmyCYJ commented Oct 23, 2018 • edited Loading

JimmyCYJ commented Oct 23, 2018

JimmyCYJ commented Oct 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

JimmyCYJ Oct 15, 2018 •

edited

Loading

JimmyCYJ Oct 22, 2018 •

edited

Loading

JimmyCYJ commented Oct 23, 2018 •

edited

Loading