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

[Security Solution][Endpoint] changes to Endpoint metadata API in support of space awareness #193490

Conversation

paul-tavares
Copy link
Contributor

@paul-tavares paul-tavares commented Sep 19, 2024

Summary

Fleet Plugin

  • Added some mocks and updates others

Security Solution

The following refactoring changes were done in preparation for forthcoming work for supporting kibana spaces:

  • A new saved objects client factory service was introduced for use in EndpointAppContextServices
  • Deleted older saved objects client utilities
  • Simplified the list of options passed to EndpointAppContextService#start() method
  • Updated EndpiontFleetServicesFactory with:
    • simpler list of constructor arguments
    • changed members of object return by asInternalUser()
  • Refactored the EndpintMetadataService to:
    • take in simplified constructor arguments
    • Simplified most methods of the class with removal all ES Client, SO Client or Fleet services arguments from class method calling signatures. These are not provided to the service class upon initialization and can be accessed internally by the methods
  • Updates to Mocks and tests to reflect the above changes

Checklist

@paul-tavares paul-tavares added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.16.0 labels Sep 19, 2024
@paul-tavares paul-tavares self-assigned this Sep 19, 2024
@paul-tavares
Copy link
Contributor Author

/ci

@paul-tavares
Copy link
Contributor Author

/ci

@paul-tavares
Copy link
Contributor Author

/ci

Copy link
Contributor

@opauloh opauloh left a comment

Choose a reason for hiding this comment

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

Cloud Security Posture changes LGTM 🚀

Copy link
Contributor

@maximpn maximpn left a comment

Choose a reason for hiding this comment

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

D&R changes LGTM

Copy link
Contributor

@tomsonpl tomsonpl left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I liked how you simplified usage of EndpointMetadataService🥇

Left a few suggestions, but as usual leaving the decision up to you :)

Thanks!

registerListsServerExtension?: ListsServerExtensionRegistrar;
licenseService: LicenseService;
exceptionListsClient: ExceptionListClient | undefined;
cases: CasesServerStart | undefined;
featureUsageService: FeatureUsageService;
experimentalFeatures: ExperimentalFeatures;
messageSigningService: MessageSigningServiceInterface | undefined;
/** An internal ES client */
esClient: ElasticsearchClient;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the name specify that it's an internal ES client?

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 rather keep it as is, since its consistent with how we normally name the ES client variables through out our code base. Is that cool with you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure 👍 thanks!

