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 2021/05/14 16:46:21 UTC
[GitHub] [kafka] rondagostino opened a new pull request #10694: MINOR: fix system test TestSecurityRollingUpgrade
rondagostino opened a new pull request #10694:
URL: https://github.com/apache/kafka/pull/10694
SecurityConfig for a Kafka cluster in a system test is cached due to https://github.com/apache/kafka/pull/8917, but we mutate the security config during some system tests, and those mutations were not being passed through after-the-fact. These system tests were not testing what they were supposed to be testing. This patch passes through the potential changes so that we again test what we are supposed to be testing.
Also, since we became very specific about what SASL mechanisms to enable when updating the system tests for KRaft, we need to explicitly explicitly indicate to the SecurityConfig any additional SASL mechanisms that we want to enable. This was always necessary once we made the KRaft changes, but it was not apparent due to the above bug (where mutations were not being passed through). This patch provides a way to pass additional SASL mechanisms to the SecurityConfig by adding an option `sasl_mechanism` to KafkaListener -- this is what gets passed into the SecurityConfig when we enable a new security protocol in the middle of a system test.
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
### 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #10694: MINOR: fix system test TestSecurityRollingUpgrade
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10694:
URL: https://github.com/apache/kafka/pull/10694#issuecomment-842403412
Thanks, for the review @mumrah! Regarding your question:
> Looks like we're checking client security protocol + sasl mechanism as well as the inter broker protocol + sasl mechanism.
Do we need to do the same for "intercontroller_security_protocol" in KRaft mode?
The answer is yes -- it's a good point. I've opened https://issues.apache.org/jira/browse/KAFKA-12799 to extend the existing tests to apply to KRaft controllers, and I indicated in that ticket that we will have to take security config mutations into account for that implementation as we did here. I hope it's okay that we defer this to when we get to that ticket.
--
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 #10694: MINOR: fix system test TestSecurityRollingUpgrade
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10694:
URL: https://github.com/apache/kafka/pull/10694#issuecomment-841512146
Full system test output here: https://confluent-kafka-branch-builder-system-test-results.s3-us-west-2.amazonaws.com/2021-05-14--001.1621023792--rondagostino--fix_TestSecurityRollingUpgrade--78762fb8a/report.html
Streams test failures are unrelated. The one test failure in `TestSecurityRollingUpgrade` is fixed by the followup commit in this PR (https://github.com/apache/kafka/pull/10694/commits/6e4007fa3a805fde1e9933d4c1a200b472a16137)
--
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 #10694: MINOR: fix system test TestSecurityRollingUpgrade
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #10694:
URL: https://github.com/apache/kafka/pull/10694#discussion_r633598560
##########
File path: tests/kafkatest/services/security/security_config.py
##########
@@ -258,8 +259,10 @@ def enable_sasl(self):
def enable_ssl(self):
self.has_ssl = True
- def enable_security_protocol(self, security_protocol):
+ def enable_security_protocol(self, security_protocol, sasl_mechanism = None):
Review comment:
Good catch! I missed leveraging this, and it results in `sasl.enabled.mechanisms=` (i.e. blank) on the first of the two rolls. The test was still passing, but I think that's just luck. I've fixed it in `kafka.py` -- we now pass in the SASL mechanism, and we are now seeing `sasl.enabled.mechanisms=PLAIN` as expected.
--
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] mumrah merged pull request #10694: MINOR: fix system test TestSecurityRollingUpgrade
Posted by GitBox <gi...@apache.org>.
mumrah merged pull request #10694:
URL: https://github.com/apache/kafka/pull/10694
--
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] mumrah commented on a change in pull request #10694: MINOR: fix system test TestSecurityRollingUpgrade
Posted by GitBox <gi...@apache.org>.
mumrah commented on a change in pull request #10694:
URL: https://github.com/apache/kafka/pull/10694#discussion_r633552208
##########
File path: tests/kafkatest/services/security/security_config.py
##########
@@ -258,8 +259,10 @@ def enable_sasl(self):
def enable_ssl(self):
self.has_ssl = True
- def enable_security_protocol(self, security_protocol):
+ def enable_security_protocol(self, security_protocol, sasl_mechanism = None):
self.has_sasl = self.has_sasl or self.is_sasl(security_protocol)
+ if sasl_mechanism:
Review comment:
Safer to check `is not None`
##########
File path: tests/kafkatest/services/security/security_config.py
##########
@@ -387,6 +390,7 @@ def enabled_sasl_mechanisms(self):
sasl_mechanisms += list(self.uses_raft_sasl)
if self.zk_sasl:
sasl_mechanisms += [SecurityConfig.SASL_MECHANISM_GSSAPI]
+ sasl_mechanisms += self.additional_sasl_mechanisms
Review comment:
If we're adding all of one list to another list, we should use `extend`, e.g.
```python
sasl_mechanisms.extend(self.additional_sasl_mechanisms)
```
##########
File path: tests/kafkatest/tests/core/security_rolling_upgrade_test.py
##########
@@ -89,18 +89,21 @@ def add_sasl_mechanism(self, new_client_sasl_mechanism):
self.bounce()
def roll_in_sasl_mechanism(self, security_protocol, new_sasl_mechanism):
- # Roll cluster to update inter-broker SASL mechanism. This disables the old mechanism.
+ # Roll cluster to update inter-broker SASL mechanism.
+ # We need the inter-broker SASL mechanism to still be enabled through this roll.
+ self.kafka.client_sasl_mechanism = "%s,%s" % (self.kafka.interbroker_sasl_mechanism, new_sasl_mechanism)
self.kafka.interbroker_sasl_mechanism = new_sasl_mechanism
self.bounce()
# Bounce again with ACLs for new mechanism.
+ self.kafka.client_sasl_mechanism = new_sasl_mechanism # Removes old SASL mechanism completely
Review comment:
nit: two spaces between code and `#`
##########
File path: tests/kafkatest/services/security/security_config.py
##########
@@ -258,8 +259,10 @@ def enable_sasl(self):
def enable_ssl(self):
self.has_ssl = True
- def enable_security_protocol(self, security_protocol):
+ def enable_security_protocol(self, security_protocol, sasl_mechanism = None):
Review comment:
Do we make use of this new argument anywhere? I can't seem to find any
--
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 #10694: MINOR: fix system test TestSecurityRollingUpgrade
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #10694:
URL: https://github.com/apache/kafka/pull/10694#discussion_r633480466
##########
File path: tests/kafkatest/tests/core/security_rolling_upgrade_test.py
##########
@@ -89,18 +89,21 @@ def add_sasl_mechanism(self, new_client_sasl_mechanism):
self.bounce()
def roll_in_sasl_mechanism(self, security_protocol, new_sasl_mechanism):
- # Roll cluster to update inter-broker SASL mechanism. This disables the old mechanism.
+ # Roll cluster to update inter-broker SASL mechanism.
+ # We need the inter-broker SASL mechanism to still be enabled through this roll.
+ self.kafka.client_sasl_mechanism = "%s,%s" % (self.kafka.interbroker_sasl_mechanism, new_sasl_mechanism)
self.kafka.interbroker_sasl_mechanism = new_sasl_mechanism
self.bounce()
- # Bounce again with ACLs for new mechanism.
+ # Bounce again with ACLs for new mechanism. Use old SimpleAclAuthorizer here to ensure that is also tested.
+ self.kafka.client_sasl_mechanism = new_sasl_mechanism # Removes old SASL mechanism completely
self.set_authorizer_and_bounce(security_protocol, security_protocol)
def add_separate_broker_listener(self, broker_security_protocol, broker_sasl_mechanism):
# Enable the new internal listener on all brokers first
self.kafka.open_port(self.kafka.INTERBROKER_LISTENER_NAME)
self.kafka.port_mappings[self.kafka.INTERBROKER_LISTENER_NAME].security_protocol = broker_security_protocol
- self.kafka.client_sasl_mechanism = broker_sasl_mechanism
+ self.kafka.port_mappings[self.kafka.INTERBROKER_LISTENER_NAME].sasl_mechanism = broker_sasl_mechanism
Review comment:
We can't rely on the client SASL mechanism here with the KRaft-related changes that were made in https://github.com/apache/kafka/pull/10199/ because those KRaft changes made the code very explicit about which SASL mechanisms to add: if the client security protocol is PLAINTEXT then the client SASL mechanism isn't listed as an enabled mechanism. And in fact for this test the client security protocol is PLAINTEXT. So we need another way to transmit the SASL mechanism, and that's what the new optional `sas_mechanism` on the KafkaListener in the port mapping is for.
--
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] mumrah commented on pull request #10694: MINOR: fix system test TestSecurityRollingUpgrade
Posted by GitBox <gi...@apache.org>.
mumrah commented on pull request #10694:
URL: https://github.com/apache/kafka/pull/10694#issuecomment-842509752
Did a `--repeat 4` run of `security_rolling_upgrade_test.py` and everything passed https://jenkins.confluent.io/job/system-test-kafka-branch-builder/4514/
--
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 #10694: MINOR: fix system test TestSecurityRollingUpgrade
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10694:
URL: https://github.com/apache/kafka/pull/10694#issuecomment-842403412
Thanks, for the review @mumrah! Regarding your question:
> Looks like we're checking client security protocol + sasl mechanism as well as the inter broker protocol + sasl mechanism.
Do we need to do the same for "intercontroller_security_protocol" in KRaft mode?
The answer is yes -- it's a good point. I've opened https://issues.apache.org/jira/browse/KAFKA-12799 to extend the existing tests to apply to KRaft controllers, and I indicated in that ticket that we will have to take security config mutations into account for that implementation as we did here. I hope it's okay that we defer this to when we get to that ticket.
--
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] mumrah commented on a change in pull request #10694: MINOR: fix system test TestSecurityRollingUpgrade
Posted by GitBox <gi...@apache.org>.
mumrah commented on a change in pull request #10694:
URL: https://github.com/apache/kafka/pull/10694#discussion_r633552208
##########
File path: tests/kafkatest/services/security/security_config.py
##########
@@ -258,8 +259,10 @@ def enable_sasl(self):
def enable_ssl(self):
self.has_ssl = True
- def enable_security_protocol(self, security_protocol):
+ def enable_security_protocol(self, security_protocol, sasl_mechanism = None):
self.has_sasl = self.has_sasl or self.is_sasl(security_protocol)
+ if sasl_mechanism:
Review comment:
Safer to check `is not None`
##########
File path: tests/kafkatest/services/security/security_config.py
##########
@@ -387,6 +390,7 @@ def enabled_sasl_mechanisms(self):
sasl_mechanisms += list(self.uses_raft_sasl)
if self.zk_sasl:
sasl_mechanisms += [SecurityConfig.SASL_MECHANISM_GSSAPI]
+ sasl_mechanisms += self.additional_sasl_mechanisms
Review comment:
If we're adding all of one list to another list, we should use `extend`, e.g.
```python
sasl_mechanisms.extend(self.additional_sasl_mechanisms)
```
##########
File path: tests/kafkatest/tests/core/security_rolling_upgrade_test.py
##########
@@ -89,18 +89,21 @@ def add_sasl_mechanism(self, new_client_sasl_mechanism):
self.bounce()
def roll_in_sasl_mechanism(self, security_protocol, new_sasl_mechanism):
- # Roll cluster to update inter-broker SASL mechanism. This disables the old mechanism.
+ # Roll cluster to update inter-broker SASL mechanism.
+ # We need the inter-broker SASL mechanism to still be enabled through this roll.
+ self.kafka.client_sasl_mechanism = "%s,%s" % (self.kafka.interbroker_sasl_mechanism, new_sasl_mechanism)
self.kafka.interbroker_sasl_mechanism = new_sasl_mechanism
self.bounce()
# Bounce again with ACLs for new mechanism.
+ self.kafka.client_sasl_mechanism = new_sasl_mechanism # Removes old SASL mechanism completely
Review comment:
nit: two spaces between code and `#`
##########
File path: tests/kafkatest/services/security/security_config.py
##########
@@ -258,8 +259,10 @@ def enable_sasl(self):
def enable_ssl(self):
self.has_ssl = True
- def enable_security_protocol(self, security_protocol):
+ def enable_security_protocol(self, security_protocol, sasl_mechanism = None):
Review comment:
Do we make use of this new argument anywhere? I can't seem to find any
--
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 #10694: MINOR: fix system test TestSecurityRollingUpgrade
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #10694:
URL: https://github.com/apache/kafka/pull/10694#discussion_r633480466
##########
File path: tests/kafkatest/tests/core/security_rolling_upgrade_test.py
##########
@@ -89,18 +89,21 @@ def add_sasl_mechanism(self, new_client_sasl_mechanism):
self.bounce()
def roll_in_sasl_mechanism(self, security_protocol, new_sasl_mechanism):
- # Roll cluster to update inter-broker SASL mechanism. This disables the old mechanism.
+ # Roll cluster to update inter-broker SASL mechanism.
+ # We need the inter-broker SASL mechanism to still be enabled through this roll.
+ self.kafka.client_sasl_mechanism = "%s,%s" % (self.kafka.interbroker_sasl_mechanism, new_sasl_mechanism)
self.kafka.interbroker_sasl_mechanism = new_sasl_mechanism
self.bounce()
- # Bounce again with ACLs for new mechanism.
+ # Bounce again with ACLs for new mechanism. Use old SimpleAclAuthorizer here to ensure that is also tested.
+ self.kafka.client_sasl_mechanism = new_sasl_mechanism # Removes old SASL mechanism completely
self.set_authorizer_and_bounce(security_protocol, security_protocol)
def add_separate_broker_listener(self, broker_security_protocol, broker_sasl_mechanism):
# Enable the new internal listener on all brokers first
self.kafka.open_port(self.kafka.INTERBROKER_LISTENER_NAME)
self.kafka.port_mappings[self.kafka.INTERBROKER_LISTENER_NAME].security_protocol = broker_security_protocol
- self.kafka.client_sasl_mechanism = broker_sasl_mechanism
+ self.kafka.port_mappings[self.kafka.INTERBROKER_LISTENER_NAME].sasl_mechanism = broker_sasl_mechanism
Review comment:
We can't rely on the client SASL mechanism here with the KRaft-related changes that were made in https://github.com/apache/kafka/pull/10199/ because those KRaft changes made the code very explicit about which SASL mechanisms to add: if the client security protocol is PLAINTEXT then the client SASL mechanism isn't listed as an enabled mechanism. And in fact for this test the client security protocol is PLAINTEXT. So we need another way to transmit the SASL mechanism, and that's what the new optional `sas_mechanism` on the KafkaListener in the port mapping is for.
##########
File path: tests/kafkatest/services/security/security_config.py
##########
@@ -258,8 +259,10 @@ def enable_sasl(self):
def enable_ssl(self):
self.has_ssl = True
- def enable_security_protocol(self, security_protocol):
+ def enable_security_protocol(self, security_protocol, sasl_mechanism = None):
Review comment:
Good catch! I missed leveraging this, and it results in `sasl.enabled.mechanisms=` (i.e. blank) on the first of the two rolls. The test was still passing, but I think that's just luck. I've fixed it in `kafka.py` -- we now pass in the SASL mechanism, and we are now seeing `sasl.enabled.mechanisms=PLAIN` as expected.
--
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] mumrah merged pull request #10694: MINOR: fix system test TestSecurityRollingUpgrade
Posted by GitBox <gi...@apache.org>.
mumrah merged pull request #10694:
URL: https://github.com/apache/kafka/pull/10694
--
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] mumrah commented on pull request #10694: MINOR: fix system test TestSecurityRollingUpgrade
Posted by GitBox <gi...@apache.org>.
mumrah commented on pull request #10694:
URL: https://github.com/apache/kafka/pull/10694#issuecomment-842509752
Did a `--repeat 4` run of `security_rolling_upgrade_test.py` and everything passed https://jenkins.confluent.io/job/system-test-kafka-branch-builder/4514/
--
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