Skip to content

Fleet Package Code Review Comments

Jump to bottom
Andrew Kroh edited this page May 9, 2025 · 3 revisions

This page compiles common comments made during reviews of Fleet packages, providing a single reference point for detailed explanations through shorthand references. It is a list of frequent issues rather than a comprehensive development guide.

Table of Contents

Manifests

Variable shadowing is not allowed

Variables may be declared in several places within a Fleet package (package, policy template, input, data_stream). You must not declare a variable with the same name as a variable in an outer scope.

See elastic/package-spec#421 for more information.

Avoid YAML key names containing dots

Always use the proper YAML dictionary representation and avoid non-standard shortcut key names that employ dots to denote nested structures.

Correct ✅

elasticsearch:
  dynamic_dataset: false

Incorrect ❌

elasticsearch.dynamic_dataset: true

See https://github.com/elastic/package-spec/issues/538.

Ingest Pipelines

Unnecessary null-safe ctx dereference

Within Ingest Pipelines, the ctx value is always accessible where Painless is accepted. Instead of writing ctx?, directly access ctx without the null-safe operator.

This applies to if conditions on all processors and script processors using the Painless language.

Convert processors for IP type must have a side effect

When using the convert processor with type: ip, the processor will not make any changes to the field; it only validates whether the value is a valid IP address. Therefore, some action must result from that validation in order for the processor to have an effect.

Valid uses of convert with type: ip include:

  • Conditionally copying a valid IP to a target_field. Using ignore_failure is valid in this case, assuming the source field is either not mapped as an IP or is later removed from the pipeline.
  • Validating a source field and handling failures by using an on_failure handler to delete or rename the source field.

Grok patterns should be anchored

Grok patterns should include anchors. Anchors pin regular expressions to specific positions in the string. By adding start (^) and end ($) anchors to our grok expressions, we ensure that matches occur only against the entire string from beginning to end.

This is especially important because if anchors are missing and the regex engine fails to match the entire string, it may attempt to find partial matches within substrings of the input. This can lead to performance degradation.

related.ip is populated when any other ECS ip fields are used

The ECS related.ip field should be populated with an array of all the associated IP addresses contained in the document.

Pipeline on_failure handler must set event.kind: pipeline_error

When an ingest pipeline fails due to an unhandled error the ECS event.kind field should be set to pipeline_error. This makes it clear that the pipeline failed in some way.

Pipeline on_failure handler must set error.message

When an ingest pipeline fails due to an unhandled error the ECS error.message field should be set to a value that includes the error and any additional context.

This is the recommended format for the error message:

on_failure:
  - append:
      field: event.kind
      value: pipeline_error
  - append:
      field: error.message
      value: >-
        Processor '{{{ _ingest.on_failure_processor_type }}}'
        {{#_ingest.on_failure_processor_tag}}with tag '{{{ _ingest.on_failure_processor_tag }}}'
        {{/_ingest.on_failure_processor_tag}}failed with message '{{{ _ingest.on_failure_message }}}'

Script processors should have a tag and description

The description field documents what the script does which helps other developers, and it is displayed in the Ingest Pipeline editor in Kibana. The tag field is included in the error message metadata and ingest node stats to help distinguish it from other processors.

Case-insensitive string comparison should use equalsIgnoreCase

Instead of using a x.toLowerCase() comparison against a lowercase comparand, use x.equalsIgnoreCase(y). When x is the result of a null-safe access chain, the expression should be compared to true to ensure that the final expression is a boolean. The second form does not allocate a new string and will not fail to correctly compare strings under case folding, although this latter issue is likely to be rare in nearly all cases where we use this.

if: ctx.field?.equalsIgnoreCase('value') == true
if: ctx.field.equalsIgnoreCase('value')

Use a terminate processor with inputs that produce failure events

When an input sends a document that represents a collection error as opposed to event data, then the pipeline should skip processing that expects to operate on event data. This prevents those steps from introducing new errors that detract from (or override) the input collection error.

A terminate processor that is conditional on an error object from the input should be inserted early in the ingest pipeline. When this processor executes, then the processing within the current pipeline will terminate.

The terminate processor was introduced in Elasticsearch 8.16.0.

  - terminate:
      tag: data_collection_error
      if: ctx.error?.message != null && ctx.message == null && ctx.event?.original == null

Field Definitions

Defining an ECS field without using an external definition

ECS fields should be defined by referencing the external ECS definition. If you declare a field name that exists in ECS you must use the external: ecs option.

You may override properties of the ECS definition assuming the field type remains in a compatible field family.

- name: event.dataset
  external: ecs
  type: constant_keyword
  value: uav.adsb

Field names must not contain asterisks

Defining a field name that contains asterisks results in literal field names in the mapping for *. It is not the correct way to define dynamic Elasticsearch path_match mapping rules. It is not expected that field names contain literal asterisks, therefore, it is always an error to declare one.

See the object_type fields in elastic/package-spec for details on how to correctly define dynamic fields.

Non-ECS root fields must be namespaced by the package name

Any root fields defined in the package that are not part of ECS must be namespaced by the package name. That is, they must be prefixed with the package name followed by a dot (.).

The purpose of this guidance is to prevent field type conflicts between packages. Two packages should not declare the same field with different types. This practice gives each package a namespace over which it has exclusive control and eliminates data view conflicts.

Optimizing Indices Using Constant Keyword Fields

A constant_keyword field is a specialized keyword type in Elasticsearch where all documents in the index share the same value. This data type offers benefits in terms of storage efficiency and query optimization. However, there's a crucial caveat: once the constant value is set by the first ingested document, all subsequent documents must either have the same value or omit the field entirely; otherwise, ingestion will fail.

To avoid issues and optimize storage when using constant_keyword, follow these best practices:

  1. Define the constant value for the field within your index mappings.
  2. Exclude the field from _source. Ensure that the document does not include the field, preventing the value from being stored in _source.

Example Field Definition:

# fields/ecs.yml
- name: observer.vendor
  external: ecs
  type: constant_keyword
  value: Elastic

If the data shipper (e.g. Elastic Agent) sends the field, add a step to remove it in the ingest pipeline:

# ingest_pipeline/default.yml
- remove:
    field: observer.vendor
    ignore_missing: true

Commonly defined constant keyword fields:

  • event.dataset
  • event.module
  • threat.feed.name
  • threat.feed.dashboard_id
  • observer.product
  • observer.type
  • observer.vendor
  • cloud.provider

Special considerations for Elasticsearch Transforms:

  1. Transforms and multiple data streams: If your transform consumes data from multiple data streams, avoid using constant_keyword in the transform's mappings if it results in documents with varying values being merged into the destination index. For instance, use a keyword type instead of constant_keyword for fields like data_stream.dataset.
  2. Field visibility in transforms: If the source index declares a field as constant_keyword but does not include it in _source, the transform will not see the field.

Transforms

Increment transform version on changes

Any change to the transform.yml file requires you to bump _meta.fleet_transform_version otherwise existing installations of the transform will not be updated.

A change to a transform’s field mappings requires "version" bumps for the destination index and the transform.

Agent Config Templates

Variables referenced in Handlebars must be declared in manifests

Check that any variable referenced in the Handlebars template is declared in the package's manifest. Any undeclared variable is definitely an error.

Request trace logging must be safely configured if used

When using inputs that allow request trace logging, and it is added as a feature, the configuration must be made as safe as possible for the supported stack version. Inputs that support request trace logging are CEL, HTTP JSON, HTTP Endpoint, and the Jamf and Okta Entity Analytics providers.

Two forms of the feature enablement exist; for stacks before v8.15, and for stacks after this version (<input> is a placeholder for the input that is used in the package).

Before v8.15.x:

Agent template file:

{{#if enable_request_tracer}}
… .tracer.filename: "../../logs/<input>/http-request-trace-*.ndjson"
… .tracer.maxbackups: 5
{{/if}}

Manifest file:

- name: enable_request_tracer
  type: bool
  title: Enable request tracing
  multi: false
  required: false
  show_user: false

From v8.15.x onward:

Agent template file:

… .tracer:
enabled: {{enable_request_tracer}}
filename: "../../logs/<input>/http-request-trace-*.ndjson"
maxbackups: 5

Manifest file:

- name: enable_request_tracer
  type: bool
  title: Enable request tracing
  default: false ← NOTE THIS IS REQUIRED
  multi: false
  required: false
  show_user: false

CEL / httpjson: cursor updates require a published event

When using the CEL or HTTPJSON input, cursor updates are not automatically.

When cursor updates are required and either CEL or HTTPJSON are being used as the input, at least one (potentially dummy) event must be published since cursor updates only happen when events are published. This may entail that response.split.ignore_empty_value be set to false even when no useful event is received.

The dummy event can be discarded by using a Beats drop_event processor to prevent publishing it to Elasticsearch.

Git

Commit messages

Commit messages must be included in the PR description and must be manually applied when squashing and merging the pull request.

A commit message should provide a concise and informative description of the change, explaining why it was necessary and what it does. If necessary, expand on the short description to include details such as: "what was not working and what was done to fix it," "why the feature was needed," and "if the feature is not obvious, a brief description of it and perhaps briefly how it works."

When tests are added, the provenance of the test cases should be included in the commit message, either a link to the on-line source or a reasonable description of how/why to reconstruct the test input.

See r’s comment on the value of commit messages.

Clone this wiki locally