private getFleetAuthzService(): FleetStartContract['authz'] {
if (!this.startDependencies?.fleetAuthzService) {
if (!this.startDependencies?.fleetStartServices) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we also check for .authz , or not necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its not necessary

fleetServices = endpointContextStartContract.endpointFleetServicesFactory.asInternalUser();
fleetServices = mockedFleetServices.service.asInternalUser();
logger = loggingSystemMock.createLogger();
productFeatureService = createProductFeaturesServiceMock(
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think we could refactor createProductFeaturesServiceMock to accept an object, instead of multiple arguments where some of them are passed as undefined ?

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'm ok either way... but perhaps if you feel we should, lets address it in a separate PR

productFeaturesService,
} = this.startDependencies;
const endpointMetadataService = this.getEndpointMetadataService();
const soClient = this.savedObjects.createInternalScopedSoClient(undefined, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about changing createInternalScopedSoClient to accept an object, instead of undefined passing undefined to get a default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I can do that. Will push through a change soon

@@ -41,35 +55,13 @@ export class EndpointFleetServicesFactory implements EndpointFleetServicesFactor
return {
agent: agentService.asInternalUser,
agentPolicy,

Copy link
Contributor

Choose a reason for hiding this comment

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

was this on purpose? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it was... more for readability in separating related services

Copy link
Contributor

@szwarckonrad szwarckonrad left a comment

Choose a reason for hiding this comment

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

The code looks good! I didn’t spot any leftover logic with these changes. Thanks for addressing this!

@paul-tavares paul-tavares added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Sep 26, 2024
@paul-tavares paul-tavares enabled auto-merge (squash) September 26, 2024 15:09
paul-tavares and others added 2 commits September 26, 2024 16:06
…int-list-details-space-aware

# Conflicts:
#	x-pack/plugins/security_solution/server/endpoint/endpoint_app_context_services.ts
#	x-pack/plugins/security_solution/server/endpoint/mocks/mocks.ts
#	x-pack/plugins/security_solution/server/plugin.ts
Copy link
Contributor

@criamico criamico left a comment

Choose a reason for hiding this comment

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

Fleet changes LGTM

@kibana-ci
Copy link
Collaborator

kibana-ci commented Sep 27, 2024

💛 Build succeeded, but was flaky

  • Buildkite Build
  • Commit: da2b662
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-193490-da2b662c82b3

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #6 / Create renders correctly with optional fields

Metrics [docs]

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
fleet 74 73 -1
Unknown metric groups

ESLint disabled in files

id before after diff
securitySolution 84 85 +1

Total ESLint disabled count

id before after diff
securitySolution 626 627 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @paul-tavares

@paul-tavares paul-tavares merged commit 0b1e9f4 into elastic:main Sep 27, 2024
65 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11072646710

@paul-tavares paul-tavares deleted the task/olm-8538-endpoint-list-details-space-aware branch September 27, 2024 14:39
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.x Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 193490

Questions ?

Please refer to the Backport tool documentation

@paul-tavares
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.x

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

Questions ?

Please refer to the Backport tool documentation

paul-tavares added a commit to paul-tavares/kibana that referenced this pull request Sep 27, 2024
…port of space awareness (elastic#193490)

## Summary

### Fleet Plugin

- Added some mocks and updates others

### Security Solution

The following refactoring changes were done in preparation for
forthcoming work for supporting kibana spaces:

- A new saved objects client factory service was introduced for use in
`EndpointAppContextServices`
- Deleted older saved objects client utilities
- Simplified the list of options passed to
`EndpointAppContextService#start()` method
- Updated `EndpiontFleetServicesFactory` with:
    - simpler list of constructor arguments
    - changed members of object return by `asInternalUser()`
- Refactored the `EndpintMetadataService` to:
    - take in simplified constructor arguments
- Simplified most methods of the class with removal all ES Client, SO
Client or Fleet services arguments from class method calling signatures.
These are not provided to the service class upon initialization and can
be accessed internally by the methods
- Updates to Mocks and tests to reflect the above changes

(cherry picked from commit 0b1e9f4)

# Conflicts:
#	x-pack/plugins/security_solution/tsconfig.json
paul-tavares added a commit that referenced this pull request Sep 27, 2024
…in support of space awareness (#193490) (#194309)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Security Solution][Endpoint] changes to Endpoint metadata API in
support of space awareness
(#193490)](#193490)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Paul
Tavares","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-27T14:38:17Z","message":"[Security
Solution][Endpoint] changes to Endpoint metadata API in support of space
awareness (#193490)\n\n## Summary\r\n\r\n### Fleet Plugin\r\n\r\n- Added
some mocks and updates others\r\n\r\n\r\n### Security
Solution\r\n\r\nThe following refactoring changes were done in
preparation for\r\nforthcoming work for supporting kibana
spaces:\r\n\r\n- A new saved objects client factory service was
introduced for use in\r\n`EndpointAppContextServices`\r\n- Deleted older
saved objects client utilities\r\n- Simplified the list of options
passed to\r\n`EndpointAppContextService#start()` method\r\n- Updated
`EndpiontFleetServicesFactory` with:\r\n - simpler list of constructor
arguments\r\n - changed members of object return by
`asInternalUser()`\r\n- Refactored the `EndpintMetadataService` to:\r\n
- take in simplified constructor arguments\r\n- Simplified most methods
of the class with removal all ES Client, SO\r\nClient or Fleet services
arguments from class method calling signatures.\r\nThese are not
provided to the service class upon initialization and can\r\nbe accessed
internally by the methods\r\n- Updates to Mocks and tests to reflect the
above
changes","sha":"0b1e9f475440d60260203ce2a85c8b57c4363130","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Fleet","v9.0.0","Team:Defend
Workflows","backport:prev-minor","ci:project-deploy-observability","Team:obs-ux-infra_services","apm:review","v8.16.0"],"number":193490,"url":"https://github.com/elastic/kibana/pull/193490","mergeCommit":{"message":"[Security
Solution][Endpoint] changes to Endpoint metadata API in support of space
awareness (#193490)\n\n## Summary\r\n\r\n### Fleet Plugin\r\n\r\n- Added
some mocks and updates others\r\n\r\n\r\n### Security
Solution\r\n\r\nThe following refactoring changes were done in
preparation for\r\nforthcoming work for supporting kibana
spaces:\r\n\r\n- A new saved objects client factory service was
introduced for use in\r\n`EndpointAppContextServices`\r\n- Deleted older
saved objects client utilities\r\n- Simplified the list of options
passed to\r\n`EndpointAppContextService#start()` method\r\n- Updated
`EndpiontFleetServicesFactory` with:\r\n - simpler list of constructor
arguments\r\n - changed members of object return by
`asInternalUser()`\r\n- Refactored the `EndpintMetadataService` to:\r\n
- take in simplified constructor arguments\r\n- Simplified most methods
of the class with removal all ES Client, SO\r\nClient or Fleet services
arguments from class method calling signatures.\r\nThese are not
provided to the service class upon initialization and can\r\nbe accessed
internally by the methods\r\n- Updates to Mocks and tests to reflect the
above
changes","sha":"0b1e9f475440d60260203ce2a85c8b57c4363130"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193490","number":193490,"mergeCommit":{"message":"[Security
Solution][Endpoint] changes to Endpoint metadata API in support of space
awareness (#193490)\n\n## Summary\r\n\r\n### Fleet Plugin\r\n\r\n- Added
some mocks and updates others\r\n\r\n\r\n### Security
Solution\r\n\r\nThe following refactoring changes were done in
preparation for\r\nforthcoming work for supporting kibana
spaces:\r\n\r\n- A new saved objects client factory service was
introduced for use in\r\n`EndpointAppContextServices`\r\n- Deleted older
saved objects client utilities\r\n- Simplified the list of options
passed to\r\n`EndpointAppContextService#start()` method\r\n- Updated
`EndpiontFleetServicesFactory` with:\r\n - simpler list of constructor
arguments\r\n - changed members of object return by
`asInternalUser()`\r\n- Refactored the `EndpintMetadataService` to:\r\n
- take in simplified constructor arguments\r\n- Simplified most methods
of the class with removal all ES Client, SO\r\nClient or Fleet services
arguments from class method calling signatures.\r\nThese are not
provided to the service class upon initialization and can\r\nbe accessed
internally by the methods\r\n- Updates to Mocks and tests to reflect the
above
changes","sha":"0b1e9f475440d60260203ce2a85c8b57c4363130"}},{"branch":"8.x","label":"v8.16.0","labelRegex":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:review backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution Team:Fleet Team label for Observability Data Collection Fleet team Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.