Implementing Yang's secound round of comments.

include/grpcpp/security/tls_credentials_options.h

@@ -112,39 +112,44 @@ class TlsCredentialReloadArg {
 };
 
 /** An interface that the application derives and uses to instantiate a
- * TlsCredentialReloadConfig instance. All 3 methods must be defined. **/
+ * TlsCredentialReloadConfig instance. Refer to the definition of the
+ * grpc_tls_credential_reload_config in grpc_tls_credentials_options.h for more
+ * details on the expectations of the member functions of the interface. **/
 struct TlsCredentialReloadInterface {
-  /** An application-provided callback that invokes the credential reload. **/
-  virtual int Schedule(TlsCredentialReloadArg* arg) = 0;
-  /** An application-provided callback that cancels a credential reload request.
-   * **/
-  virtual void Cancel(TlsCredentialReloadArg* arg) = 0;
-  /** An application-provided callback that cleans up any data associated to the
-   * interface or the config. **/
-  virtual void Release() = 0;
+  virtual ~TlsCredentialReloadInterface() = default;
+  /** A callback that invokes the credential reload. **/
+  virtual int Schedule(TlsCredentialReloadArg* arg) { return 1; }
+  /** A callback that cancels a credential reload request. **/
+  virtual void Cancel(TlsCredentialReloadArg* arg) {}
+  /** A callback that cleans up any data associated to the
+   * interface or the config. It will be called when the config is no longer
+   * using the interface. **/
+  virtual void Release() {}
 };
 
 /** TLS credential reloag config, wraps grpc_tls_credential_reload_config. It is
  * used for experimental purposes for now and it is subject to change. **/
 class TlsCredentialReloadConfig {
  public:
-  /** The constructor takes ownership of the interface argument. **/
-  TlsCredentialReloadConfig(
-      std::shared_ptr<TlsCredentialReloadInterface> interface);
+  /** The config takes ownership of the credential reload interface. **/
+  TlsCredentialReloadConfig(std::unique_ptr<TlsCredentialReloadInterface>
+                                credential_reload_interface);
   ~TlsCredentialReloadConfig();
 
   int Schedule(TlsCredentialReloadArg* arg) const {
-    return interface_->Schedule(arg);
+    return credential_reload_interface_->Schedule(arg);
   }
 
-  void Cancel(TlsCredentialReloadArg* arg) const { interface_->Cancel(arg); }
+  void Cancel(TlsCredentialReloadArg* arg) const {
+    credential_reload_interface_->Cancel(arg);
+  }
 
   /** Returns a C struct for the credential reload config. **/
   grpc_tls_credential_reload_config* c_config() const { return c_config_; }
 
  private:
   grpc_tls_credential_reload_config* c_config_;
-  std::shared_ptr<TlsCredentialReloadInterface> interface_;
+  std::unique_ptr<TlsCredentialReloadInterface> credential_reload_interface_;
 };
 
 /** TLS server authorization check arguments, wraps
@@ -195,19 +200,20 @@ class TlsServerAuthorizationCheckArg {
 };
 
 /** An interface that the application derives and uses to instantiate a
- * TlsServerAuthorizationCheckConfig instance. All 3 methods must be defined.
+ * TlsServerAuthorizationCheckConfig instance. Refer to the definition of the
+ * grpc_tls_server_authorization_check_config in grpc_tls_credentials_options.h
+ * for more details on the expectations of the member functions of the
+ * interface.
  * **/
 struct TlsServerAuthorizationCheckInterface {
-  /** An application-provided callback that invokes the server authorization
-   * check. **/
-  virtual int Schedule(TlsServerAuthorizationCheckArg* arg) = 0;
-  /** An application-provided callback that cancels a server authorization check
-   * request.
-   * **/
-  virtual void Cancel(TlsServerAuthorizationCheckArg* arg) = 0;
-  /** An application-provided callback that cleans up any data associated to the
+  virtual ~TlsServerAuthorizationCheckInterface() = default;
+  /** A callback that invokes the server authorization check. **/
+  virtual int Schedule(TlsServerAuthorizationCheckArg* arg) { return 1; }
+  /** A callback that cancels a server authorization check request. **/
+  virtual void Cancel(TlsServerAuthorizationCheckArg* arg){};
+  /** A callback that cleans up any data associated to the
    * interface or the config. **/
-  virtual void Release() = 0;
+  virtual void Release(){};
 };
 
 /** TLS server authorization check config, wraps
@@ -215,17 +221,19 @@ struct TlsServerAuthorizationCheckInterface {
  *  purposes for now and it is subject to change. **/
 class TlsServerAuthorizationCheckConfig {
  public:
-  /** The constructor takess ownership of the interface argument. **/
+  /** The config takes ownership of the server authorization check interface.
+   * **/
   TlsServerAuthorizationCheckConfig(
-      std::shared_ptr<TlsServerAuthorizationCheckInterface> interface);
+      std::unique_ptr<TlsServerAuthorizationCheckInterface>
+          server_authorization_check_interface);
   ~TlsServerAuthorizationCheckConfig();
 
   int Schedule(TlsServerAuthorizationCheckArg* arg) const {
-    return interface_->Schedule(arg);
+    return server_authorization_check_interface_->Schedule(arg);
   }
 
   void Cancel(TlsServerAuthorizationCheckArg* arg) const {
-    interface_->Cancel(arg);
+    server_authorization_check_interface_->Cancel(arg);
   }
 
   /** Creates C struct for the server authorization check config. **/
