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

Add transforms creation to the CSP plugin initialization #129905

Merged
merged 17 commits into from
Apr 18, 2022

Conversation

eyalkraft
Copy link
Contributor

@eyalkraft eyalkraft commented Apr 11, 2022

Summary

Following the decision to hold back the integration package-spec change to support transform assets,
We will temporarily create the required transforms required for the CSP plugin as part of the plugin initialization itself.
These transforms will be moved to be part of our integration package as soon as the package-spec will allow it.

Checklist

  • This change was tested locally

@eyalkraft eyalkraft added release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related v8.3.0 labels Apr 11, 2022
@eyalkraft eyalkraft self-assigned this Apr 11, 2022
@eyalkraft
Copy link
Contributor Author

eyalkraft commented Apr 17, 2022

I was forced to add findings-*

export const PRIVILEGED_CSP_KUBEBEAT_INDEX_PATTERN = 'logs-cis_kubernetes_benchmark.findings-*';

and not use the existing (no hyphen) findings*

export const CSP_KUBEBEAT_INDEX_PATTERN = 'logs-cis_kubernetes_benchmark.findings*';

since this is the index pattern the kibana user has the proper privileges for as a result of this PR to Elasticsearch.

Otherwise the following error occurs:

[2022-04-17T17:07:12.066+03:00][ERROR][plugins.cloudSecurityPosture] Failed to create transform cloud_security_posture.latest-default-0.0.1: security_exception: Cannot create transform [cloud_security_posture.latest-default-0.0.1] because user kibana_system lacks the required permissions [logs-cis_kubernetes_benchmark.findings*:[read], logs-cloud_security_posture.findings_latest-default:[]]

wdyt @kfirpeled?

We can also change the original CSP_KUBEBEAT_INDEX_PATTERN ofcourse but it seems more risky, and given that we rename it soon enough maybe better to postpone this change.

Edit: On second thought just adding the - to the original constant seems more reasonable...

@eyalkraft eyalkraft marked this pull request as ready for review April 17, 2022 14:27
@elasticmachine
Copy link
Contributor

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

kfirpeled
kfirpeled previously approved these changes Apr 17, 2022
Copy link
Contributor

@kfirpeled kfirpeled left a comment

Choose a reason for hiding this comment

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

as is, it looks good to me.
the index pattern was wrong and I've changed that only on the integration part (adding the - before the astrix).

now I see it is needed here as well.

@@ -17,7 +17,7 @@ export const LATEST_FINDINGS_INDEX_NAME = 'cloud_security_posture.findings_lates
export const BENCHMARK_SCORE_INDEX_NAME = 'cloud_security_posture.scores';

export const AGENT_LOGS_INDEX_PATTERN = '.logs-cis_kubernetes_benchmark.metadata*';
export const CSP_KUBEBEAT_INDEX_PATTERN = 'logs-cis_kubernetes_benchmark.findings*';
export const CSP_KUBEBEAT_INDEX_PATTERN = 'logs-cis_kubernetes_benchmark.findings-*';
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@kfirpeled kfirpeled self-requested a review April 17, 2022 15:00
Copy link
Contributor

@kfirpeled kfirpeled left a comment

Choose a reason for hiding this comment

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

added few comments

benchmarkScoreMapping,
logger
);
return Promise.all([
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

transform_id: 'cloud_security_posture.latest-default-0.0.1',
description: 'Defines findings transformation to view only the latest finding per resource',
source: {
index: CSP_KUBEBEAT_INDEX_PATTERN,
Copy link
Contributor

@kfirpeled kfirpeled Apr 17, 2022

Choose a reason for hiding this comment

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

fyi, when cloudbeat would support namespaces, this transform would take results from other spaces into the default's latest index.

what if the index pattern of this transform would be logs-cis_kubernetes_benchmark.findings-default-*. looking ahead, it might prevent a state that would be harder to fix if leaving it as it is now.

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 pretty sure the pattern you suggested won't work as the datastream name is logs-cis_kubernetes_benchmark.findings-default and the underlying index name is .ds-logs-cis_kubernetes_benchmark.findings-default-2022.04.18-000001

Screen Shot 2022-04-18 at 10 33 24

Screen Shot 2022-04-18 at 10 35 11

Copy link
Contributor

Choose a reason for hiding this comment

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

correct, it looks like the datastream name would achieve the same result though.
anyway, we can fix that later

@kfirpeled kfirpeled self-requested a review April 17, 2022 17:28
@kfirpeled kfirpeled dismissed their stale review April 17, 2022 17:29

will re-review on next updates

Copy link
Contributor

@kfirpeled kfirpeled left a comment

Choose a reason for hiding this comment

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

🚀

@eyalkraft
Copy link
Contributor Author

tested locally

@eyalkraft eyalkraft enabled auto-merge (squash) April 18, 2022 12:33
});
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it returns true although it was not created. consider documentation/ better naming for the function

@kfirpeled
Copy link
Contributor

🚢 it

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
cloudSecurityPosture 407.1KB 407.1KB +1.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
cloudSecurityPosture 14 15 +1

Total ESLint disabled count

id before after diff
cloudSecurityPosture 15 16 +1

History

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

cc @eyalkraft

@eyalkraft eyalkraft merged commit d55c7b3 into elastic:main Apr 18, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 18, 2022
@eyalkraft eyalkraft deleted the create-transforms-in-plugin branch April 18, 2022 15:16
kertal pushed a commit to kertal/kibana that referenced this pull request May 24, 2022
)

* add transforms

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* catch 404

* add hyphen

* start transforms, promise.all

* return is clearer

* add tests

* transform rename

* add test

* use exact pattern

* only start if created

Co-authored-by: kibanamachine <[email protected]>
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.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants