snowflake: runtime driver config checks

R/driver-databricks.R

@@ -17,6 +17,10 @@ NULL
 #' Workbench or the Databricks CLI on desktop.
 #' All of these credentials are detected automatically if present using
 #' [standard environment variables](https://docs.databricks.com/en/dev-tools/auth.html#environment-variables-and-fields-for-client-unified-authentication).
+#' In addition, on macOS platforms, the `dbConnect` method will check
+#' for irregularities with how the driver is configured,
+#' and attempt to fix in-situ, unless the `odbc.no_config_override`
+#' environment variable is set.
 #'
 #' @inheritParams DBI::dbConnect
 #' @param httpPath,HTTPPath To query a cluster, use the HTTP Path value found
@@ -45,7 +49,6 @@ NULL
 #' }
 #' @export
 databricks <- function() {
-  configure_spark()
   new("DatabricksOdbcDriver")
 }
 
@@ -84,6 +87,9 @@ setMethod("dbConnect", "DatabricksOdbcDriver",
       pwd = pwd,
       ...
     )
+    # Perform some sanity checks on MacOS
+    configure_simba((function() {spark_simba_config(args$driver)}),
+      action = "modify")
     inject(dbConnect(odbc(), !!!args))
   }
 )
@@ -315,3 +321,25 @@ workbench_databricks_token <- function(host, cfg_file) {
   }
   token
 }
+
+# p. 44 https://downloads.datastax.com/odbc/2.6.5.1005/Simba%20Spark%20ODBC%20Install%20and%20Configuration%20Guide.pdf
+spark_simba_config <- function(driver) {
+  spark_env <- Sys.getenv("SIMBASPARKINI")
+  if (!identical(spark_env, "")) {
+    return(spark_env)
+  }
+  common_dirs <- c(
+    driver_dir(driver),
+    "/Library/simba/spark/lib",
+    "/etc",
+    getwd(),
+    Sys.getenv("HOME")
+  )
+  return(list(
+    config = list.files(
+      common_dirs,
+      pattern = "simba\\.sparkodbc\\.ini$",
+      full.names = TRUE),
+    driver_url = "https://www.databricks.com/spark/odbc-drivers-download"
+  ))
+}

R/driver-snowflake.R

@@ -98,7 +98,10 @@ setMethod("odbcDataType", "Snowflake",
 #'
 #' In particular, the custom `dbConnect()` method for the Snowflake ODBC driver
 #' detects ambient OAuth credentials on platforms like Snowpark Container
-#' Services or Posit Workbench.
+#' Services or Posit Workbench.  In addition, on macOS platforms, the
+#' `dbConnect` method will check and warn if it detects irregularities with
+#' how the driver is configured, unless the `odbc.no_config_override`
+#' environment variable is set.
 #'
 #' @inheritParams DBI::dbConnect
 #' @param account A Snowflake [account
@@ -174,6 +177,9 @@ setMethod(
       pwd = pwd,
       ...
     )
+    # Perform some sanity checks on MacOS
+    configure_simba((function() {snowflake_simba_config(args$driver)}),
+      action = "modify")
     inject(dbConnect(odbc(), !!!args))
   }
 )
@@ -246,13 +252,41 @@ snowflake_default_driver_paths <- function() {
       "/usr/lib/snowflake/odbc/lib/libSnowflake.so"
     )
   } else if (Sys.info()["sysname"] == "Darwin") {
-    paths <- "/opt/snowflake/snowflakeodbc/lib/universal/libSnowflake.dylib"
+    paths <- c(
+      "/opt/snowflake/snowflakeodbc/lib/universal/libSnowflake.dylib",
+      "/opt/snowflake-osx-x64/bin/lib/libsnowflakeodbc_sb64-universal.dylib"
+    )
   } else {
     paths <- character()
   }
   paths[file.exists(paths)]
 }
 
