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 CUR 2.0 support to Terraform cur-source module #1040

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

christrotter
Copy link

Issue #, if available:
#1029

Description of changes:
For those who are starting their CID/CUDOS journey, the advice from AWS TAMs and documentation is that CUDOS v4 is deprecated and no longer supported. Furthermore, CUR 1.0 is similarly deprecated and no longer supported. Therefore, the recommended starting point is via CUDOS v5 and CUR 2.0.

The key point of this change is that the source account (payer account) - i.e. where you want to set up the Data Export - is currently only able to build a Legacy CUR 1.0 export (the aws_cur_report_definition resource). In order to use CUR 2.0, you must use the new resource type aws_bcmdataexports_export.

This new resource type, which I have made the default, also requires the following changes:

  • IAM policy changes to allow the new AWS service to access it
  • an output to match the old resource
  • logic variable so you can force CUR 1.0 if needed
  • a variable to set a custom query for report fields (defaulting to all, as per AWS recommendation)
  • readme info to help advise users of potential pitfalls (e.g. cost)

Also, I was unfortunately not able to run the bats tests locally due to account restrictions, but added some relevant changes that I think are what you'd want?

Finally, there is one item I still need to validate, and that is how the rest of the CFN stack handles the S3 pathing - I think it should be fine?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@christrotter christrotter changed the title Add cur2 tf source module Add CUR 2.0 support to Terraform cur-source module Nov 22, 2024
Copy link
Contributor

@sean-nixon sean-nixon left a comment

Choose a reason for hiding this comment

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

Apologies for the delay reviewing this. I started adding some inline feedback; however, as you mentioned on the linked issue, I don't think retrofitting the existing modules to work with CURv2 will work. Instead, I think we will need to create new modules specifically for data exports (e.g. data-exports-source and data-exports-destination modules), effectively converting the CFN template data-exports-aggregation.yaml.

If you have a working version and want to contribute, I definitely encourage you to submit another PR which we can use as a base for getting it this functionality merged into mainline.

Some things to consider:

  • FOCUS support
  • Cost Optimization Hub data support
  • S3 bucket naming consistent with CFN

Comment on lines -222 to 245
inline_policy {
name = "S3Replication"
policy = data.aws_iam_policy_document.replication.json
}
}

resource "aws_iam_role_policy_attachment" "replication_attach" {
role = aws_iam_role.replication.name
policy_arn = aws_iam_policy.replication.arn
}

resource "aws_iam_policy" "replication" {
name = "CUR-replication"
description = "Replication policy for S3 CUR"
policy = data.aws_iam_policy_document.replication.json
}
Copy link
Contributor

Choose a reason for hiding this comment

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

To match our CloudFormation implementation and avoid creating unnecessary customer-managed policies, can you use the aws_iam_role_policy resource instead?

export {
name = "${var.resource_prefix}-${var.cur_name_suffix}"
data_query {
query_statement = var.bcm_query
Copy link
Contributor

Choose a reason for hiding this comment

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

This query needs to be adjusted based on var.enable_split_cost_allocation_data . See SCAD query here: https://github.com/aws-samples/aws-cudos-framework-deployment/blob/main/cfn-templates/data-exports-aggregation.yaml#L148

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants