You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "mingdaoy (via GitHub)" <gi...@apache.org> on 2024/10/11 05:53:21 UTC
[PR] KAFKA-17715 emove force_use_zk_connection from e2e [kafka]
mingdaoy opened a new pull request, #17465:
URL: https://github.com/apache/kafka/pull/17465
https://issues.apache.org/jira/browse/KAFKA-17715
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17715 remove force_use_zk_connection from e2e [kafka]
Posted by "mingdaoy (via GitHub)" <gi...@apache.org>.
mingdaoy commented on code in PR #17465:
URL: https://github.com/apache/kafka/pull/17465#discussion_r1797690289
##########
tests/kafkatest/services/kafka/kafka.py:
##########
Review Comment:
@mimaison Additionally, L1380 is not related to `force_use_zk_connection` but I think this needs to be removed, right?
`return "--zookeeper %s " % self.zk_connect_setting()`
```
def _connect_setting_reassign_partitions(self, node):
if self.all_nodes_reassign_partitions_command_supports_bootstrap_server():
return "--bootstrap-server %s " % self.bootstrap_servers(self.security_protocol)
else:
return "--zookeeper %s " % self.zk_connect_setting()
```
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -1010,84 +1010,68 @@ def kafka_metadata_quorum_cmd(self, node, kafka_security_protocol=None, use_cont
kafka_metadata_script = self.path.script("kafka-metadata-quorum.sh", node)
return "{} {}".format(kafka_metadata_script, bootstrap)
- def kafka_topics_cmd_with_optional_security_settings(self, node, force_use_zk_connection, kafka_security_protocol=None, offline_nodes=[]):
+ def kafka_topics_cmd_with_optional_security_settings(self, node, kafka_security_protocol=None, offline_nodes=[]):
if self.quorum_info.using_kraft and not self.quorum_info.has_brokers:
raise Exception("Must invoke kafka-topics against a broker, not a KRaft controller")
- if force_use_zk_connection:
- bootstrap_server_or_zookeeper = "--zookeeper %s" % (self.zk_connect_setting())
- skip_optional_security_settings = True
- else:
- if kafka_security_protocol is None:
- # it wasn't specified, so use the inter-broker security protocol if it is PLAINTEXT,
- # otherwise use the client security protocol
- if self.interbroker_security_protocol == SecurityConfig.PLAINTEXT:
- security_protocol_to_use = SecurityConfig.PLAINTEXT
- else:
- security_protocol_to_use = self.security_protocol
+ if kafka_security_protocol is None:
+ # it wasn't specified, so use the inter-broker security protocol if it is PLAINTEXT,
+ # otherwise use the client security protocol
+ if self.interbroker_security_protocol == SecurityConfig.PLAINTEXT:
+ security_protocol_to_use = SecurityConfig.PLAINTEXT
else:
- security_protocol_to_use = kafka_security_protocol
- bootstrap_server_or_zookeeper = "--bootstrap-server %s" % (self.bootstrap_servers(security_protocol_to_use, offline_nodes=offline_nodes))
- skip_optional_security_settings = security_protocol_to_use == SecurityConfig.PLAINTEXT
- if skip_optional_security_settings:
- optional_jass_krb_system_props_prefix = ""
- optional_command_config_suffix = ""
+ security_protocol_to_use = self.security_protocol
else:
- # we need security configs because aren't going to ZooKeeper and we aren't using PLAINTEXT
- if (security_protocol_to_use == self.interbroker_security_protocol):
- # configure JAAS to provide the broker's credentials
- # since this is an authenticating cluster and we are going to use the inter-broker security protocol
- jaas_conf_prop = KafkaService.ADMIN_CLIENT_AS_BROKER_JAAS_CONF_PROPERTY
- use_inter_broker_mechanism_for_client = True
- else:
- # configure JAAS to provide the typical client credentials
- jaas_conf_prop = KafkaService.JAAS_CONF_PROPERTY
- use_inter_broker_mechanism_for_client = False
- # We are either using SASL (SASL_SSL or SASL_PLAINTEXT) or we are using SSL
- using_sasl = security_protocol_to_use != "SSL"
- optional_jass_krb_system_props_prefix = "KAFKA_OPTS='-D%s -D%s' " % (jaas_conf_prop, KafkaService.KRB5_CONF) if using_sasl else ""
- optional_command_config_suffix = " --command-config <(echo '%s')" % (self.security_config.client_config(use_inter_broker_mechanism_for_client = use_inter_broker_mechanism_for_client))
+ security_protocol_to_use = kafka_security_protocol
+ bootstrap_server_or_zookeeper = "--bootstrap-server %s" % (self.bootstrap_servers(security_protocol_to_use, offline_nodes=offline_nodes))
Review Comment:
@mimaison Sure. Rename the variable to `bootstrap_server`?
Do you recommend removing comments that have the word "zookeeper" in them?
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -1111,15 +1094,14 @@ def maybe_setup_client_scram_credentials(self, node):
security_config = self.security_config
# we only need to create client credentials when the client mechanism is SASL/SCRAM
if security_config.is_sasl(self.security_protocol) and security_config.is_sasl_scram(self.client_sasl_mechanism):
- force_use_zk_connection = not self.all_nodes_configs_command_uses_bootstrap_server_scram()
# ignored if forcing the use of Zookeeper, but we need a value to send, so calculate it anyway
Review Comment:
~ignored if forcing the use of Zookeeper, but~ we need a value to send, so calculate it anyway
=> # we need a value to send, so calculate it anyway
@mimaison is this what you meant?
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -1010,84 +1010,68 @@ def kafka_metadata_quorum_cmd(self, node, kafka_security_protocol=None, use_cont
kafka_metadata_script = self.path.script("kafka-metadata-quorum.sh", node)
return "{} {}".format(kafka_metadata_script, bootstrap)
- def kafka_topics_cmd_with_optional_security_settings(self, node, force_use_zk_connection, kafka_security_protocol=None, offline_nodes=[]):
+ def kafka_topics_cmd_with_optional_security_settings(self, node, kafka_security_protocol=None, offline_nodes=[]):
if self.quorum_info.using_kraft and not self.quorum_info.has_brokers:
raise Exception("Must invoke kafka-topics against a broker, not a KRaft controller")
- if force_use_zk_connection:
- bootstrap_server_or_zookeeper = "--zookeeper %s" % (self.zk_connect_setting())
- skip_optional_security_settings = True
- else:
- if kafka_security_protocol is None:
- # it wasn't specified, so use the inter-broker security protocol if it is PLAINTEXT,
- # otherwise use the client security protocol
- if self.interbroker_security_protocol == SecurityConfig.PLAINTEXT:
- security_protocol_to_use = SecurityConfig.PLAINTEXT
- else:
- security_protocol_to_use = self.security_protocol
+ if kafka_security_protocol is None:
+ # it wasn't specified, so use the inter-broker security protocol if it is PLAINTEXT,
+ # otherwise use the client security protocol
+ if self.interbroker_security_protocol == SecurityConfig.PLAINTEXT:
+ security_protocol_to_use = SecurityConfig.PLAINTEXT
else:
- security_protocol_to_use = kafka_security_protocol
- bootstrap_server_or_zookeeper = "--bootstrap-server %s" % (self.bootstrap_servers(security_protocol_to_use, offline_nodes=offline_nodes))
- skip_optional_security_settings = security_protocol_to_use == SecurityConfig.PLAINTEXT
- if skip_optional_security_settings:
- optional_jass_krb_system_props_prefix = ""
- optional_command_config_suffix = ""
+ security_protocol_to_use = self.security_protocol
else:
- # we need security configs because aren't going to ZooKeeper and we aren't using PLAINTEXT
- if (security_protocol_to_use == self.interbroker_security_protocol):
- # configure JAAS to provide the broker's credentials
- # since this is an authenticating cluster and we are going to use the inter-broker security protocol
- jaas_conf_prop = KafkaService.ADMIN_CLIENT_AS_BROKER_JAAS_CONF_PROPERTY
- use_inter_broker_mechanism_for_client = True
- else:
- # configure JAAS to provide the typical client credentials
- jaas_conf_prop = KafkaService.JAAS_CONF_PROPERTY
- use_inter_broker_mechanism_for_client = False
- # We are either using SASL (SASL_SSL or SASL_PLAINTEXT) or we are using SSL
- using_sasl = security_protocol_to_use != "SSL"
- optional_jass_krb_system_props_prefix = "KAFKA_OPTS='-D%s -D%s' " % (jaas_conf_prop, KafkaService.KRB5_CONF) if using_sasl else ""
- optional_command_config_suffix = " --command-config <(echo '%s')" % (self.security_config.client_config(use_inter_broker_mechanism_for_client = use_inter_broker_mechanism_for_client))
+ security_protocol_to_use = kafka_security_protocol
+ bootstrap_server_or_zookeeper = "--bootstrap-server %s" % (self.bootstrap_servers(security_protocol_to_use, offline_nodes=offline_nodes))
+ skip_optional_security_settings = security_protocol_to_use == SecurityConfig.PLAINTEXT
+
+ # we need security configs because aren't going to ZooKeeper and we aren't using PLAINTEXT
Review Comment:
@mimaison Is this what you meant?
# we need security configs because ~aren't going to ZooKeeper and~ we aren't using PLAINTEXT
=>
# we need security configs because we aren't using PLAINTEXT
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17715 remove force_use_zk_connection from e2e [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 closed pull request #17465: KAFKA-17715 remove force_use_zk_connection from e2e
URL: https://github.com/apache/kafka/pull/17465
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17715 remove force_use_zk_connection from e2e [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #17465:
URL: https://github.com/apache/kafka/pull/17465#issuecomment-2712494791
This PR is being marked as stale since it has not had any activity in 90 days. If you
would like to keep this PR alive, please leave a comment asking for a review. If the PR has
merge conflicts, update it with the latest from the base branch.
<p>
If you are having difficulty finding a reviewer, please reach out on the
[mailing list](https://kafka.apache.org/contact).
<p>
If this PR is no longer valid or desired, please feel free to close it. If no activity
occurs in the next 30 days, it will be automatically closed.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17715 remove force_use_zk_connection from e2e [kafka]
Posted by "mingdaoy (via GitHub)" <gi...@apache.org>.
mingdaoy commented on PR #17465:
URL: https://github.com/apache/kafka/pull/17465#issuecomment-2523325204
tests/kafkatest/tests/**client**/quota_test.py
Path should be **client**, not "clients"
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17715 remove force_use_zk_connection from e2e [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #17465:
URL: https://github.com/apache/kafka/pull/17465#discussion_r1799053345
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -1010,84 +1010,68 @@ def kafka_metadata_quorum_cmd(self, node, kafka_security_protocol=None, use_cont
kafka_metadata_script = self.path.script("kafka-metadata-quorum.sh", node)
return "{} {}".format(kafka_metadata_script, bootstrap)
- def kafka_topics_cmd_with_optional_security_settings(self, node, force_use_zk_connection, kafka_security_protocol=None, offline_nodes=[]):
+ def kafka_topics_cmd_with_optional_security_settings(self, node, kafka_security_protocol=None, offline_nodes=[]):
if self.quorum_info.using_kraft and not self.quorum_info.has_brokers:
raise Exception("Must invoke kafka-topics against a broker, not a KRaft controller")
- if force_use_zk_connection:
- bootstrap_server_or_zookeeper = "--zookeeper %s" % (self.zk_connect_setting())
- skip_optional_security_settings = True
- else:
- if kafka_security_protocol is None:
- # it wasn't specified, so use the inter-broker security protocol if it is PLAINTEXT,
- # otherwise use the client security protocol
- if self.interbroker_security_protocol == SecurityConfig.PLAINTEXT:
- security_protocol_to_use = SecurityConfig.PLAINTEXT
- else:
- security_protocol_to_use = self.security_protocol
+ if kafka_security_protocol is None:
+ # it wasn't specified, so use the inter-broker security protocol if it is PLAINTEXT,
+ # otherwise use the client security protocol
+ if self.interbroker_security_protocol == SecurityConfig.PLAINTEXT:
+ security_protocol_to_use = SecurityConfig.PLAINTEXT
else:
- security_protocol_to_use = kafka_security_protocol
- bootstrap_server_or_zookeeper = "--bootstrap-server %s" % (self.bootstrap_servers(security_protocol_to_use, offline_nodes=offline_nodes))
- skip_optional_security_settings = security_protocol_to_use == SecurityConfig.PLAINTEXT
- if skip_optional_security_settings:
- optional_jass_krb_system_props_prefix = ""
- optional_command_config_suffix = ""
+ security_protocol_to_use = self.security_protocol
else:
- # we need security configs because aren't going to ZooKeeper and we aren't using PLAINTEXT
- if (security_protocol_to_use == self.interbroker_security_protocol):
- # configure JAAS to provide the broker's credentials
- # since this is an authenticating cluster and we are going to use the inter-broker security protocol
- jaas_conf_prop = KafkaService.ADMIN_CLIENT_AS_BROKER_JAAS_CONF_PROPERTY
- use_inter_broker_mechanism_for_client = True
- else:
- # configure JAAS to provide the typical client credentials
- jaas_conf_prop = KafkaService.JAAS_CONF_PROPERTY
- use_inter_broker_mechanism_for_client = False
- # We are either using SASL (SASL_SSL or SASL_PLAINTEXT) or we are using SSL
- using_sasl = security_protocol_to_use != "SSL"
- optional_jass_krb_system_props_prefix = "KAFKA_OPTS='-D%s -D%s' " % (jaas_conf_prop, KafkaService.KRB5_CONF) if using_sasl else ""
- optional_command_config_suffix = " --command-config <(echo '%s')" % (self.security_config.client_config(use_inter_broker_mechanism_for_client = use_inter_broker_mechanism_for_client))
+ security_protocol_to_use = kafka_security_protocol
+ bootstrap_server_or_zookeeper = "--bootstrap-server %s" % (self.bootstrap_servers(security_protocol_to_use, offline_nodes=offline_nodes))
Review Comment:
> +1, but we should limit the cleanup to only the comments in kafka.py to avoid creating a large patch.
Agreed
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17715 remove force_use_zk_connection from e2e [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #17465:
URL: https://github.com/apache/kafka/pull/17465#issuecomment-2507850803
Thanks for the updates. Have you been able to run the updated system tests and validate they still pass?
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17715 remove force_use_zk_connection from e2e [kafka]
Posted by "mingdaoy (via GitHub)" <gi...@apache.org>.
mingdaoy commented on PR #17465:
URL: https://github.com/apache/kafka/pull/17465#issuecomment-2507752032
PTAL @mimaison @chia7712 I limit the comments cleanup to `force_use_zk_connection` code block because other comments area still have **zk** or **zookeeper** related code to be removed later.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17715 remove force_use_zk_connection from e2e [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #17465:
URL: https://github.com/apache/kafka/pull/17465#issuecomment-2718305049
@mimaison this ticket will be handled by @mingyen066
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17715 remove force_use_zk_connection from e2e [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #17465:
URL: https://github.com/apache/kafka/pull/17465#discussion_r1816602068
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -1010,84 +1010,68 @@ def kafka_metadata_quorum_cmd(self, node, kafka_security_protocol=None, use_cont
kafka_metadata_script = self.path.script("kafka-metadata-quorum.sh", node)
return "{} {}".format(kafka_metadata_script, bootstrap)
- def kafka_topics_cmd_with_optional_security_settings(self, node, force_use_zk_connection, kafka_security_protocol=None, offline_nodes=[]):
+ def kafka_topics_cmd_with_optional_security_settings(self, node, kafka_security_protocol=None, offline_nodes=[]):
if self.quorum_info.using_kraft and not self.quorum_info.has_brokers:
raise Exception("Must invoke kafka-topics against a broker, not a KRaft controller")
- if force_use_zk_connection:
- bootstrap_server_or_zookeeper = "--zookeeper %s" % (self.zk_connect_setting())
- skip_optional_security_settings = True
- else:
- if kafka_security_protocol is None:
- # it wasn't specified, so use the inter-broker security protocol if it is PLAINTEXT,
- # otherwise use the client security protocol
- if self.interbroker_security_protocol == SecurityConfig.PLAINTEXT:
- security_protocol_to_use = SecurityConfig.PLAINTEXT
- else:
- security_protocol_to_use = self.security_protocol
+ if kafka_security_protocol is None:
+ # it wasn't specified, so use the inter-broker security protocol if it is PLAINTEXT,
+ # otherwise use the client security protocol
+ if self.interbroker_security_protocol == SecurityConfig.PLAINTEXT:
+ security_protocol_to_use = SecurityConfig.PLAINTEXT
else:
- security_protocol_to_use = kafka_security_protocol
- bootstrap_server_or_zookeeper = "--bootstrap-server %s" % (self.bootstrap_servers(security_protocol_to_use, offline_nodes=offline_nodes))
- skip_optional_security_settings = security_protocol_to_use == SecurityConfig.PLAINTEXT
- if skip_optional_security_settings:
- optional_jass_krb_system_props_prefix = ""
- optional_command_config_suffix = ""
+ security_protocol_to_use = self.security_protocol
else:
- # we need security configs because aren't going to ZooKeeper and we aren't using PLAINTEXT
- if (security_protocol_to_use == self.interbroker_security_protocol):
- # configure JAAS to provide the broker's credentials
- # since this is an authenticating cluster and we are going to use the inter-broker security protocol
- jaas_conf_prop = KafkaService.ADMIN_CLIENT_AS_BROKER_JAAS_CONF_PROPERTY
- use_inter_broker_mechanism_for_client = True
- else:
- # configure JAAS to provide the typical client credentials
- jaas_conf_prop = KafkaService.JAAS_CONF_PROPERTY
- use_inter_broker_mechanism_for_client = False
- # We are either using SASL (SASL_SSL or SASL_PLAINTEXT) or we are using SSL
- using_sasl = security_protocol_to_use != "SSL"
- optional_jass_krb_system_props_prefix = "KAFKA_OPTS='-D%s -D%s' " % (jaas_conf_prop, KafkaService.KRB5_CONF) if using_sasl else ""
- optional_command_config_suffix = " --command-config <(echo '%s')" % (self.security_config.client_config(use_inter_broker_mechanism_for_client = use_inter_broker_mechanism_for_client))
+ security_protocol_to_use = kafka_security_protocol
+ bootstrap_server_or_zookeeper = "--bootstrap-server %s" % (self.bootstrap_servers(security_protocol_to_use, offline_nodes=offline_nodes))
+ skip_optional_security_settings = security_protocol_to_use == SecurityConfig.PLAINTEXT
+
+ # we need security configs because aren't going to ZooKeeper and we aren't using PLAINTEXT
Review Comment:
Yes, basically remove any mentions of ZooKeeper, and adjust the comments as needed so they still make sense.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17715 remove force_use_zk_connection from e2e [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #17465:
URL: https://github.com/apache/kafka/pull/17465#discussion_r1797199822
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -1010,84 +1010,68 @@ def kafka_metadata_quorum_cmd(self, node, kafka_security_protocol=None, use_cont
kafka_metadata_script = self.path.script("kafka-metadata-quorum.sh", node)
return "{} {}".format(kafka_metadata_script, bootstrap)
- def kafka_topics_cmd_with_optional_security_settings(self, node, force_use_zk_connection, kafka_security_protocol=None, offline_nodes=[]):
+ def kafka_topics_cmd_with_optional_security_settings(self, node, kafka_security_protocol=None, offline_nodes=[]):
if self.quorum_info.using_kraft and not self.quorum_info.has_brokers:
raise Exception("Must invoke kafka-topics against a broker, not a KRaft controller")
- if force_use_zk_connection:
- bootstrap_server_or_zookeeper = "--zookeeper %s" % (self.zk_connect_setting())
- skip_optional_security_settings = True
- else:
- if kafka_security_protocol is None:
- # it wasn't specified, so use the inter-broker security protocol if it is PLAINTEXT,
- # otherwise use the client security protocol
- if self.interbroker_security_protocol == SecurityConfig.PLAINTEXT:
- security_protocol_to_use = SecurityConfig.PLAINTEXT
- else:
- security_protocol_to_use = self.security_protocol
+ if kafka_security_protocol is None:
+ # it wasn't specified, so use the inter-broker security protocol if it is PLAINTEXT,
+ # otherwise use the client security protocol
+ if self.interbroker_security_protocol == SecurityConfig.PLAINTEXT:
+ security_protocol_to_use = SecurityConfig.PLAINTEXT
else:
- security_protocol_to_use = kafka_security_protocol
- bootstrap_server_or_zookeeper = "--bootstrap-server %s" % (self.bootstrap_servers(security_protocol_to_use, offline_nodes=offline_nodes))
- skip_optional_security_settings = security_protocol_to_use == SecurityConfig.PLAINTEXT
- if skip_optional_security_settings:
- optional_jass_krb_system_props_prefix = ""
- optional_command_config_suffix = ""
+ security_protocol_to_use = self.security_protocol
else:
- # we need security configs because aren't going to ZooKeeper and we aren't using PLAINTEXT
- if (security_protocol_to_use == self.interbroker_security_protocol):
- # configure JAAS to provide the broker's credentials
- # since this is an authenticating cluster and we are going to use the inter-broker security protocol
- jaas_conf_prop = KafkaService.ADMIN_CLIENT_AS_BROKER_JAAS_CONF_PROPERTY
- use_inter_broker_mechanism_for_client = True
- else:
- # configure JAAS to provide the typical client credentials
- jaas_conf_prop = KafkaService.JAAS_CONF_PROPERTY
- use_inter_broker_mechanism_for_client = False
- # We are either using SASL (SASL_SSL or SASL_PLAINTEXT) or we are using SSL
- using_sasl = security_protocol_to_use != "SSL"
- optional_jass_krb_system_props_prefix = "KAFKA_OPTS='-D%s -D%s' " % (jaas_conf_prop, KafkaService.KRB5_CONF) if using_sasl else ""
- optional_command_config_suffix = " --command-config <(echo '%s')" % (self.security_config.client_config(use_inter_broker_mechanism_for_client = use_inter_broker_mechanism_for_client))
+ security_protocol_to_use = kafka_security_protocol
+ bootstrap_server_or_zookeeper = "--bootstrap-server %s" % (self.bootstrap_servers(security_protocol_to_use, offline_nodes=offline_nodes))
Review Comment:
Should we now rename this variable?
Also there are still plenty of ZooKeeper related comments that don't make sense anymore, we should probably delete them. WDYT?
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17715 remove force_use_zk_connection from e2e [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #17465:
URL: https://github.com/apache/kafka/pull/17465#issuecomment-2718236601
@mingdaoy Are you still working on this?
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17715 remove force_use_zk_connection from e2e [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #17465:
URL: https://github.com/apache/kafka/pull/17465#issuecomment-2510213001
@mingdaoy Could you please run following e2e on your local?
```
tests/kafkatest/tests/clients/quota_test.py
tests/kafkatest/tests/core/controller_mutation_quota_test.py
tests/kafkatest/tests/core/authorizer_test.py
```
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17715 remove force_use_zk_connection from e2e [kafka]
Posted by "mingdaoy (via GitHub)" <gi...@apache.org>.
mingdaoy commented on PR #17465:
URL: https://github.com/apache/kafka/pull/17465#issuecomment-2406628249
Hey maintainers, when you have a sec, could you give the workflow a gentle nudge? Much appreciated!
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17715 remove force_use_zk_connection from e2e [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #17465:
URL: https://github.com/apache/kafka/pull/17465#discussion_r1798466501
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -1010,84 +1010,68 @@ def kafka_metadata_quorum_cmd(self, node, kafka_security_protocol=None, use_cont
kafka_metadata_script = self.path.script("kafka-metadata-quorum.sh", node)
return "{} {}".format(kafka_metadata_script, bootstrap)
- def kafka_topics_cmd_with_optional_security_settings(self, node, force_use_zk_connection, kafka_security_protocol=None, offline_nodes=[]):
+ def kafka_topics_cmd_with_optional_security_settings(self, node, kafka_security_protocol=None, offline_nodes=[]):
if self.quorum_info.using_kraft and not self.quorum_info.has_brokers:
raise Exception("Must invoke kafka-topics against a broker, not a KRaft controller")
- if force_use_zk_connection:
- bootstrap_server_or_zookeeper = "--zookeeper %s" % (self.zk_connect_setting())
- skip_optional_security_settings = True
- else:
- if kafka_security_protocol is None:
- # it wasn't specified, so use the inter-broker security protocol if it is PLAINTEXT,
- # otherwise use the client security protocol
- if self.interbroker_security_protocol == SecurityConfig.PLAINTEXT:
- security_protocol_to_use = SecurityConfig.PLAINTEXT
- else:
- security_protocol_to_use = self.security_protocol
+ if kafka_security_protocol is None:
+ # it wasn't specified, so use the inter-broker security protocol if it is PLAINTEXT,
+ # otherwise use the client security protocol
+ if self.interbroker_security_protocol == SecurityConfig.PLAINTEXT:
+ security_protocol_to_use = SecurityConfig.PLAINTEXT
else:
- security_protocol_to_use = kafka_security_protocol
- bootstrap_server_or_zookeeper = "--bootstrap-server %s" % (self.bootstrap_servers(security_protocol_to_use, offline_nodes=offline_nodes))
- skip_optional_security_settings = security_protocol_to_use == SecurityConfig.PLAINTEXT
- if skip_optional_security_settings:
- optional_jass_krb_system_props_prefix = ""
- optional_command_config_suffix = ""
+ security_protocol_to_use = self.security_protocol
else:
- # we need security configs because aren't going to ZooKeeper and we aren't using PLAINTEXT
- if (security_protocol_to_use == self.interbroker_security_protocol):
- # configure JAAS to provide the broker's credentials
- # since this is an authenticating cluster and we are going to use the inter-broker security protocol
- jaas_conf_prop = KafkaService.ADMIN_CLIENT_AS_BROKER_JAAS_CONF_PROPERTY
- use_inter_broker_mechanism_for_client = True
- else:
- # configure JAAS to provide the typical client credentials
- jaas_conf_prop = KafkaService.JAAS_CONF_PROPERTY
- use_inter_broker_mechanism_for_client = False
- # We are either using SASL (SASL_SSL or SASL_PLAINTEXT) or we are using SSL
- using_sasl = security_protocol_to_use != "SSL"
- optional_jass_krb_system_props_prefix = "KAFKA_OPTS='-D%s -D%s' " % (jaas_conf_prop, KafkaService.KRB5_CONF) if using_sasl else ""
- optional_command_config_suffix = " --command-config <(echo '%s')" % (self.security_config.client_config(use_inter_broker_mechanism_for_client = use_inter_broker_mechanism_for_client))
+ security_protocol_to_use = kafka_security_protocol
+ bootstrap_server_or_zookeeper = "--bootstrap-server %s" % (self.bootstrap_servers(security_protocol_to_use, offline_nodes=offline_nodes))
Review Comment:
> Should we now rename this variable?
+1
> Also there are still plenty of ZooKeeper related comments that don't make sense anymore, we should probably delete them. WDYT?
+1, but we should limit the cleanup to only the comments in kafka.py to avoid creating a large patch.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org