Skip to content

[GOBBLIN-1977] Log when PasswordManager fails to load any master password #3849

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.

Sign up for GitHub

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

Merged
arjun4084346 merged 1 commit into from
Dec 15, 2023
Merged

[GOBBLIN-1977] Log when PasswordManager fails to load any master password #3849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 11 additions & 5 deletions gobblin-api/src/main/java/org/apache/gobblin/password/PasswordManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ private List<TextEncryptor> getEncryptors(CachedInstanceKey cacheKey) {
try (Closer closer = Closer.create()) {
if (!fs.exists(currentMasterPasswordFile) ||
fs.getFileStatus(currentMasterPasswordFile).isDirectory()) {
LOG.warn("Master password path '" + currentMasterPasswordFile + "' not a FileSystem file.");
continue;
}
InputStream in = closer.register(fs.open(currentMasterPasswordFile));
Expand All @@ -124,18 +125,23 @@ private List<TextEncryptor> getEncryptors(CachedInstanceKey cacheKey) {
suffix = "." + String.valueOf(i);
} catch (FileNotFoundException fnf) {
// It is ok for password files not being present
LOG.warn("Master password file " + currentMasterPasswordFile + " not found.");
LOG.warn("Master password file '" + currentMasterPasswordFile + "' not found.");
} catch (IOException ioe) {
exception = ioe;
LOG.warn("Master password could not be read from file " + currentMasterPasswordFile);
LOG.warn("Master password file could not be read from '" + currentMasterPasswordFile + "'");
} catch (Exception e) {
LOG.warn("Encryptor could not be instantiated.");
LOG.warn("Encryptor could not be instantiated using file '" + currentMasterPasswordFile + "'.", e);
}
} while (i++ < numOfEncryptionKeys);

// Throw exception if could not read any existing password file
if (encryptors.size() < 1 && exception != null) {
throw new RuntimeException("Master Password could not be read from any master password file.", exception);
if (encryptors.size() < 1) {
if (exception != null) {
throw new RuntimeException("Master password could not be read from any master password file.", exception);
} else {
// TODO: determine whether to always throw whenever no encryptors, despite `exception == null`! (for now, at least give notice by logging)
Copy link
Copy Markdown
Contributor Author

@phet phet Dec 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wavered on this, but ultimately decided not to pursue, as it would NOT be backwards-compatible semantics and I'm unsure whether any of these situations are essentially harmless with users happy to continue w/o disruption

LOG.error("No master password loaded, despite " + numOfEncryptionKeys + " encryption keys!");
}
}
return encryptors;
}
Expand Down
2 changes: 2 additions & 0 deletions ...in-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,6 +720,8 @@ private void cancelDagNode(DagNode<JobExecutionPlan> dagNodeToCancel) throws Exe
String serializedFuture = DagManagerUtils.getSpecProducer(dagNodeToCancel).serializeAddSpecResponse(future);
props.put(ConfigurationKeys.SPEC_PRODUCER_SERIALIZED_FUTURE, serializedFuture);
sendCancellationEvent(dagNodeToCancel.getValue());
} else {
log.warn("No Job future when canceling DAG node (hence, not sending cancellation event) - {}", dagNodeToCancel.getValue().getJobSpec().getUri());
Comment on lines +723 to +724
Copy link
Copy Markdown
Contributor Author

@phet phet Dec 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not strictly related... just sprinkling in better logging concerning a question I was recently pondering during another investigation

}
if (dagNodeToCancel.getValue().getJobSpec().getConfig().hasPath(ConfigurationKeys.FLOW_EXECUTION_ID_KEY)) {
props.setProperty(ConfigurationKeys.FLOW_EXECUTION_ID_KEY,
Expand Down