[GitHub] [kafka] mumrah commented on a change in pull request #10237: MINOR: fix failing system test delegation_token_test

[GitBox <gi...@apache.org>]

mumrah commented on a change in pull request #10237:
URL: https://github.com/apache/kafka/pull/10237#discussion_r585784850



##########
File path: tests/kafkatest/services/security/security_config.py
##########
@@ -374,7 +374,8 @@ def interbroker_sasl_mechanism(self):
     def enabled_sasl_mechanisms(self):
         sasl_mechanisms = []
         if self.is_sasl(self.security_protocol):
-            sasl_mechanisms += [self.client_sasl_mechanism]
+            # .csv is supported so be sure to account for that possibility
+            sasl_mechanisms += self.client_sasl_mechanism.strip().split(',')

Review comment:
       Not sure if we should bother in this PR, but the usages of `client_sasl_mechanism` could stand to be cleaned up. In `SecurityConfig.__init__` we default it to a simple string, but as you found here (and as seen in `SecurityConfig.client_config`) we support it being a comma delimited string. 
   
   https://github.com/apache/kafka/blob/58b3b1b557e9ba19cffde91bd117a89b947f1fc1/tests/kafkatest/services/security/security_config.py#L240-L253
   
   It's probably a lot safer to declare this as a list in the class and not worry about having to do the `split(",")` everywhere. Though maybe there's a reason why we split it lazily.. not sure.
   
   Either way, this change looks good. However, you might consider doing something like:
   
   ```python
   sasl_mechanisms += [mechanism.strip() for mechanism in self.client_sasl_mechanism.split(',')]
   ```
   
   since `self.client_sasl_mechanism.strip()` won't catch spaces in the middle of the string.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org