Adding a proxy to delegate to appropriate IAMSaslClientProvider based on ClassLoader #82
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dannycranmer
force-pushed
the
classloader-issue
branch
from
408d60c to
6efbfaa
Compare
|
Have we tested that this works to resolve the specific failure mode specified here? #36 (comment) If so, we might want to mention it in the overview |
iemre
approved these changes
Oct 13, 2022
| CallbackHandler cbh) throws SaslException { | ||
| String mechanismName = getMechanismNameForClassLoader(cbh.getClass().getClassLoader()); | ||
|
|
||
| // Creat a client by delegating to the SaslClientFactory for the classloader of the CallbackHandler |
|
Updated description wrt testing |
sayantacC
reviewed
Oct 13, 2022
| } | ||
| } | ||
|
|
||
| public static String getMechanismNameForClassLoader(ClassLoader classLoader) { |
There was a problem hiding this comment.
If you do create a IAMClassAwareLoginModule, this static method could be a utility static method in that class.
…der based on ClassLoader
dannycranmer
force-pushed
the
classloader-issue
branch
from
6efbfaa to
f3069d3
Compare
sayantacC
approved these changes
Oct 14, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
#36
Description of changes:
This change fixes the classloader issue experienced in this issue. I have introduced a new
SaslClientFactorycalledClassLoaderAwareIAMSaslClientFactoryregistered under theAWS_MSK_IAMmechanism. The existingIAMSaslClientFactoryhas been moved to a new mechanism nameAWS_MSK_IAM.<classloader>. EachIAMLoginModulewill register both mechanisms, we will end up with oneClassLoaderAwareIAMSaslClientFactoryper JVM andncopies ofIAMSaslClientFactory,nequals the number of ClassLoaders used.ClassLoaderAwareIAMSaslClientFactorypicks the correctIAMSaslClientFactorybased on the classloader of theCallbackHandlerFor single classloader environments, there will be 1
ClassLoaderAwareIAMSaslClientFactoryand 1IAMSaslClientFactory. We will incur a negligible performance hit delegating from one to the other.Testing
I have tested against MSK using a Flink cluster running on EC2 box. I verified this issue resolves issue #36. I am waiting for additional test feedback from other stakeholders, and am hoping that this can be verified against MSK e2e tests.
Concerns:
Since we register
ncopies ofIAMSaslClientFactorythere is an opportunity for this to infinitely grow and leak, for example if a Flink job is continuously failing over. However, in this situation the job is already broken and this tradeoff might be acceptable against the current state, where multi classloader environments are not supported.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.