gigamon: add CEF follow-up fixes - #20396
Conversation
ReviewersBuildkite won't run for external contributors automatically; you need to add a comment:
NOTE: https://github.com/elastic/integrations/blob/main/.buildkite/pull-requests.json contains all those details. |
andrewkroh
added
Integration:gigamon
|
Pinging @elastic/integration-experience (Team:Integration-Experience) |
![vera-review-bot[bot]](https://avatars.githubusercontent.com/in/3297022?s=60&v=4){kind=small}
596bbd7 to
17ea4c7
Compare
825b40c to
79e95f8
Compare
79e95f8 to
0a98a4e
Compare
| } | ||
| }, | ||
| { | ||
| "json": { |
There was a problem hiding this comment.
Severity: 🟠 High confidence: high path: packages/gigamon/data_stream/ami/_dev/test/pipeline/test-ami.json:2456
The new ASN.1 UTCTime/GeneralizedTime certificate-date parsing has no test fixture at all; add a test document with a 13-char YYMMDDHHMMSSZ value and a 15-char YYYYMMDDHHMMSSZ value so the new scripts and date format are actually exercised.
Details
The changelog advertises "Parse ASN.1 UTCTime and GeneralizedTime certificate validity dates" as a bugfix, and default.yml adds two painless processors (script_expand_asn1_utctime_not_before / _not_after) plus a new yyyyMMddHHmmss'Z' date format. None of that code is reached by any fixture. The only ssl_validity values in either pipeline test input are 2017-08-31 06:30:04 and 2020-08-31 06:30:04 (test-ami.json lines 2268-2269), which match the pre-existing yyyy-MM-dd HH:mm:ss format and never satisfy the new scripts' length() == 13 && endsWith('Z') guard. test-cef.json contains no ssl_validity keys either. The two documents this PR does add to test-ami.json carry no certificate fields. The RFC 5280 pivot (YY >= 50 maps to 19YY) is likewise unverified in both directions.
Recommendation:
Add a document to test-ami.json covering both ASN.1 encodings and both sides of the century pivot, and regenerate test-ami.json-expected.json:
{
"json": {
"ts": "Wed Dec 13 15:25:54 2023",
"id": "691388880109625349",
"seq_num": "52203187",
"ssl_validity_not_before": "231213152554Z",
"ssl_validity_not_after": "20281213152554Z"
}
}The expected output should show 2023-12-13T15:25:54.000Z and 2028-12-13T15:25:54.000Z. Add a second document with a pre-pivot value such as "ssl_validity_not_before": "981213152554Z" to assert it resolves to 1998-12-13T15:25:54.000Z rather than 2098.
🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| - append: | ||
| field: error.message | ||
| value: 'Processor {{{_ingest.on_failure_processor_type}}} with tag {{{_ingest.on_failure_processor_tag}}} in pipeline {{{_ingest.pipeline}}} failed with message: {{{_ingest.on_failure_message}}}' | ||
| - script: |
There was a problem hiding this comment.
Severity: 🟡 Medium confidence: medium path: packages/gigamon/data_stream/ami/elasticsearch/ingest_pipeline/default.yml:229
The two new ASN.1 painless scripts have no processor-level on_failure and no digit guard, so a malformed 13-char value marks the whole document pipeline_error; add an on_failure block and check the first two characters are digits.
Details
script_expand_asn1_utctime_not_before (line 229) and script_expand_asn1_utctime_not_after (line 255) are the only processors added by this PR without an on_failure block; every neighbouring date and convert processor in this file recovers locally by removing the offending field and appending to error.message. The if guard only tests length() == 13 and endsWith('Z'), which does not constrain the first two characters to digits, so Integer.parseInt(v.substring(0, 2)) is not guaranteed to succeed for a value that passes the guard. When it does not, the failure escalates to the pipeline-level on_failure at the bottom of the file, which sets event.kind: pipeline_error and aborts the remaining ~1400 lines of enrichment for that document, instead of losing just the certificate date. The pipeline already uses Character.isDigit for exactly this kind of guard at line 1295.
Recommendation:
Tighten the guard and add local error handling to both scripts:
- script:
lang: painless
description: Expand ASN.1 UTCTime 2-digit years using the RFC 5280 pivot (YY >= 50 is 19YY).
tag: script_expand_asn1_utctime_not_before
if: >-
ctx.gigamon?.ami?.ssl_validity_not_before instanceof String &&
ctx.gigamon.ami.ssl_validity_not_before.length() == 13 &&
ctx.gigamon.ami.ssl_validity_not_before.endsWith('Z') &&
Character.isDigit(ctx.gigamon.ami.ssl_validity_not_before.charAt(0)) &&
Character.isDigit(ctx.gigamon.ami.ssl_validity_not_before.charAt(1))
source: |-
def v = ctx.gigamon.ami.ssl_validity_not_before;
int yy = Integer.parseInt(v.substring(0, 2));
ctx.gigamon.ami.ssl_validity_not_before = (yy >= 50 ? '19' : '20') + v;
on_failure:
- append:
field: error.message
value: 'Processor {{{_ingest.on_failure_processor_type}}} with tag {{{_ingest.on_failure_processor_tag}}} in pipeline {{{_ingest.pipeline}}} failed with message: {{{_ingest.on_failure_message}}}'Apply the same change to script_expand_asn1_utctime_not_after.
🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| "tcp_mss": "1460" | ||
| } | ||
| }, | ||
| { |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: high path: packages/gigamon/data_stream/ami/_dev/test/pipeline/test-ami.json:2502
The second test document added to test-ami.json is byte-identical to the first (same id 691388880109625348 and seq_num 52203186); drop it or change it into a distinct case.
Details
Lines 2455-2501 and 2502-2548 of test-ami.json are identical character for character, including the event id and sequence number. The corresponding expected file duplicates 88 lines for the same reason. The second copy exercises no processor branch the first does not, so it adds fixture and expected-output maintenance cost with no coverage gain, and having two documents share one event.id makes failure output ambiguous when the test diff is read.
Recommendation:
Either delete the duplicate block (lines 2502-2548 here and its counterpart in test-ami.json-expected.json), or replace it with a document that covers a branch the first one misses, e.g. zero-window fields absent and the min/max window values differing:
{
"json": {
"ts": "Wed Dec 13 15:25:54 2023",
"id": "691388880109625349",
"seq_num": "52203187",
"src_ip": "10.115.83.4",
"dst_ip": "10.115.81.118",
"tcp_win_size": "0",
"TcpServerWinMin": "0",
"TcpServerWinMax": "262144",
"tcp_mss": "536"
}
}🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| target_field: gigamon.ami.http_uri_full | ||
| ignore_missing: true | ||
|
|
||
| - rename: |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: high path: packages/gigamon/data_stream/ami/elasticsearch/ingest_pipeline/cef-pipeline.yml:310
The new GigamonMdata_http_file_type rename is the only processor this PR adds without a tag; add tag: rename_http_file_type to match the sixteen sibling renames below it.
Details
Every other processor added by this PR to cef-pipeline.yml (lines 443-537) carries a tag, and the pipeline-level on_failure at the bottom of the file interpolates _ingest.on_failure_processor_tag into error.message. Without a tag, a failure in this rename produces an error message with an empty tag section, which makes the failing processor unidentifiable from the indexed document.
Recommendation:
Add the tag:
- rename:
field: gigamon.ami.GigamonMdata_http_file_type
target_field: gigamon.ami.http_file_type
ignore_missing: true
tag: rename_http_file_type🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| type: double | ||
| - name: tcp_rtt_app_mean | ||
| type: double | ||
| - name: tcp_win_size |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: high path: packages/gigamon/data_stream/ami/fields/fields.yml:325
All sixteen new gigamon.ami.tcp_* field definitions omit description, so they render with an empty Description column in the generated README; add a one-line description to each.
Details
fields.yml lines 325-356 declare tcp_win_size, tcp_client_win_max/min/mean, tcp_client_zw/_ack/_probe, tcp_server_win_max/mean/min, tcp_server_zw/_ack/_probe, tcp_client_mss, tcp_server_mss and tcp_mss with only name and type. The generated table in docs/README.md (lines 354-360 and 374-381 of the diff) consequently shows a blank Description for every one of them. These names are not self-explanatory to a user browsing the field reference: zw, zw_ack and zw_probe are zero-window counters and mss is maximum segment size, neither of which is inferable from the identifier.
Recommendation:
Add descriptions and regenerate the README with elastic-package build:
- name: tcp_win_size
type: long
description: TCP receive window size observed on the flow.
- name: tcp_client_win_max
type: long
description: Maximum TCP receive window advertised by the client during the flow.
- name: tcp_client_zw
type: long
description: Number of zero-window segments sent by the client.
- name: tcp_client_zw_ack
type: long
description: Number of zero-window acknowledgements sent by the client.
- name: tcp_client_zw_probe
type: long
description: Number of zero-window probes sent by the client.
- name: tcp_client_mss
type: long
description: TCP maximum segment size advertised by the client.🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| field: gigamon.ami.GigamonMdata_tcp_flag_reset | ||
| target_field: gigamon.ami.tcp_flag_reset | ||
| ignore_missing: true | ||
|
|
There was a problem hiding this comment.
Severity: 🔵 Low confidence: low path: packages/gigamon/data_stream/ami/elasticsearch/ingest_pipeline/cef-pipeline.yml:442
A whitespace-only line and several stray double blank lines were introduced in the pipelines; strip the trailing spaces on cef-pipeline.yml line 442 and collapse the duplicated blank lines in default.yml.
Details
Line 442 of cef-pipeline.yml is six spaces of trailing whitespace rather than an empty line. default.yml picks up matching artefacts at lines 280-281, 858-859 and 898-899, where two blank lines now separate processors that are single-blank-separated everywhere else in the file. elastic-package format normalises YAML whitespace, so these will be rewritten the next time the package is formatted.
Recommendation:
Replace the whitespace-only line with a plain blank line:
- rename:
field: gigamon.ami.GigamonMdata_tcp_flag_reset
target_field: gigamon.ami.tcp_flag_reset
ignore_missing: true
- rename:
field: gigamon.ami.GigamonMdataTcpWinSize
target_field: gigamon.ami.tcp_win_size
ignore_missing: true
tag: rename_tcp_win_sizeThen run elastic-package format in packages/gigamon to collapse the duplicated blank lines in default.yml.
🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
0a98a4e to
c890b31
Compare
| ignore_missing: true | ||
|
|
||
| - rename: | ||
| field: gigamon.ami.GigamonMdata_http_file_type |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: high path: packages/gigamon/data_stream/ami/elasticsearch/ingest_pipeline/cef-pipeline.yml:311
The new http_file_type rename has no tag, unlike the 16 other renames added in this same PR - add tag: rename_http_file_type so on_failure error messages identify the processor.
Details
Every other rename processor this PR adds to cef-pipeline.yml (rename_tcp_win_size, rename_tcp_client_win_max, ... rename_tcp_mss) carries a tag, but this one does not. The pipeline-level on_failure handler interpolates {{{_ingest.on_failure_processor_tag}}} into error.message, so a failure in an untagged processor produces an error record that cannot be traced back to a specific processor.
Recommendation:
Add a tag consistent with the surrounding processors:
- rename:
field: gigamon.ami.GigamonMdata_http_file_type
target_field: gigamon.ami.http_file_type
ignore_missing: true
tag: rename_http_file_type🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| type: double | ||
| - name: tcp_rtt_app_mean | ||
| type: double | ||
| - name: tcp_win_size |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: high path: packages/gigamon/data_stream/ami/fields/fields.yml:325
The 16 new TCP window/zero-window/MSS field definitions have no description, so they render with an empty Description column in the generated docs - add a short description to each.
Details
The new entries (tcp_win_size through tcp_mss) declare only name and type. The generated field table in packages/gigamon/docs/README.md shows these rows with a blank Description cell, which gives users no way to tell tcp_client_win_mean from tcp_server_win_mean or to know the unit. Names such as tcp_client_zw_probe and tcp_mss are not self-explanatory without the vendor's AMI metadata reference.
Recommendation:
Add a description to each new field, for example:
- name: tcp_win_size
type: long
description: TCP window size advertised in the flow, in bytes.
- name: tcp_client_win_max
type: long
description: Maximum TCP receive window advertised by the client, in bytes.
- name: tcp_client_zw
type: long
description: Number of TCP zero-window events observed from the client.
- name: tcp_client_zw_probe
type: long
description: Number of TCP zero-window probes sent by the client.
- name: tcp_client_mss
type: long
description: TCP maximum segment size negotiated by the client, in bytes.
- name: tcp_mss
type: long
description: Effective TCP maximum segment size for the flow, in bytes.🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
|
/test |
c890b31 to
1b599e3
Compare
|
🟢 No issues across the latest commits 63900b5…1b599e3 (137 commits). Review summaryIssues found across earlier commits c890b31 — 2 low
Issues found across earlier commits 0a98a4e — 1 high, 1 medium, 4 low
Issues found across earlier commits 79e95f8 — 2 medium, 3 low
Issues found across earlier commits 596bbd7 — 1 high, 1 medium, 2 low
Issues found across earlier commits 451ed4b — 1 high, 5 medium, 1 low
🤖 AI-Generated Review | Vera Review Bot - v0.2.6 | 📚 Knowledge base: integration-skills
|
|
@qcorporation can you initiate the test |
|
/test |
|
✅ All changelog entries have the correct PR link. |
🚀 Benchmarks reportTo see the full report comment with |
💚 Build Succeeded
History
|
|
thank you @sanjay-2307 for the contribution! PR has been merged! |
|
Package gigamon - 2.4.0 containing this change is available at https://epr.elastic.co/package/gigamon/2.4.0/ |
3 participants
Proposed commit message
gigamon: add follow-up CEF field mappings and parsing fixes for AMI
This change adds follow-up fixes for the Gigamon AMI CEF ingestion path to align CEF and JSON field normalization and improve dashboard compatibility.
What
tcp_rtt_mintcp_rtt_meantcp_rtt_maxtcp_rtt_app_mintcp_rtt_app_meantcp_rtt_app_maxtcp_win_sizetcp_client_win_min/mean/maxtcp_client_zwtcp_client_zw_acktcp_client_zw_probetcp_server_win_min/mean/maxtcp_server_zwtcp_server_zw_acktcp_server_zw_probehttp_file_typemapping for the CEF path.yyMMddHHmmssZandyyyyMMddHHmmssZ.fields.yml.Why
Gigamon AMI does not emit all JSON and CEF attributes with the same naming convention. Several newer TCP RTT aggregate fields and TCP window / zero-window fields were arriving in mixed PascalCase/raw CEF forms and were not being normalized into the package schema. This caused inconsistent field behavior between JSON and CEF ingestion paths and prevented some fields from being used reliably in dashboards, Discover, and anomaly detection workflows. The SSL validity date update fixes CEF certificate date parsing so certificate-related dashboards can populate correctly.
Checklist
changelog.ymlfile.Author's Checklist
How to test this PR locally