Conversation
Note issue with sequence number in first test case.
minor change in 7th test case from the example in beats which appears to have been broken in elastic/beats@c2befaf. again note issue in bit truncation of mantissa in large-value sequence numbers.
|
Pinging @elastic/security-external-integrations (Team:Security-External Integrations) |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
|
The Kibana constraint is 7.14.0 so CI is using that version. I assume the pipeline test data was generated on a newer stack version because it has a different time format. Something must have changed in the date processor because in 7.14.0 you get I noticed a typo in the manifest that's "UCT" instead of UTC. And I think using |
|
I think it would be good to add some pipeline tests that vary the value of |
|
|
||
| # Set timezone, default to local. | ||
| - set: | ||
| field: event.timezone |
There was a problem hiding this comment.
This adds a default which is good for keeping the logic in this pipeline safe, but I think this needs an override: false to prevent clobbering the event.timezone value added by Filebeat's add_locale. This way a configured tz_offset of local uses the Agent's local timezone.
There was a problem hiding this comment.
With this change I cannot test _conf.tz_offset: local. I will revert this.
| value: "+0000" | ||
| - set: | ||
| field: event.timezone | ||
| value: "{{_conf.tz_offset}}" |
There was a problem hiding this comment.
| value: "{{_conf.tz_offset}}" | |
| value: "{{{_conf.tz_offset}}}" |
to prevent any HTML escaping being added by the mustache template engine in ES.
There was a problem hiding this comment.
Does this also need to happen elsewhere with event.timezone (below).
There was a problem hiding this comment.
I use the triple-brace version everywhere. I can't think of a use-case where I would want HTML escaping added.
And I try to avoid templates where possible. In the case of set it's usually better to use copy_from.
There was a problem hiding this comment.
The specific place I'm thinking of is the date processor
- date:
field: "_temp_.generated_time"
formats:
- "yyyy/MM/dd HH:mm:ss"
- "strict_date_optional_time_nanos"
timezone: "{{ event.timezone }}" # HERE
| # event.created is the time the event was received at the management plane. | ||
| - date: | ||
| if: "ctx?.event?.timezone != null && ctx?.event?.created != null " | ||
| if: "ctx?.event?.timezone != null && ctx?.event?.created != null" |
There was a problem hiding this comment.
Now that the pipeline adds a default event.timezone, can conditional for ctx?.event?.timezone != null be removed?
There was a problem hiding this comment.
🙂 Yes, I was leaving them in while this was decided. Also in two of the subpipelines.
|
@efd6 thanks for taking care of this one. Could we also ensure the PANW integration docs are updated to reflect support for GlobalProtect, User-ID and HIP Match events? https://github.com/elastic/integrations/tree/main/packages/panw/docs. Might be worth hyperlinking to the Palo docs for each category. e.g. GlobalProtect linking to here: https://docs.paloaltonetworks.com/pan-os/10-2/pan-os-admin/monitoring/use-syslog-for-monitoring/syslog-field-descriptions/globalprotect-log-fields.html |
What does this PR do?
Checklist
changelog.ymlfile.Author's Checklist
sequence_number. This is due to encoding/json remarshalling inelastic-packageas can be seen with this reproducer https://play.golang.com/p/fkTBk9V_606.How to test this PR locally
Related issues
Screenshots