KAFKA-18447 Support SASL_SSL protocol with java.security.auth.login.config

[m1a2st]

The new test framework does not yet support the SASL_SSL security
protocol; we should add support for it.

Reviewers: Igor Soarez i@soarez.me, Chia-Ping Tsai
chia7712@gmail.com

[github-actions]

This PR is being marked as stale since it has not had any activity in 90 days. If you
would like to keep this PR alive, please leave a comment asking for a review. If the PR has
merge conflicts, update it with the latest from the base branch.

If you are having difficulty finding a reviewer, please reach out on the [mailing list](https://kafka.apache.org/contact).

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.

[soarez]

Thanks for the PR @m1a2st.

I have a similar change for the SSL protocol: #22184 , with a different approach for the keystore and truststore generation. I wonder what your thoughts are.

[soarez]

Did you consider reusing TestSslUtils from the clients subproject?

[m1a2st]

The existing TestSslUtils resides in clients/src/test/java, so it is only available in the test scope of the clients module. Since test-common-runtime is to be used as shared test infrastructure, it cannot depend on the clients test classpath, as that would introduce an invalid dependency.

As a follow-up, we could consider moving common client-side SSL utility methods into test-common,

[soarez]

Thanks for the reply.

Since test-common-runtime is to be used as shared test infrastructure, it cannot depend on the clients test classpath, as that would introduce an invalid dependency.

What makes that dependency invalid?

Currently, :test-common:test-common-runtime already depends on :clients, it's just missing the client tests classpath.

[m1a2st]

The concern is architectural, adding the clients test output to the test-common-runtime main scope would mix test-only code into a shared production classpath. This would cause all modules depending on test-common-runtime to transitively pull in clients test classes, which is not appropriate for a shared test infrastructure module.

[soarez]

Thank you @m1a2st.

I want to make sure I understand the concern correctly, because I still can't explain it myself.

My current understanding is that test-common-runtime is not actually a production classpath, as its classes are only used for testing.

Looking through build.gradle, test classes are already pulled in across modules. e.g. The main classpath for ':jmh-benchmarks' pulls in the test classpath from :clients. Another example is :transaction-coordinator , whose test classpath pulls in the same dependency.

This would cause all modules depending on test-common-runtime to transitively pull in clients test classes

They already do - see core, storage, or tools - just declared explicitly rather than transitively. If anything, moving the dependency into test-common-runtime would reduce duplication of declarations across modules.

1. Are these existing examples problematic too, and something we should change?
2. If test-common-runtime can include test classes from other modules (e.g. :test-common:test-common-util) why does it matter if those classes originate from the main classpath or from another (test) classpath in the dependency module?
3. What specifically makes it inappropriate for modules depending on test-common-runtimeto also include test classes from clients?

It seems important to me to understand this because: introducing competing test kits for SSL cuts against the current coding guidelines, and determining if this cross dependency is acceptable seems critical to decide how to address the duplication.

[chia7712]

I agree with @soarez that duplicating TestSslUtils is ugly and violates DRY. However, since test-common-runtime is meant to be a shared test infrastructure, keeping its dependencies clean and avoiding test-classpath leakage is important for downstream developers.

What if we introduce a new module, like :test-common:test-common-base, to house generic, zero-kafka-dependency utilities? We could move the shared SSL utils there now, and it could eventually house other generic tools like wait functions or file-related ops. WDYT?

[soarez]

Good suggestion @chia7712, this is an attractive idea. The issue is that if we make it generic, without any dependency on Kafka, then it cannot house TestSslUtils.

The purpose of TestSslUtils is to generate certificates and configuration to test SSL in Kafka specifically. Precisely what we need for our test framework, AFAICT. So it depends on classes from clients: SslConfigs, Password, DefaultSslEngineFactory, ConnectionMode, which we would need anyway. Not just for the new test framework, but also probably for any other use within this repository: would we ever generate keys and certificates for something other than Kafka? It seems to me these two things want to live together.

If we move TestSslUtils to a new module, it would have to depend on clients too, it couldn't be "zero-kafka". The benefit here is that the modules that require this new dependency would not be adding clients' test classpath, only the main classpath. Right now, I can't tell how significant this is. I'd still like to understand the classpath leakage concern. A concrete example would help.

[chia7712]

So it depends on classes from clients: SslConfigs, Password, DefaultSslEngineFactory, ConnectionMode, which we would need anyway. Not just for the new test framework, but also probably for any other use within this repository: would we ever generate keys and certificates for something other than Kafka? It seems to me these two things want to live together.

I believe connecting to the clients module should be fine, as it is essentially the foundation of Kafka

Right now, I can't tell how significant this is. I'd still like to understand the classpath leakage concern. A concrete example would help.

For example, users could use the cluster instance as an embedded Kafka, which is useful not only for testing but also for POCs. Therefore, depending on the test module would be an anti-pattern here.

[soarez]

I see. Thank you for your patience helping me understand the issue.

I gave this a try in #22193. PTAL, there's a large surface area for merge conflicts 😅