Ensure the Transaction Manager node name is less than or equal to 28 bytes #40613
Conversation
|
I added a comment to the issue indicating that the spring boot port should just avoid using the UTF-8 encoding and use ISO-LATIN-1 instead. This would mean the original code submitted by @turing85 would stand. |
|
Closed as not a bug |
.../narayana-jta/runtime/src/main/java/io/quarkus/narayana/jta/runtime/NarayanaJtaRecorder.java
Outdated
Show resolved
Hide resolved
|
This is going to need a better title |
marcosgopen
changed the title
This comment has been minimized.
This comment has been minimized.
geoand
requested review from
mmusgrov and
yrodiere
and removed request for
mmusgrov
.../narayana-jta/runtime/src/main/java/io/quarkus/narayana/jta/runtime/NarayanaJtaRecorder.java
Outdated
Show resolved
Hide resolved
| MessageDigest messageDigest224 = MessageDigest.getInstance(HASH_ALGORITHM_FOR_SHORTENING); | ||
| transactions.nodeName = new String(messageDigest224.digest(nodeNameAsBytes), StandardCharsets.UTF_8); |
There was a problem hiding this comment.
I get that your problem was with the length of the resulting string, but it seems to me there was an equally important problem: given the content of the hash is arbitrary, this particular line could throw exceptions due to malformed input... not all byte arrays are valid in UTF-8. See java.lang.String#decodeUTF8_UTF16 for example. So it seems to me base64 encoding is necessary regardless of length issues.
Even without that, you could (and likely would) end up with non-printable characters in your node name... Though from what I read elsewhere it seems the node name gets re-hashed afterwards, so I suppose Narayana takes care of making it human-readable.
There was a problem hiding this comment.
Thanks @yrodiere for your analysis. I would like to clarify some points (I hope we all agree about them):
- only Strings longer then 28 bytes are shorten by the following process.
- the original string has an arbitrary length ( >28 bytes), then it is hashed and it's output is 28 bytes (SHA-224). This is the only hash used in this process.
- Base64 encoding is done after the byte array is hashed.
- the base64 encoded byte array has an arbitrary length and so it is truncate so that it is a 28 bytes array
- Using the Base64 encoding for the byte array the resulting Strings are printable, are utf-8 valid and can be easily encoded to UTF-8 without changing the length
There was a problem hiding this comment.
Shouldn't this also be documented in the JavaDoc of
?There was a problem hiding this comment.
Btw. I heard about different numbers that are actually known limits. 28 for Narayana itself and 23 for usage under Kubernetes. Would it make sense to make the actual algorithm more configurable. E.g. a number property with valid range from 1 to 28 and 28 as default. Maybe even replace TransactionManagerConfiguration.shortenNodeNameIfNecessary and let the default of the new property be 0 to signal no shortening at all.
There was a problem hiding this comment.
I will update the JavaDoc, thanks @graben . Concerning having a parameter to configure the length of the string I think it might be risky because the hash collision would be directly impacted when the string length is changed. So we would need to make sure that the users know exactly about the impact of that configuration's change. However we need to remember that only string with byte array's length > 28 are shorten, other strings are not. So when the users have a strict limitation about the string length (e.g. for Kubernetes 23 as you said) they can pass 23 bytes string and that string will not be changed.
There was a problem hiding this comment.
Well, doesn't the same argument belong to 28, too? If 28 is the allowed max, disallow greater values and force user to shrink the config? :-)
There was a problem hiding this comment.
Good point @graben. Yes, it does. But I would say that the collision avoidance with 28 bytes is still acceptable (see the stress test) IMO, but it probably would not with a random number of bytes between 1 to 27. So again I am OK to make that number configurable but I would recommend anyway to shorten the hash with the longest string allowed (which is 28).
There was a problem hiding this comment.
IMHO, the question is if 28 is the maximum allowed by Narayana, why offer an optional property to shorten values that are obviously wrong at all? Shouldn't Quarkus disallow values larger than 28 and force users to shorten themselves? No potential problems with collisions anymore! I know it's a bit of a radical solution, but it feels odd that users can define invalid properties and the software should solve it with a workaround with potential drawbacks. Just my 2 cents.
There was a problem hiding this comment.
Thanks @graben for your comment here. I am not sure I am following you here. You first were talking about adding an extra parameter for the length of the node name, now you are talking about removing this 'Generate right-length node name' feature (the one discussed in the issue #30491 ). I might be wrong but this PR's goal is to patch the existing implementation, not to change it. Could I maybe ask you to discuss this in a different issue or on Zulip where it is easier to get the community's feedback about this topic?
Getting back to your comment I would like to underline that the default behaviour is the one that you described: in fact the 'shortenNodeNameIfNecessary' variable is set to false by default (see https://github.com/quarkusio/quarkus/blob/main/extensions/narayana-jta/runtime/src/main/java/io/quarkus/narayana/jta/runtime/TransactionManagerConfiguration.java#L34), the original reasons to implement this feature are discussed in the issue #30491 and zulip topic https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/manipulate.20environment.20variables.20in.20configuration.20properties. We had a discussion about it also in narayana-users group https://groups.google.com/g/narayana-users/c/ttSff9HvXdA.
There was a problem hiding this comment.
Keep it as is, it just came into my mind, while following the process involving from the very first beginning. ;-)
marcosgopen
force-pushed
the
node-name
branch
2 times, most recently
from
0a2ce53
to
a595636
Compare
...ayana-jta/runtime/src/test/java/io/quarkus/narayana/jta/runtime/NarayanaJtaRecorderTest.java
Outdated
Show resolved
Hide resolved
...ayana-jta/runtime/src/test/java/io/quarkus/narayana/jta/runtime/NarayanaJtaRecorderTest.java
Outdated
Show resolved
Hide resolved
...ayana-jta/runtime/src/test/java/io/quarkus/narayana/jta/runtime/NarayanaJtaRecorderTest.java
Outdated
Show resolved
Hide resolved
Add unit test, stress test commented and updated the JavaDoc
We need to include the node name in the fixed size Xid data structure(limited to 28 bytes) so that the TM can determine which XA resource branches it is responsible for.
related to PR: #36752
please note: this is fixing the issue truncating the byte array which means that it impacts the collision avoidance. This is why I added a stress test to 'test' the collision. I think that the risk of collision is still very low but a preferable solution may be to have a byte array type 'nodeName' so that the collision resistance exclusively relies on the hash algorithm.