test: add IPv6 testing to IntegrationTest.Echo by hennna · Pull Request #883 · envoyproxy/envoy

hennna · 2017-05-02T23:17:41Z

Add Ipv6 testing to the Integration Echo test. This is achieved by parameterizing:

The loopback address used by RawConnectionDriver
The echo listener address in test/config/integration/server.json.

This PR supports ongoing work in #351 .

Asan build requires change to envoy-filter-example: envoyproxy/envoy-filter-example#1.

dnoe · 2017-05-03T14:25:14Z

 Address::InstanceConstSharedPtr findOrCheckFreePort(const std::string& addr_port,
                                                    Address::SocketType type);

+/**


Technically the address is "::1", and "[::1]" is a special case used in URL because : was already used as the port delimiter and the IPv6 address shortening scheme (double ::) means an URL parser can't figure out whether the final : is a port delimiter or just part of the address without some way of surrounding the address.

If the purpose of this method is to return an "URL ready" formatted address I think the name should be changed to indicate this. Otherwise maybe it should be the caller's responsibility to add the [ ] when creating the URL in a fmt::format.

+1. Feel free to review the rest of the PR.

mattklein123

looks good, few comments/questions.

mattklein123 · 2017-05-03T15:18:20Z

@@ -91,8 +91,7 @@ Address::InstanceConstSharedPtr Utility::resolveUrl(const std::string& url) {
  // TODO(mattklein123): We still support the legacy tcp:// and unix:// names. We should
  // support/parse ip:// and pipe:// as better names.
  if (url.find(TCP_SCHEME) == 0) {


I think it's missing right now, but can we add a few explicit tests for this function in https://github.com/lyft/envoy/blob/master/test/common/network/utility_test.cc (to verify we get back the right address type on the returning address). Also, I think you can go ahead and delete the TODO at line 90 above, and also please delete the TODO at 91 since I don't think we are going to do that anymore.

mattklein123 · 2017-05-03T15:19:28Z

-# TODO(htuch): Clean this up when Bazelifying the hot restart test below. At the same time, restore
-# some test behavior lost in #650, when we switched to 0 port binding - the hot restart tests no
-# longer check socket passing. See #654.
+# TODO(htuch): Clean this up when Bazelifying the hot restart test below.


Can you follow up with @htuch and see that this TODO is about? I'm wondering if it's still relevant. If so, can you add a bit more color? (I know not really related to your change but while I'm noticing).

Yeah. I think we could have a simple C++ binary that links against test_environment.cc that uses the substitution methods provided there, rather than the adhoc sed below. That way we don't need to worry about keeping this special case in sync with TestEnvironment.

mattklein123 · 2017-05-03T15:19:55Z

  registerTestServerPorts(port_names);
 }

+// TODO(hennna): Depricate when IPv6 test support is finished.


nit: s/Depricate/Deprecate

mattklein123 · 2017-05-03T15:24:59Z

 }

-TEST_F(IntegrationTest, RouterNotFound) { testRouterNotFound(Http::CodecClient::Type::HTTP1); }
+TEST_P(IntegrationTest, RouterNotFound) { testRouterNotFound(Http::CodecClient::Type::HTTP1); }


For all the rest of these tests that are now parameterized, I'm a little confused on what exactly is happening. They are doing both IPv4 and IPv6 right? Are the IPv6 ones working because the listener is in dual bind mode and the IPv4 connection from the fake clients just gets converted to IPv6? I think this is fine but I just want to make sure I grok what is happening.

Also, on this topic, have we discussed what the final plan is for dual bind vs. explicit bind? I talked with @jamessynge about this a while back but I can't remember where we landed.

I followed up with @jamessynge and I believe we settled on explicit as the default for "principle of least surprise".

I'm not sure if this answers your question. For the unparameterized tests, the echo listener is instantiated once with an IPv4 address and once with an IPv6 address. However, the tests (except for the echo integration test) are basically just running twice with IPv4 addresses because the addresses are hard coded in the test code. My next PR is to plumb through all the integration tests and TODOs in this PR.

Ah OK, I see, you only parameterized the echo listener, so that even though all the tests are running through v4 and v6, the other ones use a v4 loopback for the other listeners.

If we are going to be explicit, and now that we are doing full integration tests, I would recommend that you take this opportunity to set IPV6_V6ONLY on v6 sockets. This will prevent mapped v4 addresses from still working. It will double check what you are doing and make all the tests better. Later, we can potentially add a listener option to allow dual bind, but for now I agree we should be explicit.

hennna@ Are you planning on making the upstream IPv6 as well in this PR or pushing it to later? I think it can wait, since getting just basic client <-> Envoy IPv6 happening is a self-contained effort. But, we should probably add some TODOs in the code base to keep track that we also need to validate the upstream in the integration tests.

There's also the question of whether we want to validate IPv4 downstream and IPv6 upstream combos for example, or all IPv4 and all IPv6. I think the latter provides sufficient coverage and will simplify the TEST_P, but worth discussing.

I'll add upstream support in the next PR. Will add a TODO for that.

The current framework probably can't be extended easily to test for arbitrary combinations of IPv4 and Ipv6 address combinations. If that's desired, we can discuss here and I can apply the changes to this PR.

I would suggest that we ignore IPv4 and IPv6 mix for now. I agree with @htuch that we should get good coverage the the current all IPv4 and IPv6 mix. We may want to add a few targeted mixed cases in the future for sanity checking, but I think that can be a later add on.

htuch

Nice, this seems a good beach head on the integration tests IPv6 support.

htuch · 2017-05-03T16:49:43Z

-# TODO(htuch): Clean this up when Bazelifying the hot restart test below. At the same time, restore
-# some test behavior lost in #650, when we switched to 0 port binding - the hot restart tests no
-# longer check socket passing. See #654.
+# TODO(htuch): Clean this up when Bazelifying the hot restart test below.


Yeah. I think we could have a simple C++ binary that links against test_environment.cc that uses the substitution methods provided there, rather than the adhoc sed below. That way we don't need to worry about keeping this special case in sync with TestEnvironment.

htuch · 2017-05-03T16:53:23Z

+   * Initializer for individual integration tests.
   */
