Copilot commented on code in PR #2764:
URL: https://github.com/apache/karaf/pull/2764#discussion_r3537781200
##########
itests/test/src/test/java/org/apache/karaf/itests/ssh/SshCommandTestBase.java:
##########
@@ -52,41 +54,67 @@ enum Result { OK, NOT_FOUND, NO_CREDENTIALS }
void addUsers(String manageruser, String vieweruser) throws Exception {
ByteArrayOutputStream out = new ByteArrayOutputStream();
OutputStream pipe = openSshChannel("karaf", "karaf", out);
- pipe.write(("jaas:realm-manage --realm=karaf"
+ writeCommandAndWait(pipe, out, "jaas:realm-manage --realm=karaf"
Review Comment:
`writeCommandAndWait()` polls `out.toString()` while the SSHD client is
concurrently writing to the same `ByteArrayOutputStream` (via
`channel.setOut(out)`). `ByteArrayOutputStream` is not thread-safe, so
concurrent reads/writes can intermittently throw (e.g.,
`StringIndexOutOfBoundsException`) or yield corrupted reads, reintroducing
flakiness. Consider using a synchronized `ByteArrayOutputStream` implementation
for all outputs passed to `openSshChannel()` when `writeCommandAndWait()` is
used.
##########
itests/test/src/test/java/org/apache/karaf/itests/ssh/SshCommandTestBase.java:
##########
@@ -52,41 +54,67 @@ enum Result { OK, NOT_FOUND, NO_CREDENTIALS }
void addUsers(String manageruser, String vieweruser) throws Exception {
ByteArrayOutputStream out = new ByteArrayOutputStream();
OutputStream pipe = openSshChannel("karaf", "karaf", out);
- pipe.write(("jaas:realm-manage --realm=karaf"
+ writeCommandAndWait(pipe, out, "jaas:realm-manage --realm=karaf"
+ ";jaas:user-add " + manageruser + " " + manageruser
+ ";jaas:role-add " + manageruser + " manager"
+ ";jaas:role-add " + manageruser + " viewer"
+ ";jaas:role-add " + manageruser + " ssh"
+ ";jaas:user-add " + vieweruser + " " + vieweruser
+ ";jaas:role-add " + vieweruser + " viewer"
+ ";jaas:role-add " + vieweruser + " ssh"
- + ";jaas:update;jaas:realm-manage
--realm=karaf;jaas:user-list\n").getBytes());
- pipe.flush();
+ + ";jaas:update;jaas:realm-manage
--realm=karaf;jaas:user-list");
closeSshChannel(pipe);
System.out.println(new String(out.toByteArray()));
}
void addViewer(String vieweruser) throws Exception {
ByteArrayOutputStream out = new ByteArrayOutputStream();
OutputStream pipe = openSshChannel("karaf", "karaf", out);
- pipe.write(("jaas:realm-manage --realm=karaf"
+ writeCommandAndWait(pipe, out, "jaas:realm-manage --realm=karaf"
Review Comment:
`addViewer()` also passes a non-thread-safe `ByteArrayOutputStream` into
`openSshChannel()`, but `writeCommandAndWait()` now reads it concurrently while
the SSH channel is still writing. Use the same synchronized output stream here
to avoid intermittent concurrency issues.
##########
itests/test/src/test/java/org/apache/karaf/itests/ssh/SshCommandTestBase.java:
##########
@@ -52,41 +54,67 @@ enum Result { OK, NOT_FOUND, NO_CREDENTIALS }
void addUsers(String manageruser, String vieweruser) throws Exception {
ByteArrayOutputStream out = new ByteArrayOutputStream();
OutputStream pipe = openSshChannel("karaf", "karaf", out);
- pipe.write(("jaas:realm-manage --realm=karaf"
+ writeCommandAndWait(pipe, out, "jaas:realm-manage --realm=karaf"
+ ";jaas:user-add " + manageruser + " " + manageruser
+ ";jaas:role-add " + manageruser + " manager"
+ ";jaas:role-add " + manageruser + " viewer"
+ ";jaas:role-add " + manageruser + " ssh"
+ ";jaas:user-add " + vieweruser + " " + vieweruser
+ ";jaas:role-add " + vieweruser + " viewer"
+ ";jaas:role-add " + vieweruser + " ssh"
- + ";jaas:update;jaas:realm-manage
--realm=karaf;jaas:user-list\n").getBytes());
- pipe.flush();
+ + ";jaas:update;jaas:realm-manage
--realm=karaf;jaas:user-list");
closeSshChannel(pipe);
System.out.println(new String(out.toByteArray()));
}
void addViewer(String vieweruser) throws Exception {
ByteArrayOutputStream out = new ByteArrayOutputStream();
OutputStream pipe = openSshChannel("karaf", "karaf", out);
- pipe.write(("jaas:realm-manage --realm=karaf"
+ writeCommandAndWait(pipe, out, "jaas:realm-manage --realm=karaf"
+ ";jaas:user-add " + vieweruser + " " + vieweruser
+ ";jaas:role-add " + vieweruser + " viewer"
+ ";jaas:role-add " + vieweruser + " ssh"
- + ";jaas:update;jaas:realm-manage
--realm=karaf;jaas:user-list\n").getBytes());
- pipe.flush();
+ + ";jaas:update;jaas:realm-manage
--realm=karaf;jaas:user-list");
closeSshChannel(pipe);
System.out.println(new String(out.toByteArray()));
}
- String assertCommand(String user, String command, Result result) throws
Exception {
+ /**
+ * Writes the given command(s) to the SSH channel and blocks until they
have been fully
+ * processed by the remote shell.
+ *
+ * <p>A sentinel {@code echo <marker>} command is appended after the
supplied command and
+ * this method waits until the unique marker shows up in the captured
output. Because the
+ * remote shell reads and executes its input line by line, the marker
cannot appear before
+ * the supplied command has been fully executed and its output flushed
back to the client.
+ * Without this synchronization the SSH session could be torn down (see
+ * {@link #closeSshChannel(OutputStream)}) while the command output is
still in flight,
+ * producing truncated output and spurious assertion failures - regularly
observed on slower
+ * Windows CI runners. {@code echo} is a gogo built-in that is not
restricted by any command
+ * ACL, so it is safe to use for every test user.</p>
+ */
+ private void writeCommandAndWait(OutputStream pipe, ByteArrayOutputStream
out, String command) throws IOException {
if (!command.endsWith("\n"))
command += "\n";
+ pipe.write(command.getBytes());
+ String marker = "KARAF_ITEST_MARKER_" + System.nanoTime();
+ pipe.write(("echo " + marker + "\n").getBytes());
+ pipe.flush();
+ try {
+ Awaitility.await().atMost(60, TimeUnit.SECONDS)
+ .pollInterval(200, TimeUnit.MILLISECONDS)
+ .until(() -> out.toString().contains(marker));
+ } catch (ConditionTimeoutException e) {
+ // Fall through: proceed with whatever output was captured so far.
For assertions
+ // this yields a more useful failure message than the timeout
itself.
+ }
+ }
+
+ String assertCommand(String user, String command, Result result) throws
Exception {
ByteArrayOutputStream out = new ByteArrayOutputStream();
OutputStream pipe = openSshChannel(user, user, out, out);
- pipe.write(command.getBytes());
- pipe.flush();
+ writeCommandAndWait(pipe, out, command);
Review Comment:
`assertCommand()` also uses `writeCommandAndWait()`, which reads the output
stream while SSHD writes to it concurrently. Use the synchronized output stream
here as well to avoid rare concurrency exceptions or partial reads during
Awaitility polling.
##########
itests/test/src/test/java/org/apache/karaf/itests/ssh/SshCommandTestBase.java:
##########
@@ -52,41 +54,67 @@ enum Result { OK, NOT_FOUND, NO_CREDENTIALS }
void addUsers(String manageruser, String vieweruser) throws Exception {
ByteArrayOutputStream out = new ByteArrayOutputStream();
OutputStream pipe = openSshChannel("karaf", "karaf", out);
- pipe.write(("jaas:realm-manage --realm=karaf"
+ writeCommandAndWait(pipe, out, "jaas:realm-manage --realm=karaf"
+ ";jaas:user-add " + manageruser + " " + manageruser
+ ";jaas:role-add " + manageruser + " manager"
+ ";jaas:role-add " + manageruser + " viewer"
+ ";jaas:role-add " + manageruser + " ssh"
+ ";jaas:user-add " + vieweruser + " " + vieweruser
+ ";jaas:role-add " + vieweruser + " viewer"
+ ";jaas:role-add " + vieweruser + " ssh"
- + ";jaas:update;jaas:realm-manage
--realm=karaf;jaas:user-list\n").getBytes());
- pipe.flush();
+ + ";jaas:update;jaas:realm-manage
--realm=karaf;jaas:user-list");
closeSshChannel(pipe);
System.out.println(new String(out.toByteArray()));
}
void addViewer(String vieweruser) throws Exception {
ByteArrayOutputStream out = new ByteArrayOutputStream();
OutputStream pipe = openSshChannel("karaf", "karaf", out);
- pipe.write(("jaas:realm-manage --realm=karaf"
+ writeCommandAndWait(pipe, out, "jaas:realm-manage --realm=karaf"
+ ";jaas:user-add " + vieweruser + " " + vieweruser
+ ";jaas:role-add " + vieweruser + " viewer"
+ ";jaas:role-add " + vieweruser + " ssh"
- + ";jaas:update;jaas:realm-manage
--realm=karaf;jaas:user-list\n").getBytes());
- pipe.flush();
+ + ";jaas:update;jaas:realm-manage
--realm=karaf;jaas:user-list");
closeSshChannel(pipe);
System.out.println(new String(out.toByteArray()));
}
- String assertCommand(String user, String command, Result result) throws
Exception {
+ /**
+ * Writes the given command(s) to the SSH channel and blocks until they
have been fully
+ * processed by the remote shell.
+ *
+ * <p>A sentinel {@code echo <marker>} command is appended after the
supplied command and
+ * this method waits until the unique marker shows up in the captured
output. Because the
+ * remote shell reads and executes its input line by line, the marker
cannot appear before
+ * the supplied command has been fully executed and its output flushed
back to the client.
+ * Without this synchronization the SSH session could be torn down (see
+ * {@link #closeSshChannel(OutputStream)}) while the command output is
still in flight,
+ * producing truncated output and spurious assertion failures - regularly
observed on slower
+ * Windows CI runners. {@code echo} is a gogo built-in that is not
restricted by any command
+ * ACL, so it is safe to use for every test user.</p>
+ */
+ private void writeCommandAndWait(OutputStream pipe, ByteArrayOutputStream
out, String command) throws IOException {
if (!command.endsWith("\n"))
command += "\n";
+ pipe.write(command.getBytes());
+ String marker = "KARAF_ITEST_MARKER_" + System.nanoTime();
+ pipe.write(("echo " + marker + "\n").getBytes());
+ pipe.flush();
+ try {
+ Awaitility.await().atMost(60, TimeUnit.SECONDS)
+ .pollInterval(200, TimeUnit.MILLISECONDS)
+ .until(() -> out.toString().contains(marker));
+ } catch (ConditionTimeoutException e) {
+ // Fall through: proceed with whatever output was captured so far.
For assertions
+ // this yields a more useful failure message than the timeout
itself.
+ }
Review Comment:
Swallowing `ConditionTimeoutException` can hide cases where the remote shell
never processes the command(s) (or the marker never makes it back). In setup
flows like `addUsers()`/`addViewer()`, continuing after the timeout risks
follow-up failures that are harder to diagnose. Consider failing fast with an
error that includes the partial output captured so far.
--
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]
