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
