rondagostino commented on a change in pull request #10694:
URL: https://github.com/apache/kafka/pull/10694#discussion_r633480466



##########
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.
+        # Bounce again with ACLs for new mechanism. Use old 
SimpleAclAuthorizer here to ensure that is also tested.
+        self.kafka.client_sasl_mechanism = new_sasl_mechanism # Removes old 
SASL mechanism completely
         self.set_authorizer_and_bounce(security_protocol, security_protocol)
 
     def add_separate_broker_listener(self, broker_security_protocol, 
broker_sasl_mechanism):
         # Enable the new internal listener on all brokers first
         self.kafka.open_port(self.kafka.INTERBROKER_LISTENER_NAME)
         
self.kafka.port_mappings[self.kafka.INTERBROKER_LISTENER_NAME].security_protocol
 = broker_security_protocol
-        self.kafka.client_sasl_mechanism = broker_sasl_mechanism
+        
self.kafka.port_mappings[self.kafka.INTERBROKER_LISTENER_NAME].sasl_mechanism = 
broker_sasl_mechanism

Review comment:
       We can't rely on the client SASL mechanism here with the KRaft-related 
changes that were made in https://github.com/apache/kafka/pull/10199/ because 
those KRaft changes made the code very explicit about which SASL mechanisms to 
add: if the client security protocol is PLAINTEXT then the client SASL 
mechanism isn't listed as an enabled mechanism.  And in fact for this test the 
client security protocol is PLAINTEXT.  So we need another way to transmit the 
SASL mechanism, and that's what the new optional `sas_mechanism` on the 
KafkaListener in the port mapping is for.

##########
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:
       Good catch!  I missed leveraging this, and it results in 
`sasl.enabled.mechanisms=` (i.e. blank) on the first of the two rolls.  The 
test was still passing, but I think that's just luck.  I've fixed it in 
`kafka.py` -- we now pass in the SASL mechanism, and we are now seeing 
`sasl.enabled.mechanisms=PLAIN` as expected.
   
   




-- 
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


Reply via email to