You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/10/06 00:59:02 UTC
[GitHub] [kafka] rondagostino opened a new pull request #9378: MINOR: ACLs for secured cluster system tests
rondagostino opened a new pull request #9378:
URL: https://github.com/apache/kafka/pull/9378
This PR adds missing broker ACLs required to create topics and SCRAM credentials when ACLs are enabled for a system test. These ACLs were missed for system tests in the PR for KAFKA-10131 (https://github.com/apache/kafka/pull/9274/). This PR also adds support for using PLAINTEXT as the inter broker security protocol when using SCRAM from the client in a system test with a secured cluster-- without this it would always be necessary to set both the inter-broker and client mechanisms to a SCRAM mechanism.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on a change in pull request #9378: MINOR: ACLs for secured cluster system tests
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #9378:
URL: https://github.com/apache/kafka/pull/9378#discussion_r500464258
##########
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:
This code is identifying the port to contact when we are using `--bootstrap-server` instead of `--zookeeper`, and we use the port associated with `PLAINTEXT` rather than `SASL_{PLAINTEXT,SSL}` or `SSL` if `PLAINTEXT` is in use. The assumption is that if `PLAINTEXT` is in use it will be the inter-broker security protocol rather than (or in addition to) the client security protocol, which is the case.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #9378: MINOR: ACLs for secured cluster system tests
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #9378:
URL: https://github.com/apache/kafka/pull/9378#issuecomment-705196327
@rajinisivaram I ran some tests locally and found some issues, which I fixed with an additional commit. I'll kick off a system test run -- I think we should run everything just to be safe since creating topics happens for every test.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] rajinisivaram commented on a change in pull request #9378: MINOR: ACLs for secured cluster system tests
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on a change in pull request #9378: MINOR: ACLs for secured cluster system tests
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #9378:
URL: https://github.com/apache/kafka/pull/9378#discussion_r500460487
##########
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:
Yes, Alter is needed to create user SCRAM credentials, and Create is needed to create topics. When we start up a cluster we create the `__consumer_offsets` topic and a `test_topic` (typically). If the test is using SCRAM we also create the SCRAM credentials at this point. We now use `--bootstrap-server` instead of `--zookeeper` for these CLI operations, and without these ACLs a system test will not be able to perform these necessary actions if security is enabled.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] rajinisivaram commented on pull request #9378: MINOR: ACLs for secured cluster system tests
Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on pull request #9378:
URL: https://github.com/apache/kafka/pull/9378#issuecomment-706216748
@rondagostino Thanks for the PR updates and test runs. Merging to trunk.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #9378: MINOR: ACLs for secured cluster system tests
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #9378:
URL: https://github.com/apache/kafka/pull/9378#issuecomment-705060101
@rajinisivaram Thanks for the insightful feedback -- I think the latest commit does much at address the clarity/refactoring issue as well as make the extra ACLs optional such that all system tests will now run with the same ACLs as before unless explicitly requested otherwise. Take a look and let me know what you think.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] rajinisivaram commented on a change in pull request #9378: MINOR: ACLs for secured cluster system tests
Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on a change in pull request #9378:
URL: https://github.com/apache/kafka/pull/9378#discussion_r500981702
##########
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:
I guess the problem is we don't have a separate principal in tests that we can assign those to. Could we separate out `Create` and `Alter` into another method, so that tests can set them as necessary or would that impact every test? We expect brokers to have only `ClusterAction`, but our docs and generally everyone expects broker to have more permissions. This sort of makes our tests run with a combination of permissions that we never expect a principal to have - broker principal should have only ClusterAction, no one else should have ClusterAction. Separating out into two methods may be better even if we end up using the same principal in 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:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on a change in pull request #9378: MINOR: ACLs for secured cluster system tests
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #9378:
URL: https://github.com/apache/kafka/pull/9378#discussion_r500465547
##########
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:
Yes, good point, I think it would be a helpful to consolidate the assumptions into one method (or a couple or a few, at most). I'll see what I can do about refactoring those assumptions out and reusing the logic instead of having it sprinkled around so much as it is now.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] rajinisivaram merged pull request #9378: MINOR: ACLs for secured cluster system tests
Posted by GitBox <gi...@apache.org>.
rajinisivaram merged pull request #9378:
URL: https://github.com/apache/kafka/pull/9378
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #9378: MINOR: ACLs for secured cluster system tests
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #9378:
URL: https://github.com/apache/kafka/pull/9378#issuecomment-705552615
Rebased onto trunk despite lack of conflicts due to Python 3 for system tests being merged.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #9378: MINOR: ACLs for secured cluster system tests
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #9378:
URL: https://github.com/apache/kafka/pull/9378#issuecomment-706242425
Test runs as follows:
Status of trunk 2 days ago:
http://confluent-kafka-system-test-results.s3-us-west-2.amazonaws.com/2020-10-07--001.1602079305--apache--trunk--af27c2ddf/report.html
Tests = 684 | Passed = 487 | Failed = 27 | Ignored = 170
-- | -- | -- | --
Status of this PR before final `ZooKeeperSecurityUpgradeTest` fix:
http://confluent-kafka-branch-builder-system-test-results.s3-us-west-2.amazonaws.com/2020-10-08--001.1602215668--rondagostino--rtd_scram_acl_system_test--8ca559f83/report.html
Tests = 606 | Passed = 407 | Failed = 29 | Ignored = 170
-- | -- | -- | --
Status of `ZooKeeperSecurityUpgradeTest` tests after fix:
http://confluent-kafka-branch-builder-system-test-results.s3-us-west-2.amazonaws.com/2020-10-09--001.1602252468--rondagostino--rtd_scram_acl_system_test--a72d5986d/report.html
Tests = 4 | Passed = 4 | Failed = 0 | Ignored = 0
-- | -- | -- | --
Net effect of this PR:
Tests = 606 | Passed = 411 | Failed = 25 | Ignored = 170
-- | -- | -- | --
What about the 78 missing tests? They disappeared on or after the python3 merge PR.
Here are missing tests succeeding just prior to the python3 merge: http://confluent-kafka-branch-builder-system-test-results.s3-us-west-2.amazonaws.com/2020-10-09--001.1602255415--rondagostino--rtd_just_before_python3_merge--40a23cc0c/report.html
Here are the tests results for the same tests on current trunk: http://testing.confluent.io/confluent-kafka-system-test-results/?prefix=2020-10-09--001.1602251990--apache--trunk--7947c18b5/. There are no tests listed there (no report.html) because they failed with `[ERROR:2020-10-09 13:59:48,990]: Failed to import kafkatest.sanity_checks.test_performance_services, which may indicate a broken test that cannot be loaded: ImportError: No module named server`
Will run against the actual python3 merge to see if that is the culprit (seems likely).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #9378: MINOR: ACLs for secured cluster system tests
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #9378:
URL: https://github.com/apache/kafka/pull/9378#issuecomment-705552615
Rebased onto trunk despite lack of conflicts due to Python 3 for system tests being merged.
----------------------------------------------------------------
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:
users@infra.apache.org