Use shared_ptr for SSL context objects by qiwzhang · Pull Request #3754 · envoyproxy/envoy

qiwzhang · 2018-06-28T23:27:35Z

Description: This is one of PR to support dynamic secret for SDS. The master design is in this PR #3748.
This PR only changes Ssl context to use shared_ptr.

ssl_socket is holding one ref_count of ctx;
socketFactory is holding another ref_count, but this one can be updated if dynamic secret is changed.
With shared_ptr, existing sockets still work since they are holding ref_count to the old ctx so it will not be deleted.

Risk Level: Low

Testing: all unit tests passed.

Docs Changes: None

Release Notes: None
[Optional Fixes #Issue]
[Optional Deprecated:]

qiwzhang · 2018-06-28T23:28:38Z

@lizan @PiotrSikora Could you help to review it? Thanks.

mattklein123 · 2018-06-28T23:30:03Z

I thought we discussed changing the context directly with appropriate locking? Why do we need this? Will that not work?

qiwzhang · 2018-06-28T23:52:37Z

Thanks for pointing that out, will try. Close this for now

qiwzhang · 2018-06-29T23:02:58Z

@mattklein123 Please look at the comments at #3765. Due to that, I re-open this PR

mattklein123 · 2018-06-29T23:12:22Z

@qiwzhang yup agreed this is a cleaner approach. We can review this one.

qiwzhang · 2018-07-03T01:42:27Z

@PiotrSikora @lizan please disregard the mac test failure. This PR is ready for review.

lizan · 2018-07-09T05:09:19Z

include/envoy/ssl/context_manager.h

why do you need this in the interface?

Removed it. Please see my other comment

lizan · 2018-07-09T05:10:51Z

source/common/ssl/context_manager_impl.cc

update the comment above about rename if we do the rename.

Reverted it. Please see my other comment

lizan · 2018-07-09T05:12:55Z

source/common/ssl/ssl_socket.cc

in ~ContextImpl it already calls removeContext, isn't this removing twice? since remove doesn't raise exceptions when there is nothing to remove, you may want to add an ASSERT in removeContext.

Good point. It was added in the final PR with secret update code to fix some integration test failures. I reverted the changes for now.

qiwzhang · 2018-07-09T18:25:59Z

@mattklein123 @lizan @PiotrSikora This PR is ready for review. PTAL. Thanks

lizan · 2018-07-10T22:56:56Z

source/common/ssl/context_manager_impl.cc

nit: make_shared (throughout all news)

lizan · 2018-07-10T22:58:36Z

source/common/ssl/ssl_socket.cc

Is it possible to make ctx_ as std::shared_ptr<Ssl::ContextImpl> and use std::dynamic_pointer_cast? It will eliminates dynamic_casts below.

lizan · 2018-07-10T23:09:15Z

source/common/ssl/context_manager_impl.cc

since now contexts are shared_ptr, shouldn't contexts_ be a list of weak_ptr?

There is not easy way to remove the item in the list after its weak_ptr ref_count is 0. Then there will be weak_ptr leak.

weak_ptr wouldn't be leaked, actually with a list of weak_ptr we can eliminates the releaseContext logic and erase the weak_ptrs when the weak_ptrs are expired when iterating over them. With dynamic secret there will be more inefficient erase call of list which is O(n), so that might be more efficient. I'm OK to make that as a follow up PR later.

Done. releaseContext is removed.

lizan · 2018-07-11T04:23:13Z

test/mocks/ssl/mocks.h

I don't think you need createSslClientContext_ any more because shared_ptr is copyable. You can use MOCK_METHOD2 directly.

lizan

can you take a look on CI failure?

lizan · 2018-07-11T21:07:35Z

source/common/ssl/context_manager_impl.cc

use https://en.cppreference.com/w/cpp/memory/weak_ptr/expired

lizan · 2018-07-11T21:09:38Z

source/common/ssl/context_manager_impl.cc

since iterateContexts is not const method, consider remove empty context here.

lizan · 2018-07-11T22:53:50Z

@qiwzhang can you merge master and fix tests?

lizan · 2018-07-11T22:59:31Z

source/common/ssl/context_manager_impl.cc

use std::list::remove_if

lizan · 2018-07-11T23:01:25Z

source/common/ssl/context_manager_impl.cc

I prefer calling removeEmptyContexts instead of inline remove. It will iterate twice but cleaner code.

lizan · 2018-07-12T00:58:34Z

source/common/ssl/context_manager_impl.cc

use const ref for parameter

lizan · 2018-07-12T01:00:35Z

source/common/ssl/context_manager_impl.cc

Sorry I'm taking back my previous comment, let's not do this since we're removing in each create. shared_lock above is not enough to perform this.

qiwzhang · 2018-07-12T23:25:51Z

@mattklein123 @PiotrSikora Could you help to review this PR? Thanks

mattklein123

LGTM, thanks. 1 question.

mattklein123 · 2018-07-13T21:02:52Z

source/common/ssl/context_manager_impl.h

Is this lock needed anymore? I'm pretty sure all operations happen on the main thread now...

I don't think this PR changed the thread models calling this, no? To minimize risks, I'll defer removing this mutex to #3700.

I think it did change the thread model. Previously, the lock was only required because IIRC the context was indirectly owned by ClusterInfo, which can be released on any thread. With this change I'm pretty sure that all of the interactions with the context list/map only happen on the main thread, thus the lock is no longer necessary (which is a really nice improvement IMO). If that's the case, I would remove the lock as part of this change as IMO it's a logical part of this change and we can deploy/test it discretely.

Ah you're right, now when the context is released we're not immediately removing it from contexts_ but wait until next create, which happens in main thread. So we do not need a lock for contexts_ any more. @qiwzhang can you remove this mutex and lock?

mattklein123 · 2018-07-16T20:27:37Z

@qiwzhang please merge master for flake fixes.

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

mattklein123 assigned PiotrSikora, lizan and mattklein123 Jun 28, 2018

qiwzhang closed this Jun 28, 2018

qiwzhang deleted the shared_ctx_ptr branch June 28, 2018 23:52

qiwzhang restored the shared_ctx_ptr branch June 29, 2018 22:50

qiwzhang reopened this Jun 29, 2018

qiwzhang mentioned this pull request Jun 29, 2018

Add updateCertChain to SSL ContextImpl #3765

Closed

qiwzhang force-pushed the shared_ctx_ptr branch from be1cc85 to 3816bf8 Compare July 2, 2018 15:50

qiwzhang mentioned this pull request Jul 3, 2018

Sds api: implement SDS api #3700

Closed

qiwzhang force-pushed the shared_ctx_ptr branch from 97197cc to d6d8163 Compare July 6, 2018 23:34

lizan reviewed Jul 9, 2018

View reviewed changes

lizan reviewed Jul 10, 2018

View reviewed changes

lizan reviewed Jul 11, 2018

View reviewed changes

qiwzhang force-pushed the shared_ctx_ptr branch from 2cdc6b7 to 19753b3 Compare July 12, 2018 00:14

lizan reviewed Jul 12, 2018

View reviewed changes

lizan requested review from PiotrSikora and mattklein123 July 12, 2018 23:47

lizan previously approved these changes Jul 12, 2018

View reviewed changes

mattklein123 reviewed Jul 13, 2018

View reviewed changes

qiwzhang dismissed lizan’s stale review via 575048e July 16, 2018 18:36

qiwzhang added 12 commits July 16, 2018 20:49

Use shared_ptr for Context

c9ad46d

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

Fix tests

232387b

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

Fix mac test failure

4631c09

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

Revert releaseContext changes

2c515bf

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

Remove un-used manager_ in SocketFactory

41e7c0d

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

Update per comment

b6de48c

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

Use weak_ptr for context_manager

e83b5b5

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

use weak_ptr expired

5ecc0af

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

use remove_if

1dc061f

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

Fix rebase error

d17287a

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

Not to removeEmptyContext in iterate

5b8f879

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

Remove lock from context_manager_impl

054e169

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

qiwzhang force-pushed the shared_ctx_ptr branch from 575048e to 054e169 Compare July 16, 2018 20:49

mattklein123 approved these changes Jul 16, 2018

View reviewed changes

lizan approved these changes Jul 16, 2018

View reviewed changes

lizan merged commit 445f365 into envoyproxy:master Jul 16, 2018

Conversation

qiwzhang commented Jun 28, 2018

Uh oh!

qiwzhang commented Jun 28, 2018

Uh oh!

mattklein123 commented Jun 28, 2018

Uh oh!

qiwzhang commented Jun 28, 2018

Uh oh!

qiwzhang commented Jun 29, 2018

Uh oh!

mattklein123 commented Jun 29, 2018

Uh oh!

qiwzhang commented Jul 3, 2018

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!

qiwzhang commented Jul 9, 2018

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!

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

lizan commented Jul 11, 2018

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!

qiwzhang commented Jul 12, 2018

Uh oh!