quiche: more platform implementations and enhancement by danzh2010 · Pull Request #7283 · envoyproxy/envoy

danzh2010 · 2019-06-14T18:49:31Z

Roll up quiche tar ball to 88e3e05c341147f4052e17a2769ac2722739c498.
Add more platform implementations: quic_optional_impl, quic_macros_impl, quic_error_code_wrappers_imp.

Enhance quic_client_stats_impl and quic_mem_slice_span_impl with more functionalities or make them more robust.

Add an overload function of std::operator<< to streaming out a std::vector.

Risk Level: low, not in use
Testing: Added tests for newly added impl's and functionality.
Part of #2557

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

danzh2010 · 2019-06-14T18:49:58Z

/assign @wu-bin @mattklein123

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

danzh2010 · 2019-06-14T20:34:29Z

/retest

repokitteh-read-only · 2019-06-14T20:34:33Z

🔨 rebuilding ci/circleci: asan (failed build)

🐱

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

see: more, trace.

wu-bin · 2019-06-14T20:07:16Z

bazel/external/quiche.BUILD


 envoy_cc_library(
-    name = "quic_core_alarm_lib",
+    name = "quic_core_alarm_interface",


Why rename these quic core libraries? We've discussed this before and agreed to mirror the internal libraries names.

This is envoy convention. For interface libraries, use suffix _interface instead of _lib. This is clearer than internal libraries names.

This is a quic core library, the code does not use envoy convention, the build rule also doesn't have to. (recall that we want to export the build rules in quiche and use it from envoy some day)

That said, in this case we can use envoy convention and the same name at the same time, just rename the internal library to _interface.

wu-bin · 2019-06-17T13:55:37Z

bazel/external/quiche.BUILD

+)
+
+envoy_cc_library(
+    name = "quic_platform_logging",


Why put logging into a separate library? As we discussed before, we should avoid adding standalone platform libraries unless necessary.

I put it outside of quic_platform_base so that quic_socket_address.h can be placed in quic_platform_base. So other core libraries don't have to explicitly depend on quic_platform_socket_address. Becaue quic_platform_ip_address depends on it, and quic_socket_address depends on quic_platform_ip_address.

But considering that quic_socket_address will be moved to core eventually, I think it makes much more sends to make these 2 network address libraries stand-alone and keep logging as part of quic_platform_base.

wu-bin · 2019-06-17T13:56:23Z

bazel/external/quiche.BUILD

 )

