ENH: automatic credential refresh in kafka source #4258
Conversation
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
chenqi0805
force-pushed
the
enh/credential-refresh-kafka-source
branch
from
32ced36 to
4311217
Compare
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
chenqi0805
requested review from
engechas,
graytaylor0,
dinujoh,
kkondaka,
asifsmohammed,
dlvenable and
oeyh
as code owners
Signed-off-by: George Chen <[email protected]>
|
|
||
| final char[] password = basicCredentials.getPassword().toCharArray(); | ||
| nc.setPassword(password); | ||
| } |
There was a problem hiding this comment.
Are there more conditions here? Perhaps add a log line (WARN) for an else? If we do get another Callback subclass we might have very subtle errors.
There was a problem hiding this comment.
So all Callbacks are already handled in super.handle(callbacks). Here it is just an overwrite on those two callbacks
There was a problem hiding this comment.
I'm not clear on who is overriding this for only these two callbacks?
I'd still tend to think that adding a few lines to log a warning would be helpful in case either of these: 1) We add a bug that adds a new callback elsewhere, but don't support here; or 2) the Kafka implementation changes.
else {
log.warn("Received an unexpected Callback type {}", callback.getClass());
}
There was a problem hiding this comment.
By inspecting their source code: https://github.com/a0x8o/kafka/blob/master/clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslClientCallbackHandler.java#L75-L98
I saw UnsupportedCallbackException would be thrown.
| // Used for testing only | ||
| protected DynamicBasicCredentialsProvider() {} | ||
|
|
||
| public BasicCredentials getBasicCredentials() { |
There was a problem hiding this comment.
Is DynamicSaslClientCallbackHandler::handle and thus this method called for every interaction with Kafka?
I'm wondering what overhead the read/write lock will have here. And if it is necessary. If we make a request and the username/password is stale, is it that big of a deal? The user needs to setup a proper rotation. Or there will more failures while waiting to get the updated secrets than the difference between two requests to read the value.
There was a problem hiding this comment.
Is DynamicSaslClientCallbackHandler::handle and thus this method called for every interaction with Kafka?
Yes.
If we make a request and the username/password is stale, is it that big of a deal?
w/o readwrite lock, I am not sure how stale the credentials could be.
In my POC sasl handshake happens in the background with cadence decoupled from individual API call.
Description
This PR
Issues Resolved
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.