Cleanup error handling for google_default_credentials

6 files changed, 77 insertions, 35 deletions

diff --git a/src/core/lib/iomgr/error.c b/src/core/lib/iomgr/error.c
index 05488af4bb..c70eb1a3f0 100644
--- a/src/core/lib/iomgr/error.c
+++ b/src/core/lib/iomgr/error.c
@@ -134,6 +134,8 @@ static const char *error_str_name(grpc_error_strs key) {
       return "raw_bytes";
     case GRPC_ERROR_STR_TSI_ERROR:
       return "tsi_error";
+    case GRPC_ERROR_STR_FILENAME:
+      return "filename";
   }
   GPR_UNREACHABLE_CODE(return "unknown");
 }
diff --git a/src/core/lib/iomgr/error.h b/src/core/lib/iomgr/error.h
index bf275fa408..695724c0be 100644
--- a/src/core/lib/iomgr/error.h
+++ b/src/core/lib/iomgr/error.h
@@ -78,6 +78,7 @@ typedef enum {
   GRPC_ERROR_STR_GRPC_MESSAGE,
   GRPC_ERROR_STR_RAW_BYTES,
   GRPC_ERROR_STR_TSI_ERROR,
+  GRPC_ERROR_STR_FILENAME,
 } grpc_error_strs;
 
 typedef enum {
diff --git a/src/core/lib/security/credentials/google_default/google_default_credentials.c b/src/core/lib/security/credentials/google_default/google_default_credentials.c
index 019def95c2..465668476e 100644
--- a/src/core/lib/security/credentials/google_default/google_default_credentials.c
+++ b/src/core/lib/security/credentials/google_default/google_default_credentials.c
@@ -45,6 +45,7 @@
 #include "src/core/lib/security/credentials/oauth2/oauth2_credentials.h"
 #include "src/core/lib/support/env.h"
 #include "src/core/lib/support/load_file.h"
+#include "src/core/lib/support/string.h"
 #include "src/core/lib/surface/api_trace.h"
 
 /* -- Constants. -- */
@@ -154,19 +155,31 @@ static int is_stack_running_on_compute_engine(void) {
 }
 
 /* Takes ownership of creds_path if not NULL. */
-static grpc_call_credentials *create_default_creds_from_path(char *creds_path) {
+static grpc_error *create_default_creds_from_path(
+    char *creds_path, grpc_call_credentials **creds) {
   grpc_json *json = NULL;
   grpc_auth_json_key key;
   grpc_auth_refresh_token token;
   grpc_call_credentials *result = NULL;
   gpr_slice creds_data = gpr_empty_slice();
-  int file_ok = 0;
-  if (creds_path == NULL) goto end;
-  creds_data = gpr_load_file(creds_path, 0, &file_ok);
-  if (!file_ok) goto end;
+  grpc_error *error = GRPC_ERROR_NONE;
+  if (creds_path == NULL) {
+    error = GRPC_ERROR_CREATE("creds_path unset");
+    goto end;
+  }
+  error = gpr_load_file(creds_path, 0, &creds_data);
+  if (error != GRPC_ERROR_NONE) {
+    goto end;
+  }
   json = grpc_json_parse_string_with_len(
       (char *)GPR_SLICE_START_PTR(creds_data), GPR_SLICE_LENGTH(creds_data));
-  if (json == NULL) goto end;
+  if (json == NULL) {
+    char *dump = gpr_dump_slice(creds_data, GPR_DUMP_HEX | GPR_DUMP_ASCII);
+    error = grpc_error_set_str(GRPC_ERROR_CREATE("Failed to parse JSON"),
+                               GRPC_ERROR_STR_RAW_BYTES, dump);
+    gpr_free(dump);
+    goto end;
+  }
 
   /* First, try an auth json key. */
   key = grpc_auth_json_key_create_from_json(json);
@@ -174,6 +187,11 @@ static grpc_call_credentials *create_default_creds_from_path(char *creds_path) {
     result =
         grpc_service_account_jwt_access_credentials_create_from_auth_json_key(
             key, grpc_max_auth_token_lifetime());
+    if (result == NULL) {
+      error = GRPC_ERROR_CREATE(
+          "grpc_service_account_jwt_access_credentials_create_from_auth_json_"
+          "key failed");
+    }
     goto end;
   }
 
@@ -182,19 +200,28 @@ static grpc_call_credentials *create_default_creds_from_path(char *creds_path) {
   if (grpc_auth_refresh_token_is_valid(&token)) {
     result =
         grpc_refresh_token_credentials_create_from_auth_refresh_token(token);
+    if (result == NULL) {
+      error = GRPC_ERROR_CREATE(
+          "grpc_refresh_token_credentials_create_from_auth_refresh_token "
+          "failed");
+    }
     goto end;
   }
 
 end:
+  GPR_ASSERT((result == NULL) + (error == GRPC_ERROR_NONE) == 1);
   if (creds_path != NULL) gpr_free(creds_path);
   gpr_slice_unref(creds_data);
   if (json != NULL) grpc_json_destroy(json);
-  return result;
+  *creds = result;
+  return error;
 }
 
 grpc_channel_credentials *grpc_google_default_credentials_create(void) {
   grpc_channel_credentials *result = NULL;
   grpc_call_credentials *call_creds = NULL;
+  grpc_error *error = GRPC_ERROR_CREATE("Failed to create Google credentials");
+  grpc_error *err;
 
   GRPC_API_TRACE("grpc_google_default_credentials_create(void)", 0, ());
 
@@ -208,14 +235,16 @@ grpc_channel_credentials *grpc_google_default_credentials_create(void) {
   }
 
   /* First, try the environment variable. */
-  call_creds = create_default_creds_from_path(
-      gpr_getenv(GRPC_GOOGLE_CREDENTIALS_ENV_VAR));
-  if (call_creds != NULL) goto end;
+  err = create_default_creds_from_path(
+      gpr_getenv(GRPC_GOOGLE_CREDENTIALS_ENV_VAR), &call_creds);
+  if (err == GRPC_ERROR_NONE) goto end;
+  error = grpc_error_add_child(error, err);
 
   /* Then the well-known file. */
-  call_creds = create_default_creds_from_path(
-      grpc_get_well_known_google_credentials_file_path());
-  if (call_creds != NULL) goto end;
+  err = create_default_creds_from_path(
+      grpc_get_well_known_google_credentials_file_path(), &call_creds);
+  if (err == GRPC_ERROR_NONE) goto end;
+  error = grpc_error_add_child(error, err);
 
   /* At last try to see if we're on compute engine (do the detection only once
      since it requires a network test). */
@@ -224,6 +253,10 @@ grpc_channel_credentials *grpc_google_default_credentials_create(void) {
     compute_engine_detection_done = 1;
     if (need_compute_engine_creds) {
       call_creds = grpc_google_compute_engine_credentials_create(NULL);
+      if (call_creds == NULL) {
+        error = grpc_error_add_child(
+            error, GRPC_ERROR_CREATE("Failed to get credentials from network"));
+      }
     }
   }
 
@@ -247,6 +280,11 @@ end:
     }
   }
   gpr_mu_unlock(&g_state_mu);
+  if (result == NULL) {
+    GRPC_LOG_IF_ERROR("grpc_google_default_credentials_create", error);
+  } else {
+    GRPC_ERROR_UNREF(error);
+  }
   return result;
 }
 
diff --git a/src/core/lib/security/transport/security_connector.c b/src/core/lib/security/transport/security_connector.c
index 72173e7c9d..1803dbe824 100644
--- a/src/core/lib/security/transport/security_connector.c
+++ b/src/core/lib/security/transport/security_connector.c
@@ -635,7 +635,8 @@ static gpr_slice compute_default_pem_root_certs_once(void) {
   char *default_root_certs_path =
       gpr_getenv(GRPC_DEFAULT_SSL_ROOTS_FILE_PATH_ENV_VAR);
   if (default_root_certs_path != NULL) {
-    result = gpr_load_file(default_root_certs_path, 0, NULL);
+    GRPC_LOG_IF_ERROR("load_file",
+                      gpr_load_file(default_root_certs_path, 0, &result));
     gpr_free(default_root_certs_path);
   }
 
@@ -653,7 +654,8 @@ static gpr_slice compute_default_pem_root_certs_once(void) {
   /* Fall back to installed certs if needed. */
   if (GPR_SLICE_IS_EMPTY(result) &&
       ovrd_res != GRPC_SSL_ROOTS_OVERRIDE_FAIL_PERMANENTLY) {
-    result = gpr_load_file(installed_roots_path, 0, NULL);
+    GRPC_LOG_IF_ERROR("load_file",
+                      gpr_load_file(installed_roots_path, 0, &result));
   }
   return result;
 }
diff --git a/src/core/lib/support/load_file.c b/src/core/lib/support/load_file.c
index f30aacdd4f..a78bb33a0c 100644
--- a/src/core/lib/support/load_file.c
+++ b/src/core/lib/support/load_file.c
@@ -43,21 +43,19 @@
 #include "src/core/lib/support/block_annotate.h"
 #include "src/core/lib/support/string.h"
 
-gpr_slice gpr_load_file(const char *filename, int add_null_terminator,
-                        int *success) {
+grpc_error *gpr_load_file(const char *filename, int add_null_terminator,
+                          gpr_slice *output) {
   unsigned char *contents = NULL;
   size_t contents_size = 0;
-  char *error_msg = NULL;
   gpr_slice result = gpr_empty_slice();
   FILE *file;
   size_t bytes_read = 0;
+  grpc_error *error = GRPC_ERROR_NONE;
 
   GRPC_SCHEDULING_START_BLOCKING_REGION;
   file = fopen(filename, "rb");
   if (file == NULL) {
-    gpr_asprintf(&error_msg, "Could not open file %s (error = %s).", filename,
-                 strerror(errno));
-    GPR_ASSERT(error_msg != NULL);
+    error = GRPC_OS_ERROR(errno, "fopen");
     goto end;
   }
   fseek(file, 0, SEEK_END);
@@ -67,25 +65,25 @@ gpr_slice gpr_load_file(const char *filename, int add_null_terminator,
   contents = gpr_malloc(contents_size + (add_null_terminator ? 1 : 0));
   bytes_read = fread(contents, 1, contents_size, file);
   if (bytes_read < contents_size) {
+    error = GRPC_OS_ERROR(errno, "fread");
     GPR_ASSERT(ferror(file));
-    gpr_asprintf(&error_msg, "Error %s occured while reading file %s.",
-                 strerror(errno), filename);
-    GPR_ASSERT(error_msg != NULL);
     goto end;
   }
-  if (success != NULL) *success = 1;
   if (add_null_terminator) {
     contents[contents_size++] = 0;
   }
   result = gpr_slice_new(contents, contents_size, gpr_free);
 
 end:
-  if (error_msg != NULL) {
-    gpr_log(GPR_ERROR, "%s", error_msg);
-    gpr_free(error_msg);
-    if (success != NULL) *success = 0;
-  }
+  *output = result;
   if (file != NULL) fclose(file);
+  if (error != GRPC_ERROR_NONE) {
+    grpc_error *error_out = grpc_error_set_str(
+        GRPC_ERROR_CREATE_REFERENCING("Failed to load file", &error, 1),
+        GRPC_ERROR_STR_FILENAME, filename);
+    GRPC_ERROR_UNREF(error);
+    error = error_out;
+  }
   GRPC_SCHEDULING_END_BLOCKING_REGION;
-  return result;
+  return error;
 }
diff --git a/src/core/lib/support/load_file.h b/src/core/lib/support/load_file.h
index 9a4b27942e..d4008a442f 100644
--- a/src/core/lib/support/load_file.h
+++ b/src/core/lib/support/load_file.h
@@ -38,15 +38,16 @@
 
 #include <grpc/support/slice.h>
 
+#include "src/core/lib/iomgr/error.h"
+
 #ifdef __cplusplus
 extern "C" {
 #endif
 
 /* Loads the content of a file into a slice. add_null_terminator will add
-   a NULL terminator if non-zero. The success parameter, if not NULL,
-   will be set to 1 in case of success and 0 in case of failure. */
-gpr_slice gpr_load_file(const char *filename, int add_null_terminator,
-                        int *success);
+   a NULL terminator if non-zero. */
+grpc_error *gpr_load_file(const char *filename, int add_null_terminator,
+                          gpr_slice *slice);
 
 #ifdef __cplusplus
 }