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