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.

Sign up for GitHub

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

Jump to bottom

[Security Solution] improve endpoint metadata tests #125883

Merged
merged 3 commits into from
May 19, 2022
Merged

[Security Solution] improve endpoint metadata tests #125883

merged 3 commits into from
May 19, 2022

Conversation

joeypoon
Copy link
Member

@joeypoon joeypoon commented Feb 16, 2022 •
edited
Loading

Summary

Update EndpointTestResouces.loadEndpointData to additionally handle metadata_united index. This should improve test accuracy as endpoint list is based on the united index now.

flaky test runner: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/170 https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/173
https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/618

For maintainers

@joeypoon joeypoon added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 v8.2.0 labels Feb 16, 2022
@joeypoon joeypoon force-pushed the chore/better-integ-tests branch from 2588402 to 0212418 Compare February 17, 2022 01:01
joeypoon
joeypoon commented Feb 17, 2022
View reviewed changes
x-pack/test/security_solution_endpoint_api_int/apis/endpoint_authz.ts
before(async () => {
// create role/user
await createUserAndRole(getService, ROLES.t1_analyst);
loadedData = await endpointTestResources.loadEndpointData();
Copy link
Member Author

@joeypoon joeypoon Feb 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

since we're only testing for 403s here, the data load seems unnecessary.

@joeypoon joeypoon marked this pull request as ready for review February 17, 2022 15:22
@joeypoon joeypoon requested review from a team as code owners February 17, 2022 15:22
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt)

@joeypoon joeypoon force-pushed the chore/better-integ-tests branch 3 times, most recently from 6d4217f to 12f2b74 Compare February 17, 2022 23:12
paul-tavares
paul-tavares reviewed Feb 22, 2022
View reviewed changes
x-pack/test/security_solution_endpoint/services/endpoint.ts
await this.waitForEndpoints(indexedData.hosts.map((host) => host.agent.id));
const metadataIds = Array.from(new Set(indexedData.hosts.map((host) => host.agent.id)));
await this.waitForEndpoints(metadataIds, waitTimeout);
await this.startTransform(METADATA_UNITED_TRANSFORM);
Copy link
Contributor

Choose a reason for hiding this comment

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

If I follow what is going on here correctly, you seem to be missing a .startTransform() at the end of this entire function, since you immediately stopped the transform when this function executes. If waitUntilTransformed is false, then you never restart the transform.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah yes, good catch. will fix.

x-pack/plugins/security_solution/common/endpoint/data_loaders/index_fleet_agent.ts
@@ -23,7 +23,7 @@ import { wrapErrorAndRejectPromise } from './utils';
const defaultFleetAgentGenerator = new FleetAgentGenerator();

export interface IndexedFleetAgentResponse {
agents: Agent[];
agents: Array<Agent & FleetServerAgent>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you have to add FleetServerAgent here?

Copy link
Member Author

@joeypoon joeypoon Feb 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Agent type doesn't contain the agent field which FleetServerAgent does. United transform uses the agent.id field to join on so it's required. Agent type is actually what the fleet agents endpoint is currently typed to return but it actually does also contain the agent field. My guess is that when we asked fleet team to add agent.id to the index for the united transform recently, this api's return type wasn't updated since we were the only ones using the new field and it was via transform.

Ideally, I think we also update the api's return type but that is fleet code so I didn't want to scope creep this PR. I think it'd be best for that to be its own PR since it's potentially going to end up needing other changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh. Ok. thanks for that explanation. I would suggest asking the Fleet team about this and then creating an issue to track the update if they are good with it.

@joeypoon joeypoon force-pushed the chore/better-integ-tests branch 2 times, most recently from 63b9eab to c3b2f32 Compare February 22, 2022 15:28
@joeypoon joeypoon force-pushed the chore/better-integ-tests branch 2 times, most recently from f250a15 to 60969bf Compare May 12, 2022 14:43
ashokaditya
ashokaditya approved these changes May 12, 2022
View reviewed changes
Copy link
Member

@ashokaditya ashokaditya left a comment

Choose a reason for hiding this comment

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

Ran the tests locally. I left some nits and suggestions but otherwise good to 🚢

x-pack/test/security_solution_endpoint/services/endpoint.ts Outdated Show resolved Hide resolved
x-pack/test/security_solution_endpoint/services/endpoint.ts Outdated Show resolved Hide resolved
@joeypoon
[Security Solution] improve endpoint metadata tests
f669de0 
improve accuracy of endpoint metadata functional tests by waiting on
metadata_united index instead of metadata_current index
@joeypoon joeypoon force-pushed the chore/better-integ-tests branch from 60969bf to f669de0 Compare May 12, 2022 16:05
@joeypoon
Copy link
Member Author

@elasticmachine merge upstream

paul-tavares
paul-tavares approved these changes May 19, 2022
View reviewed changes
Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

Approving, but did leave a few questions (optional)

x-pack/plugins/security_solution/common/endpoint/data_loaders/index_fleet_agent.ts
@@ -23,7 +23,7 @@ import { wrapErrorAndRejectPromise } from './utils';
const defaultFleetAgentGenerator = new FleetAgentGenerator();

export interface IndexedFleetAgentResponse {
agents: Agent[];
agents: Array<Agent & FleetServerAgent>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh. Ok. thanks for that explanation. I would suggest asking the Fleet team about this and then creating an issue to track the update if they are good with it.

x-pack/test/security_solution_endpoint/services/endpoint.ts
transform_id: `${transformId}*`,
});
return Promise.all(
transformsResponse.transforms.map((transform) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the number of transforms here is small and thus we don't need to throttle the calls to *.startTranform() - right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. We control these transforms and it's only 2 so no worries there.

@joeypoon
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 125883 locally

17 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 125883 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 125883 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 125883 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 125883 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 125883 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 125883 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 125883 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 125883 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 125883 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 125883 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 125883 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 125883 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 125883 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 125883 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 125883 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 125883 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 125883 locally

@joeypoon joeypoon mentioned this pull request Aug 18, 2022
Closed
joeypoon added a commit to joeypoon/kibana that referenced this pull request Aug 18, 2022
@joeypoon
[Security Solution] improve endpoint metadata tests (elastic#125883)
34511ca 
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 1ea3fc6)

# Conflicts:
#	x-pack/test/security_solution_endpoint/services/endpoint.ts
#	x-pack/test/security_solution_endpoint_api_int/apis/endpoint_authz.ts
#	x-pack/test/security_solution_endpoint_api_int/apis/metadata.ts
@joeypoon joeypoon mentioned this pull request Aug 18, 2022
Closed
@joeypoon
Copy link
Member Author

💚 All backports created successfully

Status Branch Result
✅ 8.2
✅ 8.1

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

joeypoon added a commit to joeypoon/kibana that referenced this pull request Aug 18, 2022
@joeypoon
[Security Solution] improve endpoint metadata tests (elastic#125883)
9345629 
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 1ea3fc6)

# Conflicts:
#	x-pack/test/security_solution_endpoint/services/endpoint.ts
#	x-pack/test/security_solution_endpoint_api_int/apis/endpoint_authz.ts
#	x-pack/test/security_solution_endpoint_api_int/apis/metadata.ts
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

2 similar comments
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@joeypoon joeypoon added backport:skip This commit does not require backporting and removed auto-backport Deprecated - use backport:version if exact versions are needed labels Aug 23, 2022
@kibanamachine kibanamachine removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paul-tavares paul-tavares paul-tavares approved these changes

@ashokaditya ashokaditya ashokaditya approved these changes

@parkiino parkiino Awaiting requested review from parkiino parkiino was automatically assigned from elastic/security-onboarding-and-lifecycle-mgt

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.1.0 v8.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@joeypoon @elasticmachine @kibana-ci @kibanamachine @ashokaditya @paul-tavares