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

[APM] Migrate settings API tests to be deployment-agnostic #200762

Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
1c3c51a
Create settings/anomaly_detection basic deployment agnostic api tests
iblancof Nov 18, 2024
744db47
Create settings/anomaly_detection read user deployment agnostic api t…
iblancof Nov 18, 2024
eeb1971
Create settings/custom_link deployment agnostic api tests
iblancof Nov 19, 2024
a825c95
Create settings/apm_indices deployment agnostic api tests
iblancof Nov 19, 2024
86c262f
Create settings/agent_keys deployment agnostic api tests
iblancof Nov 19, 2024
8c4f48a
Create tests loader for settings deployment agnostic tests
iblancof Nov 19, 2024
4a4c838
Merge branch 'main' into 198989-apm-migrate-test-apm_api_integration-…
iblancof Nov 19, 2024
ed0bc6b
Clean stateful tests
iblancof Nov 19, 2024
e74f551
Refactor imports to use type-only imports in APM settings test files
iblancof Nov 19, 2024
e183e46
Invalidate samlAuth
iblancof Nov 19, 2024
48a98c9
Remove unused constant
iblancof Nov 19, 2024
8c61a72
Update x-pack/test/api_integration/deployment_agnostic/apis/observabi…
iblancof Nov 20, 2024
41e3a23
Update x-pack/test/api_integration/deployment_agnostic/apis/observabi…
iblancof Nov 20, 2024
2cb8be0
Remove migrated test
iblancof Nov 20, 2024
f2d8c39
Remove FIPS skip
iblancof Nov 20, 2024
de4cb58
Better support for public api calls
crespocarlos Nov 20, 2024
f10b108
Merge pull request #1 from crespocarlos/apm-api-service-improvement
iblancof Nov 20, 2024
adc918a
Merge branch 'main' into 198989-apm-migrate-test-apm_api_integration-…
iblancof Nov 20, 2024
d50e6bf
Use publicApi
iblancof Nov 20, 2024
294c068
Reorder apmApiIntegrationTests
iblancof Nov 20, 2024
1b1123a
Merge branch 'main' into 198989-apm-migrate-test-apm_api_integration-…
iblancof Nov 20, 2024
4fcd4dc
Merge branch 'main' into 198989-apm-migrate-test-apm_api_integration-…
iblancof Nov 20, 2024
4134664
Merge branch 'main' into 198989-apm-migrate-test-apm_api_integration-…
iblancof Nov 20, 2024
e2ed6f5
Remove duplicated test file import
iblancof Nov 20, 2024
61a9a0c
Merge branch 'main' into 198989-apm-migrate-test-apm_api_integration-…
iblancof Nov 21, 2024
4f8ad0c
Remove test case that was not deployment agnostic
iblancof Nov 21, 2024
f1e40f1
Remove duplicated writeUser test
iblancof Nov 21, 2024
681d40a
Merge branch '198989-apm-migrate-test-apm_api_integration-tests-setti…
iblancof Nov 21, 2024
96db562
Merge branch 'main' into 198989-apm-migrate-test-apm_api_integration-…
iblancof Nov 21, 2024
e8371fb
Merge branch 'main' into 198989-apm-migrate-test-apm_api_integration-…
iblancof Nov 21, 2024
269f33f
Add deleteSavedObject again afterEach
iblancof Nov 21, 2024
ea617dd
Merge branch 'main' into 198989-apm-migrate-test-apm_api_integration-…
iblancof Nov 21, 2024
f5c0e14
Merge branch 'main' into 198989-apm-migrate-test-apm_api_integration-…
iblancof Nov 21, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -12,31 +12,32 @@ export default function apmApiIntegrationTests({
}: DeploymentAgnosticFtrProviderContext) {
describe('APM', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for reorg the list. This was bothering me so much.

loadTestFile(require.resolve('./agent_explorer'));
loadTestFile(require.resolve('./errors'));
loadTestFile(require.resolve('./alerts'));
loadTestFile(require.resolve('./mobile'));
loadTestFile(require.resolve('./cold_start'));
loadTestFile(require.resolve('./correlations'));
loadTestFile(require.resolve('./custom_dashboards'));
loadTestFile(require.resolve('./data_view'));
loadTestFile(require.resolve('./dependencies'));
loadTestFile(require.resolve('./diagnostics'));
loadTestFile(require.resolve('./entities'));
loadTestFile(require.resolve('./environment'));
loadTestFile(require.resolve('./error_rate'));
loadTestFile(require.resolve('./data_view'));
loadTestFile(require.resolve('./correlations'));
loadTestFile(require.resolve('./entities'));
loadTestFile(require.resolve('./cold_start'));
loadTestFile(require.resolve('./metrics'));
loadTestFile(require.resolve('./services'));
loadTestFile(require.resolve('./errors'));
loadTestFile(require.resolve('./historical_data'));
loadTestFile(require.resolve('./observability_overview'));
loadTestFile(require.resolve('./latency'));
loadTestFile(require.resolve('./infrastructure'));
loadTestFile(require.resolve('./service_maps'));
loadTestFile(require.resolve('./inspect'));
loadTestFile(require.resolve('./latency'));
loadTestFile(require.resolve('./metrics'));
loadTestFile(require.resolve('./mobile'));
loadTestFile(require.resolve('./observability_overview'));
loadTestFile(require.resolve('./service_groups'));
loadTestFile(require.resolve('./time_range_metadata'));
loadTestFile(require.resolve('./diagnostics'));
loadTestFile(require.resolve('./service_maps'));
loadTestFile(require.resolve('./service_nodes'));
loadTestFile(require.resolve('./services'));
loadTestFile(require.resolve('./settings'));
loadTestFile(require.resolve('./span_links'));
loadTestFile(require.resolve('./suggestions'));
loadTestFile(require.resolve('./throughput'));
loadTestFile(require.resolve('./time_range_metadata'));
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/*
* 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 { PrivilegeType, ClusterPrivilegeType } from '@kbn/apm-plugin/common/privilege_type';
import type { RoleCredentials } from '../../../../../services';
import type { DeploymentAgnosticFtrProviderContext } from '../../../../../ftr_provider_context';
import { expectToReject } from '../../../../../../../apm_api_integration/common/utils/expect_to_reject';
import type { ApmApiError } from '../../../../../services/apm_api';

export default function ApiTest({ getService }: DeploymentAgnosticFtrProviderContext) {
const apmApiClient = getService('apmApi');
const samlAuth = getService('samlAuth');

const agentKeyName = 'test';
const allApplicationPrivileges = [PrivilegeType.AGENT_CONFIG, PrivilegeType.EVENT];
const clusterPrivileges = [ClusterPrivilegeType.MANAGE_OWN_API_KEY];

async function createAgentKey(
apiClient: (typeof apmApiClient)[keyof typeof apmApiClient],
roleAuthc: RoleCredentials,
privileges = allApplicationPrivileges
) {
return await apiClient({
iblancof marked this conversation as resolved.
Show resolved Hide resolved
endpoint: 'POST /api/apm/agent_keys 2023-10-31',
params: {
body: {
name: agentKeyName,
privileges,
},
},
roleAuthc,
});
}

async function invalidateAgentKey(id: string) {
return await apmApiClient.writeUser({
endpoint: 'POST /internal/apm/api_key/invalidate',
params: {
body: { id },
},
});
}

async function getAgentKeys() {
return await apmApiClient.writeUser({ endpoint: 'GET /internal/apm/agent_keys' });
}

describe('When the user does not have the required privileges', () => {
let roleAuthc: RoleCredentials;

before(async () => {
roleAuthc = await samlAuth.createM2mApiKeyWithRoleScope('editor');
});
Comment on lines +51 to +53
Copy link
Member

Choose a reason for hiding this comment

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

API key should be invalidated in after hook:

    after(async () => {
      await samlAuth.invalidateM2mApiKeyWithRoleScope(roleAuthc);
    });

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 missed that, thank you!

e183e46


after(async () => {
await samlAuth.invalidateM2mApiKeyWithRoleScope(roleAuthc);
});

describe('When the user does not have the required cluster privileges', () => {
it('should return an error when creating an agent key', async () => {
const error = await expectToReject<ApmApiError>(() =>
createAgentKey(apmApiClient.writeUser, roleAuthc)
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain why do we use different sources of authentication here: afaik apmApiClient.writeUser is already authorised with Editor privileges and a another "fresh" API key is passed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The POST /api/apm/agent_keys 2023-10-31 endpoint, when used with the apmApiClient, specifically requires the roleAuthc parameter.

From what I observed in the code, this is necessary because the endpoint is not considered internal, so additional credential information must be included in the request headers.

It seems the credentials require an API key, but the apmApiClient does not have one by default. To address this, we need to create a new role with the appropriate API key attached, ensuring the credentials are correctly passed in the request headers.

I’m not sure about the reasoning behind handling this outside of the apmApiClient instead of within it. Maybe it’s related to the need to invalidate the API key after the tests are run, avoiding the creation and invalidation of keys for every request that requires one.

Perhaps @crespocarlos could share more details about why it’s done this way in case you want to know more about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I'm looking at it, it is weird. We need to improve it.

roleAuthc needs to be passed to apmApi because, as you've said, we need to destroy the key after the test runs, but calling apmApi service with writeUser and then having to pass the apiKey looks very strange.

Copy link
Contributor

@crespocarlos crespocarlos Nov 20, 2024

Choose a reason for hiding this comment

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

For now, lets not use the apmApi to call public APIs. It's a momentary compromise on the strongly typed response, but it'd be less confusing. Let's use the SupertestWithRoleScopeType in this case.

we could type the response manually as:

let response: APIReturnType<'POST /api/apm/agent_keys 2023-10-31'>

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: iblancof#1

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 be using the new apmApiClient.publicApi introduced in iblancof#1.

d50e6bf

);
expect(error.res.status).to.be(403);
expect(error.res.body.message).contain('is missing the following requested privilege');
expect(error.res.body.attributes).to.eql({
_inspect: [],
data: {
missingPrivileges: allApplicationPrivileges,
missingClusterPrivileges: clusterPrivileges,
},
});
});

it('should return an error when invalidating an agent key', async () => {
const error = await expectToReject<ApmApiError>(() => invalidateAgentKey(agentKeyName));
expect(error.res.status).to.be(500);
});

it('should return an error when getting a list of agent keys', async () => {
const error = await expectToReject<ApmApiError>(() => getAgentKeys());
expect(error.res.status).to.be(500);
});
});
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* 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 type { DeploymentAgnosticFtrProviderContext } from '../../../../../ftr_provider_context';
import type { ApmApiError } from '../../../../../services/apm_api';

export default function apiTest({ getService }: DeploymentAgnosticFtrProviderContext) {
const apmApiClient = getService('apmApi');

type SupertestAsUser = typeof apmApiClient.readUser | typeof apmApiClient.writeUser;

function getJobs(user: SupertestAsUser) {
return user({ endpoint: `GET /internal/apm/settings/anomaly-detection/jobs` });
}

function createJobs(user: SupertestAsUser, environments: string[]) {
return user({
endpoint: 'POST /internal/apm/settings/anomaly-detection/jobs',
params: {
body: { environments },
},
});
}

async function expectForbidden(user: SupertestAsUser) {
try {
await getJobs(user);
} catch (e) {
const err = e as ApmApiError;
expect(err.res.status).to.be(403);
}

try {
await createJobs(user, ['production', 'staging']);
} catch (e) {
const err = e as ApmApiError;
expect(err.res.status).to.be(403);
}
}

describe('ML jobs return a 403 for', () => {
describe('basic', function () {
this.tags('skipFIPS');
iblancof marked this conversation as resolved.
Show resolved Hide resolved

it('read user', async () => {
await expectForbidden(apmApiClient.readUser);
});

it('write user', async () => {
await expectForbidden(apmApiClient.writeUser);
});
});
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
* 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 type { DeploymentAgnosticFtrProviderContext } from '../../../../../ftr_provider_context';
import type { ApmApiError } from '../../../../../services/apm_api';

export default function apiTest({ getService }: DeploymentAgnosticFtrProviderContext) {
const apmApiClient = getService('apmApi');

function getJobs() {
return apmApiClient.readUser({ endpoint: `GET /internal/apm/settings/anomaly-detection/jobs` });
}

function createJobs(environments: string[]) {
return apmApiClient.readUser({
endpoint: `POST /internal/apm/settings/anomaly-detection/jobs`,
params: {
body: { environments },
},
});
}

describe('ML jobs', () => {
describe(`when readUser has read access to ML`, () => {
describe('when calling the endpoint for listing jobs', () => {
it('returns a list of jobs', async () => {
const { body } = await getJobs();

expect(body.jobs).not.to.be(undefined);
expect(body.hasLegacyJobs).to.be(false);
});
});

describe('when calling create endpoint', () => {
it('returns an error because the user does not have access', async () => {
try {
await createJobs(['production', 'staging']);
expect(true).to.be(false);
} catch (e) {
const err = e as ApmApiError;
expect(err.res.status).to.be(403);
}
});
});
});
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/*
* 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 {
APM_INDEX_SETTINGS_SAVED_OBJECT_ID,
APM_INDEX_SETTINGS_SAVED_OBJECT_TYPE,
} from '@kbn/apm-data-access-plugin/server/saved_objects/apm_indices';
import expect from '@kbn/expect';
import type { DeploymentAgnosticFtrProviderContext } from '../../../../../ftr_provider_context';

export default function apmIndicesTests({ getService }: DeploymentAgnosticFtrProviderContext) {
const kibanaServer = getService('kibanaServer');
const apmApiClient = getService('apmApi');

async function deleteSavedObject() {
try {
return await kibanaServer.savedObjects.delete({
type: APM_INDEX_SETTINGS_SAVED_OBJECT_TYPE,
id: APM_INDEX_SETTINGS_SAVED_OBJECT_ID,
});
} catch (e) {
if (e.response.status !== 404) {
throw e;
}
}
}

describe('APM Indices', () => {
beforeEach(async () => {
await deleteSavedObject();
});
afterEach(async () => {
await deleteSavedObject();
});
iblancof marked this conversation as resolved.
Show resolved Hide resolved

it('returns APM Indices', async () => {
const response = await apmApiClient.readUser({
endpoint: 'GET /internal/apm/settings/apm-indices',
});
expect(response.status).to.be(200);
expect(response.body).to.eql({
transaction: 'traces-apm*,apm-*,traces-*.otel-*',
span: 'traces-apm*,apm-*,traces-*.otel-*',
error: 'logs-apm*,apm-*,logs-*.otel-*',
metric: 'metrics-apm*,apm-*,metrics-*.otel-*',
onboarding: 'apm-*',
sourcemap: 'apm-*',
});
});

it('updates apm indices', async () => {
const INDEX_VALUE = 'foo-*';

const writeResponse = await apmApiClient.writeUser({
endpoint: 'POST /internal/apm/settings/apm-indices/save',
params: {
body: { transaction: INDEX_VALUE },
},
});
expect(writeResponse.status).to.be(200);

const readResponse = await apmApiClient.readUser({
endpoint: 'GET /internal/apm/settings/apm-indices',
});

expect(readResponse.status).to.be(200);
expect(readResponse.body.transaction).to.eql(INDEX_VALUE);
});
});
}
Loading