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

turing85 · 2023-10-27T14:45:10Z

Resolves #30491

mmusgrov

Thanks

turing85 · 2023-10-27T15:51:16Z

@mmusgrov you're welcome 🙂

mmusgrov

Since approving the PR I've received some concerns about this change. The requests are:

to change the log message from info to warn
to ask users to opt into the change via a config property that controls whether or not shortening of the node id should be allowed

In addition, we have a similar problem in WildFly and Narayana so I will start a discussion in the narayana-users forum about adding the feature to Narayana so that all runtimes can benefit from the change.

turing85 · 2023-10-30T23:13:45Z

Since approving the PR I've received some concerns about this change. The requests are:
* to change the log message from info to warn

* to ask users to opt into the change via a config property that controls whether or not shortening of the node id should be allowed
In addition, we have a similar problem in WildFly and Narayana so I will start a discussion in the narayana-users forum about adding the feature to Narayana so that all runtimes can benefit from the change.

So... should I implement those changes, or do we want to wait for the result of the discussion?

mmusgrov · 2023-10-31T10:13:15Z

So... should I implement those changes, or do we want to wait for the result of the discussion?

Personally I'd say that the only one up for debate is the default value for the property. If the default is to fail the boot, the current behaviour, then the user will detect the issue during the commissioning phase so I'd maybe lean a little in that direction, besides, it would minimise surprise for existing users. Let's give people a couple of days to respond and if nothing is forthcoming then we should decide.

marcosgopen · 2023-10-31T14:10:47Z

So... should I implement those changes, or do we want to wait for the result of the discussion?

Personally I'd say that the only one up for debate is the default value for the property. If the default is to fail the boot, the current behaviour, then the user will detect the issue during the commissioning phase so I'd maybe lean a little in that direction, besides, it would minimise surprise for existing users. Let's give people a couple of days to respond and if nothing is forthcoming then we should decide.

I agree with Mike. I think that the default behaviour of the property is to fail the boot. If user want to change this behaviour they need to change the value of the property, in this way they will be aware that the 'node name' might change (when longer than 28 bytes) from the expected one because it will be shorten with SHA-224.

… with SHA-224

turing85 · 2023-10-31T18:39:52Z

@mmusgrov I added the changes (they were minimal). The property name is of course up to debate, but the change is trivial.

quarkus-bot · 2023-10-31T21:19:38Z

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

mmusgrov

Looks great.

@turing85 There were no comments on the message that I posted to the narayana google users group so I think this is ready for merging.

turing85 · 2023-11-02T12:21:27Z

Looks great.

@turing85 There were no comments on the message that I posted to the narayana google users group so I think this is ready for merging.

Okay, then all that is left to do is an approval (github still says that merging is blocked and that an approval is needed). @maxandersen Can you approve this one?

gsmet · 2023-11-06T08:51:28Z

Thanks!

gastaldi · 2023-11-06T08:52:58Z

...a/runtime/src/main/java/io/quarkus/narayana/jta/runtime/TransactionManagerConfiguration.java

+     * @see #nodeName
+     */
+    @ConfigItem(defaultValue = "false")
+    public boolean shortenNodeNameIfNecessary;


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

How about making it true by default as a precaution?

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?

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?

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

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.

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.

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.

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.

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

quarkus-bot bot added the area/narayana Transactions / Narayana label Oct 27, 2023

mmusgrov approved these changes Oct 27, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

mmusgrov suggested changes Oct 30, 2023

View reviewed changes

rsvoboda mentioned this pull request Oct 31, 2023

Maven Tests on JDK 11 Windows get stuck for 3.2 branch #36797

Closed

quarkusio#30491: If the node name is longer than 28 bytes, shorten it…

07b3a4c

… with SHA-224

turing85 force-pushed the feature/30491-shorten-node-name-if-necessary branch from e29d3a8 to 07b3a4c Compare October 31, 2023 18:39

turing85 requested a review from mmusgrov October 31, 2023 20:04

mmusgrov approved these changes Nov 2, 2023

View reviewed changes

gastaldi approved these changes Nov 6, 2023

View reviewed changes

gsmet merged commit 3eda9e4 into quarkusio:main Nov 6, 2023
41 checks passed

quarkus-bot bot added the kind/enhancement New feature or request label Nov 6, 2023

quarkus-bot bot added this to the 3.6 - main milestone Nov 6, 2023

gastaldi reviewed Nov 6, 2023

View reviewed changes

turing85 deleted the feature/30491-shorten-node-name-if-necessary branch March 6, 2024 20:40

marcosgopen mentioned this pull request May 13, 2024

Ensure the Transaction Manager node name is less than or equal to 28 bytes #40613

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

turing85 commented Oct 27, 2023

mmusgrov left a comment

turing85 commented Oct 27, 2023

This comment has been minimized.

mmusgrov left a comment

turing85 commented Oct 30, 2023

mmusgrov commented Oct 31, 2023

marcosgopen commented Oct 31, 2023 •

edited

Loading

turing85 commented Oct 31, 2023

quarkus-bot bot commented Oct 31, 2023

mmusgrov left a comment

turing85 commented Nov 2, 2023

gsmet commented Nov 6, 2023

gastaldi Nov 6, 2023 •

edited

Loading

gastaldi Nov 6, 2023

maxandersen Nov 6, 2023

maxandersen Nov 6, 2023

gastaldi Nov 6, 2023

turing85 Nov 6, 2023 •

edited

Loading

turing85 Nov 6, 2023

mmusgrov Nov 8, 2023

maxandersen Nov 8, 2023

mmusgrov Nov 8, 2023 •

edited

Loading

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

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

Conversation

turing85 commented Oct 27, 2023

mmusgrov left a comment

Choose a reason for hiding this comment

turing85 commented Oct 27, 2023

This comment has been minimized.

mmusgrov left a comment

Choose a reason for hiding this comment

turing85 commented Oct 30, 2023

mmusgrov commented Oct 31, 2023

marcosgopen commented Oct 31, 2023 • edited Loading

turing85 commented Oct 31, 2023

quarkus-bot bot commented Oct 31, 2023

mmusgrov left a comment

Choose a reason for hiding this comment

turing85 commented Nov 2, 2023

gsmet commented Nov 6, 2023

gastaldi Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

gastaldi Nov 6, 2023

Choose a reason for hiding this comment

maxandersen Nov 6, 2023

Choose a reason for hiding this comment

maxandersen Nov 6, 2023

Choose a reason for hiding this comment

gastaldi Nov 6, 2023

Choose a reason for hiding this comment

turing85 Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

turing85 Nov 6, 2023

Choose a reason for hiding this comment

mmusgrov Nov 8, 2023

Choose a reason for hiding this comment

maxandersen Nov 8, 2023

Choose a reason for hiding this comment

mmusgrov Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

marcosgopen commented Oct 31, 2023 •

edited

Loading

gastaldi Nov 6, 2023 •

edited

Loading

turing85 Nov 6, 2023 •

edited

Loading

mmusgrov Nov 8, 2023 •

edited

Loading