test/network: echo integration test ipv6 by hennna · Pull Request #953 · envoyproxy/envoy

hennna · 2017-05-12T17:10:56Z

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

htuch

LGTM modulo some minor comments. Ship it before it gets any larger!

htuch · 2017-05-12T22:13:53Z

+    return address.ip()->ipv4()->address() == htonl(INADDR_LOOPBACK);
+  } else if (address.ip()->version() == Address::IpVersion::v6) {
+    std::array<uint8_t, 16> addr = address.ip()->ipv6()->address();
+    return 0 == memcmp(&addr, &in6addr_loopback, sizeof(struct in6_addr));


sizeof(addr) or sizeof(in6addr_loopback).

htuch · 2017-05-12T22:15:22Z

    freeifaddrs(ifaddr);
  }

+  // If the local address is not found above, then return the loopback addresss by default.


Can you add a reference to #939?

htuch · 2017-05-12T22:18:34Z

+    std::array<uint8_t, 16> addr = address.ip()->ipv6()->address();
+    return 0 == memcmp(&addr, &in6addr_loopback, sizeof(struct in6_addr));
+  }
+  return false;


NOT_IMPLEMENTED;

htuch · 2017-05-12T22:31:45Z


 #include "test/integration/integration_test.h"
 #include "test/integration/utility.h"
+#include "test/test_common/environment.h"


Why is this needed (no other changes to this file)?

htuch · 2017-05-12T22:31:50Z

 #include "common/http/header_map_impl.h"

 #include "test/integration/utility.h"
+#include "test/test_common/environment.h"


mattklein123

looks good, few small comments

mattklein123 · 2017-05-13T16:06:45Z

-Address::InstanceConstSharedPtr Utility::getLocalAddress() {
+// TODO(hennna): Currently getLocalAddress does not support choosing between
+// multiple interfaces and addresses not returned by getifaddrs. In additon,
+// the default is to return a loopback address of type version. This fucntion may


typo fucntion

mattklein123 · 2017-05-13T16:07:25Z

+// multiple interfaces and addresses not returned by getifaddrs. In additon,
+// the default is to return a loopback address of type version. This fucntion may
+// need to be updated in the future.
+Address::InstanceConstSharedPtr Utility::getLocalAddress(const Address::IpVersion version) {


IIRC server.cc (or somewhere) has error checking that this function returns a null address, which can't happen anymore. Can you fix the callers to remove error checking that can't happen?

mattklein123 · 2017-05-13T16:08:43Z

  Server::ProdComponentFactory component_factory;
-  LocalInfo::LocalInfoImpl local_info(Network::Utility::getLocalAddress(), options.serviceZone(),
-                                      options.serviceClusterName(), options.serviceNodeName());
+  // TODO(henna): Parameterize local address IP version.


nit: less parameterize, more probably add CLI option.

mattklein123 · 2017-05-13T16:10:34Z

@@ -0,0 +1,23 @@
+{


Since you are moving this test into a standalone config, is there any reason to leave the echo listener in the other config? Seems like that should be removed?

mattklein123 · 2017-05-13T16:10:54Z

+    ]
+  }],
+
+  "admin": { "access_log_path": "/dev/null", "profile_path": "{{ test_tmpdir }}/envoy.prof", "address": "tcp://{{ ip_loopback_address }}:0" },


nit: can we break this at 100 cols

mattklein123 · 2017-05-13T16:13:31Z

  bool complete() { return complete_; }
-  const Http::HeaderMap& headers() { return *headers_; }
+  const Http::HeaderMap& headers() {
+    ASSERT(headers_ != nullptr);


nit: per style guide generally shy away from this type of ASSERT check because it should crash in an obvious way very soon.

STR_EQ comparisons in the integration tests like this one : https://github.com/lyft/envoy/blob/master/test/integration/server.cc#L60 cause a segfault if response->complete() is false. Would it make more sense to ASSERT_TRUE(response->complete()) and remove this change? The only problem is that this particular example is in the destructor of IntegrationTestServer.

IMO a segfaulting test is not the end of the world as it failed. :) I would probably just do nothing, but if you want to do something more clear I would use a gtest assert rather than the prod assert. So ASSERT_NE or something.

Arguably, since we're returning a reference here, if it was nullptr the failure would be harder to understand the further away from this return we get, so best to check here.

Better would probably be to just make this return a pointer instead of a reference, but that's a larger change. I don't feel very strongly about this either way.

Ok. I'll just leave as is for now (no change) and leave the change for another PR.

mattklein123

LGTM pending any final @htuch comments.

…1036) This PR parameterizes two integration tests: integration_test.cc and integration_admin_test.cc. The majority of the line changes are plumbing through the parameter version to allow code that originally hard coded an IPv4 address to now be parameterized. This is the second step in parameterizing IPv4 and IPv6 testing in the integration tests. The first step was parameterizing the echo_integration_test in #953 . In this PR, we update many of the functions in integration.cc that are called in integration_test.cc to take the parameter version. Because other integration tests currently also rely on these functions, we set the default value to be Network::Address::IpVersion::v4. In a subsequent PR, we will also parameterize the other integration tests and the default value will be removed. The constructor for RawConnectionDriver and function makeSingleRequest in test/integration/utility.h were also updated to set the default version value to v4. This required reordering the function parameters. createTestServer and temporaryFileSubstitute were also updated in a similar fashion. asan and tsan tests passing requires https://github.com/lyft/envoy-filter-example/pull/4/files due to dependence on changes made in this PR. Supports ongoing work in #351

Automatic merge from submit-queue. [DO NOT MERGE] Auto PR to update dependencies of proxy This PR will be merged automatically once checks are successful. ```release-note none ```

Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

Echo integration test ipv6 test

0650203

hennna changed the title ~~Echo integration test ipv6 test~~ test/network: echo integration test ipv6 test May 12, 2017

hennna changed the title ~~test/network: echo integration test ipv6 test~~ test/network: echo integration test ipv6 May 12, 2017

htuch reviewed May 12, 2017

View reviewed changes

mattklein123 reviewed May 13, 2017

View reviewed changes

Response to comments

d89601c

mattklein123 approved these changes May 15, 2017

View reviewed changes

htuch approved these changes May 15, 2017

View reviewed changes

htuch merged commit c9992ae into envoyproxy:master May 15, 2017

hennna deleted the echo-test-redo branch May 23, 2017 20:45

hennna mentioned this pull request Jun 1, 2017

test: integration test and admin integration test parameterization #1036

Merged

This was referenced Jun 5, 2017

test: server test and integration test ipv6 #1049

Merged

test: hot restart IPv6 #1051

Merged

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

ci: increase soft binary size limit (#953)

f12358d

Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

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

ci: increase soft binary size limit (#953)

1349736

Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

Conversation

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

Uh oh!

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!

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!

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 13, 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!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hennna commented May 12, 2017 •

edited

Loading

mattklein123 May 13, 2017 •

edited

Loading