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.

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

Disable normalization validating fields in transforms if synthetics is enabled #2011

Merged

Conversation

mrodm
Copy link
Contributor

@mrodm mrodm commented Aug 9, 2024

Relates #1971

Fields in transforms were always validated using normalization.

This PR allows to disable normalization when validating those fields (from transforms) when synthetic is enabled in the data stream created in testing.

Example of errors found in ti_anomali package:

test case failed: one or more errors found in documents stored in logs-ti_anomali.threatstream-77107 data stream: [0] field "event.category" is not normalized as expected: expected array, found "threat" (string)
[1] field "event.type" is not normalized as expected: expected array, found "indicator" (string)
[2] field "labels.is_ioc_transform_source" is undefined

Validation for event.type and event.category fields is happening when checking transforms. If checkTransforms method is commented out, validation for those fields run successfully.

As a note, labels.is_ioc_transform_source fields still fails even with this fix. Probably, some missing mapping/field definition? why is it not failing with logsDB disabled ?

Author's checklist

  • Remove setting logsDB as the default mode.
  • Support failure store when synthetics (logsDB) is enabled.
  • Update ti_anomali with contents from integrations.
  • Checked that ti_anomali_logsdb package has the validation errors if normalization is not disabled.
    • Added missing labels.is_ioc_transform_source field definition in transform.

@mrodm mrodm self-assigned this Aug 9, 2024
@@ -1462,7 +1462,7 @@ func (r *tester) validateTestScenario(ctx context.Context, result *testrunner.Re
}

// Check transforms if present
if err := r.checkTransforms(ctx, config, r.pkgManifest, scenario.kibanaDataStream, scenario.dataStream); err != nil {
if err := r.checkTransforms(ctx, config, r.pkgManifest, scenario.kibanaDataStream, scenario.dataStream, scenario.syntheticEnabled); err != nil {
Copy link
Contributor Author

@mrodm mrodm Aug 9, 2024

Choose a reason for hiding this comment

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

Use the same value as in the main data stream.
Would that assumption be right?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure what Fleet does with transform indexes. Could we add a test case that would fail without this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check to run a test package for that use case (with logsdb enabled).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a new test package ti_anomali_logsdb copied from ti_anomali. This allows to test the package with both logsdb enabled and disabled.

@mrodm mrodm changed the title Disable normalization validating transforms synthetics Disable normalization validating fields in transforms if synthetics is enabled Aug 9, 2024
@mrodm
Copy link
Contributor Author

mrodm commented Aug 9, 2024

test integrations

@elastic-vault-github-plugin-prod

Created or updated PR in integrations repository to test this version. Check elastic/integrations#10752

@mrodm
Copy link
Contributor Author

mrodm commented Aug 12, 2024

With the changes of synthetics while transforms are checked, those values are not valid:
ti_anomali validation failures

@mrodm
Copy link
Contributor Author

mrodm commented Aug 12, 2024

Failure store is failing when synthetics is enabled when unmarshalling the response (error in CI link):

Error: error running package system tests: could not complete test run: failed to check failure store: failed to decode search response: json: cannot unmarshal string into Go struct field .hits.hits._source.error.pipeline_trace of type []string

@mrodm
Copy link
Contributor Author

mrodm commented Aug 12, 2024

test integrations

@elastic-vault-github-plugin-prod

Created or updated PR in integrations repository to test this version. Check elastic/integrations#10752

Comment on lines +853 to +859
case string:
*p = append(*p, v)
case []any:
// asume it is going to be an array of strings
for _, value := range v {
*p = append(*p, fmt.Sprint(value))
}
Copy link
Contributor Author

@mrodm mrodm Aug 12, 2024

Choose a reason for hiding this comment

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

Support string and slice of strings.

Examples of the response:

  • logsdb disabled in the stack:
    {
      "type": "illegal_argument_exception",
      "message": "unable to convert [eight] to integer",
      "stack_trace": "...",
      "pipeline_trace": [
        "logs-failure_store.test-0.0.1"
      ],
      "pipeline": "logs-failure_store.test-0.0.1",
      "processor_type": "convert"
    }
  • logsdb enabled in the stack:
    {
      "message": "unable to convert [eight] to integer",
      "pipeline": "logs-failure_store.test-0.0.1",
      "pipeline_trace": "logs-failure_store.test-0.0.1",
      "processor_type": "convert",
      "stack_trace": "...",
      "type": "illegal_argument_exception"
    }

@mrodm
Copy link
Contributor Author

mrodm commented Aug 12, 2024

Integrations build running with Elastic stack 8.15.0-SNAPSHOT and logsdb enabled: https://buildkite.com/elastic/integrations/builds/14572

In that build, ti_anomali just fails with the labels.is_ioc_transform_source. The other two fields (event.category and event.type) are valid now.

ti_anomali test

@mrodm
Copy link
Contributor Author

mrodm commented Aug 12, 2024

test integrations

@elastic-vault-github-plugin-prod

Created or updated PR in integrations repository to test this version. Check elastic/integrations#10752

@mrodm mrodm marked this pull request as ready for review August 13, 2024 10:28
@mrodm mrodm requested a review from a team August 13, 2024 10:29
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Changes make sense to me, but let's try to find a test case that can help to confirm if transforms have the same index mode as the data streams of the package.

@@ -1462,7 +1462,7 @@ func (r *tester) validateTestScenario(ctx context.Context, result *testrunner.Re
}

// Check transforms if present
if err := r.checkTransforms(ctx, config, r.pkgManifest, scenario.kibanaDataStream, scenario.dataStream); err != nil {
if err := r.checkTransforms(ctx, config, r.pkgManifest, scenario.kibanaDataStream, scenario.dataStream, scenario.syntheticEnabled); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what Fleet does with transform indexes. Could we add a test case that would fail without this change?

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @mrodm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a new file to be able "source" it in the script in charge of testing false positives (test-check-false-positives.sh) and the generic one (test-check-packages.sh).

Comment on lines +88 to 91
stack_args=$(stack_version_args) # --version <version>

# Update the stack
elastic-package stack update -v ${stack_args}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

elastic-package stack update command does not allow the -U parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated test package with the latest contents from integrations repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to be able to test ti_anomali with both logsdb enabled and disabled, a new package ti_anomali_logsdb has been added (copied from ti_anomali).

This new package is tested using a Elastic stack with the option -U stack.logsdb_enabled=true, thanks to the file test/packages/parallel/ti_anomali_logsdb.stack_provider_settings

@mrodm
Copy link
Contributor Author

mrodm commented Aug 14, 2024

test integrations

@elastic-vault-github-plugin-prod

Created or updated PR in integrations repository to test this version. Check elastic/integrations#10752

@mrodm mrodm requested a review from jsoriano August 14, 2024 10:49
@mrodm mrodm merged commit 3969afa into elastic:main Aug 14, 2024
3 checks passed
@mrodm mrodm deleted the disable_normalization_validating_transforms_synthetics branch August 14, 2024 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants