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.

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

If the node name is longer than 28 bytes, shorten it with SHA-224 #36752

Merged
merged 1 commit into from
Nov 6, 2023
Merged

If the node name is longer than 28 bytes, shorten it with SHA-224 #36752

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
22 changes: 20 additions & 2 deletions ...rayana-jta/runtime/src/main/java/io/quarkus/narayana/jta/runtime/NarayanaJtaRecorder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package io.quarkus.narayana.jta.runtime;

import java.lang.reflect.Field;
import java.nio.charset.StandardCharsets;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -29,6 +32,7 @@

@Recorder
public class NarayanaJtaRecorder {
public static final String HASH_ALGORITHM_FOR_SHORTENING = "SHA-224";

private static Properties defaultProperties;

Expand All @@ -37,14 +41,28 @@ public class NarayanaJtaRecorder {
public void setNodeName(final TransactionManagerConfiguration transactions) {

try {
if (transactions.nodeName.getBytes(StandardCharsets.UTF_8).length > 28
&& transactions.shortenNodeNameIfNecessary) {
shortenNodeName(transactions);
}
arjPropertyManager.getCoreEnvironmentBean().setNodeIdentifier(transactions.nodeName);
jtaPropertyManager.getJTAEnvironmentBean().setXaRecoveryNodes(Collections.singletonList(transactions.nodeName));
TxControl.setXANodeName(transactions.nodeName);
} catch (CoreEnvironmentBeanException e) {
e.printStackTrace();
} catch (CoreEnvironmentBeanException | NoSuchAlgorithmException e) {
log.error("Could not set node name", e);
}
}

private static void shortenNodeName(TransactionManagerConfiguration transactions) throws NoSuchAlgorithmException {
String originalNodeName = transactions.nodeName;
log.warnf("Node name \"%s\" is longer than 28 bytes, shortening it by using %s.", originalNodeName,
HASH_ALGORITHM_FOR_SHORTENING);
final byte[] nodeNameAsBytes = originalNodeName.getBytes();
MessageDigest messageDigest224 = MessageDigest.getInstance(HASH_ALGORITHM_FOR_SHORTENING);
transactions.nodeName = new String(messageDigest224.digest(nodeNameAsBytes), StandardCharsets.UTF_8);
log.warnf("New node name is \"%s\"", transactions.nodeName);
}

public void setDefaultProperties(Properties properties) {
//TODO: this is a huge hack to avoid loading XML parsers
//this needs a proper SPI
Expand Down
14 changes: 14 additions & 0 deletions ...untime/src/main/java/io/quarkus/narayana/jta/runtime/TransactionManagerConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,24 @@
public final class TransactionManagerConfiguration {
/**
* The node name used by the transaction manager.
* Must not exceed a length of 28 bytes.
*
* @see #shortenNodeNameIfNecessary
*/
@ConfigItem(defaultValue = "quarkus")
public String nodeName;

/**
* Whether the node name should be shortened if necessary.
* The node name must not exceed a length of 28 bytes. If this property is set to {@code true}, and the node name exceeds 28
* bytes, the node name is shortened by calculating the <a href="https://en.wikipedia.org/wiki/SHA-2">SHA-224</a> hash,
* which has a length of 28 bytes.
*
* @see #nodeName
*/
@ConfigItem(defaultValue = "false")
public boolean shortenNodeNameIfNecessary;
Copy link
Contributor

@gastaldi gastaldi Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

How about naming it truncateNodeName or simply shortenNodeName? The ifNecessary makes my chin itch a bit 😀

Copy link
Contributor

Choose a reason for hiding this comment

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

How about making it true by default as a precaution?

Copy link
Member

Choose a reason for hiding this comment

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

this is not truncation. Truncate is rather abrupt/finite in nature.

maybe compress as this will be a lossy-but-hopefully-not-significant-compression ?

btw. why the "ifNecessary" part ? if its necessary why is it not true by default?

Copy link
Member

Choose a reason for hiding this comment

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

i.e. since this seems necessary I would argue it should be default on and possible to turn off. At least in main with good release note metnion - if this was for LTS branch I could see how one could argue it should stay off.

Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

this is not truncation. Truncate is rather abrupt/finite in nature.

maybe compress as this will be a lossy-but-hopefully-not-significant-compression ?

Right, compress makes more sense

btw. why the "ifNecessary" part ? if its necessary why is it not true by default?

+1

Copy link
Contributor Author

@turing85 turing85 Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

How about naming it truncateNodeName or simply shortenNodeName? The ifNecessary makes my chin itch a bit 😀

@gastaldi @maxandersen
"if necessary" because the shortening takes play if and only if the node-name is longer thank 28 bytes. I know that it is unwieldy. This is why I explicitly said the name is up for discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it's less unlikely to happen, but would truncating be a problem if the algorithm isn't available?

Hard to say. This depends how the node names are constructed. If one has long names and a common prefix, then this might lead to name duplication.

Copy link
Contributor

Choose a reason for hiding this comment

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

btw. why the "ifNecessary" part ? if its necessary why is it not true by default?

Historically the default is to fail startup if the nodeIdentifier, set by the user or the platform, is too long. Since the use of a hash incurs the risk of collisions and can result in non ACID behaviour it should not be the default - the user has to opt in to the added risk of using a hash since only they can quantify the risk to their application.

Copy link
Member

Choose a reason for hiding this comment

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

ok, how about simply "hashNodeNameIfNecessary" or simply "hashNodeName" which still will only happen if needed but with hash it at least signals what happens and collision awareness could be necessary.

Copy link
Contributor

@mmusgrov mmusgrov Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

"hashNodeName" sounds reasonable, the docs will let the user know what effect it will have.


/**
* The default transaction timeout.
*/
Expand Down
Loading