Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SNOW-957747: Easy logging improvements #1730

Merged
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
655c866
Easy logging improvements
sfc-gh-ext-simba-nl Apr 23, 2024
0d96d6b
Fix import to remove *
sfc-gh-ext-simba-nl Apr 23, 2024
a662adc
Fix imports
sfc-gh-ext-simba-nl Apr 23, 2024
61191f4
log format changes, add tests
sfc-gh-ext-simba-nl Apr 27, 2024
0d8958d
Parameterize test, small minor refactorings
sfc-gh-ext-simba-nl May 2, 2024
c6bca00
Fix formatting
sfc-gh-ext-simba-nl May 2, 2024
f43687a
Merge branch 'master' into SNOW-957747-easy-logging-configuration-imp…
sfc-gh-ext-simba-nl May 6, 2024
8f1a3d9
Merge branch 'master' into SNOW-957747-easy-logging-configuration-imp…
sfc-gh-ext-simba-nl May 28, 2024
701bf67
Fix error message, remove unnecessary catch in tests
sfc-gh-ext-simba-nl May 28, 2024
9121933
Fix test IOException issue
sfc-gh-ext-simba-nl May 28, 2024
ccf8929
Fix formatting
sfc-gh-ext-simba-nl May 28, 2024
74a6fcb
add try with resources, fix log messages
sfc-gh-ext-simba-nl Jun 3, 2024
208c62a
Merge master
sfc-gh-ext-simba-nl Jun 13, 2024
f690698
Fix styling
sfc-gh-ext-simba-nl Jun 13, 2024
e68e66f
Remove unnecessary code
sfc-gh-ext-simba-nl Jun 14, 2024
0a9b115
Merge branch 'master' into SNOW-957747-easy-logging-configuration-imp…
sfc-gh-ext-simba-nl Jun 21, 2024
dca8db8
Merge branch 'master' into SNOW-957747-easy-logging-configuration-imp…
sfc-gh-ext-simba-nl Jun 25, 2024
acdf254
On config file permission error, change throw to warn instead
sfc-gh-ext-simba-nl Jun 25, 2024
243b232
Update permission test since we no longer through exception
sfc-gh-ext-simba-nl Jun 25, 2024
d36b9be
Throw exception when its not a permission reason
sfc-gh-ext-simba-nl Jun 26, 2024
2a47f62
Move permissions function to a separate package scope function. Fix t…
sfc-gh-ext-simba-nl Jun 27, 2024
1906a2a
Fix formatting
sfc-gh-ext-simba-nl Jun 27, 2024
7f8481b
Remove * import
sfc-gh-ext-simba-nl Jun 27, 2024
aaf791f
Fix Permissions test case
sfc-gh-ext-simba-nl Jun 27, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/main/java/net/snowflake/client/config/SFClientConfig.java
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
package net.snowflake.client.config;

import com.fasterxml.jackson.annotation.JsonAnySetter;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Objects;

