test: remove port dependency from integration tests (#574). by htuch · Pull Request #650 · envoyproxy/envoy

htuch · 2017-03-29T20:41:45Z

No description provided.

htuch · 2017-03-29T20:42:50Z

mattklein123 · 2017-03-29T21:00:55Z

 }

+Network::ListenSocket* InstanceImpl::getListenSocketByIndex(uint32_t index) {
+  if (index < config_->listeners().size()) {


nit: this is kind of an odd impl for this function (requiring advancing from beginning by N each time). I know this is only used in test code, but can we potentially implement with a for loop over list elements which also increments an index?

Why's it faster to iterate over the list rather than advance an iterator? AFAIK, you can't randomly access an element in std::list, so you have to do something O(n).

Sorry, nevermind, I misread. This is fine.

mattklein123 · 2017-03-29T21:04:16Z

  // Network::FilterChainFactory
  bool createFilterChain(Network::Connection& connection) override;

+  uint32_t port() const { return socket_->localAddress()->ip()->port(); }


nit: can we move this above 161 (makes it clear which functions are part of interface overrides). I'm not totally crazy that we have a port() function here either since FakeUpstream also does UDS. Can we just return localAddress maybe and then have the caller deal with the right thing to do depending on case?

mattklein123 · 2017-03-29T21:05:43Z

+  if (it != port_map().end()) {
+    return it->second;
+  }
+  return 0;


should we crash/fail/print here?

mattklein123 · 2017-03-29T21:10:09Z

 }

 ServerContextPtr SslIntegrationTest::createUpstreamSslContext() {
+  static Stats::TestIsolatedStoreImpl upstream_stats_store;


Why was this change made? I think this is fragile since I think we have tests that look at stats which might then not reset.

The problem is that if you share a stats store between the Envoy server instance and upstream instance, you get into a dependency cycle. Previously the call to createUpstreamSslContext happened after the test_server_ was initialized. Now we need the test server to be created after the upstream, to allow the upstream to find its bound port and substitute the port into the JSON config prior to initializing the test server.

The test code doesn't check the stats for the upstream instances, only for the test server AFAICT.

OK sounds good, as long as nothing is being checked right now should be fine.

…free

mattklein123 · 2017-03-29T22:41:23Z

+   * Obtain socket the admin endpoint is bound to.
+   * @return Network::ListenSocket& socket reference.
+   */
+  virtual Network::ListenSocket& socket() PURE;


Can we return a const Network::ListenSocket& here since callers just need to get local address?

mattklein123 · 2017-03-29T23:26:27Z

-        server_(options_, hooks_, restart_, stats_store_, fakelock_, component_factory_,
-                local_info_) {}
+  ServerInstanceImplTest() {
+    options_.reset(new testing::NiceMock<MockOptions>(TestEnvironment::temporaryFileSubstitutePorts(


Why did this need to get changed to pointers here? It seems like we could have just passed result of temporaryFileSubstitutePorts to constructor?

Yeah, no need to change to pointers, I had at first thought the port map construction would be more involved, but it's not.

mattklein123 · 2017-03-29T23:30:03Z


+# TODO(htuch): Clean this up when Bazelifying the hot restart test below.
+HOT_RESTART_JSON="$TEST_SRCDIR"/test/config/integration/hot_restart.json
+cat "$TEST_TMPDIR"/test/config/integration/server.json |


This used to test socket passing, but I don't think it actually does anymore since each listener now binds to a random port? Is there any way to restore the old behavior that is sane? At least can we put in TODO to restore the old test behavior?

TODO added. One idea is we could add a flag to allow us to write back to the filesystem (or print to stdout) the JSON corresponding to what's actually used when zero port binding - basically the same JSON object we parsed but with addresses replaced based on the discovered ports. This would generalize to be useful in other cross-process integration tests where we can't access the ListenSocket objects directly. Another alternative is to do log scraping and JSON templating outside of Envoy. What do you think?

Yeah I kind of like the idea of just spitting the JSON back out, or just generally having some way to have an admin endpoint that maybe spits out the listener list in JSON with the real ports. I think this is complex enough to fix and the risk of regression is low enough that I'm fine deferring this for now. We should open a GH issue though that specifically tracks various tech debt cleanups (or just track specifically in the making build better issue) from the migration just to make sure that we don't lose them in "TODO noise".

Added #654. I like the idea of making this an admin endpoint feature, but if we bind the admin http address to 0 as well as we'd like to in tests, we need to find the admin endpoint. Maybe we could just have a flag that provides this address and return the rest from the admin endpoint?

I don't see how the flag would work unless it somehow talked to the existing process over RPC.

Actually, think the shared memory stuff might also be broken for bazel? How does that work? It definitely won't allow concurrently running the main server.

Let's just discuss in #654.

mattklein123 · 2017-03-29T23:32:29Z

                                               Http::CodecClient::Type type);
  IntegrationTcpClientPtr makeTcpConnection(uint32_t port);

+  // Test-wide port map.


When you say that bazel runs tests in parallel, I assume you mean in parallel in different processes, right? Otherwise it seems like this static port map is not going to work.

That's correct. Each individual _test.cc becomes an independent test target that is run in a separate process, and these may run in parallel. Within a _test.cc, gtest will run all tests sequentially in the same process.

… build issue.

…free

Add subset of struct Node protobuf for use by Wasm.

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

test: remove port dependency from integration tests (envoyproxy#574).

b5bdfb3

Fix hot restart tests.

e6369df

mattklein123 reviewed Mar 29, 2017

View reviewed changes

htuch added 2 commits March 29, 2017 17:32

Merge remote-tracking branch 'upstream/master' into integration-port-…

7158892

…free

mattklein123@ review feedback.

ebbd74e

mattklein123 reviewed Mar 29, 2017

View reviewed changes

htuch added 2 commits March 30, 2017 06:11

Further mattklein123@ review feedback, fix missing integration_test.h…

b6374d6

… build issue.

Merge remote-tracking branch 'upstream/master' into integration-port-…

d158925

…free

mattklein123 approved these changes Mar 30, 2017

View reviewed changes

mattklein123 merged commit fdddfab into envoyproxy:master Mar 30, 2017

htuch mentioned this pull request Mar 30, 2017

Restore socket passing validation in hot restart tests #654

Closed

htuch deleted the integration-port-free branch March 31, 2017 16:30

PiotrSikora pushed a commit to jplevyak/envoy that referenced this pull request Oct 9, 2020

Merge pull request envoyproxy#650 from jplevyak/nodeproto

e2aca05

Add subset of struct Node protobuf for use by Wasm.

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

weekly release: bump version 310120 (#650)

c3df07e

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

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

weekly release: bump version 310120 (#650)

dce618a

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

Conversation

htuch commented Mar 29, 2017

Uh oh!

htuch commented Mar 29, 2017

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 Mar 30, 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!

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!

2 participants

htuch Mar 30, 2017 •

edited

Loading