[SIEM] Tests for search_after and bulk index

[dhurley14]

Summary

Unit tests for reindexing mechanism for detection engine. This covers the search_after and bulk index methods used for moving events that match detection rules into the signals index. Also adds the maxSignals / maxDocs properties when using search_after to limit number of events queried against rules.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
- [ ] Documentation was added for features that require explanation or tutorials

• Unit or functional tests were updated or added to match the most common scenarios
  - [ ] This was checked for keyboard-only and screenreader accessibility

For maintainers

• This was checked for breaking API changes and was labeled appropriately
• This includes a feature addition or change that requires a release note and was labeled appropriately

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: de3e718ac6962784004e7c064b7cd75c18c445ae

[elasticmachine]

Pinging @elastic/siem (Team:SIEM)

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: c10dd18ff887a44417dda4c8eac567ba6f3350b0

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: c36a4488bdb3f5db83f64d62ab9c9bf33bc44cfb

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: 5441190ad88ef6d854952066015df6280fb8e04f

[FrankHassanabad]

Did you want this and not:

maxSignals != null ? maxSignals : 1000

Something seems a bit off though. Looking through a lot of this...

In the definition of:
https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/signals_alert_type.ts#L35

I see that it does have a default and this number is different than that one above.

maxSignals: schema.number({ defaultValue: 100 })

Since the alert params defines a default is it possible to still put it into a null/undefined state?

I see below this change:

maxSignals: number | undefined;

But I'm curious if we are seeing maxSignals capable of being set to null/undefined?

Unit tests have test cases covering the cases
https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/schemas.test.ts#L347

Using the script post_signal.sh it posts the signal from ./signals/root_or_admin_1.json which has this content and not a maxSignals defined which is a good e2e ad hoc test:

{
  "id": "rule-1",
  "description": "Detecting root and admin users",
  "index": ["auditbeat-*", "filebeat-*", "packetbeat-*", "winlogbeat-*"],
  "interval": "5m",
  "name": "Detect Root/Admin Users",
  "severity": "high",
  "type": "query",
  "from": "now-6m",
  "to": "now",
  "query": "user.name: root or user.name: admin",
  "language": "kuery",
  "references": ["http://www.example.com", "https://ww.example.com"]
}

Then on a read back: ./get_signal.sh rule-1

I see it coming back ok:

{
  "id": "03c687ff-98dd-4c29-ab10-a6370cf86cf2",
  "name": "Detect Root/Admin Users",
  "alertTypeId": "siem.signals",
  "alertTypeParams": {
    "description": "Detecting root and admin users",
    "id": "rule-1",
    "index": [
      "auditbeat-*",
      "filebeat-*",
      "packetbeat-*",
      "winlogbeat-*"
    ],
    "from": "now-6m",
    "filter": null,
    "query": "user.name: root or user.name: admin",
    "language": "kuery",
    "savedId": null,
    "filters": null,
    "maxSignals": 100,
    "severity": "high",
    "to": "now",
    "type": "query",
    "references": [
      "http://www.example.com",
      "https://ww.example.com"
    ]
  },
  "interval": "5m",
  "enabled": true,
  "actions": [
    {
      "group": "default",
      "params": {
        "message": "SIEM Alert Fired",
        "level": "info"
      },
      "id": "23d5f855-e9f1-443a-91f5-2bf261f3d69c"
    }
  ],
  "throttle": null,
  "createdBy": "elastic",
  "updatedBy": "elastic",
  "apiKeyOwner": "elastic",
  "muteAll": false,
  "mutedInstanceIds": [],
  "scheduledTaskId": "eacd87d0-0567-11ea-8933-b7a2b6b3b4ee"
}

On an update everything looks ok as well which also does not set the maxSignal:

./update_signal.sh
./get_signal.sh rule-1

