[helm] Enable SASL authentication configurations#2506
[helm] Enable SASL authentication configurations#2506morazow wants to merge 1 commit intoapache:mainfrom
Conversation
ebf267c to
743acef
Compare
affo
left a comment
There was a problem hiding this comment.
Thanks for the great job!
I think there is a bit of confusion 👍
I left many comments here and there, but let me wrap up my thoughts:
About the JAAS configuration, I think we can safely move the helper to directly template stringData into the secret and also remove all the clutter in the command section to avoid setting things there 👍
In general, I think the values provided are not enough: if INTERNAL does not have any SASL enabled, then FlussClient is not required to be configured, as Fluss needs to authenticate only if the internal listener is protected. I think this demands for a re-design of this feature 🤝
|
Hello @affo , Thanks for the review! Please have another look 🤝 |
affo
left a comment
There was a problem hiding this comment.
Looks good now 🚀
@swuferhong what do you think?
5ca48aa to
625c0f9
Compare
|
Thanks for the suggestions! Please have a look to the PR again. I have identified two follow-up issues that need to be addressed separately.
I will follow up with issues and PR for each. [DONE] Separating SASL CommunicationFor this to work we would need to prefix the JAAS contents with But this does not work for the client, as on this line the client listener name is hard coded as Special Character for SASL Usernames and PasswordsThis is also indeed an issue, which requires core change for SASL client authentication. Without escaping we would have something like below This fails on server with configuration error. It should be correctly escaped as below: But this again causes issues on client side since the SaslClientAuthenticator does not escape the user provided username and password. This is the failing test for @Test
void testSpecialCharactersForPassword() throws Exception {
final String specialPassword = "pa$$wo\\rd!@#%&\"";
final Configuration clientConfig = new Configuration();
clientConfig.setString("client.security.protocol", "sasl");
clientConfig.setString("client.security.sasl.username", "admin");
clientConfig.setString("client.security.sasl.password", specialPassword);
testAuthentication(clientConfig, getDefaultServerConfig());
}Since both of these points require changes to Fluss core packages, let's address them separately. |
|
Discussed offline with @affo, we don't have to worry about the prefixing the |
05999c4 to
f6d123b
Compare
Purpose
Linked issue: close #2503
Brief change log
Adds configuration options to Helm charts to enable SASL authentication.
Tests
API and Format
Documentation
Updated the deploying-with-helm documentation