+snowflake_simba_config <- function(driver) {
+  snowflake_env <- Sys.getenv("SIMBASNOWFLAKEINI")
+  if (!identical(snowflake_env, "")) {
+    return(snowflake_env)
+  }
+  # Posit configuration is likely at:
+  # /opt/snowflake-osx-x64/bin/lib/rstudio.snowflakeodbc.ini
+  # OEM configuration is likely at:
+  # /opt/snowflake/snowflakeodbc/lib/universal/simba.snowflake.ini
+  common_dirs <- c(
+    driver_dir(driver),
+    "/opt/snowflake-osx-x64/bin/lib/",
+    "/opt/snowflake/snowflakeodbc/lib/universal/",
+    getwd(),
+    Sys.getenv("HOME")
+  )
+  return(list(
+    config = list.files(
+      simba_config_dirs(driver),
+      pattern = "snowflake(odbc)?\\.ini$",
+      full.names = TRUE),
+    driver_url = "https://docs.snowflake.com/en/developer-guide/odbc/odbc-download"
+  ))
+}
+
 snowflake_server <- function(account) {
   if (nchar(account) == 0) {
     abort(

R/utils.R

@@ -288,28 +288,41 @@ check_attributes <- function(attributes, call = caller_env()) {
 }
 
 # apple + spark drive config (#651) --------------------------------------------
-configure_spark <- function(call = caller_env()) {
+# Method will attempt to:
+# 1. Locate the simba driver config using the user supplied callback.  Callback
+# method defaults to locate_config_spark.
+# 2. Inspect the config for some settings that can impact how our package
+# performs.
+# 3. If action == "modify" then we attempt to modify the config in-situ.
+# 4. Otherwise we throw a warning asking the user to revise.
+configure_simba <- function(locate_config_callback =
+                              (function() list(config = character(), driver_url = character())),
+                            action = "modify", call = caller_env()) {
   if (!is_macos()) {
     return(invisible())
   }
+  if (!is.null(getOption("odbc.no_config_override"))) {
+    return(invisible())
+  }
 
   unixodbc_install <- locate_install_unixodbc()
   if (length(unixodbc_install) == 0) {
     error_install_unixodbc(call)
   }
 
-  spark_config <- locate_config_spark()
-  if (length(spark_config) == 0) {
-    abort(
-      c(
-        "Unable to locate the needed spark ODBC driver.",
-        i = "Please install the needed driver from https://www.databricks.com/spark/odbc-drivers-download."
-      ),
+  res <- locate_config_callback()
+  simba_config <- res$config
+  if (length(simba_config) == 0) {
+    func <- cli::warn
+    if (action == "modify") {
+      fun <- cli::abort
+    }
+    func(
+      c(i = "Please install the needed driver from {res$driver_url}"),
       call = call
     )
   }
-
-  configure_unixodbc_spark(unixodbc_install[1], spark_config[1], call)
+  configure_unixodbc_simba(unixodbc_install[1], simba_config[1], action, call)
 }
 
 locate_install_unixodbc <- function() {
@@ -369,51 +382,49 @@ error_install_unixodbc <- function(call) {
   )
 }
 
-# p. 44 https://downloads.datastax.com/odbc/2.6.5.1005/Simba%20Spark%20ODBC%20Install%20and%20Configuration%20Guide.pdf
-locate_config_spark <- function() {
-  spark_env <- Sys.getenv("SIMBASPARKINI")
-  if (!identical(spark_env, "")) {
-    return(spark_env)
-  }
+configure_unixodbc_simba <- function(unixodbc_install, simba_config, action, call) {
 
-  common_dirs <- c(
-    "/Library/simba/spark/lib",
-    "/etc",
-    getwd(),
-    Sys.getenv("HOME")
-  )
-
-  list.files(
-    common_dirs,
-    pattern = "simba\\.sparkodbc\\.ini$",
-    full.names = TRUE
-  )
-}
-
-configure_unixodbc_spark <- function(unixodbc_install, spark_config, call) {
   # As shipped, the simba spark ini has an incomplete final line
   suppressWarnings(
-    spark_lines <- readLines(spark_config)
+    simba_lines <- readLines(simba_config)
   )
-
-  spark_lines_new <- replace_or_append(
-    lines = spark_lines,
-    pattern = "^ODBCInstLib=",
+  res <- replace_or_append(
+    lines = simba_lines,
+    key_pattern = "^ODBCInstLib=",
+    accepted_value = unixodbc_install,
     replacement = paste0("ODBCInstLib=", unixodbc_install)
   )
-
-  spark_lines_new <- replace_or_append(
-    lines = spark_lines_new,
-    pattern = "^DriverManagerEncoding=",
+  if (action != "modify" && res$modified) {
+    cli::cli_warn(c(
+      i = "Detected potentially unsafe driver settings.
+           Please consider revising the {.arg ODBCInstLib} field in
+           {simba_config} and setting its value to {unixodbc_install}"
+    ))
+  }
+  simba_lines_new <- res$new_lines
+  res <- replace_or_append(
+    lines = simba_lines_new,
+    key_pattern = "^DriverManagerEncoding=",
+    accepted_value = "UTF-16|utf-16",
     replacement = "DriverManagerEncoding=UTF-16"
   )
+  if (action != "modify" && res$modified) {
+    cli::cli_warn(c(
+      i = "Detected potentially unsafe driver settings.
+           Please consider revising the {.arg DriverManagerEncoding}
+           field in {simba_config} and setting its value to 'UTF-16'"
+    ))
+  }
+  simba_lines_new <- res$new_lines
 
-  write_spark_lines(spark_lines, spark_lines_new, spark_config, call)
+  if (action == "modify") {
+    write_simba_lines(simba_lines, simba_lines_new, simba_config, call)
+  }
 
   invisible()
 }
 
-write_spark_lines <- function(spark_lines, spark_lines_new, spark_config, call) {
+write_simba_lines <- function(spark_lines, spark_lines_new, spark_config, call) {
   if (identical(spark_lines, spark_lines_new)) {
     return(invisible())
   }
@@ -436,21 +447,50 @@ write_spark_lines <- function(spark_lines, spark_lines_new, spark_config, call)
   writeLines(spark_lines_new, spark_config)
 }
 
+# Interpret the argument as an `ODBC` driver
+# and attempt to infer the directory housing it.
+# It will return an empty character vector if unable to.
+driver_dir <- function(driver) {
+  # driver argument could be an outright path, or a name
+  # of a driver specified in odbcinst.ini  Try to discern
+  driver_spec <- subset(odbcListDrivers(), name == driver)
+  if (nrow(driver_spec) ) {
+    driver_path <- subset(driver_spec, attribute == "Driver")$value
+  } else {
+    driver_path <- driver
+  }
+
+  ret <- dirname(driver_path)
+  if (ret == ".") {
+    ret <- character()
+  }
+  return(ret)
+}
+
 is_writeable <- function(path) {
   tryCatch(file.access(path, mode = 2)[[1]] == 0, error = function(e) FALSE)
 }
 
-# given a vector of lines in an ini file, look for a given key pattern.
-# the `replacement` is the whole intended line, giving the "key=value" pair.
-# if the key is found, replace that line with `replacement`.
-# if the key isn't found, append a new line with `replacement`.
-replace_or_append <- function(lines, pattern, replacement) {
-  matching_lines_loc <- grepl(pattern, lines)
+# Given a vector of lines in an ini file, look for a given key pattern.
+# If found:
+# - No action if the `accepted_value` argument is found on line.
+# - Replace otherwise.
+# If not found: append.
+# The `replacement` is the whole intended line, giving the desired
+# "key=value" pair.
+# @return a list with two elements:
+# - new_lines: Potentially modified lines
+# - modified: Whether method modified lines, where modified means
+# both changed or appended.
+replace_or_append <- function(lines, key_pattern, accepted_value, replacement) {
+  matching_lines_loc <- grepl(key_pattern, lines)
   matching_lines <- lines[matching_lines_loc]
+  found_ok = length(matching_lines) != 0 &&
+    any(grepl(accepted_value, lines[matching_lines_loc]))
   if (length(matching_lines) == 0) {
     lines <- c(lines, replacement)
-  } else {
+  } else if (!found_ok) {
     lines[matching_lines_loc] <- replacement
   }
-  lines
+  return(list("new_lines" = lines, "modified" = !found_ok))
 }

tests/testthat/_snaps/utils.md

@@ -136,21 +136,54 @@
       ! `attributes` does not support the connection attributes "boop" and "beep".
       i Allowed connection attribute is "azure_token".
 
-# configure_spark() errors informatively on failure to install unixODBC
+# configure_simba() errors informatively on failure to install unixODBC
 
     Code
-      databricks()
+      configure_simba()
     Condition
-      Error in `databricks()`:
+      Error:
       ! Unable to locate the unixODBC driver manager.
       i Please install unixODBC using Homebrew with `brew install unixodbc`.
 
 # databricks() errors informatively when spark ini isn't writeable
 
     Code
-      write_spark_lines("", ".", ".", call2("databricks"))
+      write_simba_lines("", ".", ".", call2("databricks"))
     Condition
       Error in `databricks()`:
       ! Detected needed changes to the driver configuration file at ., but the file was not writeable.
       i Please make the changes outlined at https://solutions.posit.co/connections/db/databases/databricks/#troubleshooting-apple-macos-users.
 
+# configure_unixodbc_simba() writes reasonable entries
+
+    Code
+      configure_unixodbc_simba(unixodbc_install = unixodbc_install_path,
+        simba_config = spark_config_path, action = "warn")
+    Condition
+      Warning:
+      i Detected potentially unsafe driver settings. Please consider revising the `ODBCInstLib` field in simba.sparkodbc.ini and setting its value to libodbcinst.dylib
+      Warning:
+      i Detected potentially unsafe driver settings. Please consider revising the `DriverManagerEncoding` field in simba.sparkodbc.ini and setting its value to 'UTF-16'
+
+---
+
+    Code
+      configure_unixodbc_simba(unixodbc_install = unixodbc_install_path,
+        simba_config = spark_config_path, action = "warn")
+    Condition
+      Warning:
+      i Detected potentially unsafe driver settings. Please consider revising the `ODBCInstLib` field in simba.sparkodbc.ini and setting its value to libodbcinst.dylib
+      Warning:
+      i Detected potentially unsafe driver settings. Please consider revising the `DriverManagerEncoding` field in simba.sparkodbc.ini and setting its value to 'UTF-16'
+
+---
+
+    Code
+      configure_unixodbc_simba(unixodbc_install = unixodbc_install_path,
+        simba_config = spark_config_path, action = "warn")
+    Condition
+      Warning:
+      i Detected potentially unsafe driver settings. Please consider revising the `ODBCInstLib` field in simba.sparkodbc.ini and setting its value to libodbcinst.dylib
+      Warning:
+      i Detected potentially unsafe driver settings. Please consider revising the `DriverManagerEncoding` field in simba.sparkodbc.ini and setting its value to 'UTF-16'
+