CallCredentials debug string API

[mhaidrygoog]

Implementation for supporting a Debug String in CallCredentials for C++

gRFC PR - grpc/proposal#171

---

This change is 

[mhaidrygoog]

So following is the justification for all string types used. This is afer consulting with Esun (@veblush) -

• Use grpc::string in the surface C++ code because there are existing usages and some platform dependent macros to support it. There will later on be a LSC to replace all usages of grpc::string to std::string
• Use std::string in core implementation as we support it there. Also grpc::string is not available in core
• Use const char* for files under include as they are C89. This leads to a painful malloc and free while transferring the string from plugin to the call credentials while using plugins.

[mhaidrygoog]

Thanks for the review. I've addressed the comments raised by @vjpai and @chwarr

[veblush]

LGTM to abseil usage in build.

[mhaidrygoog]

I'm investigating the Sanity check failures. It appears that clang tidy is timing out and I am root causing why. In any case please continue the CL review ready

[markdroth]

I believe the sanity check timeout is a known issue. I think Nico or Jan was looking into it.

Overall, this looks good. Most of my comments are minor.

Please let me know if you have any questions about any of this.

Reviewed 36 of 36 files at r1.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @mhaidrygoog, @vjpai, and @yang-g)

---

include/grpcpp/security/credentials_impl.h, line 118 at r1 (raw file):

  /// Apply this instance's credentials to \a call.
  virtual bool ApplyToCall(grpc_call* call) = 0;
  virtual grpc::string DebugString() { return "CallCredentials{}"; }

I think this should be a non-virtual function that delegates to the underlying C implementation.

---

include/grpcpp/security/credentials_impl.h, line 256 at r1 (raw file):

  virtual grpc::string DebugString() const {
    return "MetadataCredentialsPlugin";

I think this should say something like "Metadata credential plugin did not provide a debug string", so that it's clear to users why there isn't more information here.

---

src/core/lib/security/credentials/credentials.h, line 257 at r1 (raw file):

  }

  virtual std::string debug_string() const { return ""; }

I think this should say something like "Call creds impl does not provide debug string", to make it clear to the user why there isn't more information here.

---

src/core/lib/security/credentials/composite/composite_credentials.cc, line 120 at r1 (raw file):

