-
Notifications
You must be signed in to change notification settings - Fork 71
Proposal for supporting connections.max.reauth.ms
configuration for SCRAM listeners
#173
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
base: main
Are you sure you want to change the base?
Conversation
… SCRAM listeners Signed-off-by: Gantigmaa Selenge <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the proposal ... I think that in general this would work fine.
But I wonder if we should consider some alternatives before we decide to add some new API field for this ...
- Should we let users to configure this cluster-wide in
.spec.kafka.config
? Are there usecases where it should be configured on a per-listener basis? - Should we find a way how to allow users configure per-listener options in
.spec.kafka.config
? (Right now, we can make it configurable at the cluster level, but not with the per-listener prefixes) - Users can probably use the custom authentication for this. Isn't that sufficient? Given Kafka has this disabled and does not seem to see this as an important option, maybe it is niche thing among Strimzi users as well?
Might be worth adding these at least to the alternatives for everyone's consideration.
It is also worth noting that strimzi/strimzi-kafka-operator#11764 suggests considering deprecating the Oauth authentication exactly because it became just a huge pile of various options. While this proposal does not do this for SCRAM-SHA, it is a reason to consider if we want to handle these options through new API fields or not.
Thanks @scholzj for the feedback.
Actually users can already configure this cluster-wide in .spec.kafka.config. So perhaps the proposal should be "support this configuration at listener level" as currently it seems to suggest that we don't support this config at all.
Can we maybe add this config to the exception list, to allow something like
I suppose that could be sufficient but I wonder if user wants to enable this config for their existing listener, would they want to reconfigure their entire listener to make it custom type? I think it could still be useful for adding this configuration for their specific listener.
Given that we decided to deprecate OAuth authentication type for the reason of having too many options to handle, maybe proposing |
Yeah, something like this. But we right now don't have any support for the
I think it should be taken into consideration. But at the same time, this is a single option. So the question also is how many other options like this are waiting to be requested by someone. The OAuth also did not looked that bad early on. But got worse as users needed it to work with this and that etc. TBH, I have thrown here all these alternative approaches. But I'm not really sure that any single one of them is somehow the obvious winner for me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The configuration seems useful, but there might still be a question mark over the agreed approach, especially with the potential for using type: custom for authentication configuration (as will happen with OAuth). I’ve left a few comments as I read through.
I don't see that many per listener configurations, especially for SCRAM so it might not end up as bad as oauth. |
Signed-off-by: Gantigmaa Selenge <[email protected]>
225e21e
to
a1a4e4a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for creating the proposal.
I'm also a bit hesitant about adding an option to authentication
given the risk of needing to add other options in future. There are currently two other listener specific options you found, so it could just be those, but other options might be added.
Looking at how the forbidden options are handled, I wonder if we could put in some handling to allow certain listener-specific configurations without the use of regex. In KafkaCluster.java we handle the configuration in the same method that we validate the set of listeners. So I wonder if we could build a set of prefixes that users might use and basically augment the forbidden prefix exceptions list in the KafkaConfiguration constructor.
What do you think @scholzj @tinaselenge ?
It would be an option, yes. I'm just not sure what would be the best way to do it the best way. Keep in mind, that with many clusters having 2 or more listeners, it is lot of parsing, string concatenation, for loops, etc. Is that still "cheaper" than regular expression(s)? Strictly speaking, I do not think we need to validate the particular listener name. It might be even bad idea as it would cause unnecessary errors when a listener is deleted etc. We just care about the pattern. So I think we can do it without the explicit listener knowledge as well. But it still seems very brute force unelegant approach ... :-/ |
You would need to loop through each listener and create the prefix string you expect, and I agree it's common to have a couple of listeners, but I don't think users tend to have more than a handful of listeners do they? But it does seem overkill to construct the listener config strings if no listener specific configurations are set. In terms of handling deleted listeners, I think it's reasonable for Strimzi to complain if a user deletes a listener but not the related listener config. If we wanted to be more lenient to the users for listener configurations perhaps we can set up the forbidden options checking so that only the configurations that begin with However we handle it, I at least would be in favour of us adding something to the forbidden options handling, rather than adding a new field. |
I agree that we should avoid using a new field. The reasons are the same already shared by Jakub and Kate. |
Discussed on community call on 2.10.2025: It seems the agreement is to allow users to configure the selected listener-specific options through unstructured configuration section (such as |
Why a new one? |
It seemed easier to me as it would not have much to do with the current one. But it is fine if the new one is updated as well. |
…spec.kafka.config Signed-off-by: Gantigmaa Selenge <[email protected]>
Thank you for all the valuable feedback. I have updated the proposal with the agreed solution of allowing the per-listener configuration in the I have tried implementing both of the suggested solutions from @scholzj and @katheris, regular expression and building a list of per listener config exceptions and then came to the conclusion that the latter is simpler and less expensive to implement. I have included the regular expression one to the Rejected Alternative section. Here is also a POC implementation. We can improve this further, by making it build the per-listener config exceptions, only if there is any @scholzj , @katheris , @PaulRMellor , @ppatierno can you please have another look when you have time? Thank you! |
Thanks @tinaselenge, I've taken a quick look at the PoC and although the implementation is only a few lines of code, I'm not convinced it will be the easiest to follow for future developers delving into the code. A few things come to mind:
I think we need to make sure it's easy to reason about which config options are going to be forbidden and which aren't and I'm not sure the current PoC does that. I'll have a think if I can suggest changes to make it easier to follow, but feel free to take a look as well. |
Related to: strimzi/strimzi-kafka-operator#5639