Skip to content

quic: use sds for upstream http/3#16462

Merged
alyssawilk merged 5 commits intoenvoyproxy:mainfrom
alyssawilk:sds
May 19, 2021
Merged

quic: use sds for upstream http/3#16462
alyssawilk merged 5 commits intoenvoyproxy:mainfrom
alyssawilk:sds

Conversation

@alyssawilk
Copy link
Copy Markdown
Contributor

Risk Level: Medium (some data plane refactors, mostly no-ops for !HTTP/3)
Testing: turned up HTTP/3 upstream SDS integration tests
Docs Changes: n/a
Release Notes: n/a
part of #14829

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
return quic_socket_factory->sslCtx();
}

std::shared_ptr<quic::QuicCryptoClientConfig> PersistentQuicInfoImpl::cryptoConfig() {
Copy link
Copy Markdown
Contributor

@danzh2010 danzh2010 May 12, 2021

Choose a reason for hiding this comment

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

This is somewhat laid-back updating. If a client connection is created before the SSL context update, will its crypto config not be updated till another createQuicNetworkConnection() is called?
Could this be a problem?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think once a quic client is created we support reloading, so AFIK it'd only be applied when we created a new client anyway.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ACK.
Another question is if we need to copy over resumption tickets stored in EnvoyQuicSessionCache before replacing crypto config?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would think if we changed client certs we'd not want to keep resumption info? cc @RyanTheOptimist

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yup, that sounds right to me.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Copy link
Copy Markdown
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

Looks good.

[](HttpConnPoolImplBase* pool) {
[](HttpConnPoolImplBase* pool) -> ::Envoy::ConnectionPool::ActiveClientPtr {
// If there's no ssl context, the secrets are not loaded. Fast-fail by returning null.
if (dynamic_cast<Quic::QuicClientTransportSocketFactory*>(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this cast guaranteed to succeed? Should it be a static_cast, and maybe an ASSERT that the dynamic_cast succeeds?

// fallback_factory_ will update the stats.
// TODO(14829) Client transport socket factory may also need to update quic crypto.
}
// fallback factory will update the context.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: capitalize Fallback or change back to fallback_factory_

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk merged commit 67bfb7c into envoyproxy:main May 19, 2021
This was referenced May 19, 2021
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
Risk Level: Medium (some data plane refactors, mostly no-ops for !HTTP/3)
Testing: turned up HTTP/3 upstream SDS integration tests
Docs Changes: n/a
Release Notes: n/a
part of envoyproxy#14829

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk deleted the sds branch February 28, 2022 21:25
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.

4 participants