Sds api split #4231

JimmyCYJ · 2018-08-22T18:59:24Z

Description: 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: Jimmy Chen [email protected]

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

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

cc: @htuch @lizan @qiwzhang @PiotrSikora

lizan · 2018-08-22T21:26:48Z

source/common/ssl/ssl_socket.cc

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

+namespace {
+// This SslSocket will be used when SSL secret is not fetched from SDS server.
+class NotReadySslSocket : public Network::TransportSocket, public Connection {


I don't think you need this to be a Ssl::Connection. just inherit TransportSocket is enough.

Done. Thanks.

lizan · 2018-08-22T21:27:13Z

source/common/secret/sds_api.cc

+    : local_info_(local_info), dispatcher_(dispatcher), random_(random), stats_(stats),
+      cluster_manager_(cluster_manager), sds_config_(sds_config), sds_config_name_(sds_config_name),
+      secret_hash_(0), unregister_secret_provider_cb_(unregister_secret_provider) {
+  // TODO(JimmyCYJ): Implement chained_init_manager, so that multiple init_manager


Once this is resolved, do we still need NotReadySslSocket?

We still need NotReadySslSocket. Chained init manager guarantees that If two listeners point to the same secret, both listeners would wait for secrets and then listen on port. A better explanation is here. NotReadySslSocket is to solve the issue that if a listener gets secret and listens on a port, but the secret is bad, we should not use the secret to handle connection, and close the connection right away.

htuch

Yeah, this is much clearer. Looks good, just some RAII ideas to improve interfaces.

htuch · 2018-08-22T23:08:02Z

include/envoy/secret/secret_provider.h

+   * @param callback callback that is executed by secret provider.
+   */
+  virtual void addUpdateCallback(SecretCallbacks& callback) PURE;
+  /**


Nit: blank line between declaration and comment belonging to next block.

Added blank line.

htuch · 2018-08-22T23:09:13Z

include/envoy/secret/secret_provider.h

+   * Remove secret callback from secret provider.
+   * @param callback callback that is executed by secret provider.
+   */
+  virtual void removeUpdateCallback(SecretCallbacks& callback) PURE;


A general pattern in Envoy is to use RAII for managing removal of callbacks; it's a robust design pattern. We actually have a callback manager that models this today, see https://github.com/envoyproxy/envoy/blob/master/source/common/common/callback_impl.h

Are you looking at this one as well?

Yes, I am working on this one. I will push my update as soon I finish it. Thanks for asking.

I have added CallbackManager into SdsApi and removed removeUpdateCallback().

In PR #4176, I am going to add Common::CallbackHandle* secret_update_callback_handle_ into ContextConfigImpl as a private member, and modify ContextConfigImpl::setSecretUpdateCallback() like this.

void setSecretUpdateCallback(std::function<()> callback) override { if (secret_update_callback_handle_) { secret_update_callback_handle_->remove(); } secret_update_callback_handle_ = tls_certficate_provider_->addUpdateCallback(callback); }

I will also modify ~ContextConfigImpl() like this.

ContextConfigImpl::~ContextConfigImpl() { if (secret_update_callback_handle_) { secret_update_callback_handle_->remove(); } }

htuch · 2018-08-22T23:10:14Z

include/envoy/secret/BUILD

@@ -8,10 +8,16 @@ load(

 envoy_package()

+envoy_cc_library(


Thanks for splitting this PR out, this is very helpful.

Glad that it helps. Will keep PR in good size.

htuch · 2018-08-22T23:19:29Z

source/common/secret/sds_api.cc

+               std::function<void()> unregister_secret_provider)
+    : local_info_(local_info), dispatcher_(dispatcher), random_(random), stats_(stats),
+      cluster_manager_(cluster_manager), sds_config_(sds_config), sds_config_name_(sds_config_name),
+      secret_hash_(0), unregister_secret_provider_cb_(unregister_secret_provider) {


I'm a bit uncomfortable with the naming (and maybe even presence) of unregister_secret_provider_cb. It should at least be a generic destructor_cb_, but I would go further. This could be refactored to have SdsApi completely oblivious to this callback. Instead, the places that have the SdsApi instances, e.g. in the listener manager, have an object that wraps both SdsApi and includes a RAII cleanup, e.g. something like https://github.com/envoyproxy/envoy/blob/master/source/common/common/cleanup.h.

I'm not sure exactly how to make this super clean in your use case, but I feel that right now, this is not the right interface. Can you try out some of the above ideas to try and get SdsApi out of the business of dealing with object lifetimes of its owner?

Added a Cleanup member into SdsApi that holds the destructor_cb, which will be invoked by ~SdsApi(). Thanks for the advice!

htuch · 2018-08-22T23:21:18Z

test/common/secret/sds_api_test.cc

+                            "Unexpected SDS secrets length: 2");
+}
+
+TEST_F(SdsApiTest, SecretUpdateWrongSecretName) {


Can you add one liner // above each test case explaining in simple terms what's happening?

Comments are added above each test case. Thanks.

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

htuch · 2018-08-23T21:56:15Z

include/envoy/secret/secret_provider.h

+   * Remove secret callback from secret provider.
+   * @param callback callback that is executed by secret provider.
+   */
+  virtual void removeUpdateCallback(SecretCallbacks& callback) PURE;


Are you looking at this one as well?

htuch · 2018-08-23T21:57:49Z

source/common/secret/sds_api.cc

+               Runtime::RandomGenerator& random, Stats::Store& stats,
+               Upstream::ClusterManager& cluster_manager, Init::Manager& init_manager,
+               const envoy::api::v2::core::ConfigSource& sds_config, std::string sds_config_name,
+               std::function<void()> destructor_cb)


This is better than before, thanks. I still think it can be cleaner if instead of having SdsApi owning the destructor CB, we have the owner of SdsApi own that object. It's pretty weird threading a destructor CB into an object when the lifetime issues are really those of the owning object.

Thanks for pointing this out. My concern is that SdsApi is held by a shared_ptr (ContextConfigImpland::tls_certficate_provider_) and could be owned by multiple objects (e.g. multiple listeners or clusters). The last owner needs to destroy SdsApi and ~SdsApi() invokes this cleanup function. I am not clear if there is a better place to own the Cleanup object other than SdsApi.

@htuch SdsApi is a shared pointer that could be owned by multiple objects as @JimmyCYJ pointed out, the structure here is similar to what we have in RDS (RdsRouteConfigSubscription). With the destructor_cb it doesn't need to be a friend class between SecretManagerImpl and SdsApi.

Yeah, OK, I still think you could wrap SdsApi in a SdsApiWithManagedLifetime object to get the same effect and separate concerns here, but I guess we have precedent for doing it this way.

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

htuch

LGTM, thanks.

htuch · 2018-08-24T03:22:13Z

source/common/secret/sds_api.h

+  }
+
+  Common::CallbackHandle* addUpdateCallback(std::function<void()> callback) override {
+    return update_callback_manager_.add(callback);


Non-actionable; for some reason I thought this was RAII, but it's just a callback removal handle. We can make that more robust independent of this PR.

htuch · 2018-08-24T03:24:02Z

source/common/secret/sds_api.cc

+               Runtime::RandomGenerator& random, Stats::Store& stats,
+               Upstream::ClusterManager& cluster_manager, Init::Manager& init_manager,
+               const envoy::api::v2::core::ConfigSource& sds_config, std::string sds_config_name,
+               std::function<void()> destructor_cb)


Yeah, OK, I still think you could wrap SdsApi in a SdsApiWithManagedLifetime object to get the same effect and separate concerns here, but I guess we have precedent for doing it this way.

JimmyCYJ added 2 commits August 22, 2018 11:56

Add Sds api and dummy socket.

cf68bc6

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

Add more files.

b3d0680

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

JimmyCYJ mentioned this pull request Aug 22, 2018

Implementation of SDS #4176

Closed

lizan reviewed Aug 22, 2018

View reviewed changes

htuch suggested changes Aug 22, 2018

View reviewed changes

danielhochman assigned htuch and lizan Aug 23, 2018

JimmyCYJ added 2 commits August 23, 2018 11:54

Revise per comments.

0766ff5

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

Add Cleanup into SdsApi.

550287f

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

htuch reviewed Aug 23, 2018

View reviewed changes

Add CallbackManager into SdsApi.

1ec1701

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

htuch approved these changes Aug 24, 2018

View reviewed changes

htuch merged commit 99df2e2 into envoyproxy:master Aug 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sds api split #4231

Sds api split #4231

JimmyCYJ commented Aug 22, 2018

JimmyCYJ commented Aug 22, 2018

lizan Aug 22, 2018

JimmyCYJ Aug 23, 2018

lizan Aug 22, 2018

JimmyCYJ Aug 22, 2018

htuch left a comment

htuch Aug 22, 2018

JimmyCYJ Aug 23, 2018

htuch Aug 22, 2018

htuch Aug 23, 2018

JimmyCYJ Aug 23, 2018

JimmyCYJ Aug 23, 2018 •

edited

Loading

htuch Aug 22, 2018

JimmyCYJ Aug 23, 2018

htuch Aug 22, 2018

JimmyCYJ Aug 23, 2018

htuch Aug 22, 2018

JimmyCYJ Aug 23, 2018

htuch Aug 23, 2018

htuch Aug 23, 2018

JimmyCYJ Aug 23, 2018

lizan Aug 23, 2018

htuch Aug 24, 2018

htuch left a comment

htuch Aug 24, 2018

htuch Aug 24, 2018

Sds api split #4231

Sds api split #4231

Conversation

JimmyCYJ commented Aug 22, 2018

JimmyCYJ commented Aug 22, 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

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

JimmyCYJ Aug 23, 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

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

JimmyCYJ Aug 23, 2018 •

edited

Loading