GitHub user merrimanr opened a pull request:
https://github.com/apache/metron/pull/1190
METRON-1771: Update REST endpoints to support eventually consistent UI
updates
## Contributor Comments
This PR updates several REST endpoints to return the state of an object on
update operations. These endpoints include:
- MetaAlertController
- create
- addAlertsToMetaAlert
- removeAlertsFromMetaAlert
- updateMetaAlertStatus
- UpdateController
- patch
- replace
- addCommentToAlert
- removeCommentFromAlert
The general approach is to change the return type of the various Dao
methods that update objects to return the updated document. In most cases this
was straightforward but did require small changes in several files due to how
the Dao classes are composed and the various implementations that exist (ES,
Solr, InMemory, etc).
Perhaps the most significant change was to the InMemoryMetaAlertDao classes
used for REST integration testing because they were no longer sufficient after
the interface change. Before they stored a mapping between metaalerts and
child alerts which worked fine when just returning a true/false on
adding/removing child alerts. With this PR the InMemoryMetaAlertDao classes
now must return the full object. As I worked through updating these methods
to do this, I found myself rewriting the methods in
AbstractLuceneMetaAlertUpdateDao. Instead of doing that, I refactored the
InMemoryMetaAlertDao classes to more closely match how the MetaAlertDao classes
are composed and was able to reuse the implementations in
AbstractLuceneMetaAlertUpdateDao. I feel this makes the InMemoryMetaAlertDao
classes smaller and easier to maintain moving forward.
### Changes Included
- Updated the various *UpdateDao interfaces and implementations to return
Documents
- Updated the REST services and controllers to match the new interfaces
- Changed the InMemoryDao classes as described above
- Updated the various unit and integration tests to verify that correct
documents are returned (includes new tests in several cases)
- Updated and added documentation as needed
- Fixed a bug in HBaseDaoIntegrationTest.java where tables were not being
cleared after each test
### Testing
In addition to unit and integration tests passing, I have tested this in
full dev with Elasticsearch. The testing approach is the same for each
endpoint listed above: send a request and verify the document that is returned
includes the expected change. Here are a couple of examples
#### Testing the patch endpoint
Send a patch request:
```
curl -X PATCH --header 'Content-Type: application/json' --header 'Accept:
application/json' -d '{
"guid": "1881af3c-88df-4e72-8e81-34538b6e774b",
"patch": [
{
"op":"add",
"path":"/newField",
"value":"newValue"
}
],
"sensorType": "bro"
}' 'http://node1:8082/api/v1/update/patch'
```
The response should contain the full document along with the update, in
this case a `newField` field added with a value of `newValue`:
```
{
"timestamp": 1536185190979,
"document": {
"bro_timestamp": "1536182284.908185",
"ip_dst_port": 8080,
...
"newField": "newValue"
},
"guid": "1881af3c-88df-4e72-8e81-34538b6e774b",
"sensorType": "bro"
}
```
#### Testing the metaalert create endpoint:
Create a metaalert:
```
curl -X POST --header 'Content-Type: application/json' --header 'Accept:
application/json' -d '{
"alerts": [
{
"guid": "0588e788-3a91-4113-94a0-d80daec00d05",
"sensorType": "snort"
}
],
"groups": [
"group1"
]
}' 'http://node1:8082/api/v1/metaalert/create'
```
This should return the full metaalert along with the child alert:
```
{
"timestamp": 1536203159546,
"document": {
"average": 10,
"max": 10,
"metron_alert": [
{
"msg": "'snort test alert'",
...
"metaalerts": [
"f65e4d0e-bdad-4ac7-ab22-4cf207c3fb2e"
]
}
],
"threat:triage:score": 10,
"count": 1,
"groups": [
"group1"
],
"sum": 10,
"source:type": "metaalert",
"min": 10,
"median": 10,
"guid": "f65e4d0e-bdad-4ac7-ab22-4cf207c3fb2e",
"timestamp": 1536203159546,
"status": "active"
},
"guid": "f65e4d0e-bdad-4ac7-ab22-4cf207c3fb2e",
"sensorType": "metaalert"
}
```
I also tested every endpoint using the Alerts UI. Everything should
continue working as expected.
### Outstanding items
There are a couple of changes I'm not quite sure about and would like some
input:
- Are the changes made to the InMemoryUpdateDao classes reasonable?
- The case of missing documents are handled in 2 different ways as far as I
can tell. Either a null is returned or an empty Optional. I just happened to
see the null approach first so that's the pattern I followed. However I think
we may want to make this consistent and the Optional empty approach feels more
appropriate to me. What do you think?
- In the MultiIndexDao, updates are performed in parallel across the
different Daos. Since we're assuming the updates should all be identical I'm
returning the first one as the return value. Is there a better approach?
I've made some comments inline in this PR to highlight these and provide
some context.
Fortunately I did not have to make any UI changes since the return value of
these endpoints is ignored in most cases. We should have a follow on task to
properly update the client side code to match these new endpoints. I believe
there are a couple outstanding bugs related to displaying inconsistent results
in the UI after updates.
I am still working on testing with Solr and adding more code comments to
add some clarity around changes that may not be obvious.
## 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:
- [x] Have you included steps to reproduce the behavior or problem that is
being changed or addressed?
- [x] Have you included steps or a guide to how the change may be verified
and tested manually?
- [x] 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 &&
dev-utilities/build-utils/verify_licenses.sh
```
- [x] Have you written or updated unit tests and or integration tests to
verify your changes?
- [x] If adding new dependencies to the code, are these dependencies
licensed in a way that is compatible for inclusion under [ASF
2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] 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](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/merrimanr/incubator-metron METRON-1771
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metron/pull/1190.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 #1190
----
commit 39ef134852f1ce8626e77413381ccc4779bbcad8
Author: merrimanr <merrimanr@...>
Date: 2018-09-07T20:18:48Z
initial commit
----
---
