Adding a test of TCP with TLS termination (test-only change) by alyssawilk · Pull Request #1183 · envoyproxy/envoy

alyssawilk · 2017-06-28T15:10:07Z

This ended up getting a bit larger than I'd anticipated - please let me know if you'd prefer I split things up a bit. Roughly:

moved existing TCP tests to tcp_proxy_integration_test.cc, adding tcp_proxy.json
moved a bunch of code from ssl_integration_test.cc to ssl_utility.cc, refactored to not use class members
refactor read-until-expected-data-read filter from integration.cc to a standalone class in utility.cc
Finally added a TCP+TLS filter and the SendTlsToTlsListener

I have this theory wherein I start flow control backup with just the network layer (TCP/TLS) but I wanted some basic integration tests before I tack on flow control and make sure resumption works :-)

dnoe · 2017-06-28T17:14:44Z

test/integration/utility.h

+  const std::string& data() { return data_; }
+
+private:
+  Event::Dispatcher* dispatcher_;


Quick comment: In Envoy-style unless there is a null state a reference (const or non-const) is preferred to passing and storing a pointer which is a departure from the Google style guide. I think there a few spots like this line where a reference would be preferred.

htuch

Looks great. Some of my comments below apply to the existing code you moved, so they are really a request to improve what was there before.

htuch · 2017-06-29T14:50:03Z

test/integration/ssl_utility.cc

+  Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(target);
+  ContextConfigImpl cfg(*loader);
+  static auto* client_stats_store = new Stats::TestIsolatedStoreImpl();
+  return context_manager.createSslClientContext(*client_stats_store, cfg);


@myidpt Can you rebase #1087 on this when it merges and generalize this utility function to allow reuse in your PR? If that works, we could reduce some of the boiler plate.

If so, I will need to change this function to support TLS (non-mTLS) case as well. In terms of refactoring, I think it's better we have this PR focusing on it (and probably we can do it better here :) ).

htuch · 2017-06-29T14:50:41Z

test/integration/ssl_utility.cc

+
+Network::Address::InstanceConstSharedPtr getSslAddress(Network::Address::IpVersion version,
+                                                       int port) {
+  std::string url =


As a general preference, prefer const for any intermediate variable that is const.

htuch · 2017-06-29T14:50:54Z

test/integration/ssl_utility.cc

+}
+)EOF";
+
+  std::string json_alpn_san = R"EOF(


const, etc. above

htuch · 2017-06-29T14:52:09Z

test/integration/tcp_proxy_integration_test.cc

