Implementation of SDS #4176

JimmyCYJ · 2018-08-16T00:18:40Z

Description: Implement SDS api that fetches secrets from remote SDS server. Secrets are stored in Secret Provider. Listeners and Clusters are updated when secrets are received.
Risk Level: Low
Testing: Unit tests and integration tests
Fixes #1194

Signed-off-by: Jimmy Chen [email protected]

JimmyCYJ · 2018-08-16T00:19:02Z

cc @lizan @qiwzhang @PiotrSikora

lizan · 2018-08-16T00:32:26Z

include/envoy/upstream/cluster_manager.h

@@ -26,6 +26,7 @@
 #include "envoy/upstream/upstream.h"

 namespace Envoy {
+


nit: revert

lizan · 2018-08-16T00:33:07Z

source/common/ssl/context_config_impl.cc

-Secret::TlsCertificateConfigProviderSharedPtr
-getTlsCertificateConfigProvider(const envoy::api::v2::auth::CommonTlsContext& config,
-                                Secret::SecretManager& secret_manager) {
+Secret::TlsCertificateConfigProviderSharedPtr ContextConfigImpl::getTlsCertificateConfigProvider(


why did this need to be a non static private function? I don't think it uses any member variable?

qiwzhang · 2018-08-16T02:07:55Z

include/envoy/ssl/context_config.h

+   * Add secret callback into context config.
+   * @param callback callback that is executed by context config.
+   */
+  virtual void setUpdateCallback(Secret::SecretCallbacks& callback) PURE;


setSecretUpdateCallback

qiwzhang · 2018-08-16T02:11:02Z

source/common/secret/sds_api.cc

+      /* rest_legacy_constructor */ nullptr,
+      "envoy.service.discovery.v2.SecretDiscoveryService.FetchSecrets",
+      "envoy.service.discovery.v2.SecretDiscoveryService.StreamSecrets");
+  Config::Utility::checkLocalInfo("sds", local_info_);


Can move subcription_ creation into the constructor so we don't need to store so many object references.

Only call start in the initialize() function

We cannot move subcription_ into constructor. Envoy::Config::SubscriptionFactory::subscriptionFromConfigSourceenvoy::api::v2::auth::Secre will access members of those objects, which are not ready when SdsApi constructor is called. That causes segmentation fault in test runs.

qiwzhang · 2018-08-16T02:13:38Z

source/common/secret/secret_manager_impl.cc

+  if (!secret_provider) {
+    ASSERT(secret_provider_context.initManager() != nullptr);
+
+    std::function<void()> unregister_secret_provider = [map_key, config_name, sds_config_source,


for easy to read, maybe create a member function removeDynamicSecretProvider() function, can the lambda function just call it.

Done. Thanks.

qiwzhang · 2018-08-16T02:14:54Z

source/common/ssl/context_config_impl.h

@@ -18,6 +19,13 @@ static const std::string INLINE_STRING = "<inline>";

 class ContextConfigImpl : public virtual Ssl::ContextConfig {
 public:
+  ~ContextConfigImpl() override {


move it .cc file

qiwzhang · 2018-08-16T02:15:05Z

source/common/ssl/context_config_impl.h

+    return !tls_certficate_provider_ || tls_certficate_provider_->secret();
+  }
+
+  void setUpdateCallback(Secret::SecretCallbacks& callback) override {


move it to .cc file

qiwzhang · 2018-08-16T02:16:58Z

source/common/ssl/context_config_impl.h

+      tls_certficate_provider_->removeUpdateCallback(*secret_callback_);
+    }
+    secret_callback_ = &callback;
+    if (tls_certficate_provider_.get() != nullptr) {


only save the callback when provider is not null.

You can do

if (secret_callback_) {
if (secret_callback_) {
secret_callback_->remove
}
secret_callback_ = &callback;
secret_callback_->add
}

qiwzhang · 2018-08-16T02:18:03Z

source/common/ssl/ssl_socket.cc

@@ -17,6 +17,38 @@ using Envoy::Network::PostIoAction;
 namespace Envoy {
 namespace Ssl {

+namespace {
+
+class NotReadySslSocket : public Network::TransportSocket, public Connection {


Please add comment for this class
// This SslSocket will be used when SSL secret is not fetched from SDS server.

qiwzhang · 2018-08-16T18:47:09Z

source/common/secret/secret_manager_impl.cc

+  ENVOY_LOG(debug, "Unregister secret provider. hash key: {}", map_key);
+  auto secret_provider = dynamic_secret_providers_.find(map_key);
+  if (secret_provider != dynamic_secret_providers_.end()) {
+    dynamic_secret_providers_.erase(map_key);


You can call erase() directly, then check the return value. If 0, log an error.

Nice advice. Thanks!

qiwzhang · 2018-08-16T18:49:32Z

source/common/secret/secret_manager_impl.cc

-                  sds_config_source.DebugString());
-      }
+    std::function<void()> unregister_secret_provider = [map_key, this]() {
+      this->removeDynamicSecretProvider(map_key);


may not need this variable, just use the lambda directly in the function argument. such

xxx(....,
[map_key, this] () { removeDynamicSecretProvider(map_key); }
);

lizan · 2018-08-16T23:45:53Z

Can you merge master and run clang-format on 6.0 per #4168?

JimmyCYJ · 2018-08-17T00:22:29Z

@lizan I have run clang-format on 6.0

lizan · 2018-08-17T00:24:12Z

include/envoy/secret/secret_callbacks.h

@@ -0,0 +1,22 @@
+#pragma once
+
+#include <memory>


nit: unused include

lizan · 2018-08-17T01:34:14Z

include/envoy/secret/secret_callbacks.h

@@ -0,0 +1,21 @@
+#pragma once
+
+#include <string>


string is not used either...

mattklein123

Thanks for the hard work here and extra thanks for the integration tests. It gives me good confidence this change works. Nice! I have some high level comments to get started with. FYI, @htuch is going to take over senior maintainer review on this since I am out for a month starting tomorrow. Thank you!

mattklein123 · 2018-08-17T17:55:14Z

include/envoy/secret/secret_manager.h

+
+namespace Server {
+namespace Configuration {
+class TransportSocketFactoryContext;


nit: It would probably be better to just move this interface into the secret or SSL/TLS namespace but not a big deal.

Thanks, I will keep this.

mattklein123 · 2018-08-17T17:56:31Z

include/envoy/ssl/context_config.h


 namespace Envoy {
+namespace Secret {
+class SecretCallbacks;


Do we need the forward declare here?

I have removed this forward declaration and included secret_callbacks.h.

mattklein123 · 2018-08-17T17:57:01Z

include/envoy/ssl/context_config.h

@@ -95,6 +98,17 @@ class ContextConfig {
   * @return The maximum TLS protocol version to negotiate.
   */
  virtual unsigned maxProtocolVersion() const PURE;
+
+  /**
+   * @return true if the ssl config is ready.


Can we add more information about what ready means?

Comment is updated.

mattklein123 · 2018-08-17T17:57:24Z

include/envoy/ssl/context_config.h

+
+  /**
+   * Add secret callback into context config.
+   * @param callback callback that is executed by context config.


Can you add more information about when callbacks will be invoked? It's not clear at the interface level why/when this happens.

More information is added into comment. Thanks.

mattklein123 · 2018-08-17T18:35:37Z

include/envoy/server/transport_socket_config.h

+   * Pass an init manager to register dynamic secret provider.
+   * @param init_manager instance of init manager.
+   */
+  virtual void setInitManager(Init::Manager& init_manager) PURE;


It's not optimal that this interface has a setter and a getter for the init manager. Is there any way to simplify this so that the init manager is known ahead of time and we only need a getter?

Ideally the init manager should exist when we create TransportSocketFactoryContext, so that we only need a getter. But we create TransportSocketFactoryContext at ClusterManagerFactory and then create init manager per cluster, we have to add it into factory context. @qiwzhang proposed #3831, I think once we are working on that, we can get rid of this setter.

Shouldn't the init manager then be part of the factory context?

For now, init manager is per cluster. At the time of the construction of TransportSocketFactoryContext, per cluster init manager is not available. We have to set it when init manager is available.

mattklein123 · 2018-08-17T18:45:23Z

source/common/ssl/ssl_socket.cc

@@ -17,6 +17,38 @@ using Envoy::Network::PostIoAction;
 namespace Envoy {
 namespace Ssl {

+namespace {
+
+// This SslSocket will be used when SSL secret is not fetched from SDS server.


Do we really need this? What causes us to require an instantiated connection? Can't we cause the control flow to return in such a way that there is no socket and we just fail whatever we are doing?

Our first attempt was to return nullptr in socket creation when ssl_context is not ready (SDS client failed to get secret). This works for ListenerImpl, but did not work for ClusterImpl. After looked at the ClusterImpl codes, most of them do not expect nullptr socket nor nullptr connection. It will require a lot of code changes to make these code to handle null connection.

For less code change, we decided to return such dummy socket which would just reset the connect.

I don't have an opinion on whether this is the best course of action or not. @htuch hopefully can have a look. In general I would prefer that we don't do this but if it's the best way I can accept that.

mattklein123 · 2018-08-17T18:47:03Z

source/common/ssl/ssl_socket.cc

 }

 bool ClientSslSocketFactory::implementsSecureTransport() const { return true; }

+void ClientSslSocketFactory::onAddOrUpdateSecret() {
+  ENVOY_LOG(debug, "Secret is updated.");
+  ssl_ctx_ = manager_.createSslClientContext(stats_scope_, *config_);


Pretty positive this needs locking. I think you will be accessing ssl_ctx_ across threads to make transport sockets. Thus, you will need a R/W lock here and ultimately should move to using TLS but a R/W lock is OK for now? I think at a high level having a locking/threading analysis of this change would be useful. Can you add that to the description somewhere?

Good point! Thanks! Will add R/W lock.

I have added a lock to protect read/write to ssl_ctx_, and added comments.

I don't think you will need a lock beyond what shared_ptr already provide, it seems unnecessary. Though you might want to have a local variable of shared_ptr during create socket, so the access to the member variable is always atomic.

@lizan Thanks for pointing this out. Yes the lock is not necessary as we already use shared_ptr. They are removed.

mattklein123 · 2018-08-17T18:47:39Z

source/common/ssl/ssl_socket.cc

 }

 bool ServerSslSocketFactory::implementsSecureTransport() const { return true; }

+void ServerSslSocketFactory::onAddOrUpdateSecret() {
+  ENVOY_LOG(debug, "Secret is updated.");
+  ssl_ctx_ = manager_.createSslServerContext(stats_scope_, *config_, server_names_);


Locking. Is there a way to share this code in a base class somehow? Would prefer to not implement a bunch of this twice.

Lock is added, thanks! ServerSslSocketFactory::onAddOrUpdateSecret() and ClientSslSocketFactory::onAddOrUpdateSecret() are different, one owns ServerContextSharedPtr ssl_ctx_ and the other owns ClientContextSharedPtr ssl_ctx_, and they are created by calling different methods at context manager. I would like to leave this method in separate class.

mattklein123 · 2018-08-17T18:49:45Z

source/common/upstream/upstream_impl.h

@@ -40,6 +40,8 @@
 #include "common/upstream/outlier_detection_impl.h"
 #include "common/upstream/resource_manager_impl.h"

+#include "server/init_manager_impl.h"


Please move init_manager_impl out of server/ and into a new subdirectory in source/ called init/. Feel free to do this in a followup but please add a TODO.

Added TODO in init_manager_impl.h. Thanks.

mattklein123 · 2018-08-17T18:50:22Z

source/common/upstream/upstream_impl.h

   */
  void onPreInitComplete();

+  /**
+   * Called by every concrete cluster after all Sds api targets registered at SDS init manager are


Remove references to SDS or make it clear that SDS is one of the things that init manager is used for. We are likely to add other things in the future.

Removed references to SDS from comment. Thanks.

Signed-off-by: JimmyCYJ <[email protected]>

htuch

Some comments to get started. This is a pretty huge PR; FWIW, I strongly encourage shorter PRs for reviewabilty and velocity.

htuch · 2018-08-21T22:24:31Z

include/envoy/secret/secret_manager.h

+   * Finds and returns a dynamic secret provider associated to SDS config. Create
+   * a new one if such provider does not exist.
+   *
+   * @param config_source a protobuf message object contains SDS config source.


Nit: s/contains/containing a/

Done. Thanks.

htuch · 2018-08-21T22:24:42Z

include/envoy/secret/secret_manager.h

+   * a new one if such provider does not exist.
+   *
+   * @param config_source a protobuf message object contains SDS config source.
+   * @param config_name a name that uniquely refers to the SDS config source


Nit: full stop at end of sentence.

Done. Thanks.

htuch · 2018-08-21T22:25:13Z

include/envoy/secret/secret_manager.h

+   * @param config_name a name that uniquely refers to the SDS config source
+   * @param secret_provider_context context that provides components for creating and initializing
+   * secret provider.
+   * @return the dynamic TLS secret provider.


Nit: @return TlsCertificateConfigProviderSharedPtr the dynamic TLS secret provider.

Done. Thanks.

htuch · 2018-08-21T22:33:50Z

include/envoy/secret/secret_provider.h

@@ -18,7 +19,17 @@ template <class SecretType> class SecretProvider {
   */
  virtual const SecretType* secret() const PURE;

-  // TODO(lizan): Add more methods for dynamic secret provider.
+  /**
+   * Add secret callback into secret provider.


Can you add some comments on thread safety? E.g. from which threads is it safe to call this, on which thread will the callback be invoked?

Comments are added. Thanks.

htuch · 2018-08-21T22:36:27Z

include/envoy/server/transport_socket_config.h

+   * Pass an init manager to register dynamic secret provider.
+   * @param init_manager instance of init manager.
+   */
+  virtual void setInitManager(Init::Manager& init_manager) PURE;


Shouldn't the init manager then be part of the factory context?

htuch · 2018-08-21T22:39:51Z

source/common/secret/sds_api.cc

+  }
+  const auto& secret = resources[0];
+  MessageUtil::validate(secret);
+  if (!(secret.name() == sds_config_name_)) {


htuch · 2018-08-21T22:42:30Z

source/common/secret/secret_manager_impl.cc

+void SecretManagerImpl::removeDynamicSecretProvider(const std::string& map_key) {
+  ENVOY_LOG(debug, "Unregister secret provider. hash key: {}", map_key);
+
+  ASSERT(dynamic_secret_providers_.erase(map_key) == 1);


This should be RELEASE_ASSERT; otherwise in opt builds, this entire line disappears.

Thanks for pointing that out. Fixed.

htuch · 2018-08-21T22:42:49Z

source/common/secret/secret_manager_impl.cc

+TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateDynamicSecretProvider(
+    const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name,
+    Server::Configuration::TransportSocketFactoryContext& secret_provider_context) {
+  std::string map_key = std::to_string(MessageUtil::hash(sds_config_source)) + config_name;


Nit:const std::string

htuch · 2018-08-21T22:44:37Z

source/common/secret/secret_manager_impl.cc

+    ASSERT(secret_provider_context.initManager() != nullptr);
+
+    std::function<void()> unregister_secret_provider = [map_key, this]() {
+      removeDynamicSecretProvider(map_key);


Maybe add some lifetime comments here. @ambuc recently hit issues where these kinds of callbacks were invoked and the equivalent of SdsApi outlived the equivalent of SecreteManagerImpl (this was in ListenerManager).

The lifetime issue has been captured by integration tests. We have adjusted the order of secret manager and other components to make sure SdsApi objects are destroyed before SecretManagerImpl. Comments are added. Thanks.

htuch · 2018-08-21T22:47:12Z

source/common/ssl/ssl_socket.cc

-  return std::make_unique<Ssl::SslSocket>(ssl_ctx_, Ssl::InitialState::Client);
+  // SDS would update ssl_ctx_ when Envoy is running.
+  // Need a read lock to let multiple threads gain read access to ssl_ctx_.
+  std::shared_lock<std::shared_timed_mutex> lock(ssl_ctx_mutex_);


I haven't looked into this yet, but this is something I'd like to understand the necessity for better; usually in Envoy, needing to do shared memory concurrency is not needed, and other mechanisms like TLS posting are the right solution.

lock is not necessary as @lizan points out here, I have removed them.

htuch · 2018-08-21T22:48:42Z

@JimmyCYJ also, personal plea to avoid force push; general GH etiquette avoids this to make reviewer lives easier (so they can just look a the delta between each PR).

Signed-off-by: JimmyCYJ <[email protected]>

JimmyCYJ · 2018-08-22T19:02:56Z

@htuch Thanks for reviewing this PR, I have created PR #4231 which has SDS api and dummy socket, and they are not in use.

Implement SDS API and dummy socket, and they are not in use. This is split from PR #4176. Risk Level: Low Testing: Unit tests Docs Changes: None Fixes #1194 Signed-off-by: JimmyCYJ <[email protected]>

Signed-off-by: JimmyCYJ <[email protected]>

JimmyCYJ · 2018-08-24T21:41:53Z

This PR didn't update correctly. I am going to close this one and create a new PR.

JimmyCYJ · 2018-08-27T17:35:31Z

I have created PR #4256, please take a look. Thanks!

lizan requested a review from PiotrSikora August 16, 2018 00:20

lizan self-assigned this Aug 16, 2018

lizan requested a review from mattklein123 August 16, 2018 00:21

lizan reviewed Aug 16, 2018

View reviewed changes

qiwzhang mentioned this pull request Aug 16, 2018

Not to send any request at GrpcSubscriptionImpl destructor #4178

Closed

qiwzhang reviewed Aug 16, 2018

View reviewed changes

JimmyCYJ force-pushed the secret_provider_context branch from 1c67da7 to 5b9e45f Compare August 16, 2018 22:46

JimmyCYJ force-pushed the secret_provider_context branch from 854b7af to b4a354d Compare August 17, 2018 00:20

lizan reviewed Aug 17, 2018

View reviewed changes

include/envoy/secret/secret_callbacks.h Outdated

@@ -0,0 +1,22 @@

#pragma once

#include <memory>

Copy link

Member

lizan Aug 17, 2018

Choose a reason for hiding this comment

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

nit: unused include

lizan reviewed Aug 17, 2018

View reviewed changes

include/envoy/secret/secret_callbacks.h Outdated

@@ -0,0 +1,21 @@

#pragma once

#include <string>

Copy link

Member

lizan Aug 17, 2018

Choose a reason for hiding this comment

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

string is not used either...

JimmyCYJ force-pushed the secret_provider_context branch from f1c054b to 02e1c52 Compare August 17, 2018 17:54

mattklein123 reviewed Aug 17, 2018

View reviewed changes

mattklein123 assigned htuch Aug 17, 2018

JimmyCYJ added 6 commits August 20, 2018 11:22

Rebase to master.

733372f

Signed-off-by: JimmyCYJ <[email protected]>

add files.

08da5ee

Signed-off-by: JimmyCYJ <[email protected]>

Fix format.

5fa9936

Signed-off-by: JimmyCYJ <[email protected]>

Revise per comments.

f301307

Signed-off-by: JimmyCYJ <[email protected]>

Revise per comments.

1f6fe1b

Signed-off-by: JimmyCYJ <[email protected]>

Revise per comments.

09ba59f

Signed-off-by: JimmyCYJ <[email protected]>

JimmyCYJ added 8 commits August 20, 2018 11:22

Revise per comments.

1e45df7

Signed-off-by: JimmyCYJ <[email protected]>

Revise per comments.

dee24cb

Signed-off-by: JimmyCYJ <[email protected]>

Fix compile issue after rebase.

ce1714f

Signed-off-by: JimmyCYJ <[email protected]>

Format fix.

a20a70a

Signed-off-by: JimmyCYJ <[email protected]>

remove unused include

99eb93f

Signed-off-by: JimmyCYJ <[email protected]>

Remove unused include.

2f678cd

Signed-off-by: JimmyCYJ <[email protected]>

Revise per comments.

1aaaa12

Signed-off-by: JimmyCYJ <[email protected]>

Revise per comments.

d6eb302

Signed-off-by: JimmyCYJ <[email protected]>

JimmyCYJ force-pushed the secret_provider_context branch from d7fe495 to d6eb302 Compare August 20, 2018 18:22

Remove unnecessary lock and fix compile issue.

aa8053b

Signed-off-by: JimmyCYJ <[email protected]>

htuch suggested changes Aug 21, 2018

View reviewed changes

Revise per comments.

2c4b1eb

Signed-off-by: JimmyCYJ <[email protected]>

JimmyCYJ mentioned this pull request Aug 22, 2018

Design a chained init manager #4224

Open

JimmyCYJ added 2 commits August 22, 2018 11:10

Remove lock.

aa58e90

Signed-off-by: JimmyCYJ <[email protected]>

Remove unused include.

a58b95a

Signed-off-by: JimmyCYJ <[email protected]>

This was referenced Aug 22, 2018

Sds api split #4230

Closed

Sds api split #4231

Merged

JimmyCYJ added 2 commits August 24, 2018 14:33

merge envoy/master.

7c59756

Signed-off-by: JimmyCYJ <[email protected]>

merge envoy/master.

a589e2f

Signed-off-by: JimmyCYJ <[email protected]>

JimmyCYJ requested a review from zuercher as a code owner August 24, 2018 21:40

JimmyCYJ closed this Aug 24, 2018

JimmyCYJ deleted the secret_provider_context branch August 24, 2018 21:55

JimmyCYJ mentioned this pull request Aug 28, 2018

Support Envoy to fetch secrets using SDS service. #4256

Merged

		@@ -26,6 +26,7 @@
		#include "envoy/upstream/upstream.h"

		namespace Envoy {

Implementation of SDS #4176

Implementation of SDS #4176

Conversation

JimmyCYJ commented Aug 16, 2018 • edited Loading

JimmyCYJ commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizan commented Aug 16, 2018

JimmyCYJ commented Aug 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiwzhang Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Aug 21, 2018

JimmyCYJ commented Aug 22, 2018

JimmyCYJ commented Aug 24, 2018

JimmyCYJ commented Aug 27, 2018

JimmyCYJ commented Aug 16, 2018 •

edited

Loading

qiwzhang Aug 17, 2018 •

edited

Loading