Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add per-listener connections.max.reauth.ms support #5639

Open
ppatierno opened this issue Oct 1, 2021 · 12 comments
Open

Add per-listener connections.max.reauth.ms support #5639

ppatierno opened this issue Oct 1, 2021 · 12 comments

Comments

@ppatierno
Copy link
Member

Apache Kafka provides the connections.max.reauth.ms [1] configuration parameter which can be set at broker level so applied on all listeners or even on a specific listener only.
Currently, Strimzi doesn't have support for specifying such a parameter at listener level because the listener. prefix is part of the list of the forbidden ones for the spec.kafka.config section.
The connections.max.reauth.ms would make sense for OAuth and SCRAM authentications enabled on a specific listener so we should add support in the authentication section of listeners via the KafkaListenerAuthenticationOAuth and KafkaListenerAuthenticationScramSha512 classes adding a new field for it.

[1] https://kafka.apache.org/documentation/#brokerconfigs_connections.max.reauth.ms

@ppatierno ppatierno self-assigned this Oct 1, 2021
@ppatierno ppatierno changed the title Add connections.max.reauth.ms support for the listeners Add per-listener connections.max.reauth.ms support Oct 1, 2021
@ppatierno ppatierno removed their assignment Feb 22, 2022
@fjbecerra
Copy link
Contributor

hey, I'd like to work on this, may I be assigned, please?

@scholzj
Copy link
Member

scholzj commented Mar 1, 2022

@ppatierno What is the plan for this? Do you have any idea how should the API look like?

@OneCricketeer
Copy link

OneCricketeer commented May 3, 2022

What if there was a config section like

listenerOverrides:
  name: SASL_SSL  # optional: validate this exists as a configured listener 
  sasl_mechanism: OAUTHBEARER
  config: 
    connections.max.reauth.ms: 3600000

When templated will add

listener.name.sasl_ssl.oauthbearer.connections.max.reauth.ms=3600000

@scholzj
Copy link
Member

scholzj commented May 3, 2022

@ppatierno ^^^ ???

I think it applies in general to all SASL listeners. So, I wonder if it should be just a property of the authentication? E.g.

listeners:
  #...
  - name: external
    port: 9094
    type: nodeport
    tls: false
    authentication:
      type: scram-sha-512
      maxReauthMs: 3600000

@scholzj
Copy link
Member

scholzj commented May 3, 2022

(For type: custom authentication, I guess it can be already configured: https://github.com/strimzi/proposals/blob/1e6b49e5e6cbb001dff7adf2c3e3ec892487d2b7/032-custom_authentication_in_kafka_brokers.md?plain=1#L76)

@OneCricketeer
Copy link

should be just a property of the authentication

Any listener's port connection properties, including auth, should be able to be overridden.

Look at max.connections in Kafka docs, for example

@scholzj
Copy link
Member

scholzj commented May 3, 2022

Any listener's port connection properties, including auth, should be able to be overridden.

The way we went with this so far is that you have the selected authentication types which enforce some properties. And then you have the custom type where you basically configure things your self. But outside of the custom type, some options are simply given by the other API fields / flags.

Look at max.connections in Kafka docs, for example

We already support this through specific fields. Look for maxConnections in https://strimzi.io/docs/operators/latest/full/configuring.html#type-GenericKafkaListenerConfiguration-schema-reference

@ppatierno
Copy link
Member Author

I think the API proposed by Jakub makes more sense.

@fvaleri
Copy link
Contributor

fvaleri commented May 23, 2022

The connections.max.reauth.ms configuration only applies to SASL authentication. The KafkaClientAuthenticationTls should not have maxReauthMs property, even if setting a value would not have any effect.

@scholzj
Copy link
Member

scholzj commented May 26, 2022

Triaged on 26.5.2022: There seem to be a different opinions on how the API should look like. So maybe we sould have a proposal to clarify all the concerns and alternatives.

@alexissellier
Copy link

What is the state of this issue. Does it need someone to take it ?

@scholzj
Copy link
Member

scholzj commented Jul 21, 2023

I think it is waiting for someone to work on it. Given the API changes needed for this, there should be a proposal of how would it be done first (https://github.com/strimzi/proposals).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants