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][Metering] Report all assets details #186777

Merged
merged 19 commits into from
Jul 4, 2024

Conversation

CohenIdo
Copy link
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
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
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
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
Collaborator

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
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
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
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
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
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 👍.

@@ -65,17 +66,13 @@ export interface UsageMetrics {
quantity: number;
period_seconds?: number;
cause?: string;
metadata?: unknown;
metadata?: ResourceSubtypeCounter;
Copy link
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
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
Contributor

elasticmachine commented Jul 4, 2024

⏳ Build in-progress

History

@CohenIdo CohenIdo merged commit 3e9ad41 into elastic:main Jul 4, 2024
39 checks passed
@kibanamachine kibanamachine added the backport:skip This commit 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 commit 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