quiche: update tar and implement EnvoyQuicProofSource by danzh2010 · Pull Request #11316 · envoyproxy/envoy

danzh2010 · 2020-05-26T05:18:49Z

Update QUICHE tar to 25da9198727ef05edeb99d9f4ce5b6acb3cb87b5 which turn on IETF QUIC by default.

Add QuicDownstream/UpstreamTransport protobuf to wrap tls.v3.Downstream/UpstreamTlsContext to distinguish QuicServer/ClientTransportSocketConfigFactory from Downstream/UpstreamSslSocketFactory by typed_config. And change configs in unit and integration tests accordingly. This is needed to retrieve filter chain correctly based on typed_config. Previously integration tests has been creating the wrong TransportSocketFactory which wasn't in use.

Fix quic_http_integration_test to work with IETF QUIC:

Add real implementation of ProofSource::GetCertChain() and ComputeTlsSignature() in EnvoyQuicProofSource which overrides EnvoyQuicFakeProofSource. The verifier is still a fake one which doesn't verify signature.
This QUICHE tar ball has an issue in ACK alarm which is not compatible with current implementation of Envoy event system(how timers work in libevent and timer granularity >1ms). So I added a walk-around in EnvoyQuicAlarm to make sure a timer scheduled within 1ms is triggered in next event loop. Otherwise QUIC ACK alarm keeps re-arm itself in same event loop and never exit. There will be an upstream fix to ensure QUICHE behavior doesn't assume platform event system behavior and timer granularity so.

Fix unit tests for ActiveQuicListener and EnvoyQuicDispatcher to work with IETF QUIC:

Add TestProofSource for unit tests
Add a util function for CHLO generation according to different versions

Risk Level: low, not in use
Testing: modify EnvoyQuicFakeProofSourceTest to test EnvoyQuicProofSource
Fixes #10767 #10422

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only · 2020-05-26T05:18:56Z

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

🐱

Caused by: #11316 was opened by danzh2010.

see: more, trace.

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2020-05-27T23:32:07Z

/assign @wu-bin @htuch

Signed-off-by: Dan Zhang <danzh@google.com>

htuch · 2020-05-29T00:21:31Z

@danzh1989 is it possible to split the tar update and the implementation part into distinct PRs?

Signed-off-by: Dan Zhang <danzh@google.com>

wu-bin

Thanks Dan, this looks good overall. It seems presubmit is failing, can you take a look at it?

wu-bin · 2020-06-04T18:16:30Z

+  // in QUICHE, and we are working on the fix. Once QUICHE is fixed of expecting this behavior, we
+  // no longer need to round up the duration.
+  timer_->enableHRTimer(std::chrono::microseconds(
+      (duration < quic::QuicTime::Delta::FromMicroseconds(1)) ? 1 : duration.ToMicroseconds()));


nit: How about std::max(1, duration.ToMicroseconds());

Doesn't show up - did you miss an upload?
Also cc @antoniovicente for if there's a better way to do this, or if it requires your libevent workarounds

Please add a TODO(antoniovicente) here. I'm looking into some improvements here.

sorry, updated with Bin's suggestion now.
Added TODO for Antonio, assuming it would be improvement with the timer impl.

wu-bin · 2020-06-04T18:36:22Z