+envoy_cc_library(
+    name = "quic_platform_ip_address",


It's ok to keep this as a standalone library because it will be moved to quic core soon.

wu-bin · 2019-06-17T14:05:53Z

source/extensions/quic_listeners/quiche/platform/quic_logging_impl.h

 QuicLogSink* SetLogSink(QuicLogSink* new_sink);

+// Overload std::operator<< to output a vector.
+template <class T> std::ostream& operator<<(std::ostream& out, const std::vector<T>& v) {


This function is not specific to QUIC, can you move it to a better place?

Only quic use this type of logging right? Envoy has it's only logging macros.

It doesn't need to be used by the QUIC_LOG macros. It can also be used in unit tests to pretty-print the content of a vector, similar to

envoy/test/common/network/cidr_range_test.cc

Line 22 in e0c9284

std::ostream& operator<<(std::ostream& out, const std::pair<First, Second>& p) {

I'm not sure where is the best place for such overload. This seems very much like an overload only for tests, but we definitely don't want to place it under /test.

source/common/common/stl_helpers.h sounds like a fine place for this.

wu-bin · 2019-06-17T14:12:53Z

source/extensions/quic_listeners/quiche/platform/quic_logging_impl.h

+template <class T> std::ostream& operator<<(std::ostream& out, const std::vector<T>& v) {
+  out << "vector { ";
+  size_t i = 0;
+  for (; i < v.size() - 1; ++i) {


I think line 186-193 can be replaced by absl::StrJoin(v, ", ", absl::StreamFormatter()).

Good point. Done

wu-bin · 2019-06-17T14:25:01Z

test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc


+TEST_F(QuicPlatformTest, QuicLogVector) {
+  std::vector<int32_t> v = {1, 2, 3, 4, 5};
+  EXPECT_LOG_CONTAINS("info", "vector { 1, 2, 3, 4, 5 }", QUIC_LOG(INFO) << v);


Does it work? IIRC, the default log level is ERROR, and INFO will not be logged unless you do GetLogger().set_level(INFO)

It shouldn't. But it did pass. It seems to be a bug in quic logging implementation. Anyway, the purpose of this test is testing operator<< overload, not logging level. I changed it to output at error instead.

It works because the macro forcefully set the log level to "trace".

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

danzh2010 · 2019-06-17T21:16:58Z

/retest

repokitteh-read-only · 2019-06-17T21:17:01Z

🐴 hold your horses - no failures detected, yet.

🐱

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

see: more, trace.

danzh2010 · 2019-06-17T21:17:17Z

/retest

repokitteh-read-only · 2019-06-17T21:17:22Z

🐴 hold your horses - no failures detected, yet.

🐱

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

see: more, trace.

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

danzh2010 · 2019-06-18T15:13:22Z

/retest

repokitteh-read-only · 2019-06-18T15:13:26Z

🤷‍♀️ nothing to rebuild.

🐱

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

see: more, trace.

mattklein123 · 2019-06-18T18:24:03Z

Please merge master to fix CI.

/wait

danzh2010 · 2019-06-18T18:36:32Z

/retest

repokitteh-read-only · 2019-06-18T18:36:36Z

🤷‍♀️ nothing to rebuild.

🐱

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

see: more, trace.

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

wu-bin · 2019-06-18T20:07:35Z

source/common/common/stl_helpers.h

+
+// NOLINT(namespace-envoy)
+// Overload functions in std library.
+namespace std {


Can we put it into the global namespace? I think it can be used everywhere that way.

in std namespace can also be used everywhere, at least in Envoy and quic namespaces as tested.
Similar overloads are all in std namespace in envoy. I just follow the convention.

wu-bin · 2019-06-18T20:10:47Z

source/extensions/quic_listeners/quiche/platform/quic_socket_address_impl.h

@@ -10,7 +10,7 @@



Since QuicSocketAddress has been de-platformized, can we delete this file now?

QuicSocketAddress has not been de-platformized in this tar ball yet. I still has problem to roll the tar ball upto where it is de-platformized, waiting for upstream fix to be merged into quiche. But quic_ip_address_impl.h should.

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

mattklein123

LGTM @wu-bin any further comments?

danzh1989 added 11 commits June 6, 2019 16:16

add quic_core_crypto_crypto_handshake_lib

c0e6ddc

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

update quic_ip_address

3290e50

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

make crypto_handshake_lib compile

b6fa1d1

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

Merge branch 'master' into cryptoconfig

1ca0d1d

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

compile quic_session_lib

9dcd6b8

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

rename alarm interface

b4c5c56

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

add bbr2 platform impl

0800797

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

build dispatcher

70f1206

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

remove unnecessary sed command

427f498

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

split out quiche targets build

67be74f

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

add test for CLIENT_HIST

438c05c

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

danzh2010 requested review from alyssawilk and mattklein123 as code owners June 14, 2019 18:49

repokitteh-read-only bot assigned mattklein123 and wu-bin Jun 14, 2019

danzh2010 changed the title ~~Cryptoconfig~~ quiche: more platform implementations and enhancement Jun 14, 2019

danzh1989 added 4 commits June 14, 2019 15:09

remove bbr2 impl

d7d1771

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

move operator<< overload into quic namespace

7b15a54

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

remove bbr2 impl

4b886f4

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

fix miss spelling

3d4d56c

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

wu-bin reviewed Jun 17, 2019

View reviewed changes

mattklein123 added the waiting label Jun 17, 2019

use StrJoin

62a4e10

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

repokitteh-read-only bot removed the waiting label Jun 17, 2019

danzh2010 closed this Jun 17, 2019

danzh2010 reopened this Jun 17, 2019

danzh1989 added 2 commits June 17, 2019 17:52

move operator<< to stl_helpers.h

0a944e1

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

revert changes to quic_logging

72d54aa

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

repokitteh-read-only bot added the waiting label Jun 18, 2019

Merge branch 'master' into cryptoconfig

d76795f

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

repokitteh-read-only bot removed the waiting label Jun 18, 2019

wu-bin reviewed Jun 18, 2019

View reviewed changes

delete quic_ip_address_impl.h

072ccaa

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

mattklein123 approved these changes Jun 18, 2019

View reviewed changes

mattklein123 merged commit 280c722 into envoyproxy:master Jun 19, 2019

Conversation

danzh2010 commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danzh2010 commented Jun 14, 2019

Uh oh!

danzh2010 commented Jun 14, 2019

Uh oh!

repokitteh-read-only bot commented Jun 14, 2019

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!

wu-bin Jun 17, 2019 • 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!

danzh2010 commented Jun 17, 2019

Uh oh!

repokitteh-read-only bot commented Jun 17, 2019

Uh oh!

danzh2010 commented Jun 17, 2019

Uh oh!

repokitteh-read-only bot commented Jun 17, 2019

Uh oh!

danzh2010 commented Jun 18, 2019

Uh oh!

repokitteh-read-only bot commented Jun 18, 2019

Uh oh!

mattklein123 commented Jun 18, 2019

Uh oh!

danzh2010 commented Jun 18, 2019

Uh oh!

repokitteh-read-only bot commented Jun 18, 2019

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

danzh2010 commented Jun 14, 2019 •

edited

Loading

wu-bin Jun 17, 2019 •

edited

Loading