[common] Avoid secured Hadoop FileSystem without Kerberos config#7839
[common] Avoid secured Hadoop FileSystem without Kerberos config#7839officialasishkumar wants to merge 1 commit into
Conversation
JingsongLi
left a comment
There was a problem hiding this comment.
Review of SecurityConfiguration.isLegal() change
Correctness
The logic change from != (XOR) to || (OR) is correct. Here is how the behavior changes:
| keytab | principal | Old result | New result |
|---|---|---|---|
| empty | empty | true |
false |
| set | empty | false |
false |
| empty | set | false |
false |
| set | set | check file | check file |
The fix correctly prevents HadoopSecuredFileSystem wrapping and SecurityContext.install() from running when no Kerberos config is present. The root cause of the InterruptedIOException during checkpoints is properly addressed.
Interaction with PR #7862 (already merged to master)
Commit 1d0f005f78 ("Preserve external UGI when paimon has no kerberos credentials") added a guard inside HadoopSecuredFileSystem.trySecureFileSystem() at lines 204-211:
if (config.isLegal()) {
if (StringUtils.isNullOrWhitespaceOnly(config.getKeytab())
&& StringUtils.isNullOrWhitespaceOnly(config.getPrincipal())) {
LOG.info("No paimon Kerberos credentials configured ...; skip ...");
return fileSystem;
}
// ...wrap in secured FS...
}With this PR applied, isLegal() already returns false when both keytab and principal are empty, so the inner guard (lines 204-211) becomes dead code — that branch is unreachable. Consider removing it in this PR or a follow-up to avoid confusing future readers.
That said, this PR is still valuable because it also fixes SecurityContext.install(), which lacks the #7862 guard and would still incorrectly attempt HadoopModule.install() with null keytab on the old logic.
Design
The semantic shift of isLegal() is an improvement: it now means "a complete and valid security configuration is present" rather than "the security configuration is internally consistent." This aligns better with the method name and caller expectations.
Test feedback
The two tests cover the happy path well. Consider adding a test for the partial configuration case (keytab set but principal missing, or vice versa) to guard against regressions in that validation path — especially since the != to || change collapses two previously distinct failure modes into one.
Minor nit
After the || change, the null-check on keytab before new File(keytab) is implicitly guaranteed by the early return. This is fine, but a brief inline comment (e.g., // Both keytab and principal are non-empty at this point) would make the invariant explicit for future readers.
Overall: the fix is correct, minimal, and addresses a real production issue. Good contribution.
|
Please rebase master. |
fa51b73 to
0c24eb4
Compare
|
Rebased onto latest |
Purpose
Fixes #7146.
SecurityConfiguration#isLegalnow returns false when either Kerberos keytab or principal is missing. This preventsHadoopFileIOfrom wrapping the native HadoopFileSysteminHadoopSecuredFileSystemfor catalogs without Kerberos configuration, while keeping valid keytab/principal configurations on the secured path.Tests
mvn -pl paimon-common -Pfast-build -Dtest=HadoopSecuredFileSystemTest testmvn -pl paimon-common spotless:check