debezium/dbz#1089 Add Infinispan connection validator#288
debezium/dbz#1089 Add Infinispan connection validator#288mfvitale merged 5 commits intodebezium:mainfrom
Conversation
...a/io/debezium/platform/environment/connection/destination/InfinispanConnectionValidator.java
Outdated
Show resolved
Hide resolved
...a/io/debezium/platform/environment/connection/destination/InfinispanConnectionValidator.java
Outdated
Show resolved
Hide resolved
|
|
||
| String serverUri; | ||
| if (user != null && password != null) { | ||
| serverUri = String.format("hotrod://%s:%s@%s:%d", user, password, serverHost, serverPort); |
There was a problem hiding this comment.
Could you create a constant for "hotrod://%s:%s@%s:%d" and the use the "".formatted() method?
There was a problem hiding this comment.
Hi @mfvitale, thanks for the review! Here's what I've updated:
Extracted HOTROD_URI_WITH_AUTH / HOTROD_URI_NO_AUTH as constants and switched to .formatted()
mfvitale
left a comment
There was a problem hiding this comment.
hi @yuanglili Overall it LGTM. I just left some minor improvements.
8a501d9 to
d62bbb0
Compare
|
@yuanglili Could you please rebase on main? |
| if (user != null && password != null) { | ||
| serverUri = HOTROD_URI_WITH_AUTH.formatted(user, password, serverHost, serverPort); | ||
| } |
There was a problem hiding this comment.
What about if the supplied user/password is not null but is an empty string? Is the expectation that this should go through the "auth" path or the "no auth" path?
There was a problem hiding this comment.
What about if the supplied user/password is not null but is an empty string? Is the expectation that this should go through the "auth" path or the "no auth" path?
@Naros Good catch! But in the case, I think neither auth nor no-auth is the right path here, it should fail validation instead. Because if a user puts in a value, even a blank one, they probably mean to use authentication, so silently ignoring it seems wrong.
Also noticed I was missing another case: if only one of user or password is set, that should be an error too.
My plan to updated the logic to handle all cases:
- Both non-blank → auth path
- Both absent → no-auth path
- One missing, or either is blank → validation error
WDYT?
There was a problem hiding this comment.
@Naros @yuanglili I think the correct anwer is to check ISPN and confir if either username or password can be empty. Based on that it will either go through auth path or fail validation.
There was a problem hiding this comment.
I checked the Infinispan codebase:
AuthenticationConfigurationBuilder(L165-168):username()just assigns the value with no empty string checkvalidate()(L231-243): only checksusername == null, so empty string passes right throughHotRodURI(L59): parseshotrod://:@host:portand assigns""directly without converting to null
Test results:
- Both blank (
hotrod://:@host) → connected successfully (treated as no-auth) - Only one blank (e.g.
hotrod://:secret@hostorhotrod://admin:@host) →TransportException(auth failure)
So I think the updated logic should be :
- Both non-blank → auth path
- Both null or both blank → no-auth path
- One blank, one not → validation error
There was a problem hiding this comment.
Updated the PR:
- Fail validation when only one of
user/passwordis provided or is empty - Add parameterized tests to cover partial credential scenarios
8438ff8 to
cf51b63
Compare
| boolean hasUser = user != null && !user.isEmpty(); | ||
| boolean hasPassword = password != null && !password.isEmpty(); | ||
|
|
||
| if (hasUser && hasPassword) { | ||
| serverUri = HOTROD_URI_WITH_AUTH.formatted(user, password, serverHost, serverPort); | ||
| } | ||
| else { | ||
| serverUri = HOTROD_URI_NO_AUTH.formatted(serverHost, serverPort); | ||
| } |
There was a problem hiding this comment.
Since you have already done the check for the password and user, could you move this block out of this function and then just pass the serverUri as parameter here?
| boolean hasUser = user != null && !user.isEmpty(); | ||
| boolean hasPassword = password != null && !password.isEmpty(); | ||
|
|
||
| if (hasUser && hasPassword) { |
There was a problem hiding this comment.
| if (hasUser && hasPassword) { | |
| if (!Strings.isNullOrEmpty(user) && !Strings.isNullOrEmpty(password)) { |
Then you can also remove the hasUser and hasPassword local vars.
f64c2c5 to
869bab5
Compare
|
Waiting this #313 for test stabilization |
|
@yuanglili Could you please rebase with latest changes on main? |
…n tests Signed-off-by: nonononoonononon <yuangli971@gmail.com>
Signed-off-by: nonononoonononon <yuangli971@gmail.com>
Signed-off-by: nonononoonononon <yuangli971@gmail.com>
Signed-off-by: nonononoonononon <yuangli971@gmail.com>
Signed-off-by: nonononoonononon <yuangli971@gmail.com>
bdc9cca to
94b7a6a
Compare
Rebased , thanks! |
|
@yuanglili Applied, thanks! |
Summary
Implements
InfinispanConnectionValidatorfor checking connection to Infinispan.Changes
connection-schemas.jsonInfinispanConnectionValidatorusinginfinispan-client-hotrodfor HotRod protocol validationdestinations.infinispan.connection.timeouttoapplication.ymlInfinispanConnectionValidatorITintegration test with TestcontainersFixes debezium/dbz#1089