Copilot commented on code in PR #12024:
URL: https://github.com/apache/cloudstack/pull/12024#discussion_r2513406988
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -238,12 +246,16 @@ static String stackTraceAsString(Throwable throwable) {
public String execute(OutputInterpreter interpreter) {
String[] command = _command.toArray(new String[_command.size()]);
String commandLine = buildCommandLine(command);
- if (_logger.isDebugEnabled() && !avoidLoggingCommand) {
+ if (_logger.isDebugEnabled() && !avoidLoggingCommand &&
sensitiveArgIndices.isEmpty()) {
Review Comment:
The condition `!avoidLoggingCommand && sensitiveArgIndices.isEmpty()`
creates an inconsistent logging strategy. When `avoidLoggingCommand` is true,
the command is completely hidden, but when `sensitiveArgIndices` is not empty,
only the sensitive parts are masked in `commandLine`. Consider either: (1)
logging the masked `commandLine` when `sensitiveArgIndices` is not empty, or
(2) treating both flags consistently. The current approach prevents any logging
when sensitive args exist, even though `buildCommandLine()` already masks them.
```suggestion
if (_logger.isDebugEnabled() && !avoidLoggingCommand) {
```
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -261,48 +273,126 @@ public String execute(OutputInterpreter interpreter) {
_thread = Thread.currentThread();
ScheduledFuture<String> future = null;
if (_timeout > 0) {
- _logger.trace(String.format("Scheduling the execution of
command [%s] with a timeout of [%s] milliseconds.", commandLine, _timeout));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format(
+ "Scheduling the execution of command [%s] with a
timeout of [%s] milliseconds.",
+ commandLine, _timeout));
+ } else {
+ _logger.trace(String.format(
+ "Scheduling the execution of command with
sensitive arguments with a timeout of [%s] milliseconds.",
+ _timeout));
+ }
Review Comment:
This code contains extensive duplication with the same `if
(sensitiveArgIndices.isEmpty())` pattern repeated over 20 times. Since
`commandLine` is already built with `buildCommandLine()` which masks sensitive
arguments, you could use it directly in all log statements. This would
eliminate all the conditional branches and significantly reduce code complexity
and maintenance burden. Consider simply using `commandLine` (which is already
sanitized) in all logging statements.
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -261,48 +273,126 @@ public String execute(OutputInterpreter interpreter) {
_thread = Thread.currentThread();
ScheduledFuture<String> future = null;
if (_timeout > 0) {
- _logger.trace(String.format("Scheduling the execution of
command [%s] with a timeout of [%s] milliseconds.", commandLine, _timeout));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format(
+ "Scheduling the execution of command [%s] with a
timeout of [%s] milliseconds.",
+ commandLine, _timeout));
+ } else {
+ _logger.trace(String.format(
+ "Scheduling the execution of command with
sensitive arguments with a timeout of [%s] milliseconds.",
+ _timeout));
+ }
future = s_executors.schedule(this, _timeout,
TimeUnit.MILLISECONDS);
}
long processPid = _process.pid();
Task task = null;
if (interpreter != null && interpreter.drain()) {
- _logger.trace(String.format("Executing interpreting task of
process [%s] for command [%s].", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format("Executing interpreting task
of process [%s] for command [%s].",
+ processPid, commandLine));
+ } else {
+ _logger.trace(String.format(
+ "Executing interpreting task of process [%s] for
command with sensitive arguments.",
+ processPid));
+ }
task = new Task(interpreter, ir);
s_executors.execute(task);
}
while (true) {
- _logger.trace(String.format("Attempting process [%s] execution
for command [%s] with timeout [%s].", processPid, commandLine, _timeout));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format("Attempting process [%s]
execution for command [%s] with timeout [%s].",
+ processPid, commandLine, _timeout));
+ } else {
+ _logger.trace(String.format(
+ "Attempting process [%s] execution for command
with sensitive arguments with timeout [%s].",
+ processPid, _timeout));
+ }
try {
if (_process.waitFor(_timeout, TimeUnit.MILLISECONDS)) {
- _logger.trace(String.format("Process [%s] execution
for command [%s] completed within timeout period [%s].", processPid,
commandLine,
- _timeout));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format(
+ "Process [%s] execution for command [%s]
completed within timeout period [%s].",
+ processPid, commandLine,
+ _timeout));
+ } else {
+ _logger.trace(String.format(
+ "Process [%s] execution for command with
sensitive arguments completed within timeout period [%s].",
+ processPid,
+ _timeout));
+ }
if (_process.exitValue() == 0) {
- _logger.debug(String.format("Successfully executed
process [%s] for command [%s].", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.debug(String.format("Successfully
executed process [%s] for command [%s].",
+ processPid, commandLine));
+ } else {
+ _logger.debug(String.format(
+ "Successfully executed process [%s]
for command with sensitive arguments.",
+ processPid));
+ }
if (interpreter != null) {
if (interpreter.drain()) {
- _logger.trace(String.format("Returning
task result of process [%s] for command [%s].", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(
+ String.format("Returning task
result of process [%s] for command [%s].",
+ processPid,
commandLine));
+ } else {
+ _logger.trace(String.format(
+ "Returning task result of
process [%s] for command with sensitive arguments.",
+ processPid));
+ }
return task.getResult();
}
- _logger.trace(String.format("Returning
interpretation of process [%s] for command [%s].", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(
+ String.format("Returning
interpretation of process [%s] for command [%s].",
+ processPid, commandLine));
+ } else {
+ _logger.trace(String.format(
+ "Returning interpretation of
process [%s] for command with sensitive arguments.",
+ processPid));
+ }
Review Comment:
This code contains extensive duplication with the same `if
(sensitiveArgIndices.isEmpty())` pattern repeated over 20 times. Since
`commandLine` is already built with `buildCommandLine()` which masks sensitive
arguments, you could use it directly in all log statements. This would
eliminate all the conditional branches and significantly reduce code complexity
and maintenance burden. Consider simply using `commandLine` (which is already
sanitized) in all logging statements.
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -261,48 +273,126 @@ public String execute(OutputInterpreter interpreter) {
_thread = Thread.currentThread();
ScheduledFuture<String> future = null;
if (_timeout > 0) {
- _logger.trace(String.format("Scheduling the execution of
command [%s] with a timeout of [%s] milliseconds.", commandLine, _timeout));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format(
+ "Scheduling the execution of command [%s] with a
timeout of [%s] milliseconds.",
+ commandLine, _timeout));
+ } else {
+ _logger.trace(String.format(
+ "Scheduling the execution of command with
sensitive arguments with a timeout of [%s] milliseconds.",
+ _timeout));
+ }
future = s_executors.schedule(this, _timeout,
TimeUnit.MILLISECONDS);
}
long processPid = _process.pid();
Task task = null;
if (interpreter != null && interpreter.drain()) {
- _logger.trace(String.format("Executing interpreting task of
process [%s] for command [%s].", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format("Executing interpreting task
of process [%s] for command [%s].",
+ processPid, commandLine));
+ } else {
+ _logger.trace(String.format(
+ "Executing interpreting task of process [%s] for
command with sensitive arguments.",
+ processPid));
+ }
Review Comment:
This code contains extensive duplication with the same `if
(sensitiveArgIndices.isEmpty())` pattern repeated over 20 times. Since
`commandLine` is already built with `buildCommandLine()` which masks sensitive
arguments, you could use it directly in all log statements. This would
eliminate all the conditional branches and significantly reduce code complexity
and maintenance burden. Consider simply using `commandLine` (which is already
sanitized) in all logging statements.
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -315,18 +405,33 @@ public String execute(OutputInterpreter interpreter) {
TimedOutLogger log = new TimedOutLogger(_process);
Task timedoutTask = new Task(log, ir);
- _logger.trace(String.format("Running timed out task of process
[%s] for command [%s].", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format("Running timed out task of
process [%s] for command [%s].", processPid,
+ commandLine));
+ } else {
+ _logger.trace(String.format(
+ "Running timed out task of process [%s] for
command with sensitive arguments.",
+ processPid));
+ }
timedoutTask.run();
- if (!_passwordCommand) {
- _logger.warn(String.format("Process [%s] for command [%s]
timed out. Output is [%s].", processPid, commandLine,
timedoutTask.getResult()));
+ if (!_passwordCommand && sensitiveArgIndices.isEmpty()) {
+ _logger.warn(String.format("Process [%s] for command [%s]
timed out. Output is [%s].", processPid,
+ commandLine, timedoutTask.getResult()));
} else {
- _logger.warn(String.format("Process [%s] for command [%s]
timed out.", processPid, commandLine));
+ _logger.warn(
+ String.format("Process [%s] for command with
sensitive arguments timed out.", processPid));
}
Review Comment:
This code contains extensive duplication with the same `if
(sensitiveArgIndices.isEmpty())` pattern repeated over 20 times. Since
`commandLine` is already built with `buildCommandLine()` which masks sensitive
arguments, you could use it directly in all log statements. This would
eliminate all the conditional branches and significantly reduce code complexity
and maintenance burden. Consider simply using `commandLine` (which is already
sanitized) in all logging statements.
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -261,48 +273,126 @@ public String execute(OutputInterpreter interpreter) {
_thread = Thread.currentThread();
ScheduledFuture<String> future = null;
if (_timeout > 0) {
- _logger.trace(String.format("Scheduling the execution of
command [%s] with a timeout of [%s] milliseconds.", commandLine, _timeout));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format(
+ "Scheduling the execution of command [%s] with a
timeout of [%s] milliseconds.",
+ commandLine, _timeout));
+ } else {
+ _logger.trace(String.format(
+ "Scheduling the execution of command with
sensitive arguments with a timeout of [%s] milliseconds.",
+ _timeout));
+ }
future = s_executors.schedule(this, _timeout,
TimeUnit.MILLISECONDS);
}
long processPid = _process.pid();
Task task = null;
if (interpreter != null && interpreter.drain()) {
- _logger.trace(String.format("Executing interpreting task of
process [%s] for command [%s].", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format("Executing interpreting task
of process [%s] for command [%s].",
+ processPid, commandLine));
+ } else {
+ _logger.trace(String.format(
+ "Executing interpreting task of process [%s] for
command with sensitive arguments.",
+ processPid));
+ }
task = new Task(interpreter, ir);
s_executors.execute(task);
}
while (true) {
- _logger.trace(String.format("Attempting process [%s] execution
for command [%s] with timeout [%s].", processPid, commandLine, _timeout));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format("Attempting process [%s]
execution for command [%s] with timeout [%s].",
+ processPid, commandLine, _timeout));
+ } else {
+ _logger.trace(String.format(
+ "Attempting process [%s] execution for command
with sensitive arguments with timeout [%s].",
+ processPid, _timeout));
+ }
try {
if (_process.waitFor(_timeout, TimeUnit.MILLISECONDS)) {
- _logger.trace(String.format("Process [%s] execution
for command [%s] completed within timeout period [%s].", processPid,
commandLine,
- _timeout));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format(
+ "Process [%s] execution for command [%s]
completed within timeout period [%s].",
+ processPid, commandLine,
+ _timeout));
+ } else {
+ _logger.trace(String.format(
+ "Process [%s] execution for command with
sensitive arguments completed within timeout period [%s].",
+ processPid,
+ _timeout));
+ }
if (_process.exitValue() == 0) {
- _logger.debug(String.format("Successfully executed
process [%s] for command [%s].", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.debug(String.format("Successfully
executed process [%s] for command [%s].",
+ processPid, commandLine));
+ } else {
+ _logger.debug(String.format(
+ "Successfully executed process [%s]
for command with sensitive arguments.",
+ processPid));
+ }
if (interpreter != null) {
if (interpreter.drain()) {
- _logger.trace(String.format("Returning
task result of process [%s] for command [%s].", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(
+ String.format("Returning task
result of process [%s] for command [%s].",
+ processPid,
commandLine));
+ } else {
+ _logger.trace(String.format(
+ "Returning task result of
process [%s] for command with sensitive arguments.",
+ processPid));
+ }
return task.getResult();
}
- _logger.trace(String.format("Returning
interpretation of process [%s] for command [%s].", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(
+ String.format("Returning
interpretation of process [%s] for command [%s].",
+ processPid, commandLine));
+ } else {
+ _logger.trace(String.format(
+ "Returning interpretation of
process [%s] for command with sensitive arguments.",
+ processPid));
+ }
return interpreter.interpret(ir);
} else {
// null return exitValue apparently
- _logger.trace(String.format("Process [%s] for
command [%s] exited with value [%s].", processPid, commandLine,
- _process.exitValue()));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format("Process [%s]
for command [%s] exited with value [%s].",
+ processPid, commandLine,
+ _process.exitValue()));
+ } else {
+ _logger.trace(String.format(
+ "Process [%s] for command with
sensitive arguments exited with value [%s].",
+ processPid,
+ _process.exitValue()));
+ }
return String.valueOf(_process.exitValue());
}
} else {
- _logger.warn(String.format("Execution of process
[%s] for command [%s] failed.", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.warn(String.format("Execution of
process [%s] for command [%s] failed.",
+ processPid, commandLine));
+ } else {
+ _logger.warn(String.format(
+ "Execution of process [%s] for command
with sensitive arguments failed.",
+ processPid));
+ }
break;
}
}
} catch (InterruptedException e) {
if (!_isTimeOut) {
- _logger.debug(String.format("Exception [%s] occurred;
however, it was not a timeout. Therefore, proceeding with the execution of
process [%s] for command "
- + "[%s].", e.getMessage(), processPid,
commandLine), e);
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.debug(String.format(
+ "Exception [%s] occurred; however, it was
not a timeout. Therefore, proceeding with the execution of process [%s] for
command "
+ + "[%s].",
+ e.getMessage(), processPid, commandLine),
e);
+ } else {
+ _logger.debug(String.format(
+ "Exception [%s] occurred; however, it was
not a timeout. Therefore, proceeding with the execution of process [%s] for
command "
+ + "with sensitive arguments.",
+ e.getMessage(), processPid), e);
+ }
Review Comment:
This code contains extensive duplication with the same `if
(sensitiveArgIndices.isEmpty())` pattern repeated over 20 times. Since
`commandLine` is already built with `buildCommandLine()` which masks sensitive
arguments, you could use it directly in all log statements. This would
eliminate all the conditional branches and significantly reduce code complexity
and maintenance burden. Consider simply using `commandLine` (which is already
sanitized) in all logging statements.
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -337,19 +442,49 @@ public String execute(OutputInterpreter interpreter) {
error = String.valueOf(_process.exitValue());
}
- _logger.warn(String.format("Process [%s] for command [%s]
encountered the error: [%s].", processPid, commandLine, error));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.warn(String.format("Process [%s] for command [%s]
encountered the error: [%s].", processPid,
+ commandLine, error));
+ } else {
+ _logger.warn(
+ String.format("Process [%s] for command with sensitive
arguments encountered the error: [%s].",
+ processPid, error));
+ }
return error;
} catch (SecurityException ex) {
- _logger.warn(String.format("Exception [%s] occurred. This may be
due to an attempt of executing command [%s] as non root.", ex.getMessage(),
commandLine),
- ex);
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.warn(String.format(
+ "Exception [%s] occurred. This may be due to an
attempt of executing command [%s] as non root.",
+ ex.getMessage(), commandLine),
+ ex);
+ } else {
+ _logger.warn(String.format(
+ "Exception [%s] occurred. This may be due to an
attempt of executing command with sensitive data as non root.",
+ ex.getMessage()),
+ ex);
+ }
return stackTraceAsString(ex);
} catch (Exception ex) {
- _logger.warn(String.format("Exception [%s] occurred when
attempting to run command [%s].", ex.getMessage(), commandLine), ex);
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.warn(String.format("Exception [%s] occurred when
attempting to run command [%s].",
+ ex.getMessage(), commandLine), ex);
+ } else {
+ _logger.warn(
+ String.format("Exception [%s] occurred when attempting
to run a command with sensitive data.",
+ ex.getMessage()),
+ ex);
+ }
return stackTraceAsString(ex);
} finally {
if (_process != null) {
- _logger.trace(String.format("Destroying process [%s] for
command [%s].", _process.pid(), commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(
+ String.format("Destroying process [%s] for command
[%s].", _process.pid(), commandLine));
+ } else {
+ _logger.trace(String.format("Destroying process [%s] for a
command with sensitive data.",
+ _process.pid()));
+ }
Review Comment:
This code contains extensive duplication with the same `if
(sensitiveArgIndices.isEmpty())` pattern repeated over 20 times. Since
`commandLine` is already built with `buildCommandLine()` which masks sensitive
arguments, you could use it directly in all log statements. This would
eliminate all the conditional branches and significantly reduce code complexity
and maintenance burden. Consider simply using `commandLine` (which is already
sanitized) in all logging statements.
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -337,19 +442,49 @@ public String execute(OutputInterpreter interpreter) {
error = String.valueOf(_process.exitValue());
}
- _logger.warn(String.format("Process [%s] for command [%s]
encountered the error: [%s].", processPid, commandLine, error));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.warn(String.format("Process [%s] for command [%s]
encountered the error: [%s].", processPid,
+ commandLine, error));
+ } else {
+ _logger.warn(
+ String.format("Process [%s] for command with sensitive
arguments encountered the error: [%s].",
+ processPid, error));
+ }
return error;
} catch (SecurityException ex) {
- _logger.warn(String.format("Exception [%s] occurred. This may be
due to an attempt of executing command [%s] as non root.", ex.getMessage(),
commandLine),
- ex);
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.warn(String.format(
+ "Exception [%s] occurred. This may be due to an
attempt of executing command [%s] as non root.",
+ ex.getMessage(), commandLine),
+ ex);
+ } else {
+ _logger.warn(String.format(
+ "Exception [%s] occurred. This may be due to an
attempt of executing command with sensitive data as non root.",
+ ex.getMessage()),
+ ex);
+ }
return stackTraceAsString(ex);
} catch (Exception ex) {
- _logger.warn(String.format("Exception [%s] occurred when
attempting to run command [%s].", ex.getMessage(), commandLine), ex);
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.warn(String.format("Exception [%s] occurred when
attempting to run command [%s].",
+ ex.getMessage(), commandLine), ex);
+ } else {
+ _logger.warn(
+ String.format("Exception [%s] occurred when attempting
to run a command with sensitive data.",
+ ex.getMessage()),
+ ex);
+ }
Review Comment:
This code contains extensive duplication with the same `if
(sensitiveArgIndices.isEmpty())` pattern repeated over 20 times. Since
`commandLine` is already built with `buildCommandLine()` which masks sensitive
arguments, you could use it directly in all log statements. This would
eliminate all the conditional branches and significantly reduce code complexity
and maintenance burden. Consider simply using `commandLine` (which is already
sanitized) in all logging statements.
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -261,48 +273,126 @@ public String execute(OutputInterpreter interpreter) {
_thread = Thread.currentThread();
ScheduledFuture<String> future = null;
if (_timeout > 0) {
- _logger.trace(String.format("Scheduling the execution of
command [%s] with a timeout of [%s] milliseconds.", commandLine, _timeout));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format(
+ "Scheduling the execution of command [%s] with a
timeout of [%s] milliseconds.",
+ commandLine, _timeout));
+ } else {
+ _logger.trace(String.format(
+ "Scheduling the execution of command with
sensitive arguments with a timeout of [%s] milliseconds.",
+ _timeout));
+ }
future = s_executors.schedule(this, _timeout,
TimeUnit.MILLISECONDS);
}
long processPid = _process.pid();
Task task = null;
if (interpreter != null && interpreter.drain()) {
- _logger.trace(String.format("Executing interpreting task of
process [%s] for command [%s].", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format("Executing interpreting task
of process [%s] for command [%s].",
+ processPid, commandLine));
+ } else {
+ _logger.trace(String.format(
+ "Executing interpreting task of process [%s] for
command with sensitive arguments.",
+ processPid));
+ }
task = new Task(interpreter, ir);
s_executors.execute(task);
}
while (true) {
- _logger.trace(String.format("Attempting process [%s] execution
for command [%s] with timeout [%s].", processPid, commandLine, _timeout));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format("Attempting process [%s]
execution for command [%s] with timeout [%s].",
+ processPid, commandLine, _timeout));
+ } else {
+ _logger.trace(String.format(
+ "Attempting process [%s] execution for command
with sensitive arguments with timeout [%s].",
+ processPid, _timeout));
+ }
try {
if (_process.waitFor(_timeout, TimeUnit.MILLISECONDS)) {
- _logger.trace(String.format("Process [%s] execution
for command [%s] completed within timeout period [%s].", processPid,
commandLine,
- _timeout));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format(
+ "Process [%s] execution for command [%s]
completed within timeout period [%s].",
+ processPid, commandLine,
+ _timeout));
+ } else {
+ _logger.trace(String.format(
+ "Process [%s] execution for command with
sensitive arguments completed within timeout period [%s].",
+ processPid,
+ _timeout));
+ }
if (_process.exitValue() == 0) {
- _logger.debug(String.format("Successfully executed
process [%s] for command [%s].", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.debug(String.format("Successfully
executed process [%s] for command [%s].",
+ processPid, commandLine));
+ } else {
+ _logger.debug(String.format(
+ "Successfully executed process [%s]
for command with sensitive arguments.",
+ processPid));
+ }
if (interpreter != null) {
if (interpreter.drain()) {
- _logger.trace(String.format("Returning
task result of process [%s] for command [%s].", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(
+ String.format("Returning task
result of process [%s] for command [%s].",
+ processPid,
commandLine));
+ } else {
+ _logger.trace(String.format(
+ "Returning task result of
process [%s] for command with sensitive arguments.",
+ processPid));
+ }
return task.getResult();
}
- _logger.trace(String.format("Returning
interpretation of process [%s] for command [%s].", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(
+ String.format("Returning
interpretation of process [%s] for command [%s].",
+ processPid, commandLine));
+ } else {
+ _logger.trace(String.format(
+ "Returning interpretation of
process [%s] for command with sensitive arguments.",
+ processPid));
+ }
return interpreter.interpret(ir);
} else {
// null return exitValue apparently
- _logger.trace(String.format("Process [%s] for
command [%s] exited with value [%s].", processPid, commandLine,
- _process.exitValue()));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format("Process [%s]
for command [%s] exited with value [%s].",
+ processPid, commandLine,
+ _process.exitValue()));
+ } else {
+ _logger.trace(String.format(
+ "Process [%s] for command with
sensitive arguments exited with value [%s].",
+ processPid,
+ _process.exitValue()));
+ }
return String.valueOf(_process.exitValue());
}
} else {
- _logger.warn(String.format("Execution of process
[%s] for command [%s] failed.", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.warn(String.format("Execution of
process [%s] for command [%s] failed.",
+ processPid, commandLine));
+ } else {
+ _logger.warn(String.format(
+ "Execution of process [%s] for command
with sensitive arguments failed.",
+ processPid));
+ }
Review Comment:
This code contains extensive duplication with the same `if
(sensitiveArgIndices.isEmpty())` pattern repeated over 20 times. Since
`commandLine` is already built with `buildCommandLine()` which masks sensitive
arguments, you could use it directly in all log statements. This would
eliminate all the conditional branches and significantly reduce code complexity
and maintenance burden. Consider simply using `commandLine` (which is already
sanitized) in all logging statements.
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -361,8 +496,9 @@ public String execute(OutputInterpreter interpreter) {
public String executeIgnoreExitValue(OutputInterpreter interpreter, int
exitValue) {
String[] command = _command.toArray(new String[_command.size()]);
- if (_logger.isDebugEnabled()) {
- _logger.debug(String.format("Executing: %s",
buildCommandLine(command).split(KeyStoreUtils.KS_FILENAME)[0]));
+ if (_logger.isDebugEnabled() && sensitiveArgIndices.isEmpty()) {
Review Comment:
Same issue as in the `execute()` method - when sensitive arguments exist, no
logging occurs even though `buildCommandLine()` already masks them. This
inconsistency could make debugging more difficult. Consider logging the masked
command line instead of suppressing the log entirely.
```suggestion
if (_logger.isDebugEnabled()) {
```
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -261,48 +273,126 @@ public String execute(OutputInterpreter interpreter) {
_thread = Thread.currentThread();
ScheduledFuture<String> future = null;
if (_timeout > 0) {
- _logger.trace(String.format("Scheduling the execution of
command [%s] with a timeout of [%s] milliseconds.", commandLine, _timeout));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format(
+ "Scheduling the execution of command [%s] with a
timeout of [%s] milliseconds.",
+ commandLine, _timeout));
+ } else {
+ _logger.trace(String.format(
+ "Scheduling the execution of command with
sensitive arguments with a timeout of [%s] milliseconds.",
+ _timeout));
+ }
future = s_executors.schedule(this, _timeout,
TimeUnit.MILLISECONDS);
}
long processPid = _process.pid();
Task task = null;
if (interpreter != null && interpreter.drain()) {
- _logger.trace(String.format("Executing interpreting task of
process [%s] for command [%s].", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format("Executing interpreting task
of process [%s] for command [%s].",
+ processPid, commandLine));
+ } else {
+ _logger.trace(String.format(
+ "Executing interpreting task of process [%s] for
command with sensitive arguments.",
+ processPid));
+ }
task = new Task(interpreter, ir);
s_executors.execute(task);
}
while (true) {
- _logger.trace(String.format("Attempting process [%s] execution
for command [%s] with timeout [%s].", processPid, commandLine, _timeout));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format("Attempting process [%s]
execution for command [%s] with timeout [%s].",
+ processPid, commandLine, _timeout));
+ } else {
+ _logger.trace(String.format(
+ "Attempting process [%s] execution for command
with sensitive arguments with timeout [%s].",
+ processPid, _timeout));
+ }
try {
if (_process.waitFor(_timeout, TimeUnit.MILLISECONDS)) {
- _logger.trace(String.format("Process [%s] execution
for command [%s] completed within timeout period [%s].", processPid,
commandLine,
- _timeout));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format(
+ "Process [%s] execution for command [%s]
completed within timeout period [%s].",
+ processPid, commandLine,
+ _timeout));
+ } else {
+ _logger.trace(String.format(
+ "Process [%s] execution for command with
sensitive arguments completed within timeout period [%s].",
+ processPid,
+ _timeout));
+ }
if (_process.exitValue() == 0) {
- _logger.debug(String.format("Successfully executed
process [%s] for command [%s].", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.debug(String.format("Successfully
executed process [%s] for command [%s].",
+ processPid, commandLine));
+ } else {
+ _logger.debug(String.format(
+ "Successfully executed process [%s]
for command with sensitive arguments.",
+ processPid));
+ }
if (interpreter != null) {
if (interpreter.drain()) {
- _logger.trace(String.format("Returning
task result of process [%s] for command [%s].", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(
+ String.format("Returning task
result of process [%s] for command [%s].",
+ processPid,
commandLine));
+ } else {
+ _logger.trace(String.format(
+ "Returning task result of
process [%s] for command with sensitive arguments.",
+ processPid));
+ }
return task.getResult();
}
- _logger.trace(String.format("Returning
interpretation of process [%s] for command [%s].", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(
+ String.format("Returning
interpretation of process [%s] for command [%s].",
+ processPid, commandLine));
+ } else {
+ _logger.trace(String.format(
+ "Returning interpretation of
process [%s] for command with sensitive arguments.",
+ processPid));
+ }
return interpreter.interpret(ir);
} else {
// null return exitValue apparently
- _logger.trace(String.format("Process [%s] for
command [%s] exited with value [%s].", processPid, commandLine,
- _process.exitValue()));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format("Process [%s]
for command [%s] exited with value [%s].",
+ processPid, commandLine,
+ _process.exitValue()));
+ } else {
+ _logger.trace(String.format(
+ "Process [%s] for command with
sensitive arguments exited with value [%s].",
+ processPid,
+ _process.exitValue()));
+ }
Review Comment:
This code contains extensive duplication with the same `if
(sensitiveArgIndices.isEmpty())` pattern repeated over 20 times. Since
`commandLine` is already built with `buildCommandLine()` which masks sensitive
arguments, you could use it directly in all log statements. This would
eliminate all the conditional branches and significantly reduce code complexity
and maintenance burden. Consider simply using `commandLine` (which is already
sanitized) in all logging statements.
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -337,19 +442,49 @@ public String execute(OutputInterpreter interpreter) {
error = String.valueOf(_process.exitValue());
}
- _logger.warn(String.format("Process [%s] for command [%s]
encountered the error: [%s].", processPid, commandLine, error));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.warn(String.format("Process [%s] for command [%s]
encountered the error: [%s].", processPid,
+ commandLine, error));
+ } else {
+ _logger.warn(
+ String.format("Process [%s] for command with sensitive
arguments encountered the error: [%s].",
+ processPid, error));
+ }
Review Comment:
This code contains extensive duplication with the same `if
(sensitiveArgIndices.isEmpty())` pattern repeated over 20 times. Since
`commandLine` is already built with `buildCommandLine()` which masks sensitive
arguments, you could use it directly in all log statements. This would
eliminate all the conditional branches and significantly reduce code complexity
and maintenance burden. Consider simply using `commandLine` (which is already
sanitized) in all logging statements.
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -315,18 +405,33 @@ public String execute(OutputInterpreter interpreter) {
TimedOutLogger log = new TimedOutLogger(_process);
Task timedoutTask = new Task(log, ir);
- _logger.trace(String.format("Running timed out task of process
[%s] for command [%s].", processPid, commandLine));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.trace(String.format("Running timed out task of
process [%s] for command [%s].", processPid,
+ commandLine));
+ } else {
+ _logger.trace(String.format(
+ "Running timed out task of process [%s] for
command with sensitive arguments.",
+ processPid));
+ }
timedoutTask.run();
- if (!_passwordCommand) {
- _logger.warn(String.format("Process [%s] for command [%s]
timed out. Output is [%s].", processPid, commandLine,
timedoutTask.getResult()));
+ if (!_passwordCommand && sensitiveArgIndices.isEmpty()) {
+ _logger.warn(String.format("Process [%s] for command [%s]
timed out. Output is [%s].", processPid,
+ commandLine, timedoutTask.getResult()));
} else {
- _logger.warn(String.format("Process [%s] for command [%s]
timed out.", processPid, commandLine));
+ _logger.warn(
+ String.format("Process [%s] for command with
sensitive arguments timed out.", processPid));
}
return ERR_TIMEOUT;
}
- _logger.debug(String.format("Exit value of process [%s] for
command [%s] is [%s].", processPid, commandLine, _process.exitValue()));
+ if (sensitiveArgIndices.isEmpty()) {
+ _logger.debug(String.format("Exit value of process [%s] for
command [%s] is [%s].", processPid,
+ commandLine, _process.exitValue()));
+ } else {
+ _logger.debug(String.format("Exit value of process [%s] for
command with sensitive arguments is [%s].",
+ processPid, _process.exitValue()));
+ }
Review Comment:
This code contains extensive duplication with the same `if
(sensitiveArgIndices.isEmpty())` pattern repeated over 20 times. Since
`commandLine` is already built with `buildCommandLine()` which masks sensitive
arguments, you could use it directly in all log statements. This would
eliminate all the conditional branches and significantly reduce code complexity
and maintenance burden. Consider simply using `commandLine` (which is already
sanitized) in all logging statements.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
