grpc: SSL credential support for Google gRPC client. by htuch · Pull Request #2598 · envoyproxy/envoy

htuch · 2018-02-13T22:33:44Z

Risk Level: Low (not in use).
Testing: new unit tests to validate SSL connections (and client certs) for all gRPC client types.
Added SSL to ads_integration_test to validate end-to-end.

Signed-off-by: Harvey Tuch htuch@google.com

Risk Level: Low (not in use). Testing: new unit tests to validate SSL connections (and client certs) for all gRPC client types. Added SSL to ads_integration_test to validate end-to-end. Signed-off-by: Harvey Tuch <htuch@google.com>

Signed-off-by: Harvey Tuch <htuch@google.com>

ggreenway

This looks good overall. Just one comment about the interface to DataSource.

ggreenway · 2018-02-13T23:18:22Z

source/common/config/datasource.h

+// envoy::api::v2::core::DataSource helper.
+class DataSource {
+public:
+  DataSource(const envoy::api::v2::core::DataSource& source) : source_(source) {}


Given how this class is used, the API is a bit weird: every use constructs a temporary, calls one method, then destructs it. Instead, should we just have two static functions here, that both take a DataSource& ?

I agree with Greg, this feels weird. Is there any reason those are not just namespaced functions that take source as a parameter, similarly to those in Envoy::Filesystem?

Was assuming the compiler would be smart enough to optimize any overhead here away, since the class is entirely const/immutable everywhere, but sure, I can make it just some namespace functions if you folks prefer this convention.

Signed-off-by: Harvey Tuch <htuch@google.com>

Done

htuch · 2018-02-14T18:41:26Z

@zuercher @ggreenway It looks like the Mac failure is genuine and probably related to this PR. I added SSL loopback for ads_integration_test, now it times out. Any thoughts on how I can debug this without an OS X setup? Should I hack on do_ci.sh to get it to print additional useful stuff until I figure it out?

ggreenway · 2018-02-14T19:01:35Z

I think in circle-ci, as an envoy maintainer, you can tell it to re-run a job with ssh access. Can you attach a debugger in that context?

zuercher · 2018-02-14T22:06:15Z

@htuch it's stuck FakeHttpConnection::waitForNewStream:

    frame #5: 0x000000010014edd1 ads_integration_test`Envoy::FakeHttpConnection::waitForNewStream(Envoy::Event::Dispatcher&, bool) + 1153
    frame #6: 0x000000010000ff7a ads_integration_test`Envoy::(anonymous namespace)::AdsIntegrationTest::initialize() + 1114
    frame #7: 0x0000000100010cd8 ads_integration_test`Envoy::(anonymous namespace)::AdsIntegrationTest_Basic_Test::TestBody() + 72
    frame #8: 0x000000010001c17c ads_integration_test`non-virtual thunk to Envoy::(anonymous namespace)::AdsIntegrationTest_Basic_Test::TestBody() + 28

Which is the same as the other tests that periodically hang. Perhaps something about this test makes it hang more reliably.

htuch · 2018-02-15T15:15:59Z

@zuercher I dug up an old MBA and managed to get ads_integration_test running. It hangs post SSL handshake:

[2018-02-15 15:13:19.600][816425][trace][connection] source/common/ssl/ssl_socket.cc:58] [C2] ssl read returns: -1
[2018-02-15 15:13:19.600][816431][trace][connection] source/common/network/connection_impl.cc:415] [C1] read ready
[2018-02-15 15:13:19.600][816425][trace][connection] source/common/network/connection_impl.cc:377] [C2] socket event: 2
[2018-02-15 15:13:19.600][816425][trace][connection] source/common/network/connection_impl.cc:445] [C2] write ready
[2018-02-15 15:13:19.600][816431][debug][connection] source/common/ssl/ssl_socket.cc:100] [C1] handshake complete
[2018-02-15 15:13:19.601][816431][debug][client] source/common/http/codec_client.cc:52] [C1] connected
[2018-02-15 15:13:19.601][816431][trace][connection] source/common/ssl/ssl_socket.cc:58] [C1] ssl read returns: -1
[2018-02-15 15:13:24.598][816431][debug][main] source/server/server.cc:130] flushing stats
[2018-02-15 15:13:29.601][816431][debug][main] source/server/server.cc:130] flushing stats
[2018-02-15 15:13:34.605][816431][debug][main] source/server/server.cc:130] flushing stats
[2018-02-15 15:13:39.606][816431][debug][main] source/server/server.cc:130] flushing stats

Are there any quirks on OS X that came up when porting ssl_integration_test or xfcc_integration_test? Both of these do SSL loopback as well.

htuch · 2018-02-15T16:28:24Z

@lizan I think the issue here is that if the SSL handshake completes in the transport socket implementation at https://github.com/envoyproxy/envoy/blob/master/source/common/network/connection_impl.cc#L419, but there is pending write data, we never kick onWriteReady(). Thoughts on the best way to solve this?

To confirm this is the issue, I added an onWriteReady() after every onReadReady(), that unblocks the connection after SSL handshake.

ggreenway · 2018-02-15T17:00:07Z

