KAFKA-17921: Support SASL_PLAINTEXT protocol with java.security.auth.login.config #17671
Conversation
FrankYang0529
force-pushed
the
KAFKA-17921
branch
2 times, most recently
from
7490314 to
3d304c4
Compare
FrankYang0529
force-pushed
the
KAFKA-17921
branch
from
3d304c4 to
fd8f47b
Compare
FrankYang0529
force-pushed
the
KAFKA-17921
branch
2 times, most recently
from
535eedb to
ee1d321
Compare
FrankYang0529
force-pushed
the
KAFKA-17921
branch
2 times, most recently
from
c8ddc37 to
e205bb8
Compare
FrankYang0529
force-pushed
the
KAFKA-17921
branch
from
e205bb8 to
cefdcde
Compare
| import java.util.Map; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class JaasModule { |
There was a problem hiding this comment.
Is this duplicate to core JaasModule?
There was a problem hiding this comment.
Yes, this is duplicated. Original JaasModule is in core module. I think we can remove original one after we migrate all sasl test case to new framework. WDYT?
| @@ -602,6 +630,9 @@ public void close() throws Exception { | |||
| waitForAllFutures(futureEntries); | |||
| futureEntries.clear(); | |||
| Utils.delete(baseDirectory); | |||
| if (jaasFile.isPresent()) { | |||
There was a problem hiding this comment.
Could you please use ifPresent instead?
|
|
||
| import javax.security.auth.login.Configuration; | ||
|
|
||
| public class JaasTestUtils { |
There was a problem hiding this comment.
It is already in xxx.test package, maybe we can call it JaasUtils?
| public static final String KAFKA_PLAIN_ADMIN = "plain-admin"; | ||
| public static final String KAFKA_PLAIN_ADMIN_PASSWORD = "plain-admin-secret"; | ||
|
|
||
| public static File writeJaasContextsToFile(List<JaasSection> jaasSections) throws IOException { |
There was a problem hiding this comment.
Do we really need this object? How about using map? It is more simple
|
|
||
| private final Map<String, String> entries; | ||
|
|
||
| private JaasModule(String name, boolean debug, Map<String, String> entries) { |
There was a problem hiding this comment.
Could you please try to use record class?
| configs.put(SaslConfigs.SASL_JAAS_CONFIG, | ||
| String.format("org.apache.kafka.common.security.plain.PlainLoginModule required username=\"%s\" password=\"%s\";", | ||
| JaasTestUtils.KAFKA_PLAIN_USER1, JaasTestUtils.KAFKA_PLAIN_USER1_PASSWORD)); | ||
| try (Admin admin = clusterInstance.admin(configs)) { |
There was a problem hiding this comment.
Those helper should return authorized client object, right? Otherwise, developers have to configure them manually
FrankYang0529
force-pushed
the
KAFKA-17921
branch
2 times, most recently
from
7e4b4a6 to
c9ea0ad
Compare
There was a problem hiding this comment.
@FrankYang0529 thanks for this patch. two minor comments are left PTAL
| assertDoesNotThrow(() -> admin.describeAcls(AclBindingFilter.ANY).values().get()); | ||
| } | ||
| String topic = "sasl-plaintext-topic"; | ||
| Assertions.assertDoesNotThrow(() -> clusterInstance.createTopic(topic, 1, (short) 1)); |
There was a problem hiding this comment.
Since the APIs in question only throw runtime exceptions, there's no need to use assertDoesNotThrow, which is typically employed to convert checked exceptions into runtime exceptions.
| } | ||
|
|
||
| // client with non-admin credentials | ||
| Map<String, Object> nonAdminConfig = Map.of( |
There was a problem hiding this comment.
the helper methods of clusterInstance should be able configure the sasl.jaas.config automatically - that is why we add those helper to clusterInstance
There was a problem hiding this comment.
Hi @chia7712, thanks for the review. Yes, we already set default user to admin, consumer, and producer helper function automatically. Here, we would like to test developer overrides sasl.jaas.config with non-admin credentials. We also test overriding with unknown credential, so we can make sure the cluster instance with sasl plaintext works.
…login.config Signed-off-by: PoAn Yang <[email protected]>
FrankYang0529
force-pushed
the
KAFKA-17921
branch
from
c9ea0ad to
dacb066
Compare
Support SASL_PLAINTEXT protocol in ClusterTest framework.
Committer Checklist (excluded from commit message)