{
  "id": "03c687ff-98dd-4c29-ab10-a6370cf86cf2",
  "name": "A different name",
  "alertTypeId": "siem.signals",
  "alertTypeParams": {
    "description": "Changed Description of only detecting root user",
    "id": "rule-1",
    "index": [
      "auditbeat-*"
    ],
    "from": "now-6m",
    "filter": null,
    "query": "user.name: root",
    "language": "kuery",
    "savedId": null,
    "filters": null,
    "maxSignals": 100,
    "severity": "high",
    "to": "now-5m",
    "type": "query",
    "references": [
      "https://update1.example.com",
      "https://update2.example.com"
    ]
  },
  "interval": "50m",
  "enabled": false,
  "actions": [
    {
      "params": {
        "level": "info",
        "message": "SIEM Alert Fired"
      },
      "group": "default",
      "id": "23d5f855-e9f1-443a-91f5-2bf261f3d69c"
    }
  ],
  "throttle": null,
  "createdBy": "elastic",
  "updatedBy": "elastic",
  "apiKeyOwner": "elastic",
  "muteAll": false,
  "mutedInstanceIds": [],
  "scheduledTaskId": null
}

Then I created this payload in /tmp/root_or_admin_1.json where I tried to update it to null:

{
  "id": "rule-1",
  "description": "Detecting root and admin users",
  "index": ["auditbeat-*", "filebeat-*", "packetbeat-*", "winlogbeat-*"],
  "interval": "5m",
  "name": "Detect Root/Admin Users",
  "severity": "high",
  "type": "query",
  "from": "now-6m",
  "max_signals": null,
  "to": "now",
  "query": "user.name: root or user.name: admin",
  "language": "kuery",
  "references": ["http://www.example.com", "https://ww.example.com"]
}

And when running:

./update_signal.sh /tmp/root_or_admin_1.json

I get back this validation error:

{
  "statusCode": 400,
  "error": "Bad Request",
  "message": "child \"max_signals\" fails because [\"max_signals\" must be a number]",
  "validation": {
    "source": "payload",
    "keys": [
      "max_signals"
    ]
  }
}

Also with a -1 it returns this:

{
  "statusCode": 400,
  "error": "Bad Request",
  "message": "child \"max_signals\" fails because [\"max_signals\" must be greater than 0]",
  "validation": {
    "source": "payload",
    "keys": [
      "max_signals"
    ]
  }
}

But a regular number in the payload such as: 100000 looks to do an update correctly:

{
  "id": "03c687ff-98dd-4c29-ab10-a6370cf86cf2",
  "name": "Detect Root/Admin Users",
  "alertTypeId": "siem.signals",
  "alertTypeParams": {
    "description": "Detecting root and admin users",
    "filter": null,
    "from": "now-6m",
    "query": "user.name: root or user.name: admin",
    "language": "kuery",
    "savedId": null,
    "filters": null,
    "index": [
      "auditbeat-*",
      "filebeat-*",
      "packetbeat-*",
      "winlogbeat-*"
    ],
    "maxSignals": 100000,
    "severity": "high",
    "to": "now",
    "type": "query",
    "references": [
      "http://www.example.com",
      "https://ww.example.com"
    ],
    "id": "rule-1"
  },
  "interval": "5m",
  "enabled": false,
  "actions": [
    {
      "params": {
        "level": "info",
        "message": "SIEM Alert Fired"
      },
      "group": "default",
      "id": "23d5f855-e9f1-443a-91f5-2bf261f3d69c"
    }
  ],
  "throttle": null,
  "createdBy": "elastic",
  "updatedBy": "elastic",
  "apiKeyOwner": "elastic",
  "muteAll": false,
  "mutedInstanceIds": [],
  "scheduledTaskId": null
}

Although the echo'ed return (my fault still) is using camelCase ... I still to need to transform eventually on the way back out. I'm just being expedient by relying on saved objects.

However, if you see this set somewhere to undefined or null let me know? I think if you have settled on your new number above of 1000 I would set that in the schema and you can remove the above code and anywhere where maxSignals is null/undefined as that shouldn't be possible as long as we keep our validation on the way in accepting defaults.

[FrankHassanabad]

See my comments above about if this type should change to be an undefined here. The source of "truth" for deriving TypeScript is from the alert schema as shown above which allows defaults:
https://github.com/elastic/kibana/pull/50129/files#r345313497

[FrankHassanabad]

'.siem-signals-10-01-2019'

Since we are going to be changing this with an upcoming PR as we finalize the index, how about we make this a const right in here:

x-pack/legacy/plugins/siem/common/constants.ts

Something in that file like this:

const DEFAULT_SIGNALS_INDEX = .siem-signals;

We should just drop the date time of -10-01-2019 in the name and then update everywhere we see this string to use that constant? It would make it easier to find all the spots and change them out when we change out the signals index way.

Looking closer though...

An additional thing we could do (as an aside) ... Just in case if we get the requirement to make the signals index output user driven/configurable ... Would be to make our functions which use the signals index "pure" and move our "impure" reading of the environment variable up more closer to the icky side effecting part of the "handler" for routes.

That's also better for your tests as you can inject what you want, and technically functions which read something global such as env are not pure ...

So, for example,

export const singleBulkIndex = async (
  sr: SignalSearchResponse,
  params: SignalAlertParams,
  service: AlertServices,
  logger: Logger
): Promise<boolean> => {

Would just become something like:

export const singleBulkIndex = async (
  sr: SignalSearchResponse,
  params: SignalAlertParams,
  service: AlertServices,
  index: string,
  logger: Logger
): Promise<boolean> => {

And then all calling code above this will keep pushing down index until you get right to the handler. If you feel the arguments are too long are hard to read, then you can make them into an options object like we have in other spots.

This would mirror the buildEventsReIndex({ API where it injects the index into its self. Lifting state and side effects up more towards the top of the application typically leads to better testable pure functions and an overall better and easier time to adapt code to changing requirements. Also the access of the environment would be grouped together in just one or two files (hopefully).

Let me know what you think. This part is optional and can be a follow on PR, but seeing these tests, this might be a great time to just do it and get it out of the way.

[FrankHassanabad]

Do you still want this as an error even though you are returning true below to indicate a non-error?

[FrankHassanabad]

Do you want this to be a warn even though below you are returning false to indicate an error?

[FrankHassanabad]

See my comments below but I think that you are always going to have a maxDocs?
https://github.com/elastic/kibana/pull/50129/files#r345313497

I think its good that we always carry a maximum value and a default even if the user tries to get around it they will have to be very intentional in increasing the number to a very high number.

Let me know otherwise. We can always increase it to a very high default, but carrying a sensible lower default feels more like the same as the other API's from Elastic we work with.

[FrankHassanabad]

Searching through the code base I think this compact form is most of what the other code writers in other sections of Kibana use for mock logger:

const mockLogger: Logger = {
  log: jest.fn(),
  trace: jest.fn(),
  debug: jest.fn(),
  info: jest.fn(),
  warn: jest.fn(),
  error: jest.fn(),
  fatal: jest.fn(),
};

Testing it and it seems to work well here.

But if you need the echo'ing out of stuff that it is fine as well as is. Just pointing it out.

[FrankHassanabad]

nit, the type is already inferred so you only need:

const sampleParams = sampleSignalAlertParams(undefined);

[FrankHassanabad]

In general I would be careful about tests that are checking how many times a mock logger debug or warn statement is being called. That gets very specific and a maintainer will be updating these numbers as they either add or remove debug and warn statements from our loggers?

If it's 3 vs 2 times vs 5, etc... I don't know what extra value that provides for the test.

I understand checking if something is called for part of branching logic, but writing a lot of tests around how many times a particular logger debug or warn is being called is going to add more overhead to maintaining and not really provide much value IMHO.

I wouldn't have expects for these

[FrankHassanabad]

Having the return value being checked is great though 👍! I like the tests around the return types.

[FrankHassanabad]

In general for most of our tests we want to have one or maybe at most two or three expects per test. These are pushing the boundary a bit, and you don't have to split these all out, but if you look around the code base most of the examples of unit tests within our plugin, core, and other plugins usually follow the motto of one expect per test section around ~95% of the time.

Exception to the rule would be table test areas where there is a big array and then a forEach ontop of that array to test all the values. That is a bit different paradigm but within that we are hopeful that as the table test executes and if it fails it will be one failure per test block still (just wrapped in an array).

[FrankHassanabad]

When testing try catch blocks like this, you have a general problem which is that expect statement will only execute and fail the test if your function does a throw and then in the catch it sees a different value.

However, if your function of singleSearchAfter never does a throw, you never reach the catch block and never fail the test. Instead it continues and reports the test as still passing. You have what looks like an extra expect to catch that case well but there is one other way.

If you want to fail your test if you do not throw then what you want to use is rejects.toThrow. Examples are in our code but this one would be:

      await expect(
        singleSearchAfter(searchAfterSortId, sampleParams, mockService, mockLogger)
      ).rejects.toThrow('Attempted to search after with empty sort id');

Then if that does not do a throw it will fail the test and you don't need to write extra expect statements for when it does not fail the test.

[FrankHassanabad]

These checks for SIEM signals do fail if I have a custom SIEM dev signals configured via my environment. We will be fixing/changing/removing the env variables but for now I would add a simple function for these tests which does:

• moves the string to our common/constants
• Simple function in your tests which gets either the configured env. or the default SIEM index.

const { DEFAULT_SIEM_INDEX } = '../../constants'
...
const getTestIndex = () => process.env.SIGNALS_INDEX || DEFAULT_SIEM_INDEX
...
expect(params.index).toEqual(getTestIndex());

[FrankHassanabad]

Per my earlier comment about trying to have only 1, maybe 2 expect statement at most per test for maintainers and developers to easily understand what is going on and reading code...

This is a bit of an example of why that is needed because starting on lines 494 thorough 497, these expect statements are never reached. Reason being is that on line 484, you have an early return statement.

This part of the test is confusing to me because I see one expect statement which says:

expect(action).toEqual('bulk');

But then on 494 I see a contradiction of:

expect(action).toEqual('search');

Since you have early return statements for this and if we followed the paradigm of the least amount of expect statements per test this would actually become this code:

    test('if one successful iteration of searchAfterAndBulkIndex', async () => {
      const searchAfterSortId = '1234567891111';
      const sampleParams = sampleSignalAlertParams(undefined);
      const savedObjectsClient = savedObjectsClientMock.create();
      buildEventsSearchQuery({
        index: sampleParams.index,
        from: sampleParams.from,
        to: sampleParams.to,
        filter: sampleParams.filter,
        size: sampleParams.size ? sampleParams.size : 1,
        searchAfterSortId,
        maxDocs: undefined,
      });
      const mockService: AlertServices = {
        callCluster: async (action: string, params: SearchParams) => {
          expect(action).toEqual('bulk');
          expect(params.index).toEqual('.siem-signals-10-01-2019');
          return {
            took: 100,
            errors: false,
            items: [
              {
                fakeItemValue: 'fakeItemKey',
              },
            ],
          };
        },
        alertInstanceFactory: jest.fn(),
        savedObjectsClient,
      };
      const result = await searchAfterAndBulkIndex(
        sampleDocSearchResultsWithSortId,
        sampleParams,
        mockService,
        mockLogger
      );
      expect(result).toEqual(true);
    });

You do want to avoid dead branching logic within tests as much as possible such as if statements or larger harnesses so we can be sure that each expect statement is reached and each expect statement does indeed do what we intend it to do which is prove a particular value's truthfulness.

[FrankHassanabad]

Same comment as above, starting on line 538-540, these expect statements look to never reached. I would remove them and the if check if that is the case.

[FrankHassanabad]

For some of the tests such as this one where the callCluster is called repeatedly with values

I would rely on using if possible just pure jest to ask jest about each time the function was called and with what parameters. Likewise Jest has ways of mocking heterogenous return values based on number of times something is called rather than mixing data driven values (which might make code changes down the road harder):
https://jestjs.io/docs/en/mock-function-api.html#mockfnmockimplementationoncefn

I see what some of these are doing, but the harder part is trying to keep the mocking portion separated from the expect values and avoid mixing them together to where you have part mock and part test pushed together. Rather a good refactor (doesn't have to be with this PR since there is so much here already) is to try and keep them separated from each other and of course keep the expects down to a concentrated 1 or 2 per test.

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: cef0969e3eb0968e0ea648368d595ae735a34f17

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: 9bb8e14

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: f0da0ae

[FrankHassanabad]

Awesome! Thanks for all the fixes and cleanups, this is going to help my next PR a LOT to ensure I didn't break anything.