/** POJO class for Snowflake's client config. */
Expand Down Expand Up @@ -57,6 +60,8 @@ public static class CommonProps {
@JsonProperty("log_path")
private String logPath;

@JsonAnySetter private Map<String, Object> unknownKeys = new LinkedHashMap<>();

public CommonProps() {}

public void CommonProps(String logLevel, String logPath) {
Expand All @@ -80,6 +85,10 @@ public void setLogPath(String logPath) {
this.logPath = logPath;
}

Map<String, Object> getUnknownKeys() {
return unknownKeys;
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,12 @@
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.nio.file.attribute.PosixFilePermission;
import java.util.Map;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import net.snowflake.client.core.Constants;
import net.snowflake.client.jdbc.SnowflakeDriver;
import net.snowflake.client.log.SFLogger;
import net.snowflake.client.log.SFLoggerFactory;
Expand All @@ -25,41 +29,77 @@ public class SFClientConfigParser {
* connection property. 2. Environment variable: SF_CLIENT_CONFIG_FILE containing full path to
* sf_client_config file. 3. Searches for default config file name(sf_client_config.json under the
* driver directory from where the driver gets loaded. 4. Searches for default config file
* name(sf_client_config.json) under user home directory 5. Searches for default config file
* name(sf_client_config.json) under tmp directory.
* name(sf_client_config.json) under user home directory.
*
* @param configFilePath SF_CLIENT_CONFIG_FILE parameter read from connection URL or connection
* properties
* @return SFClientConfig
*/
public static SFClientConfig loadSFClientConfig(String configFilePath) throws IOException {
if (configFilePath != null) {
sfc-gh-dprzybysz marked this conversation as resolved.
Show resolved Hide resolved
logger.info("Attempting to enable easy logging with file path {}", configFilePath);
}
String derivedConfigFilePath = null;
if (configFilePath != null && !configFilePath.isEmpty()) {
// 1. Try to read the file at configFilePath.
derivedConfigFilePath = configFilePath;
logger.info(
sfc-gh-dprzybysz marked this conversation as resolved.
Show resolved Hide resolved
"Using client configuration path from a connection string: {}", derivedConfigFilePath);
} else if (System.getenv().containsKey(SF_CLIENT_CONFIG_ENV_NAME)) {
// 2. If SF_CLIENT_CONFIG_ENV_NAME is set, read from env.
derivedConfigFilePath = systemGetEnv(SF_CLIENT_CONFIG_ENV_NAME);
logger.info(
"Using client configuration path from an environment variable: {}",
derivedConfigFilePath);
} else {
// 3. Read SF_CLIENT_CONFIG_FILE_NAME from where jdbc jar is loaded.
String driverLocation =
Paths.get(getConfigFilePathFromJDBCJarLocation(), SF_CLIENT_CONFIG_FILE_NAME).toString();
if (Files.exists(Paths.get(driverLocation))) {
derivedConfigFilePath = driverLocation;
logger.info(
"Using client configuration path from jar directory: {}", derivedConfigFilePath);
} else {
// 4. Read SF_CLIENT_CONFIG_FILE_NAME if it is present in user home directory.
String userHomeFilePath =
Paths.get(systemGetProperty("user.home"), SF_CLIENT_CONFIG_FILE_NAME).toString();
if (Files.exists(Paths.get(userHomeFilePath))) {
derivedConfigFilePath = userHomeFilePath;
String homePath = systemGetProperty("user.home");
if (homePath != null) {
String userHomeFilePath = Paths.get(homePath, SF_CLIENT_CONFIG_FILE_NAME).toString();
if (Files.exists(Paths.get(userHomeFilePath))) {
derivedConfigFilePath = userHomeFilePath;
logger.info(
"Using client configuration path from home directory: {}", derivedConfigFilePath);
}
}
}
}
if (derivedConfigFilePath != null) {
try {
if (Constants.getOS() != Constants.OS.WINDOWS) {
sfc-gh-ext-simba-nl marked this conversation as resolved.
Show resolved Hide resolved
// Check permissions of config file
Set<PosixFilePermission> folderPermissions =
Files.getPosixFilePermissions(Paths.get(derivedConfigFilePath));
if (folderPermissions.contains(PosixFilePermission.GROUP_WRITE)
|| folderPermissions.contains(PosixFilePermission.OTHERS_WRITE)) {
String error =
String.format(
"Error due to other users having permission to modify the config file: %s",
derivedConfigFilePath);
throw new IOException(error);
sfc-gh-dprzybysz marked this conversation as resolved.
Show resolved Hide resolved
}
}
File configFile = new File(derivedConfigFilePath);
ObjectMapper objectMapper = new ObjectMapper();
SFClientConfig clientConfig = objectMapper.readValue(configFile, SFClientConfig.class);
logger.info(
"Reading values logLevel {} and logPath {} from client configuration",
clientConfig.getCommonProps().getLogLevel(),
clientConfig.getCommonProps().getLogPath());

for (Map.Entry<String, Object> prop :
clientConfig.getCommonProps().getUnknownKeys().entrySet()) {
logger.info(
"Unknown configuration entry: {} with value: {}", prop.getKey(), prop.getValue());
}
clientConfig.setConfigFilePath(derivedConfigFilePath);

return clientConfig;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,19 @@
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.attribute.PosixFilePermission;
import java.nio.file.attribute.PosixFilePermissions;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.SQLNonTransientConnectionException;
import java.sql.Statement;
import java.util.Map;
import java.util.Properties;
import java.util.Set;
import java.util.logging.Level;
import net.snowflake.client.config.SFClientConfig;
import net.snowflake.client.config.SFClientConfigParser;
import net.snowflake.client.core.Constants;
import net.snowflake.client.core.SFBaseResultSet;
import net.snowflake.client.core.SFBaseSession;
import net.snowflake.client.core.SFBaseStatement;
Expand Down Expand Up @@ -136,13 +140,16 @@ private void setClientConfig() throws SnowflakeSQLLoggedException {
String clientConfigFilePath =
(String) connectionPropertiesMap.getOrDefault(SFSessionProperty.CLIENT_CONFIG_FILE, null);

SFClientConfig sfClientConfig;
try {
sfClientConfig = SFClientConfigParser.loadSFClientConfig(clientConfigFilePath);
} catch (IOException e) {
throw new SnowflakeSQLLoggedException(sfSession, ErrorCode.INTERNAL_ERROR, e.getMessage());
SFClientConfig sfClientConfig = sfSession.getSfClientConfig();
if (sfClientConfig == null) {
try {
sfClientConfig = SFClientConfigParser.loadSFClientConfig(clientConfigFilePath);
} catch (IOException e) {
throw new SnowflakeSQLLoggedException(
sfSession, ErrorCode.INTERNAL_ERROR, e.getMessage(), e.getCause());
}
sfSession.setSfClientConfig(sfClientConfig);
}
sfSession.setSfClientConfig(sfClientConfig);
}

/**
Expand Down Expand Up @@ -181,20 +188,18 @@ && systemGetProperty("java.util.logging.config.file") == null) {

if (logLevel != null && logPattern != null) {
try {
logger.info("Setting logger with log level {} and log pattern {}", logLevel, logPattern);
JDK14Logger.instantiateLogger(logLevel, logPattern);
} catch (IOException ex) {
throw new SnowflakeSQLLoggedException(
sfSession, ErrorCode.INTERNAL_ERROR, ex.getMessage());
}
if (sfClientConfig != null) {
logger.debug(
String.format(
"SF Client config found at location: %s.", sfClientConfig.getConfigFilePath()));
"SF Client config found at location: {}.", sfClientConfig.getConfigFilePath());
}
logger.debug(
String.format(
"Instantiating JDK14Logger with level: %s , output path: %s",
logLevel, logPattern));
"Instantiating JDK14Logger with level: {}, output path: {}", logLevel, logPattern);
}
}
}
Expand All @@ -206,11 +211,13 @@ private String constructLogPattern(String logPathFromConfig) throws SnowflakeSQL

String logPattern = "%t/snowflake_jdbc%u.log"; // java.tmpdir

Path logPath;
if (logPathFromConfig != null && !logPathFromConfig.isEmpty()) {
Path path = Paths.get(logPathFromConfig, "jdbc");
if (!Files.exists(path)) {
// Get log path from configuration
logPath = Paths.get(logPathFromConfig);
if (!Files.exists(logPath)) {
try {
Files.createDirectories(path);
Files.createDirectories(logPath);
} catch (IOException ex) {
throw new SnowflakeSQLLoggedException(
sfSession,
Expand All @@ -220,11 +227,82 @@ private String constructLogPattern(String logPathFromConfig) throws SnowflakeSQL
logPathFromConfig, ex.getMessage()));
}
}
logPattern = Paths.get(path.toString(), "snowflake_jdbc%u.log").toString();
} else {
// Get log path from home directory
String homePath = systemGetProperty("user.home");
sfc-gh-ext-simba-nl marked this conversation as resolved.
Show resolved Hide resolved
if (homePath == null || homePath.isEmpty()) {
throw new SnowflakeSQLLoggedException(
sfSession,
ErrorCode.INTERNAL_ERROR,
String.format(
"Log path not set in configfile %s and home directory not set.",
logPathFromConfig));
}
logPath = Paths.get(homePath);
}

Path path = createLogPathSubDirectory(logPath);

logPattern = Paths.get(path.toString(), "snowflake_jdbc%u.log").toString();
return logPattern;
}

private Path createLogPathSubDirectory(Path logPath) throws SnowflakeSQLLoggedException {
Path path = Paths.get(logPath.toString(), "jdbc");
if (!Files.exists(path)) {
createLogFolder(path);
} else {
checkLogFolderPermissions(path);
}
return path;
}

private void createLogFolder(Path path) throws SnowflakeSQLLoggedException {
try {
if (Constants.getOS() == Constants.OS.WINDOWS) {
Files.createDirectories(path);
} else {
Files.createDirectories(
path,
PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------")));
}
} catch (IOException ex) {
throw new SnowflakeSQLLoggedException(
sfSession,
ErrorCode.INTERNAL_ERROR,
String.format(
"Unable to create jdbc subfolder in configfile %s ,%s",
path.toString(), ex.getMessage(), ex.getCause()));
}
}

private void checkLogFolderPermissions(Path path) throws SnowflakeSQLLoggedException {
if (Constants.getOS() != Constants.OS.WINDOWS) {
try {
Set<PosixFilePermission> folderPermissions = Files.getPosixFilePermissions(path);
if (folderPermissions.contains(PosixFilePermission.GROUP_WRITE)
|| folderPermissions.contains(PosixFilePermission.GROUP_READ)
|| folderPermissions.contains(PosixFilePermission.GROUP_EXECUTE)
|| folderPermissions.contains(PosixFilePermission.OTHERS_WRITE)
|| folderPermissions.contains(PosixFilePermission.OTHERS_READ)
|| folderPermissions.contains(PosixFilePermission.OTHERS_EXECUTE)) {
logger.info(
sfc-gh-dprzybysz marked this conversation as resolved.
Show resolved Hide resolved
"Access permission for the logs directory '{}' is currently {} and is potentially "
+ "accessible to users other than the owner of the logs directory.",
path.toString(),
folderPermissions.toString());
}
} catch (IOException ex) {
throw new SnowflakeSQLLoggedException(
sfc-gh-ext-simba-nl marked this conversation as resolved.
Show resolved Hide resolved
sfSession,
ErrorCode.INTERNAL_ERROR,
String.format(
"Un-able to get permissions of log directory %s ,%s",
sfc-gh-dprzybysz marked this conversation as resolved.
Show resolved Hide resolved
path.toString(), ex.getMessage(), ex.getCause()));
}
}
}

private void initSessionProperties(SnowflakeConnectString conStr, String appID, String appVersion)
throws SFException {
Map<String, Object> properties = mergeProperties(conStr);
Expand Down
9 changes: 9 additions & 0 deletions src/test/java/net/snowflake/client/RunningOnWin.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package net.snowflake.client;

import net.snowflake.client.core.Constants;

public class RunningOnWin implements ConditionalIgnoreRule.IgnoreCondition {
public boolean isSatisfied() {
return Constants.getOS() == Constants.OS.WINDOWS;
}
}
Loading
Loading