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

Done. Thanks.

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.

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

Added blank line.

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();
  }
}

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

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

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!

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

Comments are added above each test case. Thanks.

Are you looking at this one as well?

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.

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.

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.