Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions x-pack/test/apm_api_integration/common/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { registry } from './registry';
interface Config {
name: APMFtrConfigName;
license: 'basic' | 'trial';
kibanaConfig?: Record<string, string>;
kibanaConfig?: Record<string, string | string[]>;
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 the above while I was debugging the integration tests using the following as part of the FTR kibana config and needed to pass in an array of values instead of just a string. I can remove this but figured it might be useful going forward for anybody who is debugging tests in the future.

--logging.verbose=true,
--logging.events.log=${JSON.stringify([ 'alerts', 'ruleRegistry', 'apm', 'securitySolution', 'info', 'warning', 'error', 'fatal', ])},
--logging.events.request=${JSON.stringify(['info', 'warning', 'error', 'fatal'])},
--logging.events.error='*',
--logging.events.ops=__no-ops__,

}

const supertestAsApmUser = (kibanaServer: UrlObject, apmUser: ApmUser) => async (
Expand Down Expand Up @@ -81,7 +81,9 @@ export function createTestConfig(config: Config) {
serverArgs: [
...xPackAPITestsConfig.get('kbnTestServer.serverArgs'),
...(kibanaConfig
? Object.entries(kibanaConfig).map(([key, value]) => `--${key}=${value}`)
? Object.entries(kibanaConfig).map(([key, value]) =>
Array.isArray(value) ? `--${key}=${JSON.stringify(value)}` : `--${key}=${value}`
)
Comment on lines +84 to +86
Copy link
Contributor Author

Choose a reason for hiding this comment

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

related to my above comment

: []),
],
},
Expand Down
131 changes: 80 additions & 51 deletions x-pack/test/apm_api_integration/tests/alerts/rule_registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@ export default function ApiTest({ getService }: FtrProviderContext) {
const BULK_INDEX_DELAY = 1000;
const INDEXING_DELAY = 5000;

const ALERTS_INDEX_TARGET = '.alerts-observability.apm.alerts*';
const getAlertsTargetIndicesUrl =
'/api/observability/rules/alerts/dynamic_index_pattern?namespace=default&registrationContexts=observability.apm&registrationContexts=';

const getAlertsTargetIndices = async () =>
supertest.get(getAlertsTargetIndicesUrl).send().set('kbn-xsrf', 'foo');
const APM_METRIC_INDEX_NAME = 'apm-8.0.0-transaction';

const createTransactionMetric = (override: Record<string, any>) => {
Expand Down Expand Up @@ -92,6 +96,13 @@ export default function ApiTest({ getService }: FtrProviderContext) {
.get(`/api/alerts/alert/${alert.id}`)
.set('kbn-xsrf', 'foo');

const { body: targetIndices, status: targetIndicesStatus } = await getAlertsTargetIndices();
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the purpose of this exactly? This function only cares about whether the rule was executed. There might be a scenario where the alert index does not exist, but it's okay because no alerts were written.

if (targetIndices.length === 0) {
const error = new Error('Error getting alert');
Object.assign(error, { response: { body: targetIndices, status: targetIndicesStatus } });
throw error;
}

if (status >= 300) {
const error = new Error('Error getting alert');
Object.assign(error, { response: { body, status } });
Expand All @@ -104,10 +115,22 @@ export default function ApiTest({ getService }: FtrProviderContext) {
await new Promise((resolve) => {
setTimeout(resolve, BULK_INDEX_DELAY);
});
await es.indices.refresh({
index: ALERTS_INDEX_TARGET,
});

/**
* When calling refresh on an index pattern .alerts-observability.apm.alerts* (as was originally the hard-coded string in this test)
* The response from Elasticsearch is a 200, even if no indices which match that index pattern have been created.
* When calling refresh on a concrete index alias .alerts-observability.apm.alerts-default for instance,
* we receive a 404 error index_not_found_exception when no indices have been created which match that alias (obviously).
* Since we are receiving a concrete index alias from the observability api instead of a kibana index pattern
* and we understand / expect that this index does not exist at certain points of the test, we can try-catch at certain points without caring if the call fails.
* There are points in the code where we do want to ensure we get the appropriate error message back
*/
try {
await es.indices.refresh({
index: targetIndices[0],
});
// eslint-disable-next-line no-empty
} catch (exc) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

can you leave a comment here why it's okay for this to fail?

Copy link
Contributor Author

@dhurley14 dhurley14 Aug 24, 2021

Choose a reason for hiding this comment

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

For sure I can add a comment to the code. Essentially it boils down to this:

When calling refresh on an index pattern .alerts-observability.apm.alerts* (as was originally the hard-coded string in this test) The response from Elasticsearch is a 200, even if no indices which match that index pattern have been created.

When calling refresh on a concrete index alias .alerts-observability.apm.alerts-default for instance, we receive a 404 error index_not_found_exception when no indices have been created which match that alias (obviously).

Since we are receiving a concrete index alias from the observability api instead of a kibana index pattern and we understand / expect that this index does not exist at certain points of the test, we can try-catch at certain points without caring if the call fails. There are points in the code where we do want to ensure we get the appropriate error message back (such as this expect you commented on)

Example..
# This request yields a 200 response
GET thisdoesnotexist*/_refresh

# Response
{
  "_shards" : {
    "total" : 0,
    "successful" : 0,
    "failed" : 0
  }
}


# This request yields a 404 error

GET thisdoesnotexist/_refresh

# Response
{
  "error" : {
    "root_cause" : [
      {
        "type" : "index_not_found_exception",
        "reason" : "no such index [thisdoesnotexist]",
        "resource.type" : "index_or_alias",
        "resource.id" : "thisdoesnotexist",
        "index_uuid" : "_na_",
        "index" : "thisdoesnotexist"
      }
    ],
    "type" : "index_not_found_exception",
    "reason" : "no such index [thisdoesnotexist]",
    "resource.type" : "index_or_alias",
    "resource.id" : "thisdoesnotexist",
    "index_uuid" : "_na_",
    "index" : "thisdoesnotexist"
  },
  "status" : 404
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here: 6c5e2bf

Copy link
Contributor

Choose a reason for hiding this comment

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

is allow_no_indices not good enough here?

Copy link
Contributor

Choose a reason for hiding this comment

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

or maybe we can just add an asterisk to the end?

return nextAlert;
}

Expand All @@ -120,20 +143,17 @@ export default function ApiTest({ getService }: FtrProviderContext) {

registry.when('Rule registry with write enabled', { config: 'rules', archives: [] }, () => {
it('does not bootstrap indices on plugin startup', async () => {
const { body } = await es.indices.get({
index: ALERTS_INDEX_TARGET,
expand_wildcards: 'open',
allow_no_indices: true,
});

const indices = Object.entries(body).map(([indexName, index]) => {
return {
indexName,
index,
};
});

expect(indices.length).to.be(0);
const { body: targetIndices } = await getAlertsTargetIndices();
try {
const res = await es.indices.get({
index: targetIndices[0],
expand_wildcards: 'open',
allow_no_indices: true,
});
expect(res).to.be.empty();
} catch (exc) {
expect(exc.statusCode).to.eql(404);
Copy link
Contributor

Choose a reason for hiding this comment

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

there's no assertion here when the request succeeds, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch I'll update this to fail if a 200 response is received.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here: 6c5e2bf

}
});

describe('when creating a rule', () => {
Expand Down Expand Up @@ -232,6 +252,7 @@ export default function ApiTest({ getService }: FtrProviderContext) {
});

after(async () => {
const { body: targetIndices } = await getAlertsTargetIndices();
if (createResponse.alert) {
const { body, status } = await supertest
.delete(`/api/alerts/alert/${createResponse.alert.id}`)
Expand All @@ -245,7 +266,7 @@ export default function ApiTest({ getService }: FtrProviderContext) {
}

await es.deleteByQuery({
index: ALERTS_INDEX_TARGET,
index: targetIndices[0],
body: {
query: {
match_all: {},
Expand All @@ -263,25 +284,29 @@ export default function ApiTest({ getService }: FtrProviderContext) {
expect(createResponse.status).to.be.below(299);

expect(createResponse.alert).not.to.be(undefined);

let alert = await waitUntilNextExecution(createResponse.alert);

const beforeDataResponse = await es.search({
index: ALERTS_INDEX_TARGET,
body: {
query: {
term: {
[EVENT_KIND]: 'signal',
const { body: targetIndices } = await getAlertsTargetIndices();

try {
const res = await es.search({
index: targetIndices[0],
body: {
query: {
term: {
[EVENT_KIND]: 'signal',
},
},
size: 1,
sort: {
'@timestamp': 'desc',
},
},
size: 1,
sort: {
'@timestamp': 'desc',
},
},
});

expect(beforeDataResponse.body.hits.hits.length).to.be(0);
});
expect(res).to.be.empty();
} catch (exc) {
expect(exc.message).contain('index_not_found_exception');
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, there's only an assertion when the request fails

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup I'll update this to fail if a 200 is received as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here: 6c5e2bf

Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if this is necessary. I think the intent here was to ensure there was no data written before the alert executed, and before violations were indexed, I don't think we need to test whether the index exists here, as long as we have a different test for that (which I think we do?). My concern here would be a) that these assertions are adding to the complexity of the test, b) might cause some false negatives to trigger in certain scenarios (e.g. when the index is not cleaned up after a test run, or the implementation changes).

}

await es.index({
index: APM_METRIC_INDEX_NAME,
Expand All @@ -295,22 +320,25 @@ export default function ApiTest({ getService }: FtrProviderContext) {

alert = await waitUntilNextExecution(alert);

const afterInitialDataResponse = await es.search({
index: ALERTS_INDEX_TARGET,
body: {
query: {
term: {
[EVENT_KIND]: 'signal',
try {
const res = await es.search({
index: targetIndices[0],
body: {
query: {
term: {
[EVENT_KIND]: 'signal',
},
},
size: 1,
sort: {
'@timestamp': 'desc',
},
},
size: 1,
sort: {
'@timestamp': 'desc',
},
},
});

expect(afterInitialDataResponse.body.hits.hits.length).to.be(0);
});
expect(res).to.be.empty();
} catch (exc) {
expect(exc.message).contain('index_not_found_exception');
}

await es.index({
index: APM_METRIC_INDEX_NAME,
Expand All @@ -325,7 +353,7 @@ export default function ApiTest({ getService }: FtrProviderContext) {
alert = await waitUntilNextExecution(alert);

const afterViolatingDataResponse = await es.search({
index: ALERTS_INDEX_TARGET,
index: targetIndices[0],
body: {
query: {
term: {
Expand Down Expand Up @@ -437,7 +465,7 @@ export default function ApiTest({ getService }: FtrProviderContext) {
alert = await waitUntilNextExecution(alert);

const afterRecoveryResponse = await es.search({
index: ALERTS_INDEX_TARGET,
index: targetIndices[0],
body: {
query: {
term: {
Expand Down Expand Up @@ -530,9 +558,10 @@ export default function ApiTest({ getService }: FtrProviderContext) {

registry.when('Rule registry with write not enabled', { config: 'basic', archives: [] }, () => {
it('does not bootstrap the apm rule indices', async () => {
const { body: targetIndices } = await getAlertsTargetIndices();
const errorOrUndefined = await es.indices
.get({
index: ALERTS_INDEX_TARGET,
index: targetIndices[0],
expand_wildcards: 'open',
allow_no_indices: false,
})
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import expect from '@kbn/expect';

import { superUser, obsOnlySpacesAll, secOnlyRead } from '../../../common/lib/authentication/users';
import type { User } from '../../../common/lib/authentication/types';
import { FtrProviderContext } from '../../../common/ftr_provider_context';
import { getSpaceUrlPrefix } from '../../../common/lib/authentication/spaces';

// eslint-disable-next-line import/no-default-export
export default ({ getService }: FtrProviderContext) => {
const supertestWithoutAuth = getService('supertestWithoutAuth');
const esArchiver = getService('esArchiver');

const TEST_URL = '/internal/rac/alerts';
const ALERTS_INDEX_URL = `${TEST_URL}/index`;
const SPACE1 = 'space1';
const APM_ALERT_INDEX = '.alerts-observability-apm';
Copy link
Contributor

Choose a reason for hiding this comment

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

Note for myself: I will need to fix this name in #109567

const SECURITY_SOLUTION_ALERT_INDEX = '.alerts-security.alerts';

const getAPMIndexName = async (user: User, space: string, expected: number = 200) => {
const {
body: indexNames,
}: { body: { index_name: string[] | undefined } } = await supertestWithoutAuth
.get(`${getSpaceUrlPrefix(space)}${ALERTS_INDEX_URL}?features=apm`)
.auth(user.username, user.password)
.set('kbn-xsrf', 'true')
.expect(expected);
return indexNames;
};

const getSecuritySolutionIndexName = async (
user: User,
space: string,
expectedStatusCode: number = 200
) => {
const {
body: indexNames,
}: { body: { index_name: string[] | undefined } } = await supertestWithoutAuth
.get(`${getSpaceUrlPrefix(space)}${ALERTS_INDEX_URL}?features=siem`)
.auth(user.username, user.password)
.set('kbn-xsrf', 'true')
.expect(expectedStatusCode);
return indexNames;
};

describe('Alert - Get Index - RBAC - spaces', () => {
before(async () => {
await esArchiver.load('x-pack/test/functional/es_archives/rule_registry/alerts');
});
describe('Users:', () => {
it(`${obsOnlySpacesAll.username} should be able to access the APM alert in ${SPACE1}`, async () => {
const indexNames = await getAPMIndexName(obsOnlySpacesAll, SPACE1);
const observabilityIndex = indexNames?.index_name?.find(
(indexName) => indexName === APM_ALERT_INDEX
);
expect(observabilityIndex).to.eql(APM_ALERT_INDEX); // assert this here so we can use constants in the dynamically-defined test cases below
});

it(`${superUser.username} should be able to access the APM alert in ${SPACE1}`, async () => {
const indexNames = await getAPMIndexName(superUser, SPACE1);
const observabilityIndex = indexNames?.index_name?.find(
(indexName) => indexName === APM_ALERT_INDEX
);
expect(observabilityIndex).to.eql(APM_ALERT_INDEX); // assert this here so we can use constants in the dynamically-defined test cases below
});

it(`${secOnlyRead.username} should NOT be able to access the APM alert in ${SPACE1}`, async () => {
const indexNames = await getAPMIndexName(secOnlyRead, SPACE1);
expect(indexNames?.index_name?.length).to.eql(0);
});

it(`${secOnlyRead.username} should be able to access the security solution alert in ${SPACE1}`, async () => {
const indexNames = await getSecuritySolutionIndexName(secOnlyRead, SPACE1);
const securitySolution = indexNames?.index_name?.find((indexName) =>
indexName.startsWith(SECURITY_SOLUTION_ALERT_INDEX)
);
expect(securitySolution).to.eql(`${SECURITY_SOLUTION_ALERT_INDEX}-${SPACE1}`); // assert this here so we can use constants in the dynamically-defined test cases below
});
});
});
};
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,6 @@ export default ({ loadTestFile, getService }: FtrProviderContext): void => {
loadTestFile(require.resolve('./update_alert'));
loadTestFile(require.resolve('./bulk_update_alerts'));
loadTestFile(require.resolve('./find_alerts'));
loadTestFile(require.resolve('./get_alerts_index'));
});
};