Fix nullpointer that occurs when OAuthKafkaPrincipalBuilder is used with Kerberos listener

[akaczano]

As described in #208,

The principal builder extends the DefaultKafkaPrincipalBuilder but it just passes nulls to the two objects, SslPrincipalMapper and KerberosShortNamer in the super class constructor. For the first object, some reflection is used to initialize it anyway, but this is not done for the KerberosShortNamer. The result is that, if this principal builder is used on a broker that has a listener configured for Kerberos authentication, a null pointer exception is thrown here whenever a client tries to authenticate. Since my goal is to add an Oauth listener in-place to an existing Kafka cluster and then begin migrating clients from Kerberos to Oauth, this is a huge problem. As far as I know, there is no way to configure a principal builder for a single listener.

This PR just uses the existing reflection mechanisms to instantiate the KerberosShortNamer in addition to the SslPrincipalMapper, by recreating what Kafka does here and here.

[mstruk]

The scenario when using OAuth and Kerberos authentication in the same cluster was indeed not accounted for. The use-case of needing this co-existence in order to be able to slowly migrate away from LDAP is legitimate, yet this is the first time I've heard of it :)

I don't see a problem adding this support as long as the existing testsuite tests keep passing.
We don't intend to ever use Kerberos authentication in Strimzi Operator project.

[akaczano]

@mstruk Thanks for your quick response and willingness to support our use case. As far as I can see, all the test suites are passing. Let me know if there is anything else you need me to do. Any idea when we can expect to see a strimzi-kafka-oauth release with these changes included?

[mstruk]

@akaczano I suggest you add a test to the testsuite, otherwise it's very likely some future work may inadvertently break your fix.
You could add a test to testsuite/mock-oauth-tests. There is a docker-compose.yml file where you'll need to add an additional listener in Kafka configuration that is configured with Kerberos authentication, and add some LDAP server container to start up in addition to 'mockauth', 'kafka', and 'zookeeper' containers. Your test should then try to produce some messages to that listener.

[mstruk]

Nice work @akaczano . Maybe remove some of the commented lines that are there for debugging purposes, other than that this looks good to me, unless you still plan to add something.

[akaczano]

Thanks for the review @mstruk. I just cleaned up the test file like you mentioned. I do not plan to add anything further.

[scholzj]

Some nits. But LGTM overall.

[scholzj]

Should this have the same alignment as the lines above? I guess technically, it does not matter, but would be more readable.

[scholzj]

@mstruk Are you fine with this being based on Ubuntu? Strimzi IMHO does not use Ubuntu images anywhere else. So I would feel better if this used Red Hat UBI images as all our other projects. But I can live with this if you are fine with it.

[mstruk]

Now that you pointed this out, if I remember correctly, we had some Travis build issues with Ubuntu images on some architectures.

@akaczano Could you try use:
FROM registry.access.redhat.com/ubi8/ubi

or

FROM registry.access.redhat.com/ubi8/openjdk-17

You may need to install some additional packages.

[akaczano]

Sure; I'll give that a try.

[mstruk]

I took a look at this and it looks very challenging to set it up on ubi8. Tried a few leads with no luck. The needed package krb5-server doesn't seem to be available in the repo or is behind some kind of subscription wall. The setup is apparently very complicated as it is, and putting it together with any alternative would be just as complicated if not more. I suggest we keep it as is, and exclude the test run on architectures that will cause problems due to unavailability of platform specific container images.

[scholzj]

@mstruk Why does it not run the Travis tests? We should not merge it without that.

[mstruk]

@mstruk Why does it not run the Travis tests? We should not merge it without that.

That's a good question.

[mstruk]

Travis CI has issues with ubuntu docker images. Travis CI is failing but for some reason that's not visible under Checks.