Skip to content

quiche: remove google quic support#17465

Merged
alyssawilk merged 12 commits intoenvoyproxy:mainfrom
danzh2010:removegquic
Jul 29, 2021
Merged

quiche: remove google quic support#17465
alyssawilk merged 12 commits intoenvoyproxy:mainfrom
danzh2010:removegquic

Conversation

@danzh2010
Copy link
Contributor

@danzh2010 danzh2010 commented Jul 23, 2021

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

Commit Message: Remove support of Google QUIC. QUICHE supports both IETF QUIC and Google QUIC. Given that IETF QUIC has been launched by Google and Google QUIC will be deprecated soon, remove the support of Google QUIC will lower the future maintenance burden.

Risk Level: low
Testing: existing tests passed
Fixes #16642

Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
@alyssawilk
Copy link
Contributor

Unfortunately this lowers Envoy coverage below the acceptable limit. Maybe could land with beefing up some of the QUIC unit tests (as QUIC coverage was already pretty low)

Signed-off-by: Dan Zhang <danzh@google.com>
@alyssawilk
Copy link
Contributor

Ahh, the joys of CI :-P
/wait

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

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So much cleanup!

I'd love if @wu-bin or @RenjieTang would do a QUIC pass, and I'll do another maintainer pass once CI is happy.

/wait

~EnvoyQuicProofVerifierBase() override = default;

// quic::ProofVerifier
// Return success if the certs chain is valid and signature of {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the comment removed? can it no longer return failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VerifyProof() is NOT_REACHED_GCOVR_EXCL_LINE now. So the comment is out dated.

Signed-off-by: Dan Zhang <danzh@google.com>
@@ -117,22 +93,6 @@ void EnvoyQuicClientSession::OnTlsHandshakeComplete() {
raiseConnectionEvent(Network::ConnectionEvent::Connected);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I noticed that we are not calling the base class method.
It's not a big issue since the base class method does actual work only on servers. But I think it'd nice to still have those QUIC_BUGs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return;
}
switch (version.transport_version) {
case quic::QUIC_VERSION_43:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these counters are no longer used. I think they can be removed from docs/root/configuration/http/http_conn_man/stats.rst and source/common/http/http3/codec_stats.h

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Signed-off-by: Dan Zhang <danzh@google.com>
RenjieTang
RenjieTang previously approved these changes Jul 27, 2021
Copy link
Contributor

@RenjieTang RenjieTang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'm not an expert on QUIC proof source. Bin might know better there.

Signed-off-by: Dan Zhang <danzh@google.com>
@alyssawilk
Copy link
Contributor

coverage failures are likely real. PTAL?
/wait

Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
@alyssawilk alyssawilk merged commit 110559f into envoyproxy:main Jul 29, 2021
baojr added a commit to baojr/envoy that referenced this pull request Jul 29, 2021
…bridge-stream

* upstream/main: (140 commits)
  quiche: remove google quic support (envoyproxy#17465)
  runtime: removing envoy.reloadable_features.check_ocsp_policy (envoyproxy#17524)
  upstream: not trying to do HTTP/3 where not configured (envoyproxy#17454)
  api: Remove confusing line about auto-generation (envoyproxy#17536)
  v2: removing bootstrap (envoyproxy#17523)
  connpool: Fix crash in pool removal if the cluster was already deleted (envoyproxy#17522)
  Enhance the comments clearly (envoyproxy#17517)
  mysql proxy: connection attributes parsing  (envoyproxy#17209)
  [ci] fix false positive CVE scan from node (envoyproxy#17510)
  Fixing Envoy Mobile factory strings (envoyproxy#17509)
  http3: validating codec (envoyproxy#17452)
  quic: add QUIC upstream stream reset error stats (envoyproxy#17496)
  thrift proxy: move UpstreamRequest into its own file (envoyproxy#17498)
  docs: Fixed FaultDelay docs. (envoyproxy#17495)
  updates links to jaegertracing-plugin.tar.gz (envoyproxy#17497)
  http: make custom inline headers bootstrap configurable (envoyproxy#17330)
  deps: update yaml-cpp to latest master (envoyproxy#17489)
  improving tracer coverage (envoyproxy#17493)
  Increase buffer size of `Win32RedirectRecords` (envoyproxy#17471)
  ext_proc: Fix problem with buffered body mode with empty or no body (envoyproxy#17430)
  ...

Signed-off-by: Garrett Bourg <bourg@squareup.com>
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
Commit Message: Remove support of Google QUIC. QUICHE supports both IETF QUIC and Google QUIC. Given that IETF QUIC has been launched by Google and Google QUIC will be deprecated soon, remove the support of Google QUIC will lower the future maintenance burden.

Risk Level: low
Testing: existing tests passed
Fixes envoyproxy#16642

Signed-off-by: Dan Zhang <danzh@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

remove gQUIC support from Envoy

4 participants