I think in ConnectionImpl::raiseEvent(), add "if (event == connected && write buffer isn't empty) doWrite();" The socket will almost always be writable when the connected event is raised, and if it isn't, no harm is done (the write just won't accept any data).

Also, we should add a test-case for this (write before connected; get's stuck). This seems like an existing bug, not just specific to this PR.

htuch · 2018-02-15T17:03:58Z

@ggreenway That sounds reasonable. I'll do a separate PR for this.

ggreenway · 2018-02-15T17:45:05Z

👍

When a handshake completed in transport socket doRead(), we didn't check to see if there were any pending bytes in the write buffer in Network::ConnectionImpl. This showed up in envoyproxy#2598, where ads_integration_test would hang while waiting for the server to connect to the fake management upstream. Risk Level: Medium (messing around with connection handshaking can do bad things). Testing: Additional unit test for ConnectionImpl changes. Validated ads_integration_test now passes on OS X. Signed-off-by: Harvey Tuch <htuch@google.com>

When a handshake completed in transport socket doRead(), we didn't check to see if there were any pending bytes in the write buffer in Network::ConnectionImpl. This showed up in #2598, where ads_integration_test would hang while waiting for the server to connect to the fake management upstream. Risk Level: Medium (messing around with connection handshaking can do bad things). Testing: Additional unit test for ConnectionImpl changes. Validated ads_integration_test now passes on OS X. Signed-off-by: Harvey Tuch <htuch@google.com>

…oxy#2598)" This reverts commit d0aea18.

Automated changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action Signed-off-by: JP Simard <jp@jpsim.com>

grpc: SSL credential support for Google gRPC client.

517b6c4

Risk Level: Low (not in use). Testing: new unit tests to validate SSL connections (and client certs) for all gRPC client types. Added SSL to ads_integration_test to validate end-to-end. Signed-off-by: Harvey Tuch <htuch@google.com>

htuch assigned PiotrSikora and ggreenway Feb 13, 2018

Switch fmt header.

1dec119

Signed-off-by: Harvey Tuch <htuch@google.com>

ggreenway previously requested changes Feb 13, 2018

View reviewed changes

htuch added 2 commits February 14, 2018 11:04

Review feedback.

beaf5d1

Signed-off-by: Harvey Tuch <htuch@google.com>

Merge remote-tracking branch 'upstream/master' into grpc-ssl

a3947b5

ggreenway approved these changes Feb 14, 2018

View reviewed changes

htuch mentioned this pull request Feb 15, 2018

ssl: fix handshake hang that manifested on OS X. #2624

Merged

Merge remote-tracking branch 'upstream/master' into grpc-ssl

3ca99b3

htuch merged commit d0aea18 into envoyproxy:master Feb 15, 2018

htuch deleted the grpc-ssl branch February 15, 2018 21:02

lita pushed a commit to lita/envoy that referenced this pull request Feb 15, 2018

Revert "grpc: SSL credential support for Google gRPC client. (envoypr…

4957453

…oxy#2598)" This reverts commit d0aea18.

jpsim pushed a commit that referenced this pull request Nov 28, 2022

Update Envoy (#2598)

ec04858

Automated changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action Signed-off-by: JP Simard <jp@jpsim.com>

jpsim pushed a commit that referenced this pull request Nov 29, 2022

Update Envoy (#2598)

6c2d772

Automated changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action Signed-off-by: JP Simard <jp@jpsim.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

grpc: SSL credential support for Google gRPC client.#2598

grpc: SSL credential support for Google gRPC client.#2598
htuch merged 5 commits intoenvoyproxy:masterfrom
htuch:grpc-ssl

htuch commented Feb 13, 2018

Uh oh!

ggreenway left a comment

Uh oh!

ggreenway Feb 13, 2018

Uh oh!

PiotrSikora Feb 14, 2018

Uh oh!

htuch Feb 14, 2018

Uh oh!

htuch commented Feb 14, 2018

Uh oh!

ggreenway commented Feb 14, 2018

Uh oh!

zuercher commented Feb 14, 2018

Uh oh!

htuch commented Feb 15, 2018

Uh oh!

htuch commented Feb 15, 2018

Uh oh!

ggreenway commented Feb 15, 2018 •

edited

Loading

Uh oh!

htuch commented Feb 15, 2018

Uh oh!

ggreenway commented Feb 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

htuch commented Feb 13, 2018

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway Feb 13, 2018

Choose a reason for hiding this comment

Uh oh!

PiotrSikora Feb 14, 2018

Choose a reason for hiding this comment

Uh oh!

htuch Feb 14, 2018

Choose a reason for hiding this comment

Uh oh!

htuch commented Feb 14, 2018

Uh oh!

ggreenway commented Feb 14, 2018

Uh oh!

zuercher commented Feb 14, 2018

Uh oh!

htuch commented Feb 15, 2018

Uh oh!

htuch commented Feb 15, 2018

Uh oh!

ggreenway commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch commented Feb 15, 2018

Uh oh!

ggreenway commented Feb 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ggreenway commented Feb 15, 2018 •

edited

Loading