-  static void SetUpTestCase() {
+  void SetUp() override {


I think there's a little more to it than just switching SetUpTestCase with SetUp when we want to make test setup/teardown local to each test and remove interference between tests. Take a look at https://github.com/lyft/envoy/blob/master/test/integration/integration.h#L169 and all the static fields. Some (all?) of these should probably become instance variables.

Hmm, yeah. Forgot about this. Is the only reason we are doing static removal to support TEST_P? Is there any way to do TEST_P with different static setup/teardown? Like somehow instantiating twice? I guess I'm just wondering if the all the static removal and longer test time is really worth it (since we can already run the different binaries in parallel).

My impression was that removing state shared between tests was desired regardless of this effort. In that case, I was just using that fact in this PR.

SetUpTestCase is only run once before the parameterized tests are instantiated so it doesn't have knowledge of the current IP address version parameter.

Got it. Yes I agree removing statics will make for better tests so if you are up for it, please go for it.

htuch · 2017-05-03T16:57:22Z

 }

-TEST_F(IntegrationTest, RouterNotFound) { testRouterNotFound(Http::CodecClient::Type::HTTP1); }
+TEST_P(IntegrationTest, RouterNotFound) { testRouterNotFound(Http::CodecClient::Type::HTTP1); }


hennna@ Are you planning on making the upstream IPv6 as well in this PR or pushing it to later? I think it can wait, since getting just basic client <-> Envoy IPv6 happening is a self-contained effort. But, we should probably add some TODOs in the code base to keep track that we also need to validate the upstream in the integration tests.

There's also the question of whether we want to validate IPv4 downstream and IPv6 upstream combos for example, or all IPv4 and all IPv6. I think the latter provides sufficient coverage and will simplify the TEST_P, but worth discussing.

hennna · 2017-05-05T17:09:23Z

  // Test-wide port map.
-  static void registerPort(const std::string& key, uint32_t port);
-  static uint32_t lookupPort(const std::string& key);
-  static std::string substitutePorts(const std::string& json_path);


This function doesn't seem to be used.

Yeah, please clean up any dead code.

hennna · 2017-05-05T17:17:55Z

-ClientContextPtr SslIntegrationTest::client_ssl_ctx_alpn_san_;
-
-void SslIntegrationTest::SetUpTestCase() {
+void SslIntegrationTest::SetUp() {


Just want to double check that https://github.com/lyft/envoy/blob/master/test/integration/ssl_integration_test.cc#L116 should remain static.

It's probably OK to keep it static but not ideal. It'll work as it's properly locked and we're not relying on any of the stats there AFAICT. Ideally each client would gets its own stats store, just need to keep track of the storage. You can just add a TODO to this effect if you'd like.

This change corresponds to changes in envoyproxy/envoy#883 to allow for Ipv6 parameterized integration testing. In envoyproxy/envoy#883, we SetUp the test_server for each test rather than once in SetUpTestCase.

htuch · 2017-05-07T15:41:40Z

You can reference the envoy-filter-example at 03f5353c939b0d796925c67d94db52e8055ee732 to pick up your (now merged) patch to that repo.

htuch

LGTM, some minor comments.

htuch · 2017-05-09T13:58:57Z


 int Ipv6Instance::socket(SocketType type) const {
-  return ::socket(AF_INET6, flagsFromSocketType(type), 0);
+  int fd = ::socket(AF_INET6, flagsFromSocketType(type), 0);


htuch · 2017-05-09T13:59:03Z

+  int fd = ::socket(AF_INET6, flagsFromSocketType(type), 0);
+  RELEASE_ASSERT(fd != -1);
+  // Setting IPV6_V6ONLY resticts the IPv6 socket to IPv6 connections only.
+  int v6only = 1;


htuch · 2017-05-09T14:15:03Z

-# TODO(htuch): Clean this up when Bazelifying the hot restart test below. At the same time, restore
-# some test behavior lost in #650, when we switched to 0 port binding - the hot restart tests no
-# longer check socket passing. See #654.
+# TODO(htuch): Clean this up when Bazelifying the hot restart test below.In this test script,


Nit: We have already Bazelifyed the hot restart test, so you can drop the first sentence here.

This reverts commit a55b88d.

This PR parameterizes an echo server integration test and is a redo of #883 . The differences between this PR and #883 are as follows: * Dual bind turn off was split off into a separate PR * Add admin ipv6 support * Add fake upstream ipv6 support

hennna added 3 commits May 2, 2017 19:07

Add IPv6 testing to Integration.Echo test

d41f977

Fix format

52dfafc

Fix asan build

e61ada2

dnoe reviewed May 3, 2017

View reviewed changes

hennna changed the title ~~Add IPv6 testing to IntegrationTest.Echo~~ test: add IPv6 testing to IntegrationTest.Echo May 3, 2017

Response to comment

635dcae

mattklein123 reviewed May 3, 2017

View reviewed changes

htuch reviewed May 3, 2017

View reviewed changes

Merge 'upstream/master' into ipv6-echo

920d5b5

hennna mentioned this pull request May 5, 2017

Parameterize echo listener address envoyproxy/envoy-filter-example#1

Merged

Response to comments.

f0d48f8

hennna commented May 5, 2017

View reviewed changes

hennna added 3 commits May 7, 2017 18:02

Merge 'upstream/master' into ipv6-echo

c870b7e

Update ci

9dbd2bb

Merge branch 'upstream/master' into ipv6-echo

89cfbc8

htuch reviewed May 9, 2017

View reviewed changes

Response to comments

f57efbe

htuch approved these changes May 9, 2017

View reviewed changes

mattklein123 approved these changes May 9, 2017

View reviewed changes

mattklein123 merged commit a55b88d into envoyproxy:master May 9, 2017

mattklein123 added a commit that referenced this pull request May 9, 2017

Revert "test: add IPv6 testing to IntegrationTest.Echo (#883)"

146ae0a

This reverts commit a55b88d.

mattklein123 mentioned this pull request May 9, 2017

Revert "test: add IPv6 testing to IntegrationTest.Echo" #931

Merged

hennna deleted the ipv6-echo branch May 9, 2017 22:56

hennna mentioned this pull request May 12, 2017

test/network: echo integration test ipv6 #953

Merged

Conversation

hennna commented May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 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!

mattklein123 May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hennna May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

hennna May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

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!

htuch commented May 7, 2017

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!

4 participants

hennna commented May 2, 2017 •

edited

Loading

mattklein123 May 3, 2017 •

edited

Loading

hennna May 3, 2017 •

edited

Loading

hennna May 3, 2017 •

edited

Loading