+namespace Envoy {
+namespace Quic {
+
+class EnvoyQuicProofSource : public EnvoyQuicFakeProofSource,


In that case, please address the TODOs and renames as soon as this PR lands.

wu-bin · 2020-06-04T18:49:03Z

+    return quic::QuicReferenceCountedPointer<quic::ProofSource::Chain>(
+        new quic::ProofSource::Chain({}));


What's the difference between returning a nullptr and returning a non-nullptr but the chain is empty? Does caller need to handle them differently?

Yes, AFAIK reject reason differs in these two cases.

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2020-06-05T17:31:10Z

Thanks Dan, this looks good overall. It seems presubmit is failing, can you take a look at it?

Fixed those clang-tidy error. PTAL

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2020-06-08T19:09:13Z

coverage CI failed with error: Code coverage for extension source/extensions/transport_sockets/tls is lower than limit of 94.5 (94.4)

This PR didn't touch any files under tls directory.

danzh2010 · 2020-06-08T19:59:22Z

@alyssawilk Can you take a look now?

alyssawilk

Wohoo, getting there!

alyssawilk · 2020-06-09T14:00:05Z

+// [#protodoc-title: quic transport]
+// [#extension: envoy.transport_sockets.quic]
+
+// Configuration for QUIC transport socket. This provides Google's QUIC implementation to Envoy.


for QUIC -> for Downstream QUIC (and upstream below)

"This provides" doesn't seem quite right. Also is this just for Google-QUIC or also used for IETF-QUIC, just Google's implementation?

I should say Google's GQuic and IQuic implementation.

alyssawilk · 2020-06-09T14:01:20Z

+  // in QUICHE, and we are working on the fix. Once QUICHE is fixed of expecting this behavior, we
+  // no longer need to round up the duration.
+  timer_->enableHRTimer(std::chrono::microseconds(
+      (duration < quic::QuicTime::Delta::FromMicroseconds(1)) ? 1 : duration.ToMicroseconds()));


Doesn't show up - did you miss an upload?
Also cc @antoniovicente for if there's a better way to do this, or if it requires your libevent workarounds

alyssawilk · 2020-06-09T14:05:47Z

+namespace Envoy {
+namespace Quic {
+
+class EnvoyQuicProofSource : public EnvoyQuicFakeProofSource,


alyssawilk · 2020-06-09T14:08:36Z

                         testing::ValuesIn({true, false}));

 TEST_P(EnvoyQuicServerStreamTest, GetRequestAndResponse) {
+  quic::SetVerbosityLogThreshold(1);


Was this for your debugging or is there value in leaving it in? If so maybe comment?

ditto for uses below.

alyssawilk · 2020-06-09T14:09:28Z

 namespace Envoy {
 namespace Quic {

+std::vector<std::pair<Network::Address::IpVersion, bool>> generateTestParam() {


Is this the same in all three places? If so consider a utility

moved into test utils

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk

@mattklein123 / @htuch needs another API stamp

mattklein123

Awesome to see progress here! I found one typo but feel free to fix that in your next change.

mattklein123 · 2020-06-10T15:11:55Z

-#include "envoy/extensions/transport_sockets/tls/v3/cert.pb.h"
-#include "envoy/extensions/transport_sockets/tls/v3/tls.pb.validate.h"
-
+// #include "envoy/extensions/transport_sockets/tls/v3/tls.pb.validate.h"


wrowe · 2020-06-11T14:01:31Z

This commit has broken Windows compilation due to upstream quiche.

bazel-out/x64_windows-opt/bin/external/com_googlesource_quiche/quiche/quic/core/crypto/certificate_view.cc(140): error C7555: use of designated initializers requires at least '/std:c++latest'

See https://developercommunity.visualstudio.com/idea/518768/c-designated-initializers.html and expand comments.

Until we are able to build against bleed (stdc++20), meaning all stdc++17 changes have been adopted, this patch is a nonstarter. Can this be reverted and tabled until envoy is at least at stdc++17, or until addressed upstream using stdc++?

wrowe · 2020-06-11T14:01:47Z

cc @sunjayBhatia

danzh2010 · 2020-06-11T14:32:48Z

This commit has broken Windows compilation due to upstream quiche.

bazel-out/x64_windows-opt/bin/external/com_googlesource_quiche/quiche/quic/core/crypto/certificate_view.cc(140): error C7555: use of designated initializers requires at least '/std:c++latest'

See https://developercommunity.visualstudio.com/idea/518768/c-designated-initializers.html and expand comments.

Until we are able to build against bleed (stdc++20), meaning all stdc++17 changes have been adopted, this patch is a nonstarter. Can this be reverted and tabled until envoy is at least at stdc++17, or until addressed upstream using stdc++?

It can be easily fixed in upstream. But we might easily run into same problem again in the future.

antoniovicente · 2020-06-11T14:36:49Z

Please try to get an upstream fix in quiche done ASAP. Reverting the PR may still make sense to get back to a stable state. We may want to enable one of these clang warning modes to prevent future breakage: -Wc99-designator -Wc99-extensions -Wc++20-designator (may require clang 10) -Wc++20-extensions (may require clang 10)

…

On Thu, Jun 11, 2020 at 10:33 AM danzh ***@***.***> wrote: This commit has broken Windows compilation due to upstream quiche. bazel-out/x64_windows-opt/bin/external/com_googlesource_quiche/quiche/quic/core/crypto/certificate_view.cc(140): error C7555: use of designated initializers requires at least '/std:c++latest' See https://developercommunity.visualstudio.com/idea/518768/c-designated-initializers.html and expand comments. Until we are able to build against bleed (stdc++20), meaning all stdc++17 changes have been adopted, this patch is a nonstarter. Can this be reverted and tabled until envoy is at least at stdc++17, or until addressed upstream using stdc++? It can be easily fixed in upstream. But we might easily run into same problem again in the future. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11316 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGZ33CNYPMK5Z6AVVTF66F3RWDTKFANCNFSM4NJ7MHGQ> .

danzh2010 · 2020-06-11T20:42:22Z

Please try to get an upstream fix in quiche done ASAP. Reverting the PR may still make sense to get back to a stable state. We may want to enable one of these clang warning modes to prevent future breakage: -Wc99-designator -Wc99-extensions -Wc++20-designator (may require clang 10) -Wc++20-extensions (may require clang 10)
…
On Thu, Jun 11, 2020 at 10:33 AM danzh @.***> wrote: This commit has broken Windows compilation due to upstream quiche. bazel-out/x64_windows-opt/bin/external/com_googlesource_quiche/quiche/quic/core/crypto/certificate_view.cc(140): error C7555: use of designated initializers requires at least '/std:c++latest' See https://developercommunity.visualstudio.com/idea/518768/c-designated-initializers.html and expand comments. Until we are able to build against bleed (stdc++20), meaning all stdc++17 changes have been adopted, this patch is a nonstarter. Can this be reverted and tabled until envoy is at least at stdc++17, or until addressed upstream using stdc++? It can be easily fixed in upstream. But we might easily run into same problem again in the future. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11316 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGZ33CNYPMK5Z6AVVTF66F3RWDTKFANCNFSM4NJ7MHGQ .

Fix is in #11562

Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

Signed-off-by: Dan Zhang <danzh@google.com>

Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

danzh1989 added 10 commits May 14, 2020 14:56

new proto

4409843

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into updatetar4

1ca7eef

Signed-off-by: Dan Zhang <danzh@google.com>

Update v3

8294e3f

Signed-off-by: Dan Zhang <danzh@google.com>

integration test fail because proof source

3dcc534

Signed-off-by: Dan Zhang <danzh@google.com>

fix versioning

a2b84f2

Signed-off-by: Dan Zhang <danzh@google.com>

remove v2

e198c19

Signed-off-by: Dan Zhang <danzh@google.com>

test pass

5435183

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into updatetar4

1669f6f

Signed-off-by: Dan Zhang <danzh@google.com>

format

8101432

Signed-off-by: Dan Zhang <danzh@google.com>

remove log

3a58f74

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only Bot added the api label May 26, 2020

danzh1989 added 8 commits May 26, 2020 11:25

remove comment

d0cbe98

Signed-off-by: Dan Zhang <danzh@google.com>

revert clock change

9ee3539

Signed-off-by: Dan Zhang <danzh@google.com>

rename test

b70b6ff

Signed-off-by: Dan Zhang <danzh@google.com>

fix v4 proto

cc420b9

Signed-off-by: Dan Zhang <danzh@google.com>

fix shadown api

27716f4

Signed-off-by: Dan Zhang <danzh@google.com>

add ssl dep

143b8cd

Signed-off-by: Dan Zhang <danzh@google.com>

format

53d5e15

Signed-off-by: Dan Zhang <danzh@google.com>

fix doc

f39f340

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 changed the title ~~Updatetar4~~ quiche: update tar and implement EnvoyQuicProofSource May 27, 2020

use envoy_cc_extension

a2a8869

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 marked this pull request as ready for review May 27, 2020 23:31

danzh2010 requested review from alyssawilk and mattklein123 as code owners May 27, 2020 23:31

repokitteh-read-only Bot assigned htuch and wu-bin May 27, 2020

clang_tidy

78d094b

Signed-off-by: Dan Zhang <danzh@google.com>

fix typo

103577b

Signed-off-by: Dan Zhang <danzh@google.com>

wu-bin reviewed Jun 4, 2020

View reviewed changes

clang-tidy

b42b64e

Signed-off-by: Dan Zhang <danzh@google.com>

add NOLINT

d6106b3

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk reviewed Jun 9, 2020

View reviewed changes

danzh1989 added 4 commits June 9, 2020 14:38

extract common helper function

31335aa

Signed-off-by: Dan Zhang <danzh@google.com>

typo

c1dcea2

Signed-off-by: Dan Zhang <danzh@google.com>

fix dep

dc0111a

Signed-off-by: Dan Zhang <danzh@google.com>

clang-tidy

5e6edbf

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk approved these changes Jun 10, 2020

View reviewed changes

mattklein123 approved these changes Jun 10, 2020

View reviewed changes

repokitteh-read-only Bot removed the api label Jun 10, 2020

mattklein123 merged commit 92e608f into envoyproxy:master Jun 10, 2020

danzh2010 deleted the updatetar4 branch June 10, 2020 19:01

sunjayBhatia mentioned this pull request Jun 11, 2020

Windows build: Enable RBE build in Windows CI #11107

Merged

danzh2010 mentioned this pull request Jun 11, 2020

quiche: update tarball to d88a2f7a9ff5f9f6be2f50411b15b091affe04d3 #11562

Merged

yashwant121 pushed a commit to yashwant121/envoy that referenced this pull request Jun 24, 2020

quiche: update tar and implement EnvoyQuicProofSource (envoyproxy#11316)

8e13aa0

Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

songhu pushed a commit to songhu/envoy that referenced this pull request Jun 25, 2020

quiche: update tar and implement EnvoyQuicProofSource (envoyproxy#11316)

40cb7d3

Signed-off-by: Dan Zhang <danzh@google.com>

This was referenced Jun 29, 2020

build: Disable c++20 designator syntax in clang builds #11804

Closed

server: add scaled timer utility class #11773

Closed

yashwant121 pushed a commit to yashwant121/envoy that referenced this pull request Jul 24, 2020

quiche: update tar and implement EnvoyQuicProofSource (envoyproxy#11316)

c8012a0

Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

		return quic::QuicReferenceCountedPointer<quic::ProofSource::Chain>(
		new quic::ProofSource::Chain({}));

Conversation

danzh2010 commented May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only Bot commented May 26, 2020

Uh oh!

danzh2010 commented May 27, 2020

Uh oh!

htuch commented May 29, 2020

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!

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 Jun 5, 2020

Uh oh!

danzh2010 commented Jun 8, 2020

Uh oh!

danzh2010 commented Jun 8, 2020

Uh oh!

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

alyssawilk left a comment

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!

wrowe commented Jun 11, 2020

Uh oh!

wrowe commented Jun 11, 2020

Uh oh!

danzh2010 commented Jun 11, 2020

Uh oh!

antoniovicente commented Jun 11, 2020 via email

Uh oh!

danzh2010 commented Jun 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

danzh2010 commented May 26, 2020 •

edited

Loading