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.

Sign up for GitHub

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

Jump to bottom

dns: add missing dns keywords to schema.json #10193

Closed
wants to merge 1 commit into from
Closed

dns: add missing dns keywords to schema.json #10193

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 24 additions & 0 deletions etc/schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1083,6 +1083,21 @@
}
},
"additionalProperties": false
},
"sshfp": {
"type": "object",
"properties": {
"fingerprint": {
"type": "string"
},
"algo": {
"type": "integer"
},
"type": {
"type": "string"
}
},
"additionalProperties": false
}
},
"additionalProperties": false
Expand Down Expand Up @@ -1156,6 +1171,15 @@
"opcode": {
"description": "DNS opcode as an integer",
"type": "integer"
},
"aa": {
"type": "boolean"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasonish should we add description for these added fields ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aa is already present !

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasonish should we add description for these added fields ?

Yes, will help with documentation generation.

},
"tc": {
"type": "boolean"
},
"z": {
"type": "boolean"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

z is already present as well.

@jasonish do you know how to test in CI for duplicate keys in this schema.json ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract the code out of check-eve.py back into a standalone script could be one option.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://redmine.openinfosecfoundation.org/issues/6691

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding things to the schema in alphabetic order could also be useful for catching this on review.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automation looks more trustworthy to me

Ok. Out of scope for this PR tho.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Out of scope for this PR tho.

Sure cf https://redmine.openinfosecfoundation.org/issues/6691

}
},
"additionalProperties": false
Expand Down