+#include "gtest/gtest.h"
+
+namespace Envoy {
+


I have this longstanding wish to have all tests in the anonymous namespace, but that's just an observation, I don't even do this consistently today.

FWIW I'm not a fan of this. It will make test much more verbose since all namespace references will need to be fully specified which is rarely the case when testing code within the same namespace. If there is a wish for this I would recommend opening up a separate discussion and not doing this in the current PR.

I'm not suggesting we put it in the anonymous namespace at outer scope. I'm suggesting we add an additional anonymous namespace within the existing set of namespace scopes at inner scope. This doesn't imply additional verbosity, and is safer when we link into the single coverage binary.

If we're going to bikeshed on this, I'd love to see
namespace Envoy {
namespace Test {
namespace {

where all the tests and tools are in the Envoy::Test namespace. That'd definitely be out of scope for this PR so I'll do at most the anonymous namespace here :-)

OK. Either of this is fine with if you want to do the cleanup.

htuch · 2017-06-29T14:52:33Z

test/integration/tcp_proxy_integration_test.cc

+  EXPECT_EQ("world", tcp_client->data());
+}
+
+TEST_P(TcpProxyIntegrationTest, TcpProxyDownstreamDisconnect) {


Can you add one liner comments above each TEST_P case to explain what's being checked?

mattklein123 · 2017-06-29T15:56:35Z

@alyssawilk FYI TSAN is failing: https://travis-ci.org/lyft/envoy/jobs/248409059

alyssawilk · 2017-06-29T19:09:20Z

Oy. It looks like my Stats::IsolatedStoreImpl are not so isolated as they're supposed to be. Still trying to track down how* - given I'm out tomorrow I'll ping back once I've sorted it out. *It looks like FakeUpstream always has isolated stats and createClientSslContext which I only ported appears to create its own as well so more digging required.

…

On Thu, Jun 29, 2017 at 11:56 AM, Matt Klein ***@***.***> wrote: @alyssawilk <https://github.com/alyssawilk> FYI TSAN is failing: https://travis-ci.org/lyft/envoy/jobs/248409059 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1183 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ARYFvSBY2GgQBSlivB1Wl1BkEk_yuLdtks5sI8k5gaJpZM4OIG3l> .

alyssawilk · 2017-06-30T00:32:24Z

Aha, I think I figured it out - new Scopes in the TestIsolatedStoreImpl were accessing the underlying IsolatedStoreImpl directly, without grabbing locks. I've added a TestScopeWrapper for TestIsolatedStoreImpl as a workaround. If anyone has a more elegant idea for a fix let me know, and no hurry on reviews since now I'm *actually* heading out for the 4th. happy long weekend, all! On Thu, Jun 29, 2017 at 3:09 PM, Alyssa (Rzeszutek) Wilk <alyssar@google.com

…

wrote: Oy. It looks like my Stats::IsolatedStoreImpl are not so isolated as they're supposed to be. Still trying to track down how* - given I'm out tomorrow I'll ping back once I've sorted it out. *It looks like FakeUpstream always has isolated stats and createClientSslContext which I only ported appears to create its own as well so more digging required. On Thu, Jun 29, 2017 at 11:56 AM, Matt Klein ***@***.***> wrote: > @alyssawilk <https://github.com/alyssawilk> FYI TSAN is failing: > https://travis-ci.org/lyft/envoy/jobs/248409059 > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#1183 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/ARYFvSBY2GgQBSlivB1Wl1BkEk_yuLdtks5sI8k5gaJpZM4OIG3l> > . >

htuch

LGTM modulo small comments.

htuch · 2017-07-05T14:16:44Z

test/integration/server.h

+    std::unique_lock<std::mutex> lock(lock_);
+    wrapped_scope_.reset();
+  }
+


I think these should all be override rather than virtual

htuch · 2017-07-05T14:18:52Z

test/integration/server.h

+      : lock_(lock), wrapped_scope_(std::move(wrapped_scope)) {}
+
+  virtual ~TestScopeWrapper() {
+    std::unique_lock<std::mutex> lock(lock_);


Is this needed? Won't this self-destruct?

htuch · 2017-07-05T14:19:44Z

test/integration/server.h

  ScopePtr createScope(const std::string& name) override {
    std::unique_lock<std::mutex> lock(lock_);
-    return store_.createScope(name);
+    ScopePtr tmp = store_.createScope(name);


Nit: Is the intermediate here needed?

Automatic merge from submit-queue. Support bypassing JWT verification for some requests. **What this PR does / why we need it**: **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # **Special notes for your reviewer**: **Release note**: ```release-note None ```

Description: Replace the metrics sink from the envoy one to a custom one. The custom metrics sink comes with ack function on the grpc stream. This change is based on upstream envoy change: #13919 Risk Level: High Testing: Local and ci unit tests and integration tests Signed-off-by: Jingwei Hao <jingweih@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

Working TCP+TLS tests!

e338835

dnoe reviewed Jun 28, 2017

View reviewed changes

alyssawilk added 2 commits June 28, 2017 13:21

All your pointers are belong to GFE

f6d2173

Removing more pointers

7cf2e95

htuch reviewed Jun 29, 2017

View reviewed changes

const + comments

e568657

mattklein123 mentioned this pull request Jun 29, 2017

Implementing XFCC header #1087

Merged

Making stats more thread safe

d568bbc

alyssawilk added 2 commits June 29, 2017 22:53

Merge branch 'refs/heads/master' into tcp-tests

3016ddb

alphabetic order

69e5e89

htuch reviewed Jul 5, 2017

View reviewed changes

alyssawilk added 2 commits July 5, 2017 10:30

reviewer-prompted cleanups

fb3792a

Merge branch 'refs/heads/master' into tcp-tests

29069e4

htuch approved these changes Jul 5, 2017

View reviewed changes

mattklein123 approved these changes Jul 5, 2017

View reviewed changes

mattklein123 merged commit d961183 into envoyproxy:master Jul 5, 2017

alyssawilk deleted the tcp-tests branch July 6, 2017 16:54

alyssawilk mentioned this pull request Aug 28, 2017

Reducing boilerplate code in the integration tests #1556

Merged

Conversation

alyssawilk commented Jun 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Jun 29, 2017

Uh oh!

alyssawilk commented Jun 29, 2017 via email

Uh oh!

alyssawilk commented Jun 30, 2017 via email

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants