dns: add missing dns keywords to schema.json #10193
Conversation
| @@ -1156,6 +1171,15 @@ | |||
| "opcode": { | |||
| "description": "DNS opcode as an integer", | |||
| "type": "integer" | |||
| }, | |||
| "aa": { | |||
| "type": "boolean" | |||
There was a problem hiding this comment.
@jasonish should we add description for these added fields ?
There was a problem hiding this comment.
aa is already present !
There was a problem hiding this comment.
@jasonish should we add description for these added fields ?
Yes, will help with documentation generation.
|
Looks good to me. Commit message could mention how you found them (manual code review, correct ? ) Question for us : do we want to have SV tests exercising these ? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #10193 +/- ##
==========================================
+ Coverage 82.12% 82.14% +0.01%
==========================================
Files 975 975
Lines 271724 271724
==========================================
+ Hits 223151 223198 +47
+ Misses 48573 48526 -47
Flags with carried forward coverage won't be shown. Click here to find out more. |
That was the plan, yes. This is why you gave the sshfp pcap |
yes, I can change the commit message. |
| "type": "boolean" | ||
| }, | ||
| "z": { | ||
| "type": "boolean" |
There was a problem hiding this comment.
z is already present as well.
@jasonish do you know how to test in CI for duplicate keys in this schema.json ?
There was a problem hiding this comment.
Extract the code out of check-eve.py back into a standalone script could be one option.
There was a problem hiding this comment.
There was a problem hiding this comment.
Adding things to the schema in alphabetic order could also be useful for catching this on review.
There was a problem hiding this comment.
I think the first version of the schema had all key alphabetically order, but that was a pain, and I did not know how to enforce it (or even if it should be enforced)
There was a problem hiding this comment.
Once done, its easy to keep done right? Then its pretty obvious. I think you can only enforce with review though, or doing a custom JSON parser of sorts, as most throw order out.
There was a problem hiding this comment.
Once done, its easy to keep done right? Then its pretty obvious. I think you can only enforce with review though, or doing a custom JSON parser of sorts, as most throw order out.
Does not look easy to me...
- I think I would have a 10% failure, so 1 out of 10 additions to the schema would, especially when you have objects with many fields in between
- There has already been additions to the schema not following this
- Maybe the order is more complex than alphabetical (first simple fields such as string integer and bool, then objects and arrays)
Automation looks more trustworthy to me
There was a problem hiding this comment.
Automation looks more trustworthy to me
Ok. Out of scope for this PR tho.
There was a problem hiding this comment.
Ok. Out of scope for this PR tho.
Sure cf https://redmine.openinfosecfoundation.org/issues/6691
imho, yes. Also document those fields, somehow. |
|
OISF/suricata-verify#1588 is there |
|
Was merged with commit 6c193b1 |
Feature #5642
Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/5642
Describe changes: