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/03/01 16:35:12 UTC
[GitHub] [kafka] rondagostino opened a new pull request #10237: MINOR: fix failing system test delegation_token_test
rondagostino opened a new pull request #10237:
URL: https://github.com/apache/kafka/pull/10237
The system test in `delegation_token_test.py` broke due to https://github.com/apache/kafka/pull/10199/.
That patch changed the logic of `SecurityConfig.enabled_sasl_mechanisms()` to only add the inter-broker SASL mechanism when the inter-broker protocol was `SASL_{PLAINTEXT,SSL}`. The inter-broker protocol is `PLAINTEXT` in `delegation_token_test.py`, so the default inter-broker SASL mechanism of `GSSAPI` was not being added into the set returned by `enabled_sasl_mechanisms()`. This is actually correct -- it shouldn't be added if it isn't used for inter-broker communication. It should be added because clients use it, of course -- `SASL_PLAINTEXT` is the security protocol on an advertised listener, and `client_sasl_mechanism` is set to the .csv value `"GSSAPI,SCRAM-SHA-256"`in `delegation_token_test`. Unfortunately in https://github.com/apache/kafka/pull/10199/ we did not take into account the possibility that `client_sasl_mechanism` could be a .csv value, and we therefore fail to create a `krb5.conf` file, which causes `kafka-delegation_tokens.sh` to fail. This bug of .csv omi
ssion therefore uncovered a different bug -- we were relying on the default inter-broker SASL mechanism to signal that Kerberos was being used even though the inter-broker protocol wasn't SASL. This patch explicitly includes the elements of the `client_sasl_mechanism` .csv value (which in most cases is just a single value but in `delegation_token_test` it is not).
### 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 a change in pull request #10237: MINOR: fix failing system test delegation_token_test
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #10237:
URL: https://github.com/apache/kafka/pull/10237#discussion_r585829658
##########
File path: tests/kafkatest/services/security/security_config.py
##########
@@ -374,7 +374,8 @@ def interbroker_sasl_mechanism(self):
def enabled_sasl_mechanisms(self):
sasl_mechanisms = []
if self.is_sasl(self.security_protocol):
- sasl_mechanisms += [self.client_sasl_mechanism]
+ # .csv is supported so be sure to account for that possibility
+ sasl_mechanisms += self.client_sasl_mechanism.strip().split(',')
Review comment:
> Not sure if we should bother in this PR, but the usages of client_sasl_mechanism could stand to be cleaned up
I agree it needs to be cleaned up. Given we are past code freeze for 2.8, I've opened https://issues.apache.org/jira/browse/KAFKA-12402 for this and we can address it another time.
----------------------------------------------------------------
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] cmccabe merged pull request #10237: MINOR: fix failing system test delegation_token_test
Posted by GitBox <gi...@apache.org>.
cmccabe merged pull request #10237:
URL: https://github.com/apache/kafka/pull/10237
----------------------------------------------------------------
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 #10237: MINOR: fix failing system test delegation_token_test
Posted by GitBox <gi...@apache.org>.
mumrah commented on a change in pull request #10237:
URL: https://github.com/apache/kafka/pull/10237#discussion_r585784850
##########
File path: tests/kafkatest/services/security/security_config.py
##########
@@ -374,7 +374,8 @@ def interbroker_sasl_mechanism(self):
def enabled_sasl_mechanisms(self):
sasl_mechanisms = []
if self.is_sasl(self.security_protocol):
- sasl_mechanisms += [self.client_sasl_mechanism]
+ # .csv is supported so be sure to account for that possibility
+ sasl_mechanisms += self.client_sasl_mechanism.strip().split(',')
Review comment:
Not sure if we should bother in this PR, but the usages of `client_sasl_mechanism` could stand to be cleaned up. In `SecurityConfig.__init__` we default it to a simple string, but as you found here (and as seen in `SecurityConfig.client_config`) we support it being a comma delimited string.
https://github.com/apache/kafka/blob/58b3b1b557e9ba19cffde91bd117a89b947f1fc1/tests/kafkatest/services/security/security_config.py#L240-L253
It's probably a lot safer to declare this as a list in the class and not worry about having to do the `split(",")` everywhere. Though maybe there's a reason why we split it lazily.. not sure.
Either way, this change looks good. However, you might consider doing something like:
```python
sasl_mechanisms += [mechanism.strip() for mechanism in self.client_sasl_mechanism.split(',')]
```
since `self.client_sasl_mechanism.strip()` won't catch spaces in the middle of the string.
----------------------------------------------------------------
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