From 2da8b719c2bbccb1db38f372357d771493c916bd Mon Sep 17 00:00:00 2001 From: Devin Hurley Date: Fri, 5 Jun 2020 15:24:25 -0400 Subject: [PATCH 1/2] adds/modifies e2e tests to ensure find_status returns succeeded after rules are created, instead of just 'going to run' --- .../security_and_spaces/tests/create_rules.ts | 26 ++++++++++++++++++- .../tests/create_rules_bulk.ts | 26 ++++++++++++++++++- .../tests/find_statuses.ts | 2 +- 3 files changed, 51 insertions(+), 3 deletions(-) diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules.ts b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules.ts index 449168d73f4ef..6efbdc14bc49e 100644 --- a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules.ts +++ b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules.ts @@ -6,7 +6,10 @@ import expect from '@kbn/expect'; -import { DETECTION_ENGINE_RULES_URL } from '../../../../plugins/security_solution/common/constants'; +import { + DETECTION_ENGINE_RULES_URL, + DETECTION_ENGINE_RULES_STATUS_URL, +} from '../../../../plugins/security_solution/common/constants'; import { FtrProviderContext } from '../../common/ftr_provider_context'; import { createSignalsIndex, @@ -65,6 +68,27 @@ export default ({ getService }: FtrProviderContext) => { expect(bodyToCompare).to.eql(getSimpleRuleOutput()); }); + it('should create a single rule with a rule_id and validate it ran successfully', async () => { + const simpleRule = getSimpleRule(); + const { body } = await supertest + .post(DETECTION_ENGINE_RULES_URL) + .set('kbn-xsrf', 'true') + .send(simpleRule) + .expect(200); + + // wait for Task Manager to execute the rule and update status + await new Promise((resolve) => setTimeout(resolve, 5000)); + const { body: statusBody } = await supertest + .post(DETECTION_ENGINE_RULES_STATUS_URL) + .set('kbn-xsrf', 'true') + .send({ ids: [body.id] }) + .expect(200); + + const bodyToCompare = removeServerGeneratedProperties(body); + expect(bodyToCompare).to.eql(getSimpleRuleOutput()); + expect(statusBody[body.id].current_status.status).to.eql('succeeded'); + }); + it('should create a single rule without an input index', async () => { const { index, ...payload } = getSimpleRule(); const { index: _index, ...expected } = getSimpleRuleOutput(); diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts index 6c3b1c45e202e..c7671e298a804 100644 --- a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts +++ b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts @@ -6,7 +6,10 @@ import expect from '@kbn/expect'; -import { DETECTION_ENGINE_RULES_URL } from '../../../../plugins/security_solution/common/constants'; +import { + DETECTION_ENGINE_RULES_URL, + DETECTION_ENGINE_RULES_STATUS_URL, +} from '../../../../plugins/security_solution/common/constants'; import { FtrProviderContext } from '../../common/ftr_provider_context'; import { createSignalsIndex, @@ -68,6 +71,27 @@ export default ({ getService }: FtrProviderContext): void => { expect(bodyToCompare).to.eql(getSimpleRuleOutput()); }); + it('should create a single rule with a rule_id and validate it ran successfully', async () => { + const simpleRule = getSimpleRule(); + const { body } = await supertest + .post(`${DETECTION_ENGINE_RULES_URL}/_bulk_create`) + .set('kbn-xsrf', 'true') + .send([simpleRule]) + .expect(200); + + // wait for Task Manager to execute the rule and update status + await new Promise((resolve) => setTimeout(resolve, 5000)); + const { body: statusBody } = await supertest + .post(DETECTION_ENGINE_RULES_STATUS_URL) + .set('kbn-xsrf', 'true') + .send({ ids: [body[0].id] }) + .expect(200); + + const bodyToCompare = removeServerGeneratedProperties(body[0]); + expect(bodyToCompare).to.eql(getSimpleRuleOutput()); + expect(statusBody[body[0].id].current_status.status).to.eql('succeeded'); + }); + it('should create a single rule without a rule_id', async () => { const { body } = await supertest .post(`${DETECTION_ENGINE_RULES_URL}/_bulk_create`) diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/find_statuses.ts b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/find_statuses.ts index 6ff5a8c552ab7..3a6a04e081e99 100644 --- a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/find_statuses.ts +++ b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/find_statuses.ts @@ -61,7 +61,7 @@ export default ({ getService }: FtrProviderContext): void => { .expect(200); // expected result for status should be 'going to run' or 'succeeded - expect(['succeeded', 'going to run']).to.contain(body[resBody.id].current_status.status); + expect(body[resBody.id].current_status.status).to.eql('succeeded'); }); }); }; From 8d3640f60c9aa20b9e5d21c304ca6b80d16acb5f Mon Sep 17 00:00:00 2001 From: Devin Hurley Date: Mon, 8 Jun 2020 10:44:17 -0400 Subject: [PATCH 2/2] add documentation around newly created e2e tests explaining bug and specific regression to be on the lookout for if these start failing --- .../security_and_spaces/tests/create_rules.ts | 19 +++++++++++++++++++ .../tests/create_rules_bulk.ts | 19 +++++++++++++++++++ .../tests/find_statuses.ts | 19 +++++++++++++++++++ 3 files changed, 57 insertions(+) diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules.ts b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules.ts index 6efbdc14bc49e..8847b0c3250ac 100644 --- a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules.ts +++ b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules.ts @@ -68,6 +68,25 @@ export default ({ getService }: FtrProviderContext) => { expect(bodyToCompare).to.eql(getSimpleRuleOutput()); }); + /* + This test is to ensure no future regressions introduced by the following scenario + a call to updateApiKey was invalidating the api key used by the + rule while the rule was executing, or even before it executed, + on the first rule run. + this pr https://github.com/elastic/kibana/pull/68184 + fixed this by finding the true source of a bug that required the manual + api key update, and removed the call to that function. + + When the api key is updated before / while the rule is executing, the alert + executor no longer has access to a service to update the rule status + saved object in Elasticsearch. Because of this, we cannot set the rule into + a 'failure' state, so the user ends up seeing 'going to run' as that is the + last status set for the rule before it erupts in an error that cannot be + recorded inside of the executor. + + This adds an e2e test for the backend to catch that in case + this pops up again elsewhere. + */ it('should create a single rule with a rule_id and validate it ran successfully', async () => { const simpleRule = getSimpleRule(); const { body } = await supertest diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts index c7671e298a804..47b09fd1fe3c7 100644 --- a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts +++ b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts @@ -71,6 +71,25 @@ export default ({ getService }: FtrProviderContext): void => { expect(bodyToCompare).to.eql(getSimpleRuleOutput()); }); + /* + This test is to ensure no future regressions introduced by the following scenario + a call to updateApiKey was invalidating the api key used by the + rule while the rule was executing, or even before it executed, + on the first rule run. + this pr https://github.com/elastic/kibana/pull/68184 + fixed this by finding the true source of a bug that required the manual + api key update, and removed the call to that function. + + When the api key is updated before / while the rule is executing, the alert + executor no longer has access to a service to update the rule status + saved object in Elasticsearch. Because of this, we cannot set the rule into + a 'failure' state, so the user ends up seeing 'going to run' as that is the + last status set for the rule before it erupts in an error that cannot be + recorded inside of the executor. + + This adds an e2e test for the backend to catch that in case + this pops up again elsewhere. + */ it('should create a single rule with a rule_id and validate it ran successfully', async () => { const simpleRule = getSimpleRule(); const { body } = await supertest diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/find_statuses.ts b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/find_statuses.ts index 3a6a04e081e99..f659857b89a97 100644 --- a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/find_statuses.ts +++ b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/find_statuses.ts @@ -42,6 +42,25 @@ export default ({ getService }: FtrProviderContext): void => { expect(body).to.eql({}); }); + /* + This test is to ensure no future regressions introduced by the following scenario + a call to updateApiKey was invalidating the api key used by the + rule while the rule was executing, or even before it executed, + on the first rule run. + this pr https://github.com/elastic/kibana/pull/68184 + fixed this by finding the true source of a bug that required the manual + api key update, and removed the call to that function. + + When the api key is updated before / while the rule is executing, the alert + executor no longer has access to a service to update the rule status + saved object in Elasticsearch. Because of this, we cannot set the rule into + a 'failure' state, so the user ends up seeing 'going to run' as that is the + last status set for the rule before it erupts in an error that cannot be + recorded inside of the executor. + + This adds an e2e test for the backend to catch that in case + this pops up again elsewhere. + */ it('should return a single rule status when a single rule is loaded from a find status with defaults added', async () => { // add a single rule const { body: resBody } = await supertest