std::string grpc_composite_call_credentials::debug_string() const {
  std::string debug_str("CompositeCallCredentials");

I think this would be more efficient (and more readable) written like so:

std::vector<std::string> outputs;
for (auto& inner_cred : inner_) {
  outputs.append(inner->debug_string());
}
return absl::StrCat(
    "CompositeCallCredentials{",
    absl::StringJoin(outputs, ","),
    "}");

---

src/core/lib/security/credentials/oauth2/oauth2_credentials.h, line 27 at r1 (raw file):

#include <grpc/grpc_security.h>
#include "absl/strings/str_format.h"

These two includes don't look like they're used here. They should probably be moved to the .cc file instead.

---

src/core/lib/security/credentials/plugin/plugin_credentials.cc, line 47 at r1 (raw file):

std::string grpc_plugin_credentials::debug_string() const {
  if (plugin_.debug_string != nullptr) {
    char* debug_c_str = plugin_.debug_string(plugin_.state);

What if this returns null or empty string? We should probably return something like "plugin implementation did not provide debug string" in that case.

---

src/core/lib/security/credentials/plugin/plugin_credentials.cc, line 52 at r1 (raw file):

    return debug_str;
  }
  return "";

This should say something like "plugin implementation did not provide debug string".

---

src/cpp/client/secure_credentials.h, line 69 at r1 (raw file):

  SecureCallCredentials* AsSecureCredentials() override { return this; }
  grpc::string DebugString() override {
    return "SecureCallCredentials{" + grpc::string(c_creds_->debug_string()) +

Suggest using absl::StrCat() here. It's both more efficient and simpler to use.

But as I said elsewhere, I don't think we should be overriding the DebugString() method in individual C++ subclasses in the first place, so this issue may be moot.

---

src/cpp/client/secure_credentials.cc, line 405 at r1 (raw file):

  MetadataCredentialsPluginWrapper* w =
      static_cast<MetadataCredentialsPluginWrapper*>(wrapper);
  char* debug_str = gpr_strdup(w->plugin_->DebugString().c_str());

No need for a local variable here; just directly return the result of gpr_strdup().

---

test/core/security/credentials_test.cc, line 643 at r1 (raw file):

  GPR_ASSERT(
      !gpr_stricmp(creds->debug_string().c_str(), expected_creds_debug_string));

Please use == 0 instead of !, since this is not a boolean return type.

Same thing throughout.

---

test/core/security/credentials_test.cc, line 643 at r1 (raw file):

  GPR_ASSERT(
      !gpr_stricmp(creds->debug_string().c_str(), expected_creds_debug_string));

Why use case-insensitive string comparison? Don't we know what case the result will be in here?

Same thing throughout.

[mhaidrygoog]

Reviewable status: 23 of 36 files reviewed, 11 unresolved discussions (waiting on @markdroth, @vjpai, and @yang-g)

---

include/grpcpp/security/credentials_impl.h, line 118 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I think this should be a non-virtual function that delegates to the underlying C implementation.

The issue here is that CallCredentials does not compose an underlying C implementation, that is done by the SecureCallCredentials that inherits CallCredentials.

---

include/grpcpp/security/credentials_impl.h, line 256 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I think this should say something like "Metadata credential plugin did not provide a debug string", so that it's clear to users why there isn't more information here.

Done.

---

src/core/lib/security/credentials/credentials.h, line 257 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I think this should say something like "Call creds impl does not provide debug string", to make it clear to the user why there isn't more information here.

Done.

---

src/core/lib/security/credentials/composite/composite_credentials.cc, line 120 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I think this would be more efficient (and more readable) written like so:

std::vector<std::string> outputs;
for (auto& inner_cred : inner_) {
  outputs.append(inner->debug_string());
}
return absl::StrCat(
    "CompositeCallCredentials{",
    absl::StringJoin(outputs, ","),
    "}");

Done.

---

src/core/lib/security/credentials/oauth2/oauth2_credentials.h, line 27 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

These two includes don't look like they're used here. They should probably be moved to the .cc file instead.

Done.

---

src/core/lib/security/credentials/plugin/plugin_credentials.cc, line 47 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

What if this returns null or empty string? We should probably return something like "plugin implementation did not provide debug string" in that case.

Done.

---

src/core/lib/security/credentials/plugin/plugin_credentials.cc, line 52 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This should say something like "plugin implementation did not provide debug string".

Done.

---

src/cpp/client/secure_credentials.h, line 69 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Suggest using absl::StrCat() here. It's both more efficient and simpler to use.

But as I said elsewhere, I don't think we should be overriding the DebugString() method in individual C++ subclasses in the first place, so this issue may be moot.

I have added StrCat here. We may still need to override this method as mentioned in the other comment

---

src/cpp/client/secure_credentials.cc, line 405 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

No need for a local variable here; just directly return the result of gpr_strdup().

Done.

---

test/core/security/credentials_test.cc, line 643 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please use == 0 instead of !, since this is not a boolean return type.

Same thing throughout.

Done.

---

test/core/security/credentials_test.cc, line 643 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Why use case-insensitive string comparison? Don't we know what case the result will be in here?

Same thing throughout.

Done.

[mhaidrygoog]

Reviewable status: 16 of 36 files reviewed, 15 unresolved discussions (waiting on @markdroth, @vjpai, and @yang-g)

---

include/grpcpp/security/credentials_impl.h, line 257 at r2 (raw file):

Previously, yang-g (Yang Gao) wrote…

This is a const method and the one on CallCredentials is not const. Should we make them consistent?

Done, I've removed const from all methods to avoid confusion

---

src/core/lib/security/credentials/composite/composite_credentials.h, line 24 at r2 (raw file):

Previously, yang-g (Yang Gao) wrote…

I believe you should use #include <string> throughout the files for std::string. If you need to use the C-string things, then you should use <cstring>.

Done.

---

src/core/lib/security/credentials/fake/fake_credentials.h, line 80 at r2 (raw file):

Previously, yang-g (Yang Gao) wrote…

nit: I find it a little annoying to see std::string internally and grpc::string in public header. I think it might be better if you just use grpc::string and thus you do not need to add the include everywhere.

Using grpc::string in core is not possible . It is only available in the surface layer. Please see my earlier comment - #21984 (review)

I agree it is annoying :/

---

src/core/lib/security/credentials/iam/iam_credentials.cc, line 32 at r2 (raw file):

Previously, yang-g (Yang Gao) wrote…

The include order of this file is weird. But I think you somehow had a merge issue to have duplicate includes.

Done. Thanks for catching that

[yang-g]

Thanks.
LGTM

[mhaidrygoog]

Sanity checks ironed out after fixing the BUILD file. There were duplicate entries for absl::str_format and removing that seems to have solved the issue here. Other failures are not related to this change.

[vjpai]

Minor comments only. Pre-LGTMing, please make these fixes and if that's all then I don't need to see it again.

[mhaidrygoog]

Thanks for the quick follow up review @vjpai . Won't ask you to see it again :P

[markdroth]

Just a couple of remaining nits. Feel free to merge after resolving.

Reviewed 3 of 13 files at r2, 9 of 13 files at r3, 6 of 6 files at r4, 15 of 15 files at r5.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @mhaidrygoog and @yang-g)

---

include/grpcpp/security/credentials_impl.h, line 118 at r1 (raw file):

Previously, mhaidrygoog (Moiz Haidry) wrote…

The issue here is that CallCredentials does not compose an underlying C implementation, that is done by the SecureCallCredentials that inherits CallCredentials.

Oh, right -- this is a side effect of the dumb way that we're handling insecure creds. I think Yihua is going to make this go away as part of cleaning up insecure creds.

For now, this is fine.

---

src/core/lib/security/credentials/plugin/plugin_credentials.cc, line 46 at r5 (raw file):

std::string grpc_plugin_credentials::debug_string() {
  std::string debug_str;

Suggest writing this as:

char* debug_c_str = nullptr;
if (plugin_.debug_string != nullptr) {
  debug_c_str = plugin_.debug_string(plugin_.state);
}
std::string debug_str(debug_c_str != nullptr
                      ? debug_c_str
                      : "grpc_plugin_credentials did not provide a debug string");
gpr_free(debug_c_str);
return debug_str;

---

src/core/lib/security/credentials/plugin/plugin_credentials.cc, line 49 at r5 (raw file):

  if (plugin_.debug_string != nullptr) {
    char* debug_c_str = plugin_.debug_string(plugin_.state);
    if (strlen(debug_c_str) != 0) {

This needs to check for a null return value.

Actually, upon further reflection, we probably shouldn't special case the empty string here, so the only special check should be if it's null.

[mhaidrygoog]

Reviewable status: 34 of 36 files reviewed, 9 unresolved discussions (waiting on @markdroth, @vjpai, and @yang-g)

---

include/grpc/grpc_security.h, line 430 at r3 (raw file):

Previously, vjpai (Vijay Pai) wrote…

Why the empty line?

  /** Implements debug string of the given plugin. This method returns an
   * allocated string that the caller needs to free using gpr_free(). */

Done.

---

src/core/lib/security/credentials/iam/iam_credentials.h, line 24 at r3 (raw file):

Previously, vjpai (Vijay Pai) wrote…

#include <string>

std::string is in string ; string.h or cstring are for things like strcmp etc

Done.

---

src/core/lib/security/credentials/jwt/jwt_credentials.h, line 24 at r3 (raw file):

Previously, vjpai (Vijay Pai) wrote…

But this should be string.

#include <string>

Done.

---

src/core/lib/security/credentials/oauth2/oauth2_credentials.h, line 24 at r3 (raw file):

Previously, vjpai (Vijay Pai) wrote…

#include <string>

Done.

---

src/core/lib/security/credentials/plugin/plugin_credentials.cc, line 46 at r5 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Suggest writing this as:

char* debug_c_str = nullptr;
if (plugin_.debug_string != nullptr) {
  debug_c_str = plugin_.debug_string(plugin_.state);
}
std::string debug_str(debug_c_str != nullptr
                      ? debug_c_str
                      : "grpc_plugin_credentials did not provide a debug string");
gpr_free(debug_c_str);
return debug_str;

Done.

---

src/core/lib/security/credentials/plugin/plugin_credentials.cc, line 49 at r5 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This needs to check for a null return value.

Actually, upon further reflection, we probably shouldn't special case the empty string here, so the only special check should be if it's null.

Done.

[mhaidrygoog]

Thanks for the review @markdroth