mumrah commented on a change in pull request #10694:
URL: https://github.com/apache/kafka/pull/10694#discussion_r633552208
##########
File path: tests/kafkatest/services/security/security_config.py
##########
@@ -258,8 +259,10 @@ def enable_sasl(self):
def enable_ssl(self):
self.has_ssl = True
- def enable_security_protocol(self, security_protocol):
+ def enable_security_protocol(self, security_protocol, sasl_mechanism =
None):
self.has_sasl = self.has_sasl or self.is_sasl(security_protocol)
+ if sasl_mechanism:
Review comment:
Safer to check `is not None`
##########
File path: tests/kafkatest/services/security/security_config.py
##########
@@ -387,6 +390,7 @@ def enabled_sasl_mechanisms(self):
sasl_mechanisms += list(self.uses_raft_sasl)
if self.zk_sasl:
sasl_mechanisms += [SecurityConfig.SASL_MECHANISM_GSSAPI]
+ sasl_mechanisms += self.additional_sasl_mechanisms
Review comment:
If we're adding all of one list to another list, we should use `extend`,
e.g.
```python
sasl_mechanisms.extend(self.additional_sasl_mechanisms)
```
##########
File path: tests/kafkatest/tests/core/security_rolling_upgrade_test.py
##########
@@ -89,18 +89,21 @@ def add_sasl_mechanism(self, new_client_sasl_mechanism):
self.bounce()
def roll_in_sasl_mechanism(self, security_protocol, new_sasl_mechanism):
- # Roll cluster to update inter-broker SASL mechanism. This disables
the old mechanism.
+ # Roll cluster to update inter-broker SASL mechanism.
+ # We need the inter-broker SASL mechanism to still be enabled through
this roll.
+ self.kafka.client_sasl_mechanism = "%s,%s" %
(self.kafka.interbroker_sasl_mechanism, new_sasl_mechanism)
self.kafka.interbroker_sasl_mechanism = new_sasl_mechanism
self.bounce()
# Bounce again with ACLs for new mechanism.
+ self.kafka.client_sasl_mechanism = new_sasl_mechanism # Removes old
SASL mechanism completely
Review comment:
nit: two spaces between code and `#`
##########
File path: tests/kafkatest/services/security/security_config.py
##########
@@ -258,8 +259,10 @@ def enable_sasl(self):
def enable_ssl(self):
self.has_ssl = True
- def enable_security_protocol(self, security_protocol):
+ def enable_security_protocol(self, security_protocol, sasl_mechanism =
None):
Review comment:
Do we make use of this new argument anywhere? I can't seem to find any
--
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:
us...@infra.apache.org
