Skip to content

[Cloud Security][Metering] Report all assets details#186777

Merged
CohenIdo merged 19 commits intoelastic:mainfrom
CohenIdo:report-all-csp-assets
Jul 4, 2024
Merged

[Cloud Security][Metering] Report all assets details#186777
CohenIdo merged 19 commits intoelastic:mainfrom
CohenIdo:report-all-csp-assets

Conversation

@CohenIdo
Copy link
Copy Markdown
Contributor

@CohenIdo CohenIdo commented Jun 23, 2024

solves:

Summary

The Cloud Security solution charges solely for billable assets in a serverless project.

With this PR, we utilize the metadata object from the usage-v2-schema to report the complete list of cloud assets for future debugging and optimization purposes.

Note: the API spec doesn't allow nested objects in metadata, that's the reason the tier remains at the same level as the resource dub type details. In addition the metadata object in both the UsageMetrics and UsageSource schemas can contain a dictionary of key-value pairs where both the keys and the values are strings.

Example

In the payload below, you can find the environment containing 4 unique assets, but we only charge for 3.

{
  "id": "cloud_security_cspm_missing_project_id_2024-07-02T08:30:00.000Z",
  "usage_timestamp": "2024-07-02T05:08:08.350Z",
  "creation_timestamp": "2024-07-02T08:30:00.000Z",
  "usage": {
    "type": "cloud_security",
    "sub_type": "cspm",
    "quantity": 3,
    "period_seconds": 1800,
    "metadata": {
      "aws-s3":  2,
      "aws-rds":  1,
      "aws-not-bllable-asset#1":   1,
      "aws-not-bllable-asset#2":   3,
      "aws-not-bllable-asset#3":   5
    }
  },
  "source": {
    "id": "cloud-security-usage-reporting-task:1",
    "instance_group_id": "missing_project_id",
    "metadata": {
      "tier": "complete"
    }
  }
}

@CohenIdo CohenIdo added release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related ci:project-deploy-security Create a Security Serverless Project v8.15.0 labels Jun 23, 2024
@@ -212,21 +219,20 @@ export const getCloudSecurityUsageRecord = async ({
logger,
}: CloudSecurityMeteringCallbackInput): Promise<UsageRecord[] | undefined> => {
try {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Following our decision to prevent overbilling and until we implement a recovery mechanism, getSearchStartDate is redundant.

@@ -21,6 +21,15 @@ export const cloudSecurityMetringCallback = async ({
lastSuccessfulReport,
config,
}: MeteringCallbackInput): Promise<MeteringCallBackResponse> => {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Following FP alerts we had, I found it more effective to skip cloud metering earlier.

@CohenIdo CohenIdo requested a review from kfirpeled June 23, 2024 09:35
@CohenIdo CohenIdo marked this pull request as ready for review June 24, 2024 06:04
@CohenIdo CohenIdo requested a review from a team as a code owner June 24, 2024 06:04
@elasticmachine
Copy link
Copy Markdown
Contributor

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

@CohenIdo CohenIdo force-pushed the report-all-csp-assets branch from d7ce66b to 3c86f6f Compare June 24, 2024 07:21
@CohenIdo CohenIdo force-pushed the report-all-csp-assets branch from a467e97 to c323796 Compare July 2, 2024 08:26
@kibana-ci
Copy link
Copy Markdown

kibana-ci commented Jul 2, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

✅ unchanged

History

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

@CohenIdo CohenIdo requested review from joeypoon and removed request for kfirpeled July 2, 2024 19:54
Copy link
Copy Markdown
Member

@joeypoon joeypoon left a comment

Choose a reason for hiding this comment

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

My knowledge of cloud billing isn't that intimate so with that in mind, it looks good overall. I believe we should make one change though. We should use usage.metadata here instead of source.metadata since these are billing details, not source/project details.

@CohenIdo
Copy link
Copy Markdown
Contributor Author

CohenIdo commented Jul 3, 2024

My knowledge of cloud billing isn't that intimate so with that in mind, it looks good overall. I believe we should make one change though. We should use usage.metadata here instead of source.metadata since these are billing details, not source/project details.

@joeypoon I agree, I moved it to usage.metadata

@CohenIdo CohenIdo requested a review from joeypoon July 3, 2024 06:21
Copy link
Copy Markdown
Contributor

@eyalkraft eyalkraft left a comment

Choose a reason for hiding this comment

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

Thanks @CohenIdo!

Actually I'd expect the complete deletion of BILLABLE_ASSETS_CONFIG and billable notion whatsoever from the Kibana code.
The list of billable assets should be moved to the glue function where we will be free to change what's billable and what's not without even changing Kibana.

I think that' a better separation of responsibilities and aligns with the value we're trying to achieve with this change.

wdyt?

@CohenIdo
Copy link
Copy Markdown
Contributor Author

CohenIdo commented Jul 3, 2024

Thanks @CohenIdo!

Actually I'd expect the complete deletion of BILLABLE_ASSETS_CONFIG and billable notion whatsoever from the Kibana code. The list of billable assets should be moved to the glue function where we will be free to change what's billable and what's not without even changing Kibana.

I think that' a better separation of responsibilities and aligns with the value we're trying to achieve with this change.

wdyt?

@eyalkraft, that's was the original plan, together with @kfirpeled we decided to not have breaking change but to achieve most of the goals with the current implementation.

@CohenIdo CohenIdo requested a review from eyalkraft July 3, 2024 08:07
Copy link
Copy Markdown
Member

@joeypoon joeypoon left a comment

Choose a reason for hiding this comment

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

I have one tiny comment which would be great if you can fix before merge. Otherwise 👍.

period_seconds?: number;
cause?: string;
metadata?: unknown;
metadata?: ResourceSubtypeCounter;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It feels a bit strange to import from a child file upward. We should move ResourceSubtypeCounter into this file or just use [key: string]: string directly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done:)

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jul 4, 2024

⏳ Build in-progress

History

@CohenIdo CohenIdo merged commit 3e9ad41 into elastic:main Jul 4, 2024
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting ci:project-deploy-security Create a Security Serverless Project release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related v8.15.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants