MINOR: fix system test TestSecurityRollingUpgrade#10694
Conversation
|
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 |
| self.kafka.client_sasl_mechanism = broker_sasl_mechanism | ||
| self.kafka.port_mappings[self.kafka.INTERBROKER_LISTENER_NAME].sasl_mechanism = broker_sasl_mechanism |
There was a problem hiding this comment.
We can't rely on the client SASL mechanism here with the KRaft-related changes that were made in #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.
mumrah
left a comment
mumrah
left a comment
There was a problem hiding this comment.
Thanks for the patch @rondagostino. 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?
| 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: |
| sasl_mechanisms += list(self.uses_raft_sasl) | ||
| if self.zk_sasl: | ||
| sasl_mechanisms += [SecurityConfig.SASL_MECHANISM_GSSAPI] | ||
| sasl_mechanisms += self.additional_sasl_mechanisms |
There was a problem hiding this comment.
If we're adding all of one list to another list, we should use extend, e.g.
sasl_mechanisms.extend(self.additional_sasl_mechanisms)| self.bounce() | ||
|
|
||
| # Bounce again with ACLs for new mechanism. | ||
| self.kafka.client_sasl_mechanism = new_sasl_mechanism # Removes old SASL mechanism completely |
There was a problem hiding this comment.
nit: two spaces between code and #
| self.has_ssl = True | ||
|
|
||
| def enable_security_protocol(self, security_protocol): | ||
| def enable_security_protocol(self, security_protocol, sasl_mechanism = None): |
There was a problem hiding this comment.
Do we make use of this new argument anywhere? I can't seem to find any
There was a problem hiding this 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.
|
Thanks, for the review @mumrah! Regarding your question:
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. |
|
Did a |
2 participants
SecurityConfig for a Kafka cluster in a system test is cached due to #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 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 optional
sasl_mechanismto 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)