rajinisivaram commented on a change in pull request #9378:
URL: https://github.com/apache/kafka/pull/9378#discussion_r500429377



##########
File path: tests/kafkatest/services/kafka/kafka.py
##########
@@ -451,7 +451,8 @@ def _kafka_topics_cmd(self, node, force_use_zk_connection):
         set. If Admin client is not going to be used, don't set the 
environment variable.
         """
         kafka_topic_script = self.path.script("kafka-topics.sh", node)
-        skip_security_settings = force_use_zk_connection or not 
self.all_nodes_topic_command_supports_bootstrap_server()
+        skip_security_settings = force_use_zk_connection or not 
self.all_nodes_topic_command_supports_bootstrap_server() \
+                                 or self.interbroker_security_protocol == 
SecurityConfig.PLAINTEXT

Review comment:
       should we add a method to KafkaService or SecurityConfig that tells you 
which listener to use for tools? Using inter-broker here looks a bit odd, even 
though it becomes clear when you look at the other changes.

##########
File path: tests/kafkatest/services/kafka/kafka.py
##########
@@ -983,7 +988,10 @@ def _topic_command_connect_setting(self, node, 
force_use_zk_connection):
         bootstrap server, otherwise returns zookeeper connection string.
         """
         if not force_use_zk_connection and 
self.all_nodes_topic_command_supports_bootstrap_server():
-            connection_setting = "--bootstrap-server %s" % 
(self.bootstrap_servers(self.security_protocol))
+            if self.interbroker_security_protocol == SecurityConfig.PLAINTEXT:
+                connection_setting = "--bootstrap-server %s" % 
(self.bootstrap_servers(self.interbroker_security_protocol))
+            else:
+                connection_setting = "--bootstrap-server %s" % 
(self.bootstrap_servers(self.security_protocol))

Review comment:
       Is this assuming that `else` section always means PLAINTEXT since there 
are no security configs here?

##########
File path: tests/kafkatest/services/security/kafka_acls.py
##########
@@ -93,11 +97,13 @@ def add_cluster_acl(self, kafka, principal, 
force_use_zk_connection=False):
 
         force_use_zk_connection = force_use_zk_connection or not 
kafka.all_nodes_acl_command_supports_bootstrap_server()
 
-        cmd = "%(cmd_prefix)s --add --cluster --operation=ClusterAction 
--allow-principal=%(principal)s" % {
-            'cmd_prefix': self._acl_cmd_prefix(kafka, node, 
force_use_zk_connection),
-            'principal': principal
-        }
-        kafka.run_cli_tool(node, cmd)
+        for operation in ['ClusterAction', 'Alter', 'Create']:

Review comment:
       we are adding more ACLs than we had before. Are they necessary - it is 
better to limit ACLs unless they are necessary for the tests.




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