search

strimzi / strimzi-kafka-oauth

OAuth2 support for Apache Kafka® to work with many OAuth2 authorization servers
Apache License 2.0
141 stars 89 forks source link

Remove the use of `metric.reporters` in OAuth metrics, use `strimzi.metric.reporters` instead #193

Closed mstruk closed 1 year ago

mstruk commented 1 year ago

This PR removes the use of metric.reporters in OAuth metrics to configure the reporters to instantiate. The reason is that OAuth metrics integrates with the reporters by creating new instances of them, resulting in one instance being created by Kafka broker, and another instance created by OAuth metrics. Not all reporters work properly when multiple instances are instantiated.

With this PR OAuth metrics, when enabled, will use strimzi.oauth.metric.reporters as a list of reporters to instantiate.

If strimzi.oauth.metric.reporters is not set, org.apache.kafka.common.metrics.JmxReporter will automatically be instantiated, otherwise only the specified reporters will be instantiated. The config option metric.reporters will be ignored by OAuth metrics.

In order to migrate existing broker configurations use strimzi.oauth.metric.reporters as the configuration option rather than metric.reporters.

STRIMZI_OAUTH_METRIC_REPORTERS=org.apache.kafka.common.metrics.JmxReporter,org.some.package.SomeReporter

See README.md for more information.

mstruk commented 1 year ago

Where will this new option strimzi.metrics.reporter be used? If I got it rght and it is in the general configuration of Kafka, should it rather be a strimzi.oauth.metrics.reporter?

This option is global configuration for the Kafka broker yes, so it should not be passed as part of a specific listener configuration, rather as part of client.properties on the client, or server.properties on the broker (except as I describe in the README it should really be set as env var or a system property rather than in server.properties.

Generally I use 'oauth.' properties for JAAS config options, and 'strimzi.authorization.' as globals that are specific to KeycloakAuthorizer. This option works across the board for the whole Kafka instance thus I opted for 'strimzi.*' prefix.

But it is true that it's the OAuth library that uses this option exclusively so your suggestion makes sense.

scholzj commented 1 year ago

I do not think the strimzi. itself is an issue. Its more without the .oauth behind it, it possibly collides with other Strimzi options. So if it is something only Oauth reads as far as I understood, I think strimzi.oauth... would be better if we stick with the strimzi. part. Not sure if @tombentley for example has some thoughts about it.

tombentley commented 1 year ago

strimzi.oauth... seems better to me.

mstruk commented 1 year ago

@tombentley @mimaison Any more unaddressed issues here?