High level design PR: Add SDS dynamic secret by qiwzhang · Pull Request #3748 · envoyproxy/envoy

qiwzhang · 2018-06-28T01:59:27Z

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

This PR is not a real PR, it is a high level design proposal to support fetching dynamic secrets.

Main changes:

added SecretCallback interface to notify secret has been changed
added DynamicSecretProvider interface to fetch dynamic secret.
secretManager to hold static and dynamic secret separately.
use shared_ptr for Context objects so an new object can be created while old one is in-use.

Code Status:
Passed compiling for files under /source/....

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

qiwzhang · 2018-06-28T02:00:18Z

@lizan @PiotrSikora Could you help to review it?

lizan · 2018-06-28T18:08:19Z

include/envoy/secret/dynamic_secret_provider.h

+/**
+ * An interface to fetch dynamic secret.
+ */
+class DynamicSecretProvider {


I'm curious why we need this and createDynamicSecretProvider, but not make SecretManager to manage callbacks.

Managing update callbacks by DynamicSecretProvider is more efficient since it only holds the callbacks that using that secret. If done in the secretManager, you need a map of hash + name to the vector of callbacks, you need map lookup. Beside, DynamicSecretProvider is implemented by SdsAPI. It has the onConfigUpate() call, so it is more nature to call its callbacks when onConfigUpdate() is called. It doesn't need to hold secretManager object either.

lizan · 2018-06-28T18:08:45Z

source/common/secret/secret_manager_util.h

+
+class SecretManagerUtil {
+public:
+  virtual ~SecretManagerUtil() {}


not needed.

Will remove

lizan · 2018-06-28T18:09:40Z

include/envoy/secret/secret_manager.h

+   * @return a hash string of normalized config source.
+   */
+  virtual DynamicSecretProviderSharedPtr
+  createDynamicSecretProvider(const envoy::api::v2::core::ConfigSource& config_source,


from impl, this is not a pure create, but find one if there is existing, need a better name.

how about "findOrCreateDynamicSecretProvider"?

lizan · 2018-06-28T18:17:08Z

include/envoy/ssl/context.h


 class ClientContext : public virtual Context {};
 typedef std::unique_ptr<ClientContext> ClientContextPtr;
+typedef std::shared_ptr<ClientContext> ClientContextSharedPtr;


Do we intended to make all *Context to SharedPtr? What is the usecase of unique_ptrs?

unique_ptr can be removed.

Actually, I am working on a separate PR of making ssl_socket not to hold context as its member, only use ctx at constructor. If this works, we will continue to use unique_ptr for ctx.

lizan · 2018-06-28T18:18:31Z

source/common/ssl/ssl_socket.h

  std::vector<std::string> getDnsSansFromCertificate(X509* cert);

  Network::TransportSocketCallbacks* callbacks_{};
+  ContextSharedPtr ctx_owner_;


you should be able to have this as std::shared_ptr<ContextImpl> and cast it with std::dynamic_pointer_cast

As I mentioned above, I try to remove this member variable ctx_ in a separate PR

lizan · 2018-06-28T18:19:42Z

This includes all changes in #3700 right?

While we can review this on high level, but please address review/test on that first.

JimmyCYJ · 2018-06-28T18:23:10Z

@lizan Yes, I am modifying #3700 to make it consistent with this high level design. That PR currently needs more tests to meet test coverage requirements. Once the PR is ready, I will ping you.

stale · 2018-07-06T00:19:02Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

JimmyCYJ · 2018-07-06T00:21:22Z

#3700 is under review, and we are updating this PR accordingly. Please keep this PR open.

stale · 2018-07-13T00:58:12Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2018-07-20T23:03:02Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2018-07-27T23:11:18Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Add SDS dynamic secret

0dbc3ad

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

mattklein123 assigned PiotrSikora and lizan Jun 28, 2018

lizan reviewed Jun 28, 2018

View reviewed changes

qiwzhang mentioned this pull request Jun 28, 2018

Use shared_ptr for SSL context objects #3754

Merged

mattklein123 self-assigned this Jun 28, 2018

This was referenced Jun 29, 2018

Add updateCertChain to SSL ContextImpl #3765

Closed

Sds api: implement SDS api #3700

Closed

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 6, 2018

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 6, 2018

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 13, 2018

lizan added no stalebot Disables stalebot from closing an issue and removed stale stalebot believes this issue/PR has not been touched recently labels Jul 13, 2018

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 20, 2018

stale bot closed this Jul 27, 2018

lizan mentioned this pull request Aug 7, 2018

bot: exempt label "no stalebot" for PRs #4081

Merged

Conversation

qiwzhang commented Jun 28, 2018

Uh oh!

qiwzhang commented Jun 28, 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!

qiwzhang Jun 28, 2018 • 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!

lizan commented Jun 28, 2018

Uh oh!

JimmyCYJ commented Jun 28, 2018

Uh oh!

stale bot commented Jul 6, 2018

Uh oh!

JimmyCYJ commented Jul 6, 2018

Uh oh!

stale bot commented Jul 13, 2018

Uh oh!

stale bot commented Jul 20, 2018

Uh oh!

stale bot commented Jul 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

qiwzhang Jun 28, 2018 •

edited

Loading