Skip to content

[#1074] Keep what says the connection is gone through the redaction of a connect failure #1107

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

Open
vharseko wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[#1074] Keep what says the connection is gone through the redaction of a connect failure #1107

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
117 changes: 104 additions & 13 deletions opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,8 @@ static long nextBackoffMs(long backoffMs) {
* How many links of the cause and getNextException() chains of a failure the two walks that
* have a reason to stop look at: {@link #holdsCredentials} answers "yes" past it, since the
* cost of the other answer is the password of the backend in the log, and
* {@link #redactedCopy} rebuilds that far and names the rest in one link. The walk whose verdict
* {@link #redactedCopy} rebuilds that far and names the rest in one link - carrying, where the
* rest says the connection is gone, what says it (#1074). The walk whose verdict
* decides something, {@link #isWorthRetrying}, is not one of them: a count does not leave that
* question unanswered, it answers it with the verdict of a failure that carries nothing
* (issue #1076), and the visited set of every walk here terminates it on its own.
Expand Down Expand Up @@ -2135,8 +2136,11 @@ static String stallMessage(String connectionString, int attempts, long waitedMs,
* prints a failure prints the causes along with it - a debug build of
* stackTraceToSingleLineString walks them, the config manager traces them, and
* RootContainer.open() makes the message of the cause the message of what it throws - so a
* link left as it stands would carry the password past the wrapper. The SQLState and the
* vendor code of every link survive it: they are what tells a caller what happened.
* link left as it stands would carry the password past the wrapper. The SQLState, the vendor
* code and the standard JDBC type of every link survive it: they are what tells a caller what
* happened, and JDBCStorage.write() asks the type before the state whether the connection is
* gone. Past the budget of the rebuild the one link standing for the rest says that much of it
* where the rest says so (#1074).
* <p>
* The whole chain is the causes, the further exceptions of {@code getNextException()} and what
* was suppressed on each of them - the last being where this class puts the failure of a close
Expand Down Expand Up @@ -2223,40 +2227,126 @@ private static void enqueue(Deque<Throwable> pending, Set<Throwable> visited, Th
private static SQLException redactedCopy(SQLException e, String connectionString, int[] budget) {
budget[0]--;
final SQLException copy =
new SQLException(redact(e.getMessage(), connectionString), e.getSQLState(), e.getErrorCode());
sameKind(e, redact(e.getMessage(), connectionString), e.getSQLState(), e.getErrorCode());
copy.setStackTrace(e.getStackTrace());
if (e.getNextException() != null) {
copy.setNextException(budget[0] > 0
? redactedCopy(e.getNextException(), connectionString, budget) : droppedTail());
? redactedCopy(e.getNextException(), connectionString, budget)
: droppedTail(Collections.<Throwable>singletonList(e.getNextException())));
}
if (e.getCause() != null) {
copy.initCause(budget[0] > 0 ? redactedLink(e.getCause(), connectionString, budget) : droppedTail());
copy.initCause(budget[0] > 0 ? redactedLink(e.getCause(), connectionString, budget)
: droppedTail(Collections.singletonList(e.getCause())));
}
copySuppressed(e, copy, connectionString, budget);
return copy;
}

/**
* A new exception of the kind JDBC names the given one as. Not the class of the driver itself,
* which this cannot be sure of building, but the standard type it extends: that type is what
* the JDBC contract gives a driver to say what happened beside the SQLState, and JDBCStorage
* reads it first - a SQLRecoverableException says the connection is gone whatever state it
* carries, and a copy made as a plain SQLException says nothing of the kind (#1074).
*/
private static SQLException sameKind(SQLException e, String message, String sqlState, int vendorCode) {
if (e instanceof SQLRecoverableException) {
return new SQLRecoverableException(message, sqlState, vendorCode);
}
if (e instanceof SQLNonTransientConnectionException) {
return new SQLNonTransientConnectionException(message, sqlState, vendorCode);
}
if (e instanceof SQLTransientConnectionException) {
return new SQLTransientConnectionException(message, sqlState, vendorCode);
}
if (e instanceof SQLTimeoutException) {
return new SQLTimeoutException(message, sqlState, vendorCode);
}
if (e instanceof SQLTransactionRollbackException) {
return new SQLTransactionRollbackException(message, sqlState, vendorCode);
}
if (e instanceof SQLFeatureNotSupportedException) {
return new SQLFeatureNotSupportedException(message, sqlState, vendorCode);
}
if (e instanceof SQLIntegrityConstraintViolationException) {
return new SQLIntegrityConstraintViolationException(message, sqlState, vendorCode);
}
if (e instanceof SQLInvalidAuthorizationSpecException) {
return new SQLInvalidAuthorizationSpecException(message, sqlState, vendorCode);
}
if (e instanceof SQLSyntaxErrorException) {
return new SQLSyntaxErrorException(message, sqlState, vendorCode);
}
if (e instanceof SQLDataException) {
return new SQLDataException(message, sqlState, vendorCode);
}
if (e instanceof SQLTransientException) {
return new SQLTransientException(message, sqlState, vendorCode);
}
if (e instanceof SQLNonTransientException) {
return new SQLNonTransientException(message, sqlState, vendorCode);
}
return new SQLException(message, sqlState, vendorCode);
}

// The suppressed links of a failure are rebuilt with the rest of it and counted against the
// same budget. They are not decoration here: the failure of a close that could not be made
// rides on the failure being unwound (establish(), #929), and a rebuild that dropped them
// would lose it at the one deployment whose log is redacted - which is the deployment whose
// url has a password in it, the log that is worth reading.
private static void copySuppressed(Throwable from, Throwable to, String connectionString, int[] budget) {
for (final Throwable suppressed : from.getSuppressed()) {
final Throwable[] suppressed = from.getSuppressed();
for (int i = 0; i < suppressed.length; i++) {
if (budget[0] <= 0) {
to.addSuppressed(droppedTail());
to.addSuppressed(droppedTail(Arrays.asList(suppressed).subList(i, suppressed.length)));
return;
}
to.addSuppressed(redactedLink(suppressed, connectionString, budget));
to.addSuppressed(redactedLink(suppressed[i], connectionString, budget));
}
}

// What stands where the budget ran out. Without it the same failure logs its root cause when
// the url of the backend has no password in it and loses it without a word when it has, which
// is a report of a connect nobody can read against a report of one they can.
private static SQLException droppedTail() {
return new SQLException("the rest of this failure was left out: a chain of more than "
+ MAX_CHAIN_LENGTH + " links is rebuilt only that far");
// It also says what the rest says about the connection, where the rest says it is gone: that
// is a verdict JDBCStorage.write() reads off every link of the failure - it replays the attempt
// and distrusts the pool on it - and a tail saying nothing would answer it with the "no" of a
// failure that carries nothing (#1074, the #961 thesis on the budget of this rebuild). Only
// that, and only where the rest does say it, so that the cut makes no failure say it either.
private static SQLException droppedTail(List<Throwable> rest) {
final String message = "the rest of this failure was left out: a chain of more than "
+ MAX_CHAIN_LENGTH + " links is rebuilt only that far";
final SQLException gone = firstLinkSayingTheConnectionIsGone(rest);
return gone == null
? new SQLException(message)
: sameKind(gone, message + "; a link of it says the connection is gone", gone.getSQLState(),
gone.getErrorCode());
}

// Every link of the rest, by the edges JDBCStorage.isConnectionFailure() walks - the causes, the
// further exceptions and what was suppressed - and to its end: the visited set terminates it, and
// it builds nothing, so what it costs is a walk of the links the driver has already allocated.
private static SQLException firstLinkSayingTheConnectionIsGone(List<Throwable> rest) {
final Deque<Throwable> pending = new ArrayDeque<>();
final Set<Throwable> visited = Collections.newSetFromMap(new IdentityHashMap<Throwable, Boolean>());
for (final Throwable t : rest) {
enqueue(pending, visited, t);
}
while (!pending.isEmpty()) {
final Throwable t = pending.poll();
if (t instanceof SQLException) {
final SQLException sql = (SQLException) t;
if (JDBCStorage.saysTheConnectionIsGone(sql)) {
return sql;
}
enqueue(pending, visited, sql.getNextException());
}
enqueue(pending, visited, t.getCause());
for (final Throwable suppressed : t.getSuppressed()) {
enqueue(pending, visited, suppressed);
}
}
return null;
}

// A link that is no SQLException keeps its class name in the message: its type is not one this
Expand All @@ -2270,7 +2360,8 @@ private static Throwable redactedLink(Throwable t, String connectionString, int[
+ (t.getMessage() == null ? "" : ": " + redact(t.getMessage(), connectionString)));
copy.setStackTrace(t.getStackTrace());
if (t.getCause() != null) {
copy.initCause(budget[0] > 0 ? redactedLink(t.getCause(), connectionString, budget) : droppedTail());
copy.initCause(budget[0] > 0 ? redactedLink(t.getCause(), connectionString, budget)
: droppedTail(Collections.singletonList(t.getCause())));
}
copySuppressed(t, copy, connectionString, budget);
return copy;
Expand Down
6 changes: 5 additions & 1 deletion opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4924,8 +4924,12 @@ private static void walkLinks(Throwable failure, boolean withTheRelease, int lin
* a driver that raises one of them has said so whatever state it filled in. Oracle reports ORA-03113, ORA-00028
* and ORA-01089 as {@link SQLRecoverableException} and happens to map them to 08006 as well; the type is what
* makes that robust rather than lucky.
* <p>
* Package-private for the one other reader it has, the redaction of a connect failure in {@link
* CachedConnection}: what that rebuild leaves in place of a chain past its budget has to say what this reads
* off the links it cuts (#1074), and a second copy of the question would drift from this one.
*/
private static boolean saysTheConnectionIsGone(SQLException e) {
static boolean saysTheConnectionIsGone(SQLException e) {
if (e instanceof SQLRecoverableException || e instanceof SQLNonTransientConnectionException
|| e instanceof SQLTransientConnectionException) {
return true;
Expand Down
69 changes: 69 additions & 0 deletions ...server-legacy/src/test/java/org/opends/server/backends/jdbc/CachedConnectionTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,10 @@
import java.sql.DriverPropertyInfo;
import java.sql.PreparedStatement;
import java.sql.SQLException;
import java.sql.SQLNonTransientConnectionException;
import java.sql.SQLRecoverableException;
import java.sql.SQLTimeoutException;
import java.sql.SQLTransientConnectionException;
import java.util.ArrayDeque;
import java.util.Collections;
import java.util.Deque;
Expand Down Expand Up @@ -941,6 +944,72 @@ public void testTheTailOfALongChainIsNamedRatherThanDropped() throws Exception {
"the tail a rebuild had no budget for has to say so: " + last.getMessage());
}

/**
* A link that says the connection is gone by its type alone still says so once it is rebuilt
* (#1074). The type is what the JDBC contract gives a driver to say it - write() asks it before
* the SQLState - so a copy made as a plain SQLException reads as a failure that says nothing of
* the connection, and only at the deployment whose url has a password in it.
*/
@Test(timeOut = 60000)
public void testALinkThatSaysTheConnectionIsGoneByItsTypeKeepsItThroughRedaction() throws Exception {
final String url = "jdbc:postgresql://opendj:S3cretOfTheBackend@127.0.0.1:5432/opendj";
for (final SQLException gone : new SQLException[] { new SQLRecoverableException("io error"),
new SQLNonTransientConnectionException("closed"), new SQLTransientConnectionException("reset") }) {
final SQLException failure = new SQLException("login to " + url + " failed", "S0001", 18456);
failure.setNextException(gone);

final SQLException reported = CachedConnection.reported(failure, url);

assertNoCredentials(reported);
assertTrue(JDBCStorage.isConnectionFailure(reported),
"a rebuilt " + gone.getClass().getSimpleName() + " no longer says the connection is gone");
}
}

/**
* ... and so does one standing past the budget of the rebuild, whatever it says it by: the link
* that stands for the rest of the chain there is all that the classification of write() gets to
* read of it. The url has no password, and the chain is rebuilt all the same - it is longer than
* the walk looking for credentials, which answers "yes" past it.
*/
@Test(timeOut = 60000)
public void testALinkThatSaysTheConnectionIsGonePastTheBudgetOfARebuildStillSaysSo() throws Exception {
final String url = "jdbc:postgresql://127.0.0.1:5432/opendj";
for (final SQLException gone : new SQLException[] { new SQLException("connection reset", "08S01", 10054),
new SQLRecoverableException("io error") }) {
final SQLException failure = plainChain(40);
failure.setNextException(gone);

final SQLException reported = CachedConnection.reported(failure, url);

assertTrue(reported != failure, "a chain this long is expected to be rebuilt");
assertTrue(JDBCStorage.isConnectionFailure(reported),
gone + " past the budget of the rebuild no longer says the connection is gone");
}
}

/**
* The link standing for the rest says only what the rest says: a chain that says nothing of the
* connection is not made to say it is gone by being cut. That would replay the write and distrust
* the pool over a database that refused a connection for a reason of its own.
*/
@Test(timeOut = 60000)
public void testALongChainThatSaysNothingOfTheConnectionIsNotMadeToSayItByTheRebuild() throws Exception {
final SQLException reported = CachedConnection.reported(plainChain(40), "jdbc:postgresql://127.0.0.1:5432/opendj");

assertFalse(JDBCStorage.isConnectionFailure(reported),
"the rebuild made a failure that says nothing of the connection say it is gone");
}

/** That many plain links of the next exception chain, none of which says the connection is gone. */
private static SQLException plainChain(int links) {
final SQLException head = new SQLException("error 0", "S0001", 1);
for (int i = 1; i < links; i++) {
head.setNextException(new SQLException("error " + i, "S0001", 1));
}
return head;
}

/**
* A credential named by a suppressed link alone is redacted like any other. The close of a
* connection whose set-up failed rides there (establish(), #929) and an interrupt that ended a
Expand Down
58 changes: 58 additions & 0 deletions ...erver-legacy/src/test/java/org/opends/server/backends/jdbc/CatalogConnectionTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,6 +685,64 @@ public void testAConnectionWhoseSetUpFailsIsClosed() throws Exception {
verify(failing).close();
}

/**
* A refusal that says the connection is gone reaches {@code write()} saying so, however deep in its
* chain the driver put the link that says it (#1074). The failure leaves this connect redacted, and
* a chain longer than the walk looking for credentials is rebuilt whether or not anything in it names
* them - this url has no password at all - so what the rebuild leaves past its budget is what {@code
* isConnectionFailure()} reads: the attempt is replayed where the same refusal a few links shorter is.
*/
@Test
public void testARefusalThatSaysTheConnectionIsGoneDeepInItsChainStillSaysSo() throws Exception {
final SQLException refusal = deepChain(40, new SQLException("connection reset", "08S01", 10054));
probeDriver.refusal = refusal;
probeDriver.refusalsLeft.set(Integer.MAX_VALUE);
try {
storageFor(ProbeDriver.URL).newCatalogConnection(NO_REPLAY_WINDOW, DEFAULT_BOUND, 60);
fail("a connect that will not clear was retried instead of being reported");
} catch (SQLException expected) {
assertTrue(JDBCStorage.isConnectionFailure(refusal), "the refusal of the driver says the connection is gone");
assertTrue(JDBCStorage.isConnectionFailure(expected),
"the refusal as it left the connect lost what said the connection is gone: " + expected);
}
}

/**
* ... and so does the timeout a refusal worth waiting out ends in, whose cause is that refusal
* redacted the same way: the retry changes no classification.
*/
@Test
public void testATimedOutRefusalThatSaysTheConnectionIsGoneDeepInItsChainStillSaysSo() throws Exception {
final SQLException refusal = new SQLException("too many clients already", "53300");
refusal.setNextException(deepChain(40, new SQLException("connection reset", "08S01", 10054)));
probeDriver.refusal = refusal;
probeDriver.refusalsLeft.set(Integer.MAX_VALUE);
try {
storageFor(ProbeDriver.URL).newCatalogConnection(NO_REPLAY_WINDOW, DEFAULT_BOUND, 1);
fail("a connect refused for the whole deadline was not given up on");
} catch (SQLTimeoutException expected) {
assertTrue(JDBCStorage.isConnectionFailure(refusal), "the refusal of the driver says the connection is gone");
assertTrue(JDBCStorage.isConnectionFailure(expected),
"the timeout lost what its last refusal said about the connection: " + expected);
}
}

/**
* A driver's failure the way mssql-jdbc chains every error of a message it received: that many
* plain links of the next exception chain, and the given one last.
*/
private static SQLException deepChain(int links, SQLException last) {
final SQLException head = new SQLException("error 0 of the login", "S0001", 1);
SQLException tail = head;
for (int i = 1; i < links; i++) {
final SQLException next = new SQLException("error " + i + " of the login", "S0001", 1);
tail.setNextException(next);
tail = next;
}
tail.setNextException(last);
return head;
}

/** What the dialect of this url declares, at the given number of seconds. */
private static void assertBoundedAt(Properties handed, long seconds) {
assertNotNull(handed, "no properties were handed to the driver at all");
Expand Down
Loading