You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metron.apache.org by cestella <gi...@git.apache.org> on 2016/12/21 23:06:53 UTC
[GitHub] incubator-metron pull request #402: METRON-639: The Network Stellar function...
GitHub user cestella opened a pull request:
https://github.com/apache/incubator-metron/pull/402
METRON-639: The Network Stellar functions need to have better unit testing
We have very little unit test coverage around the Networking functions in Stellar at the edge level. When diving a bit deeper on real data, I found a number of bugs around:
* Domains with TLDs that are not part of the proper set of TLDs
* URIs with schemes that Java doesn't know about.
* N_SUBNET takes multiple CIDRs, but only evaluates the first one.
* Generally calling validate on these methods can be unsafe because they do not handle null arguments correctly.
This just shores up the functions and provides more explicit and detailed unit testing. If I missed a test case or two, let me know and I'll add it in.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cestella/incubator-metron NetworkFunctionBug
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-metron/pull/402.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 #402
----
commit 16720acf00f5f9a62bbacfbba959baf0db97fc47
Author: cstella <ce...@gmail.com>
Date: 2016-12-21T23:00:57Z
Added more rigorous tests to look at edge cases for network stellar functions.
----
---
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] incubator-metron issue #402: METRON-639: The Network Stellar functions need ...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/incubator-metron/pull/402
@justinleet You ok with me just using Java URL?
---
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] incubator-metron pull request #402: METRON-639: The Network Stellar function...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on a diff in the pull request:
https://github.com/apache/incubator-metron/pull/402#discussion_r93665067
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/dsl/functions/NetworkFunctions.java ---
@@ -163,12 +163,16 @@ public Object apply(List<Object> objects) {
public static class URLToPort extends BaseStellarFunction {
@Override
public Object apply(List<Object> objects) {
--- End diff --
I did make these changes to the variables, but did not change the docs since the functions are literally called `URL_*`.
---
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] incubator-metron issue #402: METRON-639: The Network Stellar functions need ...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/incubator-metron/pull/402
@justinleet It's a good point you make and a very good catch. I am using URIs specifically because if Java doesn't know about the protocol scheme, then it bails. I'd hate for someone to ingest URLs of a custom scheme or a scheme that is more esoteric than is supported by Java and have it bail.
That being said, after further investigation, URLs are not a subset of URIs. As per http://stackoverflow.com/questions/176264/what-is-the-difference-between-a-uri-a-url-and-a-urn/1984225#1984225:
> Not "all URLs are URIs". It depends on the interpretation of the RFC. For example in Java the URI parser does not like [ or ] and that's because the spec says "should not" and not "shall not".
I'll have to consider whether or not I want to revert back to URL's and make a comment for further study/analysis or whether I want to try something else 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] incubator-metron issue #402: METRON-639: The Network Stellar functions need ...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/incubator-metron/pull/402
Ok, I'm reverting this back to just using java's URL class. The URL validator is a bit picky too. If this becomes an issue (dealing with nonstandard protocols, we can cross that bridge in another PR).
---
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] incubator-metron issue #402: METRON-639: The Network Stellar functions need ...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/incubator-metron/pull/402
+1 by inspection
---
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] incubator-metron pull request #402: METRON-639: The Network Stellar function...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/incubator-metron/pull/402#discussion_r93649682
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/dsl/functions/NetworkFunctions.java ---
@@ -163,12 +163,16 @@ public Object apply(List<Object> objects) {
public static class URLToPort extends BaseStellarFunction {
@Override
public Object apply(List<Object> objects) {
--- End diff --
Can we be consistent with the variables, such that a URI is uri and URL is url?
---
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] incubator-metron issue #402: METRON-639: The Network Stellar functions need ...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/incubator-metron/pull/402
I am concerned about the naming and documentation of the functions, given that URIs are used under the hood for everything. URIs are a super set of URLs, so now we have a set of URL specific functions that accept a superset of the documented and named input.
e.g. URL_TO_PROTOCOL doesn't actually limit itself to URLs. It happily accepts a (non-URL) URI, and returns an answer, even though by the name and documentation of the function I would really expect at least a warning, and more likely an error. "urn:isbn:0451450523" isn't even a URL, but URL_TO_PROTOCOL informs me that its scheme is "urn". This particular example obviously doesn't really apply to what we're doing, but one could imagine others do.
Even if we choose to keep the naming as-is, I want to see tests on URIs that aren't URLs, to ensure that they behave rationally.
---
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] incubator-metron issue #402: METRON-639: The Network Stellar functions need ...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/incubator-metron/pull/402
@justinleet I thought about it a bit more and decided that a sensible approach would be to validate the URL via commons validator and THEN parse using java.net.URI. This would mean that URIs which are not URL's would not parse, but URLs would parse regardless of scheme.
What other test cases would you suggest (preferably with example URLs, if you don't mind)?
---
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] incubator-metron pull request #402: METRON-639: The Network Stellar function...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/incubator-metron/pull/402#discussion_r93672602
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/dsl/functions/NetworkFunctions.java ---
@@ -182,8 +186,8 @@ public Object apply(List<Object> objects) {
public static class URLToPath extends BaseStellarFunction {
--- End diff --
Can you add testing around this? In particular, make sure that fragments are handled correctly (`URI` has a separate `getFragment()`, so I assume that they are not returned by `getPath()`. I could be wrong about this, though, I haven't run a test on it.). If this is the case, we may also want/need a `TO_FRAGMENT` function that handles this appropriately, although I'm fine with that being a separate conversation/jira.
---
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] incubator-metron pull request #402: METRON-639: The Network Stellar function...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/incubator-metron/pull/402#discussion_r93650018
--- Diff: metron-platform/metron-common/src/test/java/org/apache/metron/common/stellar/network/NetworkFunctionsTest.java ---
@@ -0,0 +1,164 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.metron.common.stellar.network;
+
+import com.google.common.collect.ImmutableMap;
+import org.junit.Test;
+
+import java.util.HashMap;
+
+import static org.apache.metron.common.utils.StellarProcessorUtils.run;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class NetworkFunctionsTest {
+
+ @Test
+ public void inSubnetTest_positive() {
+ assertTrue((Boolean) run("IN_SUBNET(ip, cidr)"
+ , ImmutableMap.of("ip", "192.168.0.1", "cidr", "192.168.0.0/24")
+ ));
+ }
+
+ @Test
+ public void inSubnetTest_negative() {
+ assertFalse((Boolean) run("IN_SUBNET(ip, cidr)"
+ , ImmutableMap.of("ip", "192.168.1.1", "cidr", "192.168.0.0/24")
+ ));
+ }
+
+ @Test
+ public void inSubnetTest_multiple() {
+ assertTrue((Boolean) run("IN_SUBNET(ip, cidr1, cidr2)"
+ , ImmutableMap.of("ip", "192.168.1.1"
+ , "cidr1", "192.168.0.0/24"
+ , "cidr2", "192.168.1.0/24")
+ ));
+ }
+
+ private static void runSimple(String function, String argument, Object expected) {
--- End diff --
I think this should be in the StellarProcessorUtils
---
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] incubator-metron issue #402: METRON-639: The Network Stellar functions need ...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/incubator-metron/pull/402
@cestella
Do you know of an example URL that isn't a URL in Java, e.g. with the square brackets issue? I didn't see one in that thread, but I could have just missed it. Are we sure commons validator wouldn't have the same issue? I'm fine with validating them and then parsing if we're stuck dealing with Java implementation quirks.
Specifically on the square brackets issue, it sounds like (and this is way out of my wheelhouse, so forgive my potential ignorance) that these characters (and others) qualify as unwise (https://www.ietf.org/rfc/rfc2396.txt) and should be escaped, which is why Java is strict about them.
Could we appropriately escape the offending characters and end up with a correct URI? If commons has something to validate a URL, is there something to do that escaping for us? If that works, do we potentially want to be doing operations on URIs instead of URLs, e.g. `URI_TO_PATH`, `URI_TO_HOST`? Obviously, this raises the question of what we do the URL* functions and coexistence.
In terms of test cases, I'd want to see something with the unwise character issue (but I don't have an example of that myself). I can help you try to dig one up if you want. In addition, I'd like to see something with a fragment defined (e.g. `http://java.sun.com/index.html#chapter1`) and make sure that the functions work appropriately (mostly just tests for `TO_PATH`).
---
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] incubator-metron issue #402: METRON-639: The Network Stellar functions need ...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/incubator-metron/pull/402
Yep, I'm good with it.
Thanks for contributing and running through this. +1
---
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] incubator-metron pull request #402: METRON-639: The Network Stellar function...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-metron/pull/402
---
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] incubator-metron issue #402: METRON-639: The Network Stellar functions need ...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/incubator-metron/pull/402
Thanks for the review @ottobackwards ! I extracted out the `runSimple` utility, generalized it a bit and put it in `StellarProcessorUtils` Let me know if you like what you see there.
---
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.
---