You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metron.apache.org by justinleet <gi...@git.apache.org> on 2017/05/31 19:20:49 UTC
[GitHub] metron pull request #600: METRON-976 KafkaUtils doesn't handle SASL_PLAINTEX...
GitHub user justinleet opened a pull request:
https://github.com/apache/metron/pull/600
METRON-976 KafkaUtils doesn't handle SASL_PLAINTEXT
## Contributor Comments
Used @cestella's original implementation before I apparently foolishly assumed we'd get Java compliant URIs. See: https://github.com/apache/metron/pull/486/commits/5d36c79effec75f6ac95fa587f80da0bd5420135
Unit testing is updated, and now checks that SASL_PLAINTEXT works (not just PLAINTEXTSASL). I still need to spin up full dev.
@cestella Let me know how you'd like to handle credit, given that you originally wrote it.
## Pull Request Checklist
Thank you for submitting a contribution to Apache Metron.
Please refer to our [Development Guidelines](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61332235) for the complete guide to follow for contributions.
Please refer also to our [Build Verification Guidelines](https://cwiki.apache.org/confluence/display/METRON/Verifying+Builds?show-miniview) for complete smoke testing guides.
In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? If not one needs to be created at [Metron Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel).
- [x] Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
### For code changes:
- [] Have you included steps to reproduce the behavior or problem that is being changed or addressed?
- [ ] Have you included steps or a guide to how the change may be verified and tested manually?
- [ ] Have you ensured that the full suite of tests and checks have been executed in the root incubating-metron folder via:
```
mvn -q clean integration-test install && build_utils/verify_licenses.sh
```
- [x] Have you written or updated unit tests and or integration tests to verify your changes?
- [ ] Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
#### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
It is also recommended that [travis-ci](https://travis-ci.org) is set up for your personal repository such that your branches are built there before submitting a pull request.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/justinleet/metron METRON-976
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metron/pull/600.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #600
----
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron issue #600: METRON-976 KafkaUtils doesn't handle SASL_PLAINTEXT
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/600
@mattf-horton Updated with a more explicit set of unit tests around it and a comment explaining the assumption made by the function. Let me know if there's anything else you want or need to make sure everything's both correct and easily understandable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #600: METRON-976 KafkaUtils doesn't handle SASL_PLAINTEX...
Posted by mattf-horton <gi...@git.apache.org>.
Github user mattf-horton commented on a diff in the pull request:
https://github.com/apache/metron/pull/600#discussion_r120429871
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/utils/KafkaUtils.java ---
@@ -68,12 +68,14 @@
return ret;
}
- public List<String> fromEndpoint(String url) throws URISyntaxException {
+ public List<String> fromEndpoint(String url){
List<String> ret = new ArrayList<>();
if(url != null) {
- URI uri = new URI(url);
- int port = uri.getPort();
- ret.add(uri.getHost() + ((port > 0)?(":" + port):""));
+ Iterable<String> splits = Splitter.on("//").split(url);
+ if(Iterables.size(splits) == 2) {
+ String hostPort = Iterables.getLast(splits);
+ ret.add(hostPort);
--- End diff --
Thanks, @justinleet , and sorry if I'm being a PITA. By inspection, I have the following concerns:
1. The URI solution always delivers exactly "host[:port]" (or fails). The split solution delivers the entire url except the protocol, ie "host[:port]/path/extension/if/any...", unless there's something upstream cutting off the path extension (if any). Is there? I didn't see anything, it looks like endpoint urls are simply read raw from a config JSON structure. So I would have expected trimming the proposed split at both ends. Unless endpoint URLs are supposed to allow path extensions, in which case the URI approach is clearly wrong.
2. The split code only returns a result if the split size is == 2. This prevents it from accepting:
* Endpoint urls without the protocol prefix
* Endpoint urls with path extensions that accidentally have double slashes in them (a common problem with machine-generated URLs, and accepted by most URL path processors)
It also does not provide any log feedback on why it silently ignores "bad" endpoint urls.
None of these issues would show up in testing unless the unit tests include thorough negative testcases for invalid configured endpoint URLs.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron issue #600: METRON-976 KafkaUtils doesn't handle SASL_PLAINTEXT
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/metron/pull/600
I will happily assign over credit to you. Thanks for catching the bug @justinleet
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #600: METRON-976 KafkaUtils doesn't handle SASL_PLAINTEX...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/600#discussion_r120369355
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/utils/KafkaUtils.java ---
@@ -68,12 +68,14 @@
return ret;
}
- public List<String> fromEndpoint(String url) throws URISyntaxException {
+ public List<String> fromEndpoint(String url){
List<String> ret = new ArrayList<>();
if(url != null) {
- URI uri = new URI(url);
- int port = uri.getPort();
- ret.add(uri.getHost() + ((port > 0)?(":" + port):""));
+ Iterable<String> splits = Splitter.on("//").split(url);
+ if(Iterables.size(splits) == 2) {
+ String hostPort = Iterables.getLast(splits);
+ ret.add(hostPort);
--- End diff --
The problem is that the URI based solution that was implemented in that patch (I believe based on my suggestion, sigh) doesn't allow for protocols with underscore characters. It's entirely independent of SASL_PLAINTEXT, other than the fact that underscore is in it.
The intention was to exactly reverse the chunk you mention, because the original didn't care about underscores.
On further inspection (which I'm glad you did!), these are not equivalent before and after. Which begs the question of why things worked in testing. I'll dig in a bit more to figure out what's going on under the hood here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #600: METRON-976 KafkaUtils doesn't handle SASL_PLAINTEX...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metron/pull/600
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #600: METRON-976 KafkaUtils doesn't handle SASL_PLAINTEX...
Posted by mattf-horton <gi...@git.apache.org>.
Github user mattf-horton commented on a diff in the pull request:
https://github.com/apache/metron/pull/600#discussion_r119961464
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/utils/KafkaUtils.java ---
@@ -68,12 +68,14 @@
return ret;
}
- public List<String> fromEndpoint(String url) throws URISyntaxException {
+ public List<String> fromEndpoint(String url){
List<String> ret = new ArrayList<>();
if(url != null) {
- URI uri = new URI(url);
- int port = uri.getPort();
- ret.add(uri.getHost() + ((port > 0)?(":" + port):""));
+ Iterable<String> splits = Splitter.on("//").split(url);
+ if(Iterables.size(splits) == 2) {
+ String hostPort = Iterables.getLast(splits);
+ ret.add(hostPort);
--- End diff --
Hi @justinleet , perhaps it is because I don't know how SASL_PLAINTEXT endpoints work, but I don't get the change above. I fully understand the deleted lines, and don't see how the substituted lines are equivalent, or what was wrong with the former. Would you mind giving a bit of explanation? Thanks.
Also, I just went back and re-read your introduction to this PR, and followed the link to (5d36c79)[https://github.com/apache/metron/commit/5d36c79effec75f6ac95fa587f80da0bd5420135]. It appears you've exactly reversed a chunk of @cestella 's patch of Mar 23. Was this intended? I'm even more confused :-)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #600: METRON-976 KafkaUtils doesn't handle SASL_PLAINTEX...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/600#discussion_r120434003
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/utils/KafkaUtils.java ---
@@ -68,12 +68,14 @@
return ret;
}
- public List<String> fromEndpoint(String url) throws URISyntaxException {
+ public List<String> fromEndpoint(String url){
List<String> ret = new ArrayList<>();
if(url != null) {
- URI uri = new URI(url);
- int port = uri.getPort();
- ret.add(uri.getHost() + ((port > 0)?(":" + port):""));
+ Iterable<String> splits = Splitter.on("//").split(url);
+ if(Iterables.size(splits) == 2) {
+ String hostPort = Iterables.getLast(splits);
+ ret.add(hostPort);
--- End diff --
@mattf-horton You're absolutely not being a PITA. Honestly, I tried to get a solution to a known problem out a little too quickly because it was actually experienced, so I'm actually pretty happy you're double checking me on this so something half baked didn't go in.
So looking back at this a bit more fresh, this only gets called from `getBrokersFromZookeeper`. The fact that's it's public isn't reflective of it's use (I'd probably argue package private, so it can be unit tested, or just better unit tested in general).
In practice, these aren't general URLs. They're specifically from https://cwiki.apache.org/confluence/display/KAFKA/Kafka+data+structures+in+Zookeeper
e.g.
```
"endpoints": ["PLAINTEXT://host1:9092", "SSL://host1:9093"]
```
So it's exactly protocol://host[:port] (to the best of my knowledge). So while the URI solution is probably technically better (outside of the underscore issue), fixing the split should be perfectly fine, if a bit kludgier.
Looking back at it, I'm pretty sure I didn't hit the right code path to trigger it in testing. I'm going to adjust the unit tests to actually do something more useful, so we avoid these issues in the future. I'm also going to add a comment, so it's very explicit what the actual assumptions made by that function are.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron issue #600: METRON-976 KafkaUtils doesn't handle SASL_PLAINTEXT
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/600
Ran up in full dev, kerberized, stopped metron, changed configs in Kafka referencing PLAINTEXT/PLAINTEXT_SASL to SASL_PLAINTEXT (listeners and security.inter.broker.protocol), and restarted kafka and metron. Ensured data still ran through to ES successfully.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron issue #600: METRON-976 KafkaUtils doesn't handle SASL_PLAINTEXT
Posted by mattf-horton <gi...@git.apache.org>.
Github user mattf-horton commented on the issue:
https://github.com/apache/metron/pull/600
+1 by inspection. Thanks for expanding the unit test to be more complete and understandable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---