METRON-1255: MetaAlert search is not filtering on status by merrimanr · Pull Request #802 · apache/metron

merrimanr · 2017-10-17T18:38:44Z

Contributor Comments

This PR primarily fixes filtering on metaalert status but also includes a couple other minor fixes as well. The nested query was moved to accompany a normal query string query (both are now 'should' clauses under a boolQuery) so that a filter will match on either a top level alert field and a nested alert field in a metaalert. There was also a regression introduced in #793 that causes a search with the index set to "metaalert" to not match because '_index* is now appended instead of just '*'. This PR fixes that by changing the "metaalert" index to "metaalert_index" and adds a test case to catch it in the future.

This has been validated by running the unit/integration tests and in full dev. To test in full dev, create a couple metaalerts with the http://node1:8082/swagger-ui.html#!/meta-alert-controller/createUsingPOST endpoint:

{
  "groups": [
    "string"
  ],
  "guidToIndices": {"4c732cb0-05cc-bdb4-9898-886a93129aba":"index_name"}
}

{
  "groups": [
    "string"
  ],
  "guidToIndices": {"c4c5e418-3938-099e-bb0d-37028a98dca8":"index_name"}
}

A search on metaalerts with the http://node1:8082/swagger-ui.html#!/search-controller/searchUsingPOST endpoint:

{
  "from": 0,
  "indices": [
    "metaalert"
  ],
  "query": "*",
  "size": 5
}

should return both metaalerts:

{
  "total": 2,
  "results": [
   ...
  ]
}

Then patch one of them to set the status to 'inactive':

{
  "guid": "fea984ad-63d3-4c8c-9bf4-aa9da11eea2b",
  "index": "metaalert_index",
  "patch": [{
    "op":"add",
    "path":"/status",
    "value":"inactive"
   }],
  "sensorType": "metaalert"
}

Now a search should only return the active metaalert:

{
  "total": 1,
  "results": [
   ...
  ]
}

Filtering metaalerts by child alert fields should also work correctly:

{
  "from": 0,
  "indices": [
    "metaalert"
  ],
  "query": "alert.ip_dst_addr:62.75.195.236 AND alert.host:62.75.195.236",
  "size": 5
}

Pull Request Checklist

Thank you for submitting a contribution to Apache Metron.
Please refer to our Development Guidelines for the complete guide to follow for contributions.
Please refer also to our Build Verification Guidelines 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:

Is there a JIRA ticket associated with this PR? If not one needs to be created at Metron Jira.
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.
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 metron folder via:
```
mvn -q clean integration-test install && build_utils/verify_licenses.sh 
```
Have you written or updated unit tests and or integration tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via site-book/target/site/index.html:
```
cd site-book
mvn site
```

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 is set up for your personal repository such that your branches are built there before submitting a pull request.

justinleet

This is a really good fix, and it's great to add more testing. Couple small comments, and I still have to spin it up, but looking through the code it looks good.

I'm good with pulling in the index fix in this PR, given how small it is, but I want to call it out again for anybody else who looks and potentially wants it separated.

justinleet · 2017-10-18T10:20:19Z

...asticsearch/src/main/java/org/apache/metron/elasticsearch/dao/ElasticsearchMetaAlertDao.java

                )
            )
        )
+        .must(boolQuery()


Would you mind adding a comment explaining this? Basically something to the effect of "Ensure that it's a meta alert with active status or that it's an alert (signified by having no status)".

Done in latest commit.

justinleet · 2017-10-18T10:46:25Z

.../java/org/apache/metron/elasticsearch/integration/ElasticsearchMetaAlertIntegrationTest.java

+    inputData.add(searchByNestedAlert0JSON);
+    Map<String, Object> searchByNestedAlert1JSON = JSONUtils.INSTANCE.load(searchByNestedAlert1, new TypeReference<Map<String, Object>>() {});
+    inputData.add(searchByNestedAlert1JSON);
+    elasticsearchAdd(inputData, INDEX, SENSOR_NAME);


Can we either modify this case, or add a new one with multiple alerts? Basically just to make sure things function as expected when there's multiple nested alerts.

No problem.

Done in latest commit.

justinleet · 2017-10-18T12:06:25Z

.../java/org/apache/metron/elasticsearch/integration/ElasticsearchMetaAlertIntegrationTest.java

        put("es.date.format", DATE_FORMAT);
      }
    };
+    accessConfig.setMaxSearchResults(1000);


What's the reason for adding this to the test?

This is necessary or it won't work. There is a check in ElasticsearchDao.search(SearchRequest searchRequest, QueryBuilder queryBuilder) that calls accessConfig.getMaxSearchResults().

Makes sense. Do you know if there's a reason we don't default there if it's not set? It's outside the scope of this, so if you don't know I'm not worried, but it seems like it's an opportunity.

I'm not sure but adding that should be trivial. The REST layer does provide a default but I can add it here too.

I wouldn't worry about it for this PR, just something to think about.

merrimanr · 2017-10-18T20:28:58Z

Feedback has been addressed. Take another look when you get a chance.

justinleet · 2017-10-18T20:29:26Z

+1, thanks for this, it's good stuff

merrimanr added 3 commits October 17, 2017 11:15

initial commit

28bb5c1

Merge remote-tracking branch 'mirror/master' into METRON-1255

3aa4c15

resolved merge conflicts

bc92354

justinleet reviewed Oct 18, 2017

View reviewed changes

addressed feedback

57aba44

asfgit closed this in 73623ec Oct 18, 2017

Comments

Conversation

merrimanr commented Oct 17, 2017

Contributor Comments

Pull Request Checklist

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

justinleet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

merrimanr commented Oct 18, 2017

Uh oh!

justinleet commented Oct 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants