-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SDS: reduce cost of secret update #19971
Conversation
Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Yuchen Dai <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/wait-any
source/extensions/transport_sockets/tls/context_manager_impl.cc
Outdated
Show resolved
Hide resolved
Signed-off-by: Yuchen Dai <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new ownership model and data structures looks good. Nice fix!
/wait
source/extensions/transport_sockets/tls/context_manager_impl.cc
Outdated
Show resolved
Hide resolved
Signed-off-by: Yuchen Dai <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a release note
@ggreenway add an item in minor behavior change |
Signed-off-by: Yuchen Dai <[email protected]>
Head branch was pushed to by a user without write access
Signed-off-by: Yuchen Dai <[email protected]>
Head branch was pushed to by a user without write access
/backport I am happy to create the PRs if it is approved |
CC @oschaaf for approval |
@lambdai I have no objections if @ggreenway or @yanavlasov would be up for reviewing the backports. |
Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Yuchen Dai <[email protected]>
Backport of #19971 Signed-off-by: Yuchen Dai <[email protected]>
Commit Message: Backport #19971 Signed-off-by: Yuchen Dai [email protected] Signed-off-by: Yuchen Dai <[email protected]>
config_->setSecretUpdateCallback([this]() { onAddOrUpdateSecret(); }); | ||
} | ||
|
||
ClientSslSocketFactory::~ClientSslSocketFactory() { manager_.removeContext(ssl_ctx_); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
manager_
could be a dangling reference when Envoy shuts down
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you create a test that fails in ASAN for this?
Commit Message:
Previously the SSL context manager scans the maintained list of context weak_ptr at any context update.
In this PR, the role of the maintenance moved to prod ClientSslSocketFactory and ServerSslSocketFactory.
The prod SSL context manager can then switch to shared_ptr from weak_ptr.
The shared_ptr can be put into hash set container and no list scan is needed.
This PR also move the context construction and destroy out of the critical contention.
Additional Description:
Risk Level: LOW
Testing: existing integration test, and updated unit test cases that bypassing the SslSocketFactory
Docs Changes:
Release Notes:
Fix #19774