http: fixing upstream stream limit bug by alyssawilk · Pull Request #14820 · envoyproxy/envoy

alyssawilk · 2021-01-25T21:21:04Z

Fixing a bug where the Envoy upstream logic didn't respect stream limits capped by upstream.

Risk Level: Medium (it's complicated code)
Testing: new unit, integration tests
Docs Changes: n/a
Release Notes: inline
Runtime guard: envoy.reloadable_features.improved_stream_limit_handling
Fixes #13933

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

yanavlasov

Some initial nits. Will review the rest later.

yanavlasov · 2021-01-26T18:12:01Z

include/envoy/http/codec.h

 };

+/**
+ * A class for sharing what HTTP/1 SETTINGS were received from the peer.


yanavlasov · 2021-01-26T18:27:09Z

include/envoy/upstream/cluster_manager.h

+  //
+  // Note that if more HTTP/2 streams have been established than are allowed by
+  // a late-received SETTINGS frame, this MAY BE NEGATIVE.
+  int64_t connecting_stream_capacity_{};


There is a bit of gotcha here. The reason checkAnd... methods work correctly is because connecting_stream_capacity_ is 64 bit. If it was 32 bit the arithmetic would be unsigned and ASSERTs would fire. Not sure what to do here. Maybe leave a comment that it needs to be 64 bit. Up to you.

It is easy to overflow capacity when talking to a server that sets the number of streams to UINT32_MAX and the proxy ends up creating multiple upstream connections for that cluster.

yanavlasov · 2021-01-27T02:36:50Z

include/envoy/http/codec.h

+/**
+ * A class for sharing what HTTP/1 SETTINGS were received from the peer.
+ */
+class ReceivedSettings {


It does not look like you are actually gaining anything by having this interface. In tests you can just construct ReceivedSettingImpl. Might be worthwhile to make it a concrete class without the need for the interface, since it only carries state and no behavior.

Sorry I'm not sure what you mean by this. The codec interface onSettings needs some way to describe what is passed. Are you suggesting we forward declare ReceivedSettingsImpl there? That doesn't seem consistent with general Envoy API policy.

yanavlasov · 2021-01-27T02:44:26Z

source/common/conn_pool/conn_pool_base.h

  // Returns the number of active streams on this connection.
  virtual uint32_t numActiveStreams() const PURE;

+  virtual bool hadNegativeDeltaOnStreamClosed() { return false; }


nit: maybe leave a comment that h2 pool overrides this and h1 and tcp clients can not have negative concurrent stream count.

yanavlasov · 2021-01-27T02:55:33Z

Thanks, integration test explains this change really well.
/wait

antoniovicente

Thanks for this important fix to handling of H2 SETTINGs on upstream connections and sorry for delay in review.

antoniovicente · 2021-01-27T08:32:42Z

test/common/http/http2/conn_pool_test.cc

+
+  // Settings frame reducing capacity to one stream per connection results in -1 capacity.
+  NiceMock<MockReceivedSettings> settings;
+  settings.max_concurrent_streams_ = 1;


nit: I would consider:
MockReceivedSettings settings;
EXPECT_CALL(settings, max_concurrent_streams()).WillOnce(Return(1));

But use of max_concurrent_streams_ seems fine too. I'm not seeing tests that don't set max_concurrent_streams_ and exercise the case where the max_concurrent_streams setting is missing.

include/envoy/http/codec.h

antoniovicente · 2021-01-27T08:37:21Z

include/envoy/upstream/cluster_manager.h


  template <class T> void checkAndIncrement(T& value, uint32_t delta) {
-    ASSERT(std::numeric_limits<T>::max() - value > delta);
+    ASSERT(value + delta >= value);


These overflow checks may depend on undefined overflow behavior if T is a signed integer type. ASAN may complain if a test were to trigger the ASSERT. Consider instead:

ASSERT(std::numeric_limits<T>::max() - delta > value);

antoniovicente · 2021-01-27T08:43:28Z

include/envoy/upstream/cluster_manager.h

  }
  void decrConnectingStreamCapacity(uint32_t delta) {
-    checkAndDecrement<uint64_t>(connecting_stream_capacity_, delta);
+    checkAndDecrement<int64_t>(connecting_stream_capacity_, delta);


optional: You can omit the template argument on this call. The compiler will deduce the right template argument from the type of the first argument.

checkAndDecrement(connecting_stream_capacity_, delta);

antoniovicente · 2021-01-27T09:08:53Z

source/common/http/http2/codec_impl.h


+class ReceivedSettingsImpl : public ReceivedSettings {
+public:
+  ReceivedSettingsImpl(const nghttp2_settings& settings);


nit: Consider marking this constructor explicit.

source/common/http/http2/conn_pool.cc

antoniovicente · 2021-01-27T09:20:41Z

source/common/conn_pool/conn_pool_base.h

  }
  bool hasPendingStreams() const { return !pending_streams_.empty(); }

+  void decrClusterStreamCapacity(int32_t delta) { state_.decrConnectingStreamCapacity(delta); }


signed to unsigned conversion since decrConnectingStreamCapacity takes an uint32_t. Should this method take a uint32_t delta?

docs/root/version_history/current.rst

antoniovicente · 2021-01-27T10:00:45Z

test/common/http/http2/conn_pool_test.cc

+  // Settings frame reducing capacity to one stream per connection results in -1 capacity.
+  NiceMock<MockReceivedSettings> settings;
+  settings.max_concurrent_streams_ = 1;
+  test_clients_[0].codec_client_->onSettings(settings);


Verify that a second onSettings call with max_concurrent_streams_ == 1; does not decrease capacity a second time.
Coverage for the case where settings increases max_concurrent_streams_; either above the max concurrency configured at Envoy or the limit set by a previous SETTINGs frame on the same upstream connection.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only · 2021-01-28T19:53:15Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #14820 was synchronize by alyssawilk.

see: more, trace.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch

/lgtm api

yanavlasov · 2021-02-01T19:22:51Z

It needs main merge to unblock CI.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2021-02-03T18:12:29Z

/wait

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2021-02-18T14:19:39Z

I believe comments are addressed. PTAL when you get a chance!

alyssawilk · 2021-02-24T16:13:54Z

@htuch can I get a fresh API stamp?

antoniovicente

Changes look good, just some minor comments.

antoniovicente · 2021-02-24T17:31:22Z

include/envoy/upstream/cluster_manager.h


  template <class T> void checkAndDecrement(T& value, uint32_t delta) {
-    ASSERT(delta <= value);
+    ASSERT(value - delta <= value);


The reason for my comment is that signed integer overflow is undefined behavior according to the C++ standard. The ASSERT condition you have is only well defined for unsigned integer types.

connecting_stream_capacity_ has a signed integer type.

antoniovicente · 2021-02-24T17:47:03Z

source/common/http/http2/conn_pool.h

+  bool hadNegativeDeltaOnStreamClosed() override {
+    int ret = negative_capacity_ != 0;
+    if (negative_capacity_ > 0) {
+      negative_capacity_--;


Neither the comment in the base class method nor the function name implies that this method has side effects. Would it be possible to document this interface method?

htuch

/lgtm api

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

antoniovicente

Thanks.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Fixing a bug where the Envoy upstream logic didn't respect stream limits capped by upstream. Backport of 06d0780 into v1.17 Signed-off-by: Yan Avlasov <yavlasov@google.com>

http: fixing upstream stream limit bug

23e4707

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk assigned antoniovicente Jan 25, 2021

yanavlasov requested changes Jan 26, 2021

View reviewed changes

yanavlasov self-assigned this Jan 26, 2021

yanavlasov requested changes Jan 27, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 27, 2021

antoniovicente reviewed Jan 27, 2021

View reviewed changes

alyssawilk added 2 commits January 28, 2021 14:31

comments

f9e9da9

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

paste error

b7bd993

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot added api and removed waiting labels Jan 28, 2021

repokitteh-read-only bot assigned htuch Jan 28, 2021

spelling

cff6b91

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch reviewed Jan 29, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jan 29, 2021

alyssawilk added 2 commits February 1, 2021 16:01

Merge branch 'main' into stream_limit

1e1e33c

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

CI

5304944

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot added the waiting label Feb 3, 2021

test push please ignore

25f1572

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Feb 3, 2021

alyssawilk added the waiting label Feb 3, 2021

alyssawilk added 2 commits February 4, 2021 09:19

Merge branch 'main' into stream_limit

e4b7ef8

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mock fix?

bec87a8

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Feb 4, 2021

alyssawilk added the waiting label Feb 4, 2021

Merge branch 'main' into stream_limit

2e2cbd8

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot added api and removed waiting labels Feb 11, 2021

alyssawilk added 2 commits February 17, 2021 16:01

Merge branch 'main' into stream_limit

3d93e43

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'main' into stream_limit

4965ca4

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the stream_limit branch 2 times, most recently from 7564935 to 4965ca4 Compare February 22, 2021 15:19

yanavlasov previously approved these changes Feb 24, 2021

View reviewed changes

antoniovicente reviewed Feb 24, 2021

View reviewed changes

htuch reviewed Feb 25, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Feb 25, 2021

comments

1f9107e

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed yanavlasov’s stale review via 1f9107e March 1, 2021 16:51

alyssawilk added 3 commits March 1, 2021 16:48

Merge branch 'main' into stream_limit

87339fc

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'main' into stream_limit

e1560b5

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'main' into stream_limit

1e3bcba

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

antoniovicente previously approved these changes Mar 4, 2021

View reviewed changes

alyssawilk added 2 commits March 4, 2021 08:51

Merge branch 'main' into stream_limit

4cc22e9

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'main' into stream_limit

060f803

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed antoniovicente’s stale review via 060f803 March 8, 2021 14:39

antoniovicente approved these changes Mar 8, 2021

View reviewed changes

alyssawilk merged commit 06d0780 into envoyproxy:main Mar 9, 2021

antoniovicente mentioned this pull request Mar 23, 2021

Envoy abruptly closes new listener H/2 connection before sending max concurrent streams settings #13948

Closed

antoniovicente mentioned this pull request May 19, 2021

Envoy HTTP2 Connection sends RST_STREAM before initial HEADERS which causes upstream server to close connection with PROTOCOL_ERROR #11369

Open

alyssawilk deleted the stream_limit branch August 18, 2021 13:59

alyssawilk mentioned this pull request Oct 5, 2021

envoy.reloadable_features.improved_stream_limit_handling deprecation #18440

Closed

Conversation

alyssawilk commented Jan 25, 2021

Uh oh!

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

yanavlasov commented Jan 27, 2021

Uh oh!

antoniovicente left a comment • 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!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Jan 28, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

yanavlasov commented Feb 1, 2021

Uh oh!

alyssawilk commented Feb 3, 2021

Uh oh!

alyssawilk commented Feb 18, 2021

Uh oh!

alyssawilk commented Feb 24, 2021

Uh oh!

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

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

antoniovicente left a comment •

edited

Loading