@@ -235,7 +243,8 @@ class TlsServerAuthorizationCheckConfig {
 
  private:
   grpc_tls_server_authorization_check_config* c_config_;
-  std::shared_ptr<TlsServerAuthorizationCheckInterface> interface_;
+  std::unique_ptr<TlsServerAuthorizationCheckInterface>
+      server_authorization_check_interface_;
 };
 
 /** TLS credentials options, wrapper for grpc_tls_credentials_options. It is
@@ -271,6 +280,10 @@ class TlsCredentialsOptions {
   }
 
  private:
+  /** The cert_request_type_ flag is only relevant when the
+   * TlsCredentialsOptions are used to instantiate server credentials; the flag
+   * goes unused when creating channel credentials, and the user can set it to
+   * GRPC_SSL_DONT_REQUEST_CLIENT_CERTIFICATE. **/
   grpc_ssl_client_certificate_request_type cert_request_type_;
   std::shared_ptr<TlsKeyMaterialsConfig> key_materials_config_;
   std::shared_ptr<TlsCredentialReloadConfig> credential_reload_config_;

src/cpp/common/tls_credentials_options.cc

@@ -87,8 +87,8 @@ void TlsCredentialReloadArg::OnCredentialReloadDoneCallback() {
 
 /** gRPC TLS credential reload config API implementation **/
 TlsCredentialReloadConfig::TlsCredentialReloadConfig(
-    std::shared_ptr<TlsCredentialReloadInterface> interface)
-    : interface_(std::move(interface)) {
+    std::unique_ptr<TlsCredentialReloadInterface> credential_reload_interface)
+    : credential_reload_interface_(std::move(credential_reload_interface)) {
   c_config_ = grpc_tls_credential_reload_config_create(
       nullptr, &TlsCredentialReloadConfigCSchedule,
       &TlsCredentialReloadConfigCCancel, nullptr);
@@ -96,7 +96,7 @@ TlsCredentialReloadConfig::TlsCredentialReloadConfig(
 }
 
 TlsCredentialReloadConfig::~TlsCredentialReloadConfig() {
-  interface_->Release();
+  credential_reload_interface_->Release();
 }
 
 /** gRPC TLS server authorization check arg API implementation **/
@@ -164,8 +164,10 @@ void TlsServerAuthorizationCheckArg::OnServerAuthorizationCheckDoneCallback() {
 
 /** gRPC TLS server authorization check config API implementation. **/
 TlsServerAuthorizationCheckConfig::TlsServerAuthorizationCheckConfig(
-    std::shared_ptr<TlsServerAuthorizationCheckInterface> interface)
-    : interface_(std::move(interface)) {
+    std::unique_ptr<TlsServerAuthorizationCheckInterface>
+        server_authorization_check_interface)
+    : server_authorization_check_interface_(
+          std::move(server_authorization_check_interface)) {
   c_config_ = grpc_tls_server_authorization_check_config_create(
       nullptr, &TlsServerAuthorizationCheckConfigCSchedule,
       &TlsServerAuthorizationCheckConfigCCancel, nullptr);
@@ -173,7 +175,7 @@ TlsServerAuthorizationCheckConfig::TlsServerAuthorizationCheckConfig(
 }
 
 TlsServerAuthorizationCheckConfig::~TlsServerAuthorizationCheckConfig() {
-  interface_->Release();
+  server_authorization_check_interface_->Release();
 }
 
 /** gRPC TLS credential options API implementation **/

test/cpp/client/credentials_test.cc

@@ -74,8 +74,6 @@ class TestTlsCredentialReloadInterface : public TlsCredentialReloadInterface {
     arg->set_status(GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_FAIL);
     arg->set_error_details("cancelled");
   }
-
-  void Release() override { return; }
 };
 
 static void tls_server_authorization_check_callback(
@@ -109,8 +107,6 @@ class TestTlsServerAuthorizationCheckInterface
     arg->set_status(GRPC_STATUS_PERMISSION_DENIED);
     arg->set_error_details("cancelled");
   }
-
-  void Release() override { return; }
 };
 
 }  // namespace
@@ -348,9 +344,9 @@ TEST_F(CredentialsTest, TlsCredentialReloadArgCallback) {
 }
 
 TEST_F(CredentialsTest, TlsCredentialReloadConfigSchedule) {
-  std::shared_ptr<TestTlsCredentialReloadInterface> interface(
+  std::unique_ptr<TestTlsCredentialReloadInterface> interface(
       new TestTlsCredentialReloadInterface());
-  TlsCredentialReloadConfig config(interface);
+  TlsCredentialReloadConfig config(std::move(interface));
   grpc_tls_credential_reload_arg c_arg;
   TlsCredentialReloadArg arg(&c_arg);
   arg.set_cb_user_data(static_cast<void*>(nullptr));
@@ -393,9 +389,9 @@ TEST_F(CredentialsTest, TlsCredentialReloadConfigSchedule) {
 }
 
 TEST_F(CredentialsTest, TlsCredentialReloadConfigCppToC) {
-  std::shared_ptr<TestTlsCredentialReloadInterface> interface(
+  std::unique_ptr<TestTlsCredentialReloadInterface> interface(
       new TestTlsCredentialReloadInterface());
-  TlsCredentialReloadConfig config = TlsCredentialReloadConfig(interface);
+  TlsCredentialReloadConfig config(std::move(interface));
   grpc_tls_credential_reload_arg c_arg;
   c_arg.cb_user_data = static_cast<void*>(nullptr);
   grpc_tls_key_materials_config c_key_materials;
@@ -487,10 +483,9 @@ TEST_F(CredentialsTest, TlsServerAuthorizationCheckArgCallback) {
 }
 
 TEST_F(CredentialsTest, TlsServerAuthorizationCheckConfigSchedule) {
-  std::shared_ptr<TestTlsServerAuthorizationCheckInterface> interface(
+  std::unique_ptr<TestTlsServerAuthorizationCheckInterface> interface(
       new TestTlsServerAuthorizationCheckInterface());
-  TlsServerAuthorizationCheckConfig config =
-      TlsServerAuthorizationCheckConfig(interface);
+  TlsServerAuthorizationCheckConfig config(std::move(interface));
   grpc_tls_server_authorization_check_arg c_arg;
   TlsServerAuthorizationCheckArg arg(&c_arg);
   arg.set_cb_user_data(nullptr);
@@ -524,10 +519,9 @@ TEST_F(CredentialsTest, TlsServerAuthorizationCheckConfigSchedule) {
 }
 
 TEST_F(CredentialsTest, TlsServerAuthorizationCheckConfigCppToC) {
-  std::shared_ptr<TestTlsServerAuthorizationCheckInterface> interface(
+  std::unique_ptr<TestTlsServerAuthorizationCheckInterface> interface(
       new TestTlsServerAuthorizationCheckInterface());
-  TlsServerAuthorizationCheckConfig config =
-      TlsServerAuthorizationCheckConfig(interface);
+  TlsServerAuthorizationCheckConfig config(std::move(interface));
   grpc_tls_server_authorization_check_arg c_arg;
   c_arg.cb = tls_server_authorization_check_callback;
   c_arg.cb_user_data = nullptr;
@@ -574,17 +568,17 @@ TEST_F(CredentialsTest, TlsCredentialsOptionsCppToC) {
   std::vector<TlsKeyMaterialsConfig::PemKeyCertPair> pair_list = {pair};
   key_materials_config->set_key_materials("pem_root_certs", pair_list);
 
-  std::shared_ptr<TestTlsCredentialReloadInterface> credential_reload_interface(
+  std::unique_ptr<TestTlsCredentialReloadInterface> credential_reload_interface(
       new TestTlsCredentialReloadInterface());
   std::shared_ptr<TlsCredentialReloadConfig> credential_reload_config(
-      new TlsCredentialReloadConfig(credential_reload_interface));
+      new TlsCredentialReloadConfig(std::move(credential_reload_interface)));
 
-  std::shared_ptr<TestTlsServerAuthorizationCheckInterface>
+  std::unique_ptr<TestTlsServerAuthorizationCheckInterface>
       server_authorization_check_interface(
           new TestTlsServerAuthorizationCheckInterface());
   std::shared_ptr<TlsServerAuthorizationCheckConfig>
       server_authorization_check_config(new TlsServerAuthorizationCheckConfig(
-          server_authorization_check_interface));
+          std::move(server_authorization_check_interface)));
 
   TlsCredentialsOptions options = TlsCredentialsOptions(
       GRPC_SSL_REQUEST_CLIENT_CERTIFICATE_AND_VERIFY, key_materials_config,
@@ -672,17 +666,17 @@ TEST_F(CredentialsTest, TlsCredentialsOptionsCppToC) {
 
 // This test demonstrates how the SPIFFE credentials will be used.
 TEST_F(CredentialsTest, LoadSpiffeChannelCredentials) {
-  std::shared_ptr<TestTlsCredentialReloadInterface> credential_reload_interface(
+  std::unique_ptr<TestTlsCredentialReloadInterface> credential_reload_interface(
       new TestTlsCredentialReloadInterface());
   std::shared_ptr<TlsCredentialReloadConfig> credential_reload_config(
-      new TlsCredentialReloadConfig(credential_reload_interface));
+      new TlsCredentialReloadConfig(std::move(credential_reload_interface)));
 
-  std::shared_ptr<TestTlsServerAuthorizationCheckInterface>
+  std::unique_ptr<TestTlsServerAuthorizationCheckInterface>
       server_authorization_check_interface(
           new TestTlsServerAuthorizationCheckInterface());
   std::shared_ptr<TlsServerAuthorizationCheckConfig>
       server_authorization_check_config(new TlsServerAuthorizationCheckConfig(
-          server_authorization_check_interface));
+          std::move(server_authorization_check_interface)));
 
   TlsCredentialsOptions options = TlsCredentialsOptions(
       GRPC_SSL_REQUEST_CLIENT_CERTIFICATE_AND_VERIFY, nullptr,