Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[streamalert][bug] Fix to allow nested structured data and normalization #339

Closed
wants to merge 3 commits into from

Conversation

javuto
Copy link
Contributor

@javuto javuto commented Oct 2, 2017

to @ryandeivert
cc @mime-frame

This code fixes the problem of having nested data in normalized fields and not be detected by the rules as datatype. Resolves: #333

Change

The method match_types now sets the value of record['normalized_types'] instead of returning it. Given that the method was being invoked recursively, the impacted field was not updated correctly and the nested normalized fields that we were interested on, they were not set. With this fix the existing rules work as expected while other new rules can be implemented and the normalization can be used with nested fields.

Testing

Tested for existing rules using python manage.py lambda test --processor all
Added coverage for this in the existing test tests/unit/stream_alert_rule_processor/test_rules_engine.py and checked with the following command:

$ ./tests/scripts/unit_tests.sh
...
Rules Engine - Match normalized types against record ... ok
Rules Engine - Recursively walk though all nested keys and update ... ok
...

@ghost
Copy link

ghost commented Oct 2, 2017

Update your PR description with a Testing section with how you tested your changes @javuto :)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.693% when pulling 0f025c9 on normalized_nested_field_bug into 3cf6951 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.693% when pulling cd77aea on normalized_nested_field_bug into 3cf6951 on master.

@ghost
Copy link

ghost commented Oct 2, 2017

Awesome, thanks @javuto ! @ryandeivert - you're up :)

@ryandeivert
Copy link
Contributor

@javuto please add the line resolves: #333 below the to & cc lines so the issue will be closed when this PR is merged

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.71% when pulling cac8827 on normalized_nested_field_bug into bd72ce2 on master.

@javuto javuto closed this Oct 11, 2017
@javuto
Copy link
Contributor Author

javuto commented Oct 11, 2017

@chunyong-lin is taking care of this

@javuto javuto deleted the normalized_nested_field_bug branch October 11, 2017 18:00
@javuto javuto added this to the 1.6.0 milestone Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Normalized field is not found in a nested structure
3 participants