Observability AI Assistant Tests Deployment Agnostic #205194

arturoliduena · 2024-12-27T11:29:53Z

Closes #192718

Summary

This PR add a deployment-agnostic testing environment for Observability AI Assistant tests by unifying the duplicated tests for stateful and serverless environments. It create the ObservabilityAIAssistantApiClient to work seamlessly in both environments, enabling a single test to run across stateful, CI, and MKI.

Initial efforts focus on deduplicating the conversations.spec.ts and connectors.spec.ts files, as these already run in all environments.

Move / dedup the tests that exist in stateful and serverless. They run in serverless CI but not MKI and add the skipMki tag.
chat.spec.ts
complete.spec.ts
elasticsearch.spec.ts
public_complete.spec.ts
alerts.spec.ts

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

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
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
Flaky Test Runner was used on any tests changed
The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

elasticmachine · 2024-12-27T11:29:55Z

Pinging @elastic/obs-ai-assistant (Team:Obs AI Assistant)

github-actions · 2024-12-27T12:05:44Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

sorenlouv · 2025-01-02T14:43:14Z

...ntegration/deployment_agnostic/apis/observability/ai_assistant/connectors/connectors.spec.ts

+
+    it('Returns a 2xx for enterprise license', async () => {
+      const { status } = await observabilityAIAssistantAPIClient.editor({
+        endpoint: `GET ${CONNECTOR_API_URL}`,


nit: I suggest not using variables here. It's harder to read and search for. Also, I'm not sure the typescript intellisense for parameters and typed responses works when using variables

Suggested change

endpoint: `GET ${CONNECTOR_API_URL}`,

endpoint: `GET /internal/observability_ai_assistant/connectors`,

If you really want to make it DRY I suggest adding a helper and re-use that instead:

const getConnectors = () => observabilityAIAssistantAPIClient.editor({ endpoint: `GET /internal/observability_ai_assistant/connectors`, });

Agree. Now that I’m moving many of these tests, I won’t use a variable and will ensure they follow a consistent pattern since we currently have a mix.

sorenlouv · 2025-01-02T14:52:39Z

...ntegration/deployment_agnostic/apis/observability/ai_assistant/connectors/connectors.spec.ts

+  }
+}
+
+async function createProxyActionConnector({


Should we have a shared helper for creating the proxy action connector? It's already defined at least 3 times:

kibana/x-pack/test/observability_ai_assistant_api_integration/common/action_connectors.ts

Line 31 in c4cf9fe

export async function createProxyActionConnector({

kibana/x-pack/test_serverless/api_integration/test_suites/observability/ai_assistant/common/action_connectors.ts

Line 40 in c4cf9fe

export async function createProxyActionConnector({

kibana/x-pack/test/observability_ai_assistant_functional/common/connectors.ts

Line 10 in c4cf9fe

export async function createConnector(proxy: LlmProxy, supertest: SuperTestAgent) {

I will keep only one, I will get rid of the duplicate functions.

sorenlouv · 2025-01-02T14:56:14Z

...tion/deployment_agnostic/apis/observability/ai_assistant/conversations/conversations.spec.ts

-        await observabilityAIAssistantAPIClient
-          .slsEditor({
+        try {
+          await observabilityAIAssistantAPIClient.editor({


Feels good getting rid of the serverless-specific client. Is the plan to get rid of x-pack/test_serverless/api_integration/test_suites/observability/ai_assistant/common/observability_ai_assistant_api_client.ts entirely?

We can remove it entirely if we get all the tests running in deployment agnostic, unless there is some case where we want a serverless only test because of some serverless only feature. Probably wouldn't hurt to leave it.

...tion/deployment_agnostic/apis/observability/ai_assistant/conversations/conversations.spec.ts

neptunian · 2025-01-02T19:00:24Z

...ck/test/observability_ai_assistant_api_integration/tests/conversations/conversations.spec.ts

@@ -46,212 +46,6 @@ export default function ApiTest({ getService }: FtrProviderContext) {
  });

  describe('Conversations', () => {
-    describe('without conversations', () => {


This whole file should be safe to move over to deployment agnostic and not exist in stateful tests. I'm assuming you decided not to move it was because of the security roles and access privileges test suite which doesn't exist in serverless, but it soon will: #205210

Exactly. I only moved the common tests between stateful and serverless. I’ll move the security roles and access privaleges to deployment-agnostic and remove it from stateful and serverless.

neptunian · 2025-01-02T19:01:29Z

x-pack/test/observability_ai_assistant_api_integration/tests/connectors/connectors.spec.ts

@@ -8,13 +8,11 @@
 import expect from '@kbn/expect';
 import type { Agent as SuperTestAgent } from 'supertest';


Same as https://github.com/elastic/kibana/pull/205194/files#r1901174829

sorenlouv · 2025-01-03T10:23:14Z

.github/CODEOWNERS

@@ -1264,7 +1264,7 @@ packages/kbn-monaco/src/esql @elastic/kibana-esql
 /x-pack/test/observability_ai_assistant_functional @elastic/obs-ai-assistant
 /x-pack/test_serverless/**/test_suites/observability/ai_assistant @elastic/obs-ai-assistant
 /x-pack/test/functional/es_archives/observability/ai_assistant @elastic/obs-ai-assistant
-
+/x-pack/test/api_integration/deployment_agnostic/apis/observability/ai_assistant @elastic/obs-ai-assistant


Doesn't look related to your change but Github complains about errors in this file

It's valid on main which is odd

Thanks for pointing that out, @sorenlouv. I checked, and the file is now valid in this branch as well. It might have been a temporary issue or something resolved during recent changes(I did a rebase recently).

sorenlouv

Just one comment about CODEOWNERS file

…ic test

…rverless

…o deployment-agnostic

…eployment-agnostic

…tests

…ostic tests and remove unused supertest import from chat.spec.ts

…test

… test

…c test

sorenlouv · 2025-01-06T12:20:09Z

...tion/deployment_agnostic/apis/observability/ai_assistant/conversations/conversations.spec.ts

+            throw new ForbiddenApiError(
+              'Expected unauthorizedUser() to throw a 403 Forbidden error'
+            );
+          } catch (e) {
+            expect(e.status).to.be(403);
+          }


This doesn't look right. By manually throwing ForbiddenApiError you are ensuring that the test will pass even if observabilityAIAssistantAPIClient.unauthorizedUser(..) does not throw (eg if it receives a 200).

Meaning, this test will pass if an unauthorized user is able to successfully call the given endpoint. Is that intentional?

@sorenlouv TBH, in the first iteration, I just moved the serverless/stateful test to the deployment-agnostic test and made them work without rewriting the test implementation. Later, I also noticed the same issue, that manually throwing ForbiddenApiError undermines the purpose of validating that observabilityAIAssistantAPIClient.unauthorizedUser(..) behaves as expected.

I’ve already addressed this in the latest commit of the file in the PR. Let me know if you have any further feedback.

I think it was updated this way based on this comment - #202817 (comment)
cc: @arturoliduena @sorenlouv

I think it was updated this way based on this comment - #202817 (comment)

@viduni94 What I suggested was to throw a generic error (that did not have a status=403) and would thus not pass.

Ah got it. Throwing a generic error would always fail the test because we are expecting the status to be 403.
We can remove the try catch block and directly check for expect(status).to.be(403); in that case.

We can remove the try catch block and directly expect(status).to.be(403); in that case.

I can't recall if observabilityAIAssistantAPIClient throws on 403. If it doesn't, we can remove the try/catch as you suggest

@sorenlouv and @viduni94.
I’ve already addressed by removing the manual throw and the try catch block. Now, it directly calls observabilityAIAssistantAPIClient.unauthorizedUser(...) and verifies the status like this:

const { status } = await observabilityAIAssistantAPIClient.unauthorizedUser({ endpoint: 'DELETE /internal/observability_ai_assistant/conversation/{conversationId}', params: { path: { conversationId: createResponse.body.conversation.id, }, }, }); expect(status).to.be(403);

This ensures the test properly validates the behavior without relying on manual errors, I changed it in all the test I already migrated to deployment agnostic.

Much better! Thanks!

sorenlouv · 2025-01-06T12:31:31Z

x-pack/test/api_integration/deployment_agnostic/services/observability_ai_assistant_api.ts

@@ -201,6 +201,7 @@ export function ObservabilityAIAssistantApiProvider(context: DeploymentAgnosticF
    admin: observabilityAIAssistantApiClient.makeInternalApiRequest('admin'),
    viewer: observabilityAIAssistantApiClient.makeInternalApiRequest('viewer'),
    editor: observabilityAIAssistantApiClient.makeInternalApiRequest('editor'),
+    unauthorizedUser: observabilityAIAssistantApiClient.makeInternalApiRequest('viewer'),


Can't we simply use the existing viewer? Also, what do you expect to happen if unauthorizedUser is used to call an endpoint that viewer is allowed to access?

Hey @arturoliduena
You might not be able to use the viewer role for unauthorizedUser in this manner for DA tests. It was done for serverless this way, because we force the user to be unauthorized by not passing the cookie header.

https://github.com/elastic/kibana/pull/205210/files#diff-febc7c89d3de714505db725fb9c264974eea6a5b29542b8782ebb0bab98a92d7R219

However, since the cookie is always included in makeInternalApiRequest, this user will be a viewer with authorization.

Hey @sorenlouv and @viduni94,
We can simulate an unauthorized user by omitting credentials entirely with a function like makeInternalApiRequestWithoutCredentials:

function makeInternalApiRequest(role: string) { return async <TEndpoint extends InternalEndpoint<APIEndpoint>>( options: Options<TEndpoint> ): Promise<SupertestReturnType<TEndpoint>> => { const headers: Record<string, string> = { ...samlAuth.getInternalRequestHeader(), ...(await samlAuth.getM2MApiCookieCredentialsWithRoleScope(role)), }; return makeApiRequest({ options, headers, }); }; } function makeInternalApiRequestWithoutCredentials() { return async <TEndpoint extends InternalEndpoint<APIEndpoint>>( options: Options<TEndpoint> ): Promise<SupertestReturnType<TEndpoint>> => { const headers: Record<string, string> = { ...samlAuth.getInternalRequestHeader(), }; return makeApiRequest({ options, headers, }); }; }

However, the problem with this approach is that omitting credentials will result in a 401 Unauthorized response instead of a 403 Forbidden. This doesn’t align with the intention of the test, which is to verify whether a user with insufficient permissions can access specific endpoints (DELETE, POST, PUT).

To properly simulate this, the user needs to be authenticated (avoiding 401 Unauthorized) but lack the necessary permissions, resulting in a 403 Forbidden. This is why I think it makes sense to use the viewer role here.

I agree with Søren that adding an additional unauthorizedUser role like this:

unauthorizedUser: observabilityAIAssistantApiClient.makeInternalApiRequest('viewer')

is unnecessary. Instead, we can simply use the existing viewer role:

viewer: observabilityAIAssistantApiClient.makeInternalApiRequest('viewer')

This keeps the implementation clean and ensures that we’re testing for insufficient permissions correctly. Let me know if you agree

If I read that correctly, you'll authenticate as the viewer user. That sounds good to me.

…dUser

sorenlouv · 2025-01-07T09:46:50Z

x-pack/test/api_integration/deployment_agnostic/services/observability_ai_assistant_api.ts

+
+type PublicEndpoint<T extends APIEndpoint> = T extends `${string} /api/${string}` ? T : never;
+
+function createObservabilityAIAssistantApiClient({


fyi: @dgieselaar is extracting the API client in https://github.com/elastic/kibana/pull/204309/files#diff-cb59199a241f60514e9ba52c57b587929f6be6bcedb24c5a138d5ba72184fe85.

I don't think this is a blocker but eventually we should use that implementation so we don't have multiple. Afaict we now have three API clients:

https://github.com/elastic/kibana/blob/main/x-pack/test/observability_ai_assistant_api_integration/common/observability_ai_assistant_api_client.ts

https://github.com/elastic/kibana/blob/main/x-pack/test_serverless/api_integration/test_suites/observability/ai_assistant/common/observability_ai_assistant_api_client.ts

elasticmachine · 2025-01-07T10:38:57Z

💚 Build Succeeded

Buildkite Build
Commit: b80b0ee
Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-205194-b80b0ee2b0a6

Metrics [docs]

✅ unchanged

History

💛 Build #264203 was flaky d7e28b5
💛 Build #264164 was flaky dd65895
💚 Build #264133 succeeded fce7822
💔 Build #264124 failed 1dc1987
💚 Build #263170 succeeded c1c4339

arturoliduena added release_note:skip Skip the PR/issue when compiling release notes Team:Obs AI Assistant Observability AI Assistant v8.18.0 labels Dec 27, 2024

arturoliduena requested a review from a team as a code owner December 27, 2024 11:29

arturoliduena requested a review from a team as a code owner December 27, 2024 12:05

botelastic bot added the ci:project-deploy-observability Create an Observability project label Dec 27, 2024

sorenlouv reviewed Jan 2, 2025

View reviewed changes

...tion/deployment_agnostic/apis/observability/ai_assistant/conversations/conversations.spec.ts Outdated Show resolved Hide resolved

neptunian reviewed Jan 2, 2025

View reviewed changes

sorenlouv reviewed Jan 3, 2025

View reviewed changes

sorenlouv approved these changes Jan 3, 2025

View reviewed changes

This was referenced Jan 3, 2025

[Obs AI Assistant] Add alerts.spec.ts to serverless tests #205524

Closed

[Obs AI Assistant] move kb migration test suite to serverless #205537

Open

arturoliduena added 5 commits January 3, 2025 21:15

Add ObservabilityAIAssistantApi Client deployment agnostic

cc983ea

Migrating existing conversations.spec.ts test to to deployment-agnost…

f322689

…ic test

Add CODEOWNERS entry for deployment-agnostic AI Assistant API tests

fe98f91

remove duplicated conversations.spec.ts tests between stateful and se…

6612952

…rverless

Migrating existing connectors.spec.ts test to deployment-agnostic test

1dc1987

arturoliduena force-pushed the obs-ai-assistant-192718-deployment-agnostic-api-integration-tests branch from c1c4339 to 1dc1987 Compare January 3, 2025 20:38

Migrating existing chat.spec.ts test to deployment-agnostic test

fce7822

arturoliduena added the backport:skip This commit does not require backporting label Jan 4, 2025

arturoliduena added 4 commits January 4, 2025 20:50

Add unauthorized user role handling and migrate conversations tests t…

2d47596

…o deployment-agnostic

Add unauthorized user role handling and migrate connectors tests to d…

d0505ee

…eployment-agnostic

Add security role checks for AI assistant chat API and remove legacy …

dd65895

…tests

Add complete.spec.ts to AI assistant API integration deployment diagn…

59e9148

…ostic tests and remove unused supertest import from chat.spec.ts

arturoliduena added 5 commits January 5, 2025 20:59

Migrating existing alerts.spec.ts test to deployment-agnostic test

2db6c01

Add skipMKI tag to alerts function deployment-agnostic test

c82e571

Migrating existing elasticsearch.spec.ts test to deployment-agnostic …

addf2d1

…test

Add skipMKI tag to elasticsearch.spec.ts function deployment-agnostic…

f4b3961

… test

Migrating existing public_complete.spec.ts test to deployment-agnosti…

d7e28b5

…c test

sorenlouv reviewed Jan 6, 2025

View reviewed changes

viduni94 mentioned this pull request Jan 6, 2025

[Obs AI Assistant] Add knowledge base migration test to the serverless test suite #205631

Open

2 tasks

Refactor ai_assistant tests to use viewer role instead of unauthorize…

b80b0ee

…dUser

sorenlouv reviewed Jan 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Observability AI Assistant Tests Deployment Agnostic #205194

Observability AI Assistant Tests Deployment Agnostic #205194

arturoliduena commented Dec 27, 2024 •

edited

Loading

elasticmachine commented Dec 27, 2024

github-actions bot commented Dec 27, 2024

sorenlouv Jan 2, 2025 •

edited

Loading

sorenlouv Jan 2, 2025

arturoliduena Jan 3, 2025

sorenlouv Jan 2, 2025

arturoliduena Jan 3, 2025

sorenlouv Jan 2, 2025

neptunian Jan 2, 2025

neptunian Jan 2, 2025

arturoliduena Jan 3, 2025

neptunian Jan 2, 2025

sorenlouv Jan 3, 2025

sorenlouv Jan 3, 2025

arturoliduena Jan 6, 2025

sorenlouv left a comment

sorenlouv Jan 6, 2025 •

edited

Loading

arturoliduena Jan 6, 2025

viduni94 Jan 6, 2025

sorenlouv Jan 6, 2025 •

edited

Loading

viduni94 Jan 6, 2025 •

edited

Loading

sorenlouv Jan 6, 2025

arturoliduena Jan 7, 2025

sorenlouv Jan 7, 2025

sorenlouv Jan 6, 2025 •

edited

Loading

viduni94 Jan 6, 2025 •

edited

Loading

arturoliduena Jan 7, 2025

sorenlouv Jan 7, 2025 •

edited

Loading

sorenlouv Jan 7, 2025 •

edited

Loading

elasticmachine commented Jan 7, 2025 •

edited

Loading

	endpoint: `GET ${CONNECTOR_API_URL}`,
	endpoint: `GET /internal/observability_ai_assistant/connectors`,

		@@ -8,13 +8,11 @@
		import expect from '@kbn/expect';
		import type { Agent as SuperTestAgent } from 'supertest';


		type PublicEndpoint<T extends APIEndpoint> = T extends `${string} /api/${string}` ? T : never;

		function createObservabilityAIAssistantApiClient({

Observability AI Assistant Tests Deployment Agnostic #205194

Are you sure you want to change the base?

Observability AI Assistant Tests Deployment Agnostic #205194

Conversation

arturoliduena commented Dec 27, 2024 • edited Loading

Summary

Checklist

elasticmachine commented Dec 27, 2024

github-actions bot commented Dec 27, 2024

🤖 GitHub comments

sorenlouv Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv left a comment

Choose a reason for hiding this comment

sorenlouv Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

viduni94 Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

viduni94 Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

sorenlouv Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Jan 7, 2025 • edited Loading

💚 Build Succeeded

Metrics [docs]

History

arturoliduena commented Dec 27, 2024 •

edited

Loading

sorenlouv Jan 2, 2025 •

edited

Loading

sorenlouv Jan 6, 2025 •

edited

Loading

sorenlouv Jan 6, 2025 •

edited

Loading

viduni94 Jan 6, 2025 •

edited

Loading

sorenlouv Jan 6, 2025 •

edited

Loading

viduni94 Jan 6, 2025 •

edited

Loading

sorenlouv Jan 7, 2025 •

edited

Loading

sorenlouv Jan 7, 2025 •

edited

Loading

elasticmachine commented Jan 7, 2025 •

edited

Loading