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

[Cloud Security] Misconfiguration preview & Refactor CSP Plugin to include new package PHASE 2 #190933

Merged

Conversation

animehart
Copy link
Contributor

@animehart animehart commented Aug 20, 2024

Summary

The previous #190105 was way too big and made it hard to review without missing any bugs or potential bugs, Thus we decided we are going to make series of smaller PR to make things more manageable

We will be splitting it into 4 PR
Phase 1: Creating empty packages for csp and csp-common
Phase 2: Move Types from CSP plugin to the Package + Deleting duplicates in the CSP plugin where possible
Phase 3: Move Functions, Utils or Helpers, Hooks to Package
Phase 4: Misconfiguration Preview feature (with Cypress test and other required test)

This is Phase 2 of the Process

@animehart
Copy link
Contributor Author

/ci

@animehart
Copy link
Contributor Author

/ci

@animehart
Copy link
Contributor Author

/ci

@animehart animehart marked this pull request as ready for review August 21, 2024 08:35
@animehart animehart requested a review from a team as a code owner August 21, 2024 08:35
@animehart animehart added the Team:Cloud Security Cloud Security team related label Aug 21, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security)

@animehart animehart added v8.16.0 release_note:skip Skip the PR/issue when compiling release notes labels Aug 21, 2024
@animehart animehart requested a review from a team as a code owner August 23, 2024 22:56
Copy link
Contributor

@maxcold maxcold left a comment

Choose a reason for hiding this comment

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

The main question we should decide on is what to do with our versioned types and schemas.
Also pls check all imports and add import type when importing types

import type { EcsDataStream, EcsEvent } from '@elastic/ecs';
import { CspBenchmarkRuleMetadata } from '../types/latest';
import { CspBenchmarkRuleMetadata } from './rules';

export interface CspFinding {
Copy link
Contributor

Choose a reason for hiding this comment

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

This file doesn't contain schema anymore, only CspFinding interface , maybe make sense to move the code to types.ts?

@@ -6,44 +6,17 @@
*/

import { schema, TypeOf } from '@kbn/config-schema';
import { CSPM_POLICY_TEMPLATE, KSPM_POLICY_TEMPLATE } from '../../constants';

import { cspBenchmarkRuleMetadataSchema } from '@kbn/cloud-security-posture-common/schema';
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think we need to move the whole common/types with all the versions into our common package. Otherwise the versioning won't work for us. Imagine you need to make a breaking change to the schema. If you do it in the cspBenchmarkRuleMetadataSchema in the package you would break the versioning as it is imported here in the v3. To do it properly you need to introduce the new version of the schema, the same way as we migrated cspBenchmarkRuleMetadataSchema between v2 and v3.
I'm not sure if we can reduce the number of changes caused by moving the whole versioned types and schemas, but without it we are kind of breaking the versioning

Copy link
Contributor

Choose a reason for hiding this comment

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

again to not make the change too large, maybe it's ok to proceed with this PR but then create a new PR right away moving the versioned types and schemas to our shared package so that we don't risk

@@ -6,15 +6,11 @@
*/

import { schema, TypeOf } from '@kbn/config-schema';
import { CspBenchmarkRulesStates } from '@kbn/cloud-security-posture-common';
import { ruleStateAttributes, rulesStates } from '@kbn/cloud-security-posture-common/schema';
Copy link
Contributor

Choose a reason for hiding this comment

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

same problem with versioning here. We lose information about the version when importing from our shared package without versioning. Again to make a breaking change you would need to version it somehow on the package level, that's why it's better to move the whole versioned types and schemas to our common package

import {
CDR_MISCONFIGURATIONS_INDEX_PATTERN,
LATEST_FINDINGS_RETENTION_POLICY,
} from '../../../../common/constants';
import { MAX_FINDINGS_TO_LOAD } from '../../../common/constants';
CspBenchmarkRulesStates,
Copy link
Contributor

Choose a reason for hiding this comment

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

import type

@@ -6,6 +6,7 @@
*/

import { Truthy } from 'lodash';
import { BaseCspSetupStatus } from '@kbn/cloud-security-posture-common';
Copy link
Contributor

Choose a reason for hiding this comment

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

import type

@@ -5,7 +5,7 @@
* 2.0.
*/
import { QueryDslQueryContainer } from '@kbn/data-views-plugin/common/types';
import { CspBenchmarkRulesStates } from '../types/latest';
import { CspBenchmarkRulesStates } from '@kbn/cloud-security-posture-common';
Copy link
Contributor

Choose a reason for hiding this comment

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

import type

@@ -8,6 +8,7 @@
import React from 'react';

import { coreMock } from '@kbn/core/public/mocks';
import { BaseCspSetupStatus, CspStatusCode } from '@kbn/cloud-security-posture-common';
Copy link
Contributor

Choose a reason for hiding this comment

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

import type

ComplianceDashboardDataV2,
CspStatusCode,
} from '../../../common/types_old';
import { ComplianceDashboardDataV2 } from '../../../common/types_old';
Copy link
Contributor

Choose a reason for hiding this comment

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

impot type

@@ -5,7 +5,7 @@
* 2.0.
*/

import { CspFinding } from '../../../../common/schemas/csp_finding';
import { CspFinding } from '@kbn/cloud-security-posture-common';
Copy link
Contributor

Choose a reason for hiding this comment

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

import type

@@ -7,7 +7,7 @@

import type { HttpSetup } from '@kbn/core/public';
import React from 'react';
import { CspFinding } from '../../../../common/schemas/csp_finding';
import { CspFinding } from '@kbn/cloud-security-posture-common';
Copy link
Contributor

Choose a reason for hiding this comment

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

import type

Copy link
Contributor

@maxcold maxcold left a comment

Choose a reason for hiding this comment

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

Approving in case we decide to move the versioned types and schemas in a separate PR

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #73 / Cloud Security Posture Test adding Cloud Security Posture Integrations CNVM CNVM AWS Hyperlink on PostInstallation Modal should have the correct URL

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cloudSecurityPosture 703 705 +2

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/cloud-security-posture 0 19 +19
@kbn/cloud-security-posture-common 0 45 +45
total +64

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
cloudSecurityPosture 481.1KB 481.1KB -53.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
cloudSecurityPosture 16.9KB 17.0KB +93.0B
Unknown metric groups

API count

id before after diff
@kbn/cloud-security-posture 0 19 +19
@kbn/cloud-security-posture-common 0 45 +45
total +64

History

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

Copy link
Member

@machadoum machadoum left a comment

Choose a reason for hiding this comment

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

Security Solution changes LGTM!

@animehart animehart merged commit 6d3721a into elastic:main Aug 27, 2024
40 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 27, 2024
animehart added a commit that referenced this pull request Aug 28, 2024
…clude new package PHASE 3 (#191317)

The previous #190105 was way too
big and made it hard to review without missing any bugs or potential
bugs, Thus we decided we are going to make series of smaller PR to make
things more manageable

We will be splitting it into 4 PR
Phase 1: Creating empty packages for csp and csp-common
Phase 2: Move Types from CSP plugin to the Package + Deleting duplicates
in the CSP plugin where possible
Phase 3: Move Functions, Utils or Helpers, Hooks to Package
Phase 4: Misconfiguration Preview feature (with Cypress test and other
required test)

This is **Phase 3** of the Process,
This also includes moving rule versions type

This PR is the continuation of this PR
#190933

NOTE:
Merge phase 2 first before this

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants