quiche: update QUICHE and boringSSL dependency by danzh2010 · Pull Request #15181 · envoyproxy/envoy

danzh2010 · 2021-02-24T22:59:50Z

Signed-off-by: Dan Zhang [email protected]

Commit Message: update QUICHE to commit 6fec8e97a1ab1a945b86bd6b7252666294448ddb and boring SSL commit b049eae83d25977661556dcd913b35fbafb3a93a.

Additional Description: the latest QUICHE commit depends on broingSSL commit: https://boringssl.googlesource.com/boringssl/+/3d8b8c3df27cc780a49607be6a27f2c518f5c082 which is in chrome dev channel.

Risk Level: low
Testing: existing tests passed

Signed-off-by: Dan Zhang <[email protected]>

repokitteh-read-only · 2021-02-24T22:59:55Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #15181 was opened by danzh2010.

see: more, trace.

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 · 2021-02-24T23:57:01Z

/assign @wu-bin @alyssawilk

moderation · 2021-02-25T01:59:52Z

/lgtm deps

Signed-off-by: Dan Zhang <[email protected]>

wu-bin · 2021-02-25T22:32:20Z

bazel/external/quiche.BUILD

+    deps =
+        [
+            ":quic_platform_export",
+            ":quiche_common_platform_export",
+        ],


nit: format

wu-bin · 2021-02-25T22:44:30Z

source/extensions/quic_listeners/quiche/codec_impl.cc

+  // Only call watermark callbacks on non QUIC static streams which are
+  // crypto stream and Google QUIC headers stream.


Comment is obsolete.

This function was useful previously because it encaps the looping logic, but we have PerformActionOnActiveStreams now, this function does not seem to useful anymore. I'm inclined to remove it, and call the callbacks directly from onUnderlyingConnectionAboveWriteBufferHighWatermark and onUnderlyingConnectionBelowWriteBufferLowWatermark.

good point!

wu-bin · 2021-02-25T22:53:32Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc

    // of propagating original reset reason. In QUICHE if a stream stops reading
    // before FIN or RESET received, it resets the steam with QUIC_STREAM_NO_ERROR.
    StopReading();
+    runResetCallbacks(Http::StreamResetReason::LocalReset);


Is this a new callback, or did we miss it previously? Does it have test coverage?

StopReading() stopped calling Reset() but calling session to send reset directly. So the callback in Reset() stopped from being triggered. It caused unit tests failure before adding this callback.

wu-bin · 2021-02-25T23:10:56Z

source/extensions/quic_listeners/quiche/platform/quic_mem_slice_impl.cc

    : fragment_(std::make_unique<Envoy::Buffer::BufferFragmentImpl>(
-          buffer.release(), length,
-          [](const void* p, size_t, const Envoy::Buffer::BufferFragmentImpl*) {
-            delete[] static_cast<const char*>(p);
+          buffer.get(), length,
+          [allocator = buffer.get_deleter().allocator()](const void* p, size_t,
+                                                         const Envoy::Buffer::BufferFragmentImpl*) {
+            quic::QuicBufferDeleter deleter(allocator);
+            deleter(const_cast<char*>(static_cast<const char*>(p)));
          })) {
+  buffer.release();


How about use a function to initialize fragment_?

std::unique_ptr<Envoy::Buffer::BufferFragmentImpl> ConvertToBufferFragment( QuicUniqueBufferPtr buffer, size_t length) { const void* p = buffer.get(); return std::make_unique<Envoy::Buffer::BufferFragmentImpl>(p, length, [buf = std::move(buffer)]( const void*, size_t, const Envoy::Buffer::BufferFragmentImpl*) {}; } QuicMemSliceImpl::QuicMemSliceImpl(QuicUniqueBufferPtr buffer, size_t length) : fragment_(ConvertToBufferFragment(std::move(buffer), length))

You don't need the buffer.release() this way.

You can also change ConvertToBufferFragment to be templated on the buffer type, such that it can be used by the other constructor too(the one accepts a std::unique_ptr<char[]>).

BufferFragmentImpl constructor take a function referece and copy it over. So with unique_ptr in the capture, the lambda here is not copiable.

I see, it seems BufferFragmentImpl should take the releasor by value and std::move() into its member field. But feel free to do it in the separate change, or just add a TODO.

Added TODO for it.

wu-bin · 2021-02-25T23:19:14Z

test/extensions/quic_listeners/quiche/envoy_quic_server_session_test.cc


-  MOCK_METHOD(void, SendConnectionClosePacket, (quic::QuicErrorCode, const std::string&));
+  MOCK_METHOD(void, SendConnectionClosePacket,
+              (quic::QuicErrorCode, quic::QuicIetfTransportErrorCodes ietf_error,


Nit: Please be consistent here, either always have the argument name spelled, or always not spelled.

wu-bin · 2021-02-25T23:22:41Z

test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc

+  if (quic::VersionUsesHttp3(quic_version_.transport_version)) {
+    EXPECT_CALL(quic_session_, MaybeSendStopSendingFrame(_, _));
+  } else {
+    EXPECT_CALL(quic_session_, MaybeSendRstStreamFrame(_, _, _));
+  }


In other tests in this file, we always expect MaybeSendRstStreamFrame, and also expect a MaybeSendStopSendingFrame for h3. This test expects only one of them to happen. Is it intended?

Yes, in early response case, FIN is ready sent before resetting, so IETF quic will not send RST_STREAM again but only STOP_SENDING.

Thanks for the explanation.

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 · 2021-02-27T03:36:26Z

/retest

repokitteh-read-only · 2021-02-27T03:36:30Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15181 (comment) was created by @danzh2010.

see: more, trace.

Signed-off-by: Dan Zhang <[email protected]>

wu-bin

LGTM. Thanks Dan!

wu-bin · 2021-03-01T17:40:36Z

source/extensions/quic_listeners/quiche/platform/quic_mem_slice_impl.cc

    : fragment_(std::make_unique<Envoy::Buffer::BufferFragmentImpl>(
-          buffer.release(), length,
-          [](const void* p, size_t, const Envoy::Buffer::BufferFragmentImpl*) {
-            delete[] static_cast<const char*>(p);
+          buffer.get(), length,
+          [allocator = buffer.get_deleter().allocator()](const void* p, size_t,
+                                                         const Envoy::Buffer::BufferFragmentImpl*) {
+            quic::QuicBufferDeleter deleter(allocator);
+            deleter(const_cast<char*>(static_cast<const char*>(p)));
          })) {
+  buffer.release();


I see, it seems BufferFragmentImpl should take the releasor by value and std::move() into its member field. But feel free to do it in the separate change, or just add a TODO.

wu-bin · 2021-03-01T17:44:00Z

test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc

+  if (quic::VersionUsesHttp3(quic_version_.transport_version)) {
+    EXPECT_CALL(quic_session_, MaybeSendStopSendingFrame(_, _));
+  } else {
+    EXPECT_CALL(quic_session_, MaybeSendRstStreamFrame(_, _, _));
+  }


Thanks for the explanation.

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 · 2021-03-02T16:22:52Z

/retest

repokitteh-read-only · 2021-03-02T16:22:57Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15181 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2021-03-02T16:55:53Z

@envoyproxy/dependency-shepherds

moderation · 2021-03-02T17:13:05Z

/lgtm deps

moderation · 2021-03-02T17:20:03Z

The deps look OK from a deps hygiene point of view but copying TLS codeowners @lizan @asraa @ggreenway as this is a little unusual as it is pulling in a BoringSSL version from Chrome dev channel vs. the normal stable channel. /cc @htuch

htuch · 2021-03-02T19:21:50Z

@danzh1989 can you comment on:

this is a little unusual as it is pulling in a BoringSSL version from Chrome dev channel vs. the normal stable channel.

?

danzh2010 · 2021-03-02T19:27:27Z

@danzh1989 can you comment on:

this is a little unusual as it is pulling in a BoringSSL version from Chrome dev channel vs. the normal stable channel.

?

As noted in PR description the latest QUICHE depends on a new TLS interface from commit https://boringssl.googlesource.com/boringssl/+/3d8b8c3df27cc780a49607be6a27f2c518f5c082. This commit is not in chrome stable yet. We had similar issue before and discussed with @PiotrSikora that in such case bumping the boring SSL version is fine.

htuch · 2021-03-02T19:32:07Z

Sure, trying to help @moderation here with understanding the implications of working on the linux/dev branch. What are the properties that differ from stable? Do we lose any security coverage etc?

danzh2010 · 2021-03-02T19:44:32Z

Sure, trying to help @moderation here with understanding the implications of working on the linux/dev branch. What are the properties that differ from stable? Do we lose any security coverage etc?

AFAIK, chrome dev channel is used by a smaller percentage of users for a shorter period. These two channels differ in terms of robustness. But in terms of security, dev channel also went through release and security review. @PiotrSikora may be able to comment more about security aspect.

alyssawilk · 2021-03-02T20:14:22Z

If we have concerns about the boringssl pull @davidben could probably address as well.

davidben · 2021-03-02T20:51:28Z

Review-wise, BoringSSL CLs are reviewed before check-in. We do generally aim for HEAD to be usable at any point and generally track HEAD in most of our repositories. "Live at HEAD" and all that. The repositories aren't updated continuously (weekly, biweekly, monthly, etc.), but this is less out of stability and more the time to assemble the update, run everyone's tests, etc. Mistakes may happen, but unit tests and each projects' release cycles tend to work well to catch this stuff.

htuch · 2021-03-02T21:10:40Z

/lgtm deps
Thanks for the explanations @danzh1989 and @davidben

alyssawilk · 2021-03-02T21:29:23Z

test/extensions/quic_listeners/quiche/envoy_quic_dispatcher_test.cc

                         testing::ValuesIn(generateTestParam()), testParamsToString);

 TEST_P(EnvoyQuicDispatcherTest, CreateNewConnectionUponCHLO) {
+  quic::SetVerbosityLogThreshold(1);


was this for test debug, or something we want on permanently?

It was for debugging, removed now.

alyssawilk

LGTM, but holding merge on the verbosity question

Signed-off-by: Dan Zhang <[email protected]>

)" This reverts commit 80a88a8. Signed-off-by: Alyssa Wilk <[email protected]>

update tar

f29b4ae

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 requested review from alyssawilk, ggreenway and mattklein123 as code owners February 24, 2021 22:59

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Feb 24, 2021

danzh1989 added 2 commits February 24, 2021 18:05

remove unused files

8dd9a68

Signed-off-by: Dan Zhang <[email protected]>

update release date

79949f9

Signed-off-by: Dan Zhang <[email protected]>

repokitteh-read-only bot assigned alyssawilk and wu-bin Feb 24, 2021

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Feb 25, 2021

danzh1989 added 2 commits February 24, 2021 23:49

suppress parentheses error

a0e7297

Signed-off-by: Dan Zhang <[email protected]>

fix gcc error

ff19371

Signed-off-by: Dan Zhang <[email protected]>

wu-bin reviewed Feb 25, 2021

View reviewed changes

danzh1989 added 3 commits February 26, 2021 00:36

address comments

66aab81

Signed-off-by: Dan Zhang <[email protected]>

add more tests

a0ad789

Signed-off-by: Dan Zhang <[email protected]>

Merge branch 'master' into updatetar10

d248bed

Signed-off-by: Dan Zhang <[email protected]>

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Feb 26, 2021

danzh1989 added 2 commits February 26, 2021 23:46

fix requested server name

3b02bc5

Signed-off-by: Dan Zhang <[email protected]>

Merge branch 'master' into updatetar10

c218df5

Signed-off-by: Dan Zhang <[email protected]>

wu-bin previously approved these changes Mar 1, 2021

View reviewed changes

add TODO

68fe244

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 dismissed wu-bin’s stale review via 68fe244 March 1, 2021 18:14

typo

63bc639

Signed-off-by: Dan Zhang <[email protected]>

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Mar 2, 2021

alyssawilk reviewed Mar 2, 2021

View reviewed changes

alyssawilk previously approved these changes Mar 2, 2021

View reviewed changes

remove debug log

103ad46

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 dismissed alyssawilk’s stale review via 103ad46 March 2, 2021 21:34

remove debug logging

f52476e

Signed-off-by: Dan Zhang <[email protected]>

alyssawilk approved these changes Mar 3, 2021

View reviewed changes

alyssawilk merged commit 80a88a8 into envoyproxy:main Mar 3, 2021

This was referenced Mar 3, 2021

test: fixing CI #15282

Merged

quic: disabling one more flake #15294

Merged

alyssawilk added a commit to alyssawilk/envoy that referenced this pull request Mar 3, 2021

Revert "quiche: update QUICHE and boringSSL dependency (envoyproxy#15181

1374c26

)" This reverts commit 80a88a8. Signed-off-by: Alyssa Wilk <[email protected]>

moderation mentioned this pull request Apr 23, 2021

tls: update BoringSSL to c5ad6dcb (4491). #16104

Merged

		// Only call watermark callbacks on non QUIC static streams which are
		// crypto stream and Google QUIC headers stream.

Conversation

danzh2010 commented Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Feb 24, 2021

Uh oh!

danzh2010 commented Feb 24, 2021

Uh oh!

moderation commented Feb 25, 2021

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!

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!

danzh2010 commented Feb 27, 2021

Uh oh!

repokitteh-read-only bot commented Feb 27, 2021

Uh oh!

wu-bin 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!

danzh2010 commented Mar 2, 2021

Uh oh!

repokitteh-read-only bot commented Mar 2, 2021

Uh oh!

danzh2010 commented Mar 2, 2021

Uh oh!

moderation commented Mar 2, 2021

Uh oh!

moderation commented Mar 2, 2021

Uh oh!

htuch commented Mar 2, 2021

Uh oh!

danzh2010 commented Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch commented Mar 2, 2021

Uh oh!

danzh2010 commented Mar 2, 2021

Uh oh!

alyssawilk commented Mar 2, 2021

Uh oh!

davidben commented Mar 2, 2021

Uh oh!

htuch commented Mar 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Feb 24, 2021 •

edited

Loading

danzh2010 commented Mar 2, 2021 •

edited

Loading