From f9d203b2887e0922604ef9f32eeca3ebdea40e6e Mon Sep 17 00:00:00 2001 From: Robert Bruce Date: Tue, 17 Mar 2020 14:41:46 +0000 Subject: [PATCH 01/10] Updated options on aws_cur_report_definition to be inline with current AWS options --- aws/resource_aws_cur_report_definition.go | 111 +++++++- ...resource_aws_cur_report_definition_test.go | 245 ++++++++++++++++-- .../r/cur_report_definition.html.markdown | 8 +- 3 files changed, 340 insertions(+), 24 deletions(-) diff --git a/aws/resource_aws_cur_report_definition.go b/aws/resource_aws_cur_report_definition.go index 5ac8e66b321..b1e71d49fd0 100644 --- a/aws/resource_aws_cur_report_definition.go +++ b/aws/resource_aws_cur_report_definition.go @@ -39,7 +39,9 @@ func resourceAwsCurReportDefinition() *schema.Resource { Required: true, ForceNew: true, ValidateFunc: validation.StringInSlice([]string{ - costandusagereportservice.ReportFormatTextOrcsv}, false), + costandusagereportservice.ReportFormatTextOrcsv, + costandusagereportservice.ReportFormatParquet, + }, false), }, "compression": { Type: schema.TypeString, @@ -48,6 +50,7 @@ func resourceAwsCurReportDefinition() *schema.Resource { ValidateFunc: validation.StringInSlice([]string{ costandusagereportservice.CompressionFormatGzip, costandusagereportservice.CompressionFormatZip, + costandusagereportservice.CompressionFormatParquet, }, false), }, "additional_schema_elements": { @@ -84,12 +87,29 @@ func resourceAwsCurReportDefinition() *schema.Resource { ValidateFunc: validation.StringInSlice([]string{ costandusagereportservice.AdditionalArtifactQuicksight, costandusagereportservice.AdditionalArtifactRedshift, + costandusagereportservice.AdditionalArtifactAthena, }, false), }, Set: schema.HashString, Optional: true, ForceNew: true, }, + "refresh_closed_reports": { + Type: schema.TypeBool, + ForceNew: true, + Default: false, + Optional: true, + }, + "report_versioning": { + Type: schema.TypeString, + ForceNew: true, + Optional: true, + Default: costandusagereportservice.ReportVersioningCreateNewReport, + ValidateFunc: validation.StringInSlice([]string{ + costandusagereportservice.ReportVersioningCreateNewReport, + costandusagereportservice.ReportVersioningOverwriteReport, + }, false), + }, }, } } @@ -97,18 +117,97 @@ func resourceAwsCurReportDefinition() *schema.Resource { func resourceAwsCurReportDefinitionCreate(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).costandusagereportconn + additionalArtifacts := expandStringSet(d.Get("additional_artifacts").(*schema.Set)) + compression := aws.String(d.Get("compression").(string)) + format := aws.String(d.Get("format").(string)) + prefix := aws.String(d.Get("s3_prefix").(string)) + reportVersioning := aws.String(d.Get("report_versioning").(string)) + + hasAthena := false + + // perform various combination checks, AWS API unhelpfully just returns an empty ValidationException + // these combinations have been determined from the Create Report AWS Console Web Form + for _, artifact := range additionalArtifacts { + if *artifact == costandusagereportservice.AdditionalArtifactAthena { + hasAthena = true + break + } + } + + if hasAthena { + if len(additionalArtifacts) > 1 { + return fmt.Errorf( + "When %s exists within additional_artifacts, no other artifact type can be declared", + costandusagereportservice.AdditionalArtifactAthena, + ) + } + + if len(*prefix) == 0 { + return fmt.Errorf( + "When %s exists within additional_artifacts, prefix cannot be empty", + costandusagereportservice.AdditionalArtifactAthena, + ) + } + + if *reportVersioning != costandusagereportservice.ReportVersioningOverwriteReport { + return fmt.Errorf( + "When %s exists within additional_artifacts, report_versioning must be %s", + costandusagereportservice.AdditionalArtifactAthena, + costandusagereportservice.ReportVersioningOverwriteReport, + ) + } + } + + if *format == costandusagereportservice.ReportFormatParquet { + if *compression != costandusagereportservice.CompressionFormatParquet { + return fmt.Errorf( + "When format is %s, compression must also be %s", + costandusagereportservice.ReportFormatParquet, + costandusagereportservice.CompressionFormatParquet, + ) + } + } else { + if *compression == costandusagereportservice.CompressionFormatParquet { + return fmt.Errorf( + "When format is %s, format must not be %s", + *format, + costandusagereportservice.CompressionFormatParquet, + ) + } + } + + if hasAthena && (*format != costandusagereportservice.ReportFormatParquet) { + return fmt.Errorf( + "When %s exists within additional_artifacts, both format and compression must be %s", + costandusagereportservice.AdditionalArtifactAthena, + costandusagereportservice.ReportFormatParquet, + ) + } + + if !hasAthena && len(additionalArtifacts) > 1 && (*format == costandusagereportservice.ReportFormatParquet) { + return fmt.Errorf( + "When additional_artifacts includes %s and/or %s, format must not be %s", + costandusagereportservice.AdditionalArtifactQuicksight, + costandusagereportservice.AdditionalArtifactRedshift, + costandusagereportservice.ReportFormatParquet, + ) + } + // end checks + reportName := d.Get("report_name").(string) reportDefinition := &costandusagereportservice.ReportDefinition{ ReportName: aws.String(reportName), TimeUnit: aws.String(d.Get("time_unit").(string)), - Format: aws.String(d.Get("format").(string)), - Compression: aws.String(d.Get("compression").(string)), + Format: format, + Compression: compression, AdditionalSchemaElements: expandStringSet(d.Get("additional_schema_elements").(*schema.Set)), S3Bucket: aws.String(d.Get("s3_bucket").(string)), - S3Prefix: aws.String(d.Get("s3_prefix").(string)), + S3Prefix: prefix, S3Region: aws.String(d.Get("s3_region").(string)), - AdditionalArtifacts: expandStringSet(d.Get("additional_artifacts").(*schema.Set)), + AdditionalArtifacts: additionalArtifacts, + RefreshClosedReports: aws.Bool(d.Get("refresh_closed_reports").(bool)), + ReportVersioning: reportVersioning, } reportDefinitionInput := &costandusagereportservice.PutReportDefinitionInput{ @@ -148,6 +247,8 @@ func resourceAwsCurReportDefinitionRead(d *schema.ResourceData, meta interface{} d.Set("s3_prefix", aws.StringValue(matchingReportDefinition.S3Prefix)) d.Set("s3_region", aws.StringValue(matchingReportDefinition.S3Region)) d.Set("additional_artifacts", aws.StringValueSlice(matchingReportDefinition.AdditionalArtifacts)) + d.Set("refresh_closed_reports", matchingReportDefinition.RefreshClosedReports) + d.Set("report_versioning", matchingReportDefinition.ReportVersioning) return nil } diff --git a/aws/resource_aws_cur_report_definition_test.go b/aws/resource_aws_cur_report_definition_test.go index 4446b148db2..900ed1645e9 100644 --- a/aws/resource_aws_cur_report_definition_test.go +++ b/aws/resource_aws_cur_report_definition_test.go @@ -3,6 +3,7 @@ package aws import ( "fmt" "os" + "strings" "testing" "github.com/aws/aws-sdk-go/aws" @@ -22,6 +23,12 @@ func TestAccAwsCurReportDefinition_basic(t *testing.T) { reportName := acctest.RandomWithPrefix("tf_acc_test") bucketName := fmt.Sprintf("tf-test-bucket-%d", acctest.RandInt()) bucketRegion := "us-east-1" + bucketPrefix := "" + format := "textORcsv" + compression := "GZIP" + additionalArtifacts := []string{"REDSHIFT", "QUICKSIGHT"} + refreshClosedReports := false + reportVersioning := "CREATE_NEW_REPORT" resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t); testAccPreCheckAWSCur(t) }, @@ -29,17 +36,206 @@ func TestAccAwsCurReportDefinition_basic(t *testing.T) { CheckDestroy: testAccCheckAwsCurReportDefinitionDestroy, Steps: []resource.TestStep{ { - Config: testAccAwsCurReportDefinitionConfig_basic(reportName, bucketName, bucketRegion), + Config: testAccAwsCurReportDefinitionConfig_bucket(bucketName, bucketRegion), + }, + { + Config: testAccAwsCurReportDefinitionConfig_basic(reportName, bucketName, bucketPrefix, bucketRegion, format, compression, additionalArtifacts, refreshClosedReports, reportVersioning), + Check: resource.ComposeTestCheckFunc( + testAccCheckAwsCurReportDefinitionExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "report_name", reportName), + resource.TestCheckResourceAttr(resourceName, "time_unit", "DAILY"), + resource.TestCheckResourceAttr(resourceName, "format", format), + resource.TestCheckResourceAttr(resourceName, "compression", compression), + resource.TestCheckResourceAttr(resourceName, "additional_schema_elements.#", "1"), + resource.TestCheckResourceAttr(resourceName, "s3_bucket", bucketName), + resource.TestCheckResourceAttr(resourceName, "s3_prefix", bucketPrefix), + resource.TestCheckResourceAttr(resourceName, "s3_region", bucketRegion), + resource.TestCheckResourceAttr(resourceName, "additional_artifacts.#", "2"), + resource.TestCheckResourceAttr(resourceName, "refresh_closed_reports", "false"), + resource.TestCheckResourceAttr(resourceName, "report_versioning", reportVersioning), + ), + }, + }, + }) +} + +func TestAccAwsCurReportDefinition_parquet(t *testing.T) { + oldvar := os.Getenv("AWS_DEFAULT_REGION") + os.Setenv("AWS_DEFAULT_REGION", "us-east-1") + defer os.Setenv("AWS_DEFAULT_REGION", oldvar) + + resourceName := "aws_cur_report_definition.test" + + reportName := acctest.RandomWithPrefix("tf_acc_test") + bucketName := fmt.Sprintf("tf-test-bucket-%d", acctest.RandInt()) + bucketRegion := "us-east-1" + bucketPrefix := "" + format := "Parquet" + compression := "Parquet" + additionalArtifacts := []string{} + refreshClosedReports := false + reportVersioning := "CREATE_NEW_REPORT" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t); testAccPreCheckAWSCur(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAwsCurReportDefinitionDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAwsCurReportDefinitionConfig_bucket(bucketName, bucketRegion), + }, + { + Config: testAccAwsCurReportDefinitionConfig_basic(reportName, bucketName, bucketPrefix, bucketRegion, format, compression, additionalArtifacts, refreshClosedReports, reportVersioning), + Check: resource.ComposeTestCheckFunc( + testAccCheckAwsCurReportDefinitionExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "report_name", reportName), + resource.TestCheckResourceAttr(resourceName, "time_unit", "DAILY"), + resource.TestCheckResourceAttr(resourceName, "format", format), + resource.TestCheckResourceAttr(resourceName, "compression", compression), + resource.TestCheckResourceAttr(resourceName, "additional_schema_elements.#", "1"), + resource.TestCheckResourceAttr(resourceName, "s3_bucket", bucketName), + resource.TestCheckResourceAttr(resourceName, "s3_prefix", bucketPrefix), + resource.TestCheckResourceAttr(resourceName, "s3_region", bucketRegion), + resource.TestCheckResourceAttr(resourceName, "refresh_closed_reports", "false"), + resource.TestCheckResourceAttr(resourceName, "report_versioning", reportVersioning), + ), + }, + }, + }) +} + +func TestAccAwsCurReportDefinition_athena(t *testing.T) { + oldvar := os.Getenv("AWS_DEFAULT_REGION") + os.Setenv("AWS_DEFAULT_REGION", "us-east-1") + defer os.Setenv("AWS_DEFAULT_REGION", oldvar) + + resourceName := "aws_cur_report_definition.test" + + reportName := acctest.RandomWithPrefix("tf_acc_test") + bucketName := fmt.Sprintf("tf-test-bucket-%d", acctest.RandInt()) + bucketRegion := "us-east-1" + bucketPrefix := "data" + format := "Parquet" + compression := "Parquet" + additionalArtifacts := []string{"ATHENA"} + refreshClosedReports := false + reportVersioning := "OVERWRITE_REPORT" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t); testAccPreCheckAWSCur(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAwsCurReportDefinitionDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAwsCurReportDefinitionConfig_bucket(bucketName, bucketRegion), + }, + { + Config: testAccAwsCurReportDefinitionConfig_basic(reportName, bucketName, bucketPrefix, bucketRegion, format, compression, additionalArtifacts, refreshClosedReports, reportVersioning), + Check: resource.ComposeTestCheckFunc( + testAccCheckAwsCurReportDefinitionExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "report_name", reportName), + resource.TestCheckResourceAttr(resourceName, "time_unit", "DAILY"), + resource.TestCheckResourceAttr(resourceName, "format", format), + resource.TestCheckResourceAttr(resourceName, "compression", compression), + resource.TestCheckResourceAttr(resourceName, "additional_schema_elements.#", "1"), + resource.TestCheckResourceAttr(resourceName, "s3_bucket", bucketName), + resource.TestCheckResourceAttr(resourceName, "s3_prefix", bucketPrefix), + resource.TestCheckResourceAttr(resourceName, "s3_region", bucketRegion), + resource.TestCheckResourceAttr(resourceName, "additional_artifacts.#", "1"), + resource.TestCheckResourceAttr(resourceName, "refresh_closed_reports", "false"), + resource.TestCheckResourceAttr(resourceName, "report_versioning", reportVersioning), + ), + }, + }, + }) +} + +func TestAccAwsCurReportDefinition_refresh(t *testing.T) { + oldvar := os.Getenv("AWS_DEFAULT_REGION") + os.Setenv("AWS_DEFAULT_REGION", "us-east-1") + defer os.Setenv("AWS_DEFAULT_REGION", oldvar) + + resourceName := "aws_cur_report_definition.test" + + reportName := acctest.RandomWithPrefix("tf_acc_test") + bucketName := fmt.Sprintf("tf-test-bucket-%d", acctest.RandInt()) + bucketRegion := "us-east-1" + bucketPrefix := "" + format := "textORcsv" + compression := "GZIP" + additionalArtifacts := []string{"REDSHIFT", "QUICKSIGHT"} + refreshClosedReports := true + reportVersioning := "CREATE_NEW_REPORT" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t); testAccPreCheckAWSCur(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAwsCurReportDefinitionDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAwsCurReportDefinitionConfig_bucket(bucketName, bucketRegion), + }, + { + Config: testAccAwsCurReportDefinitionConfig_basic(reportName, bucketName, bucketPrefix, bucketRegion, format, compression, additionalArtifacts, refreshClosedReports, reportVersioning), + Check: resource.ComposeTestCheckFunc( + testAccCheckAwsCurReportDefinitionExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "report_name", reportName), + resource.TestCheckResourceAttr(resourceName, "time_unit", "DAILY"), + resource.TestCheckResourceAttr(resourceName, "format", format), + resource.TestCheckResourceAttr(resourceName, "compression", compression), + resource.TestCheckResourceAttr(resourceName, "additional_schema_elements.#", "1"), + resource.TestCheckResourceAttr(resourceName, "s3_bucket", bucketName), + resource.TestCheckResourceAttr(resourceName, "s3_prefix", bucketPrefix), + resource.TestCheckResourceAttr(resourceName, "s3_region", bucketRegion), + resource.TestCheckResourceAttr(resourceName, "additional_artifacts.#", "2"), + resource.TestCheckResourceAttr(resourceName, "refresh_closed_reports", "true"), + resource.TestCheckResourceAttr(resourceName, "report_versioning", reportVersioning), + ), + }, + }, + }) +} + +func TestAccAwsCurReportDefinition_overwrite(t *testing.T) { + oldvar := os.Getenv("AWS_DEFAULT_REGION") + os.Setenv("AWS_DEFAULT_REGION", "us-east-1") + defer os.Setenv("AWS_DEFAULT_REGION", oldvar) + + resourceName := "aws_cur_report_definition.test" + + reportName := acctest.RandomWithPrefix("tf_acc_test") + bucketName := fmt.Sprintf("tf-test-bucket-%d", acctest.RandInt()) + bucketRegion := "us-east-1" + bucketPrefix := "" + format := "textORcsv" + compression := "GZIP" + additionalArtifacts := []string{"REDSHIFT", "QUICKSIGHT"} + refreshClosedReports := false + reportVersioning := "OVERWRITE_REPORT" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t); testAccPreCheckAWSCur(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAwsCurReportDefinitionDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAwsCurReportDefinitionConfig_bucket(bucketName, bucketRegion), + }, + { + Config: testAccAwsCurReportDefinitionConfig_basic(reportName, bucketName, bucketPrefix, bucketRegion, format, compression, additionalArtifacts, refreshClosedReports, reportVersioning), Check: resource.ComposeTestCheckFunc( testAccCheckAwsCurReportDefinitionExists(resourceName), resource.TestCheckResourceAttr(resourceName, "report_name", reportName), resource.TestCheckResourceAttr(resourceName, "time_unit", "DAILY"), - resource.TestCheckResourceAttr(resourceName, "compression", "GZIP"), + resource.TestCheckResourceAttr(resourceName, "format", format), + resource.TestCheckResourceAttr(resourceName, "compression", compression), resource.TestCheckResourceAttr(resourceName, "additional_schema_elements.#", "1"), resource.TestCheckResourceAttr(resourceName, "s3_bucket", bucketName), - resource.TestCheckResourceAttr(resourceName, "s3_prefix", ""), + resource.TestCheckResourceAttr(resourceName, "s3_prefix", bucketPrefix), resource.TestCheckResourceAttr(resourceName, "s3_region", bucketRegion), resource.TestCheckResourceAttr(resourceName, "additional_artifacts.#", "2"), + resource.TestCheckResourceAttr(resourceName, "refresh_closed_reports", "false"), + resource.TestCheckResourceAttr(resourceName, "report_versioning", reportVersioning), ), }, }, @@ -104,24 +300,24 @@ func testAccPreCheckAWSCur(t *testing.T) { } } -// note: cur report definitions are currently only supported in us-east-1 -func testAccAwsCurReportDefinitionConfig_basic(reportName string, bucketName string, bucketRegion string) string { +// note: bucket needs to exist first due to ValidationException thrown if policy not yet applied +func testAccAwsCurReportDefinitionConfig_bucket(bucketName string, bucketRegion string) string { return fmt.Sprintf(` provider "aws" { region = "us-east-1" } resource "aws_s3_bucket" "test" { - bucket = "%[2]s" + bucket = "%[1]s" acl = "private" force_destroy = true - region = "%[3]s" + region = "%[2]s" } resource "aws_s3_bucket_policy" "test" { - bucket = "${aws_s3_bucket.test.id}" + bucket = "${aws_s3_bucket.test.id}" - policy = < 0 { + artifactsStr = fmt.Sprintf("additional_artifacts = [\"%s\"]", artifactsStr) + } else { + artifactsStr = "" + } + + return fmt.Sprintf(` +%[2]s resource "aws_cur_report_definition" "test" { report_name = "%[1]s" time_unit = "DAILY" - format = "textORcsv" - compression = "GZIP" + format = "%[4]s" + compression = "%[5]s" additional_schema_elements = ["RESOURCES"] s3_bucket = "${aws_s3_bucket.test.id}" - s3_prefix = "" + s3_prefix = "%[3]s" s3_region = "${aws_s3_bucket.test.region}" - additional_artifacts = ["REDSHIFT", "QUICKSIGHT"] + %[6]s + refresh_closed_reports = %[7]t + report_versioning = "%[8]s" } -`, reportName, bucketName, bucketRegion) +`, reportName, testAccAwsCurReportDefinitionConfig_bucket(bucketName, bucketRegion), bucketPrefix, format, compression, artifactsStr, refreshClosedReports, reportVersioning) } diff --git a/website/docs/r/cur_report_definition.html.markdown b/website/docs/r/cur_report_definition.html.markdown index bafa019452b..feb41d0c2a1 100644 --- a/website/docs/r/cur_report_definition.html.markdown +++ b/website/docs/r/cur_report_definition.html.markdown @@ -35,13 +35,15 @@ The following arguments are supported: * `report_name` - (Required) Unique name for the report. Must start with a number/letter and is case sensitive. Limited to 256 characters. * `time_unit` - (Required) The frequency on which report data are measured and displayed. Valid values are: HOURLY, DAILY. -* `format` - (Required) Format for report. Valid values are: textORcsv. -* `compression` - (Required) Compression format for report. Valid values are: GZIP, ZIP. +* `format` - (Required) Format for report. Valid values are: textORcsv, Parquet. If Parquet is used, then Compression must also be Parquet. +* `compression` - (Required) Compression format for report. Valid values are: GZIP, ZIP, Parquet. If Parquet is used, then format must also be Parquet. * `additional_schema_elements` - (Required) A list of schema elements. Valid values are: RESOURCES. * `s3_bucket` - (Required) Name of the existing S3 bucket to hold generated reports. * `s3_prefix` - (Optional) Report path prefix. Limited to 256 characters. * `s3_region` - (Required) Region of the existing S3 bucket to hold generated reports. -* `additional_artifacts` - (Required) A list of additional artifacts. Valid values are: REDSHIFT, QUICKSIGHT. +* `additional_artifacts` - (Required) A list of additional artifacts. Valid values are: REDSHIFT, QUICKSIGHT, ATHENA. +* `refresh_closed_reports` - (Optional) Set to true to update your reports after they have been finalized if AWS detects charges related to previous months. +* `report_versioning` - (Optional) Overwrite the previous version of each report or to deliver the report in addition to the previous versions. Valid values are: CREATE_NEW_REPORT, OVERWRITE_REPORT ## Import From 2613efb9924ecaf5a3f4d18260e8c1fe82606782 Mon Sep 17 00:00:00 2001 From: Robert Bruce Date: Tue, 17 Mar 2020 15:45:45 +0000 Subject: [PATCH 02/10] Changed default value of refresh_closed_reports to match aws behaviour --- aws/resource_aws_cur_report_definition.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws/resource_aws_cur_report_definition.go b/aws/resource_aws_cur_report_definition.go index b1e71d49fd0..10d70d5922a 100644 --- a/aws/resource_aws_cur_report_definition.go +++ b/aws/resource_aws_cur_report_definition.go @@ -97,7 +97,7 @@ func resourceAwsCurReportDefinition() *schema.Resource { "refresh_closed_reports": { Type: schema.TypeBool, ForceNew: true, - Default: false, + Default: true, Optional: true, }, "report_versioning": { From cec36a2157300179916803a06cd896984ffbaeec Mon Sep 17 00:00:00 2001 From: Robert Bruce Date: Tue, 17 Mar 2020 15:46:21 +0000 Subject: [PATCH 03/10] Added data.aws_cur_report_definition --- aws/data_source_aws_cur_report_definition.go | 8 +++ ...a_source_aws_cur_report_definition_test.go | 56 +++---------------- .../d/cur_report_definition.html.markdown | 4 +- 3 files changed, 19 insertions(+), 49 deletions(-) diff --git a/aws/data_source_aws_cur_report_definition.go b/aws/data_source_aws_cur_report_definition.go index d2cc805b7fa..f720af677ab 100644 --- a/aws/data_source_aws_cur_report_definition.go +++ b/aws/data_source_aws_cur_report_definition.go @@ -49,6 +49,14 @@ func dataSourceAwsCurReportDefinition() *schema.Resource { Set: schema.HashString, Computed: true, }, + "refresh_closed_reports": { + Type: schema.TypeBool, + Computed: true, + }, + "report_versioning": { + Type: schema.TypeString, + Computed: true, + }, }, } } diff --git a/aws/data_source_aws_cur_report_definition_test.go b/aws/data_source_aws_cur_report_definition_test.go index a266f3b1398..068bdf3ab42 100644 --- a/aws/data_source_aws_cur_report_definition_test.go +++ b/aws/data_source_aws_cur_report_definition_test.go @@ -27,6 +27,9 @@ func TestAccDataSourceAwsCurReportDefinition_basic(t *testing.T) { Providers: testAccProviders, CheckDestroy: testAccCheckAwsCurReportDefinitionDestroy, Steps: []resource.TestStep{ + { + Config: testAccAwsCurReportDefinitionConfig_bucket(bucketName, bucketRegion), + }, { Config: testAccDataSourceAwsCurReportDefinitionConfig_basic(reportName, bucketName, bucketRegion), Check: resource.ComposeTestCheckFunc( @@ -39,6 +42,8 @@ func TestAccDataSourceAwsCurReportDefinition_basic(t *testing.T) { resource.TestCheckResourceAttrPair(datasourceName, "s3_prefix", resourceName, "s3_prefix"), resource.TestCheckResourceAttrPair(datasourceName, "s3_region", resourceName, "s3_region"), resource.TestCheckResourceAttrPair(datasourceName, "additional_artifacts.#", resourceName, "additional_artifacts.#"), + resource.TestCheckResourceAttrPair(datasourceName, "refresh_closed_reports", resourceName, "refresh_closed_reports"), + resource.TestCheckResourceAttrPair(datasourceName, "report_versioning", resourceName, "report_versioning"), ), }, }, @@ -62,52 +67,7 @@ func testAccDataSourceAwsCurReportDefinitionCheckExists(datasourceName, resource // note: cur report definitions are currently only supported in us-east-1 func testAccDataSourceAwsCurReportDefinitionConfig_basic(reportName string, bucketName string, bucketRegion string) string { return fmt.Sprintf(` -provider "aws" { - region = "us-east-1" -} - -data "aws_billing_service_account" "test" {} - -resource "aws_s3_bucket" "test" { - bucket = "%[2]s" - acl = "private" - force_destroy = true - region = "%[3]s" -} - -resource "aws_s3_bucket_policy" "test" { - bucket = "${aws_s3_bucket.test.id}" - - policy = < Date: Fri, 24 Apr 2020 07:28:42 +0100 Subject: [PATCH 04/10] Updated docs as per suggestion from @andymoore-synamedia --- website/docs/r/cur_report_definition.html.markdown | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/docs/r/cur_report_definition.html.markdown b/website/docs/r/cur_report_definition.html.markdown index feb41d0c2a1..615dfb3971f 100644 --- a/website/docs/r/cur_report_definition.html.markdown +++ b/website/docs/r/cur_report_definition.html.markdown @@ -41,7 +41,7 @@ The following arguments are supported: * `s3_bucket` - (Required) Name of the existing S3 bucket to hold generated reports. * `s3_prefix` - (Optional) Report path prefix. Limited to 256 characters. * `s3_region` - (Required) Region of the existing S3 bucket to hold generated reports. -* `additional_artifacts` - (Required) A list of additional artifacts. Valid values are: REDSHIFT, QUICKSIGHT, ATHENA. +* `additional_artifacts` - (Required) A list of additional artifacts. Valid values are: REDSHIFT, QUICKSIGHT, ATHENA. When ATHENA exists within additional_artifacts, no other artifact type can be declared and report_versioning must be OVERWRITE_REPORT. * `refresh_closed_reports` - (Optional) Set to true to update your reports after they have been finalized if AWS detects charges related to previous months. * `report_versioning` - (Optional) Overwrite the previous version of each report or to deliver the report in addition to the previous versions. Valid values are: CREATE_NEW_REPORT, OVERWRITE_REPORT From e7719dc8e93d12b92aaaf6294c3afe9180d27d2d Mon Sep 17 00:00:00 2001 From: Robert Bruce Date: Tue, 25 Aug 2020 15:00:21 +0100 Subject: [PATCH 05/10] Fixed bug introduced in #14432 where JSON policy was incorrectly converted --- aws/data_source_aws_cur_report_definition_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/aws/data_source_aws_cur_report_definition_test.go b/aws/data_source_aws_cur_report_definition_test.go index 2beeda335ba..baa2705a7ca 100644 --- a/aws/data_source_aws_cur_report_definition_test.go +++ b/aws/data_source_aws_cur_report_definition_test.go @@ -87,13 +87,13 @@ resource "aws_s3_bucket_policy" "test" { "Sid": "AllowCURBillingACLPolicy", "Effect": "Allow", "Principal": { - "AWS": data.aws_billing_service_account.test.arn + "AWS": "${data.aws_billing_service_account.test.arn}" }, "Action": [ "s3:GetBucketAcl", "s3:GetBucketPolicy" ], - "Resource": aws_s3_bucket.test.arn + "Resource": "${aws_s3_bucket.test.arn}" }, { "Sid": "AllowCURPutObject", From a67323d2f0baa4977780ef3ca159cd61069cb717 Mon Sep 17 00:00:00 2001 From: Robert Bruce Date: Tue, 25 Aug 2020 15:00:48 +0100 Subject: [PATCH 06/10] Fixed missing quote from migration for terraform 0.12 --- aws/resource_aws_cur_report_definition_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws/resource_aws_cur_report_definition_test.go b/aws/resource_aws_cur_report_definition_test.go index e4c142d625c..06c835b4f5d 100644 --- a/aws/resource_aws_cur_report_definition_test.go +++ b/aws/resource_aws_cur_report_definition_test.go @@ -346,7 +346,7 @@ resource "aws_cur_report_definition" "test" { format = "%[4]s" compression = "%[5]s" additional_schema_elements = ["RESOURCES"] - s3_bucket = "aws_s3_bucket.test.id + s3_bucket = aws_s3_bucket.test.id s3_prefix = "%[3]s" s3_region = aws_s3_bucket.test.region %[6]s From 736ec878977e8bc8228d4ae869b0dacdd247887e Mon Sep 17 00:00:00 2001 From: Robert Bruce Date: Fri, 28 Aug 2020 09:02:34 +0100 Subject: [PATCH 07/10] Updated based on feedback --- aws/resource_aws_cur_report_definition.go | 49 ++++---- ...resource_aws_cur_report_definition_test.go | 110 ++++++++++++++++-- 2 files changed, 123 insertions(+), 36 deletions(-) diff --git a/aws/resource_aws_cur_report_definition.go b/aws/resource_aws_cur_report_definition.go index 3e6aeb44a1f..a16d23f78f4 100644 --- a/aws/resource_aws_cur_report_definition.go +++ b/aws/resource_aws_cur_report_definition.go @@ -29,37 +29,37 @@ func resourceAwsCurReportDefinition() *schema.Resource { Type: schema.TypeString, Required: true, ForceNew: true, - ValidateFunc: validation.StringInSlice([]string{ - costandusagereportservice.TimeUnitDaily, - costandusagereportservice.TimeUnitHourly, - }, false), + ValidateFunc: validation.StringInSlice( + costandusagereportservice.TimeUnit_Values(), + false, + ), }, "format": { Type: schema.TypeString, Required: true, ForceNew: true, - ValidateFunc: validation.StringInSlice([]string{ - costandusagereportservice.ReportFormatTextOrcsv, - costandusagereportservice.ReportFormatParquet, - }, false), + ValidateFunc: validation.StringInSlice( + costandusagereportservice.ReportFormat_Values(), + false, + ), }, "compression": { Type: schema.TypeString, Required: true, ForceNew: true, - ValidateFunc: validation.StringInSlice([]string{ - costandusagereportservice.CompressionFormatGzip, - costandusagereportservice.CompressionFormatZip, - costandusagereportservice.CompressionFormatParquet, - }, false), + ValidateFunc: validation.StringInSlice( + costandusagereportservice.CompressionFormat_Values(), + false, + ), }, "additional_schema_elements": { Type: schema.TypeSet, Elem: &schema.Schema{ Type: schema.TypeString, - ValidateFunc: validation.StringInSlice([]string{ - costandusagereportservice.SchemaElementResources, - }, false), + ValidateFunc: validation.StringInSlice( + costandusagereportservice.SchemaElement_Values(), + false, + ), }, Set: schema.HashString, Required: true, @@ -84,11 +84,10 @@ func resourceAwsCurReportDefinition() *schema.Resource { "additional_artifacts": { Type: schema.TypeSet, Elem: &schema.Schema{Type: schema.TypeString, - ValidateFunc: validation.StringInSlice([]string{ - costandusagereportservice.AdditionalArtifactQuicksight, - costandusagereportservice.AdditionalArtifactRedshift, - costandusagereportservice.AdditionalArtifactAthena, - }, false), + ValidateFunc: validation.StringInSlice( + costandusagereportservice.AdditionalArtifact_Values(), + false, + ), }, Set: schema.HashString, Optional: true, @@ -105,10 +104,10 @@ func resourceAwsCurReportDefinition() *schema.Resource { ForceNew: true, Optional: true, Default: costandusagereportservice.ReportVersioningCreateNewReport, - ValidateFunc: validation.StringInSlice([]string{ - costandusagereportservice.ReportVersioningCreateNewReport, - costandusagereportservice.ReportVersioningOverwriteReport, - }, false), + ValidateFunc: validation.StringInSlice( + costandusagereportservice.ReportVersioning_Values(), + false, + ), }, }, } diff --git a/aws/resource_aws_cur_report_definition_test.go b/aws/resource_aws_cur_report_definition_test.go index 06c835b4f5d..784eabb3f27 100644 --- a/aws/resource_aws_cur_report_definition_test.go +++ b/aws/resource_aws_cur_report_definition_test.go @@ -22,7 +22,39 @@ func TestAccAwsCurReportDefinition_basic(t *testing.T) { s3BucketResourceName := "aws_s3_bucket.test" reportName := acctest.RandomWithPrefix("tf_acc_test") bucketName := fmt.Sprintf("tf-test-bucket-%d", acctest.RandInt()) - bucketRegion := "us-east-1" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t); testAccPreCheckAWSCur(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAwsCurReportDefinitionDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAwsCurReportDefinitionConfig_basic(reportName, bucketName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAwsCurReportDefinitionExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "report_name", reportName), + resource.TestCheckResourceAttr(resourceName, "time_unit", "DAILY"), + resource.TestCheckResourceAttr(resourceName, "compression", "GZIP"), + resource.TestCheckResourceAttr(resourceName, "additional_schema_elements.#", "1"), + resource.TestCheckResourceAttr(resourceName, "s3_bucket", bucketName), + resource.TestCheckResourceAttr(resourceName, "s3_prefix", ""), + resource.TestCheckResourceAttrPair(resourceName, "s3_region", s3BucketResourceName, "region"), + resource.TestCheckResourceAttr(resourceName, "additional_artifacts.#", "2"), + ), + }, + }, + }) +} + +func TestAccAwsCurReportDefinition_textOrCsv(t *testing.T) { + oldvar := os.Getenv("AWS_DEFAULT_REGION") + os.Setenv("AWS_DEFAULT_REGION", "us-east-1") + defer os.Setenv("AWS_DEFAULT_REGION", oldvar) + + resourceName := "aws_cur_report_definition.test" + s3BucketResourceName := "aws_s3_bucket.test" + reportName := acctest.RandomWithPrefix("tf_acc_test") + bucketName := fmt.Sprintf("tf-test-bucket-%d", acctest.RandInt()) bucketPrefix := "" format := "textORcsv" compression := "GZIP" @@ -36,7 +68,7 @@ func TestAccAwsCurReportDefinition_basic(t *testing.T) { CheckDestroy: testAccCheckAwsCurReportDefinitionDestroy, Steps: []resource.TestStep{ { - Config: testAccAwsCurReportDefinitionConfig_basic(reportName, bucketName, bucketPrefix, bucketRegion, format, compression, additionalArtifacts, refreshClosedReports, reportVersioning), + Config: testAccAwsCurReportDefinitionConfig_additional(reportName, bucketName, bucketPrefix, format, compression, additionalArtifacts, refreshClosedReports, reportVersioning), Check: resource.ComposeTestCheckFunc( testAccCheckAwsCurReportDefinitionExists(resourceName), resource.TestCheckResourceAttr(resourceName, "report_name", reportName), @@ -65,7 +97,6 @@ func TestAccAwsCurReportDefinition_parquet(t *testing.T) { s3BucketResourceName := "aws_s3_bucket.test" reportName := acctest.RandomWithPrefix("tf_acc_test") bucketName := fmt.Sprintf("tf-test-bucket-%d", acctest.RandInt()) - bucketRegion := "us-east-1" bucketPrefix := "" format := "Parquet" compression := "Parquet" @@ -79,7 +110,7 @@ func TestAccAwsCurReportDefinition_parquet(t *testing.T) { CheckDestroy: testAccCheckAwsCurReportDefinitionDestroy, Steps: []resource.TestStep{ { - Config: testAccAwsCurReportDefinitionConfig_basic(reportName, bucketName, bucketPrefix, bucketRegion, format, compression, additionalArtifacts, refreshClosedReports, reportVersioning), + Config: testAccAwsCurReportDefinitionConfig_additional(reportName, bucketName, bucketPrefix, format, compression, additionalArtifacts, refreshClosedReports, reportVersioning), Check: resource.ComposeTestCheckFunc( testAccCheckAwsCurReportDefinitionExists(resourceName), resource.TestCheckResourceAttr(resourceName, "report_name", reportName), @@ -107,7 +138,6 @@ func TestAccAwsCurReportDefinition_athena(t *testing.T) { s3BucketResourceName := "aws_s3_bucket.test" reportName := acctest.RandomWithPrefix("tf_acc_test") bucketName := fmt.Sprintf("tf-test-bucket-%d", acctest.RandInt()) - bucketRegion := "us-east-1" bucketPrefix := "data" format := "Parquet" compression := "Parquet" @@ -121,7 +151,7 @@ func TestAccAwsCurReportDefinition_athena(t *testing.T) { CheckDestroy: testAccCheckAwsCurReportDefinitionDestroy, Steps: []resource.TestStep{ { - Config: testAccAwsCurReportDefinitionConfig_basic(reportName, bucketName, bucketPrefix, bucketRegion, format, compression, additionalArtifacts, refreshClosedReports, reportVersioning), + Config: testAccAwsCurReportDefinitionConfig_additional(reportName, bucketName, bucketPrefix, format, compression, additionalArtifacts, refreshClosedReports, reportVersioning), Check: resource.ComposeTestCheckFunc( testAccCheckAwsCurReportDefinitionExists(resourceName), resource.TestCheckResourceAttr(resourceName, "report_name", reportName), @@ -150,7 +180,6 @@ func TestAccAwsCurReportDefinition_refresh(t *testing.T) { s3BucketResourceName := "aws_s3_bucket.test" reportName := acctest.RandomWithPrefix("tf_acc_test") bucketName := fmt.Sprintf("tf-test-bucket-%d", acctest.RandInt()) - bucketRegion := "us-east-1" bucketPrefix := "" format := "textORcsv" compression := "GZIP" @@ -164,7 +193,7 @@ func TestAccAwsCurReportDefinition_refresh(t *testing.T) { CheckDestroy: testAccCheckAwsCurReportDefinitionDestroy, Steps: []resource.TestStep{ { - Config: testAccAwsCurReportDefinitionConfig_basic(reportName, bucketName, bucketPrefix, bucketRegion, format, compression, additionalArtifacts, refreshClosedReports, reportVersioning), + Config: testAccAwsCurReportDefinitionConfig_additional(reportName, bucketName, bucketPrefix, format, compression, additionalArtifacts, refreshClosedReports, reportVersioning), Check: resource.ComposeTestCheckFunc( testAccCheckAwsCurReportDefinitionExists(resourceName), resource.TestCheckResourceAttr(resourceName, "report_name", reportName), @@ -193,7 +222,6 @@ func TestAccAwsCurReportDefinition_overwrite(t *testing.T) { s3BucketResourceName := "aws_s3_bucket.test" reportName := acctest.RandomWithPrefix("tf_acc_test") bucketName := fmt.Sprintf("tf-test-bucket-%d", acctest.RandInt()) - bucketRegion := "us-east-1" bucketPrefix := "" format := "textORcsv" compression := "GZIP" @@ -207,7 +235,7 @@ func TestAccAwsCurReportDefinition_overwrite(t *testing.T) { CheckDestroy: testAccCheckAwsCurReportDefinitionDestroy, Steps: []resource.TestStep{ { - Config: testAccAwsCurReportDefinitionConfig_basic(reportName, bucketName, bucketPrefix, bucketRegion, format, compression, additionalArtifacts, refreshClosedReports, reportVersioning), + Config: testAccAwsCurReportDefinitionConfig_additional(reportName, bucketName, bucketPrefix, format, compression, additionalArtifacts, refreshClosedReports, reportVersioning), Check: resource.ComposeTestCheckFunc( testAccCheckAwsCurReportDefinitionExists(resourceName), resource.TestCheckResourceAttr(resourceName, "report_name", reportName), @@ -286,7 +314,67 @@ func testAccPreCheckAWSCur(t *testing.T) { } // note: cur report definitions are currently only supported in us-east-1 -func testAccAwsCurReportDefinitionConfig_basic(reportName string, bucketName string, bucketPrefix string, bucketRegion string, format string, compression string, additionalArtifacts []string, refreshClosedReports bool, reportVersioning string) string { +func testAccAwsCurReportDefinitionConfig_basic(reportName string, bucketName string) string { + return fmt.Sprintf(` +provider "aws" { + region = "us-east-1" +} +resource "aws_s3_bucket" "test" { + bucket = "%[2]s" + acl = "private" + force_destroy = true +} +resource "aws_s3_bucket_policy" "test" { + bucket = aws_s3_bucket.test.id + + policy = < 0 { From 46dae8aca9b18d15f5e9358b48779808988dd46f Mon Sep 17 00:00:00 2001 From: Robert Bruce Date: Fri, 28 Aug 2020 09:13:50 +0100 Subject: [PATCH 08/10] Split the TestAccDataSourceAwsCurReportDefinition tests to contain original and new functionality into separate test --- ...a_source_aws_cur_report_definition_test.go | 102 ++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/aws/data_source_aws_cur_report_definition_test.go b/aws/data_source_aws_cur_report_definition_test.go index baa2705a7ca..104e32dfa07 100644 --- a/aws/data_source_aws_cur_report_definition_test.go +++ b/aws/data_source_aws_cur_report_definition_test.go @@ -28,6 +28,40 @@ func TestAccDataSourceAwsCurReportDefinition_basic(t *testing.T) { Steps: []resource.TestStep{ { Config: testAccDataSourceAwsCurReportDefinitionConfig_basic(reportName, bucketName), + Check: resource.ComposeTestCheckFunc( + testAccDataSourceAwsCurReportDefinitionCheckExists(datasourceName, resourceName), + resource.TestCheckResourceAttrPair(datasourceName, "report_name", resourceName, "report_name"), + resource.TestCheckResourceAttrPair(datasourceName, "time_unit", resourceName, "time_unit"), + resource.TestCheckResourceAttrPair(datasourceName, "compression", resourceName, "compression"), + resource.TestCheckResourceAttrPair(datasourceName, "additional_schema_elements.#", resourceName, "additional_schema_elements.#"), + resource.TestCheckResourceAttrPair(datasourceName, "s3_bucket", resourceName, "s3_bucket"), + resource.TestCheckResourceAttrPair(datasourceName, "s3_prefix", resourceName, "s3_prefix"), + resource.TestCheckResourceAttrPair(datasourceName, "s3_region", resourceName, "s3_region"), + resource.TestCheckResourceAttrPair(datasourceName, "additional_artifacts.#", resourceName, "additional_artifacts.#"), + ), + }, + }, + }) +} + +func TestAccDataSourceAwsCurReportDefinition_additional(t *testing.T) { + oldvar := os.Getenv("AWS_DEFAULT_REGION") + os.Setenv("AWS_DEFAULT_REGION", "us-east-1") + defer os.Setenv("AWS_DEFAULT_REGION", oldvar) + + resourceName := "aws_cur_report_definition.test" + datasourceName := "data.aws_cur_report_definition.test" + + reportName := acctest.RandomWithPrefix("tf_acc_test") + bucketName := fmt.Sprintf("tf-test-bucket-%d", acctest.RandInt()) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t); testAccPreCheckAWSCur(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAwsCurReportDefinitionDestroy, + Steps: []resource.TestStep{ + { + Config: testAccDataSourceAwsCurReportDefinitionConfig_additional(reportName, bucketName), Check: resource.ComposeTestCheckFunc( testAccDataSourceAwsCurReportDefinitionCheckExists(datasourceName, resourceName), resource.TestCheckResourceAttrPair(datasourceName, "report_name", resourceName, "report_name"), @@ -109,6 +143,74 @@ resource "aws_s3_bucket_policy" "test" { POLICY } +resource "aws_cur_report_definition" "test" { + depends_on = [aws_s3_bucket_policy.test] # needed to avoid "ValidationException: Failed to verify customer bucket permission." + + report_name = "%[1]s" + time_unit = "DAILY" + format = "textORcsv" + compression = "GZIP" + additional_schema_elements = ["RESOURCES"] + s3_bucket = aws_s3_bucket.test.id + s3_prefix = "" + s3_region = aws_s3_bucket.test.region + additional_artifacts = ["REDSHIFT", "QUICKSIGHT"] +} + +data "aws_cur_report_definition" "test" { + report_name = aws_cur_report_definition.test.report_name +} +`, reportName, bucketName) +} + +func testAccDataSourceAwsCurReportDefinitionConfig_additional(reportName string, bucketName string) string { + return fmt.Sprintf(` +provider "aws" { + region = "us-east-1" +} + +data "aws_billing_service_account" "test" {} + +resource "aws_s3_bucket" "test" { + bucket = "%[2]s" + acl = "private" + force_destroy = true +} + +resource "aws_s3_bucket_policy" "test" { + bucket = aws_s3_bucket.test.id + + policy = < Date: Fri, 28 Aug 2020 18:31:01 +0100 Subject: [PATCH 09/10] Moved property combination check for aws_cur_report_definition resource to external function --- aws/resource_aws_cur_report_definition.go | 160 +++++++------ ...resource_aws_cur_report_definition_test.go | 214 ++++++++++++++++++ 2 files changed, 306 insertions(+), 68 deletions(-) diff --git a/aws/resource_aws_cur_report_definition.go b/aws/resource_aws_cur_report_definition.go index a16d23f78f4..596205a550e 100644 --- a/aws/resource_aws_cur_report_definition.go +++ b/aws/resource_aws_cur_report_definition.go @@ -2,11 +2,12 @@ package aws import ( "fmt" + "log" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/costandusagereportservice" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" - "log" ) func resourceAwsCurReportDefinition() *schema.Resource { @@ -122,76 +123,25 @@ func resourceAwsCurReportDefinitionCreate(d *schema.ResourceData, meta interface prefix := aws.String(d.Get("s3_prefix").(string)) reportVersioning := aws.String(d.Get("report_versioning").(string)) - hasAthena := false - - // perform various combination checks, AWS API unhelpfully just returns an empty ValidationException - // these combinations have been determined from the Create Report AWS Console Web Form - for _, artifact := range additionalArtifacts { - if *artifact == costandusagereportservice.AdditionalArtifactAthena { - hasAthena = true - break - } + // had some pointer addressing issues + additionalArtifactsList := make([]string, 0) + for i := 0; i < len(additionalArtifacts); i++ { + var artifact string + artifact = *additionalArtifacts[i] + additionalArtifactsList = append(additionalArtifactsList, artifact) } - if hasAthena { - if len(additionalArtifacts) > 1 { - return fmt.Errorf( - "When %s exists within additional_artifacts, no other artifact type can be declared", - costandusagereportservice.AdditionalArtifactAthena, - ) - } - - if len(*prefix) == 0 { - return fmt.Errorf( - "When %s exists within additional_artifacts, prefix cannot be empty", - costandusagereportservice.AdditionalArtifactAthena, - ) - } + err := checkAwsCurReportDefinitionPropertyCombination( + additionalArtifactsList, + *compression, + *format, + *prefix, + *reportVersioning, + ) - if *reportVersioning != costandusagereportservice.ReportVersioningOverwriteReport { - return fmt.Errorf( - "When %s exists within additional_artifacts, report_versioning must be %s", - costandusagereportservice.AdditionalArtifactAthena, - costandusagereportservice.ReportVersioningOverwriteReport, - ) - } - } - - if *format == costandusagereportservice.ReportFormatParquet { - if *compression != costandusagereportservice.CompressionFormatParquet { - return fmt.Errorf( - "When format is %s, compression must also be %s", - costandusagereportservice.ReportFormatParquet, - costandusagereportservice.CompressionFormatParquet, - ) - } - } else { - if *compression == costandusagereportservice.CompressionFormatParquet { - return fmt.Errorf( - "When format is %s, format must not be %s", - *format, - costandusagereportservice.CompressionFormatParquet, - ) - } - } - - if hasAthena && (*format != costandusagereportservice.ReportFormatParquet) { - return fmt.Errorf( - "When %s exists within additional_artifacts, both format and compression must be %s", - costandusagereportservice.AdditionalArtifactAthena, - costandusagereportservice.ReportFormatParquet, - ) - } - - if !hasAthena && len(additionalArtifacts) > 1 && (*format == costandusagereportservice.ReportFormatParquet) { - return fmt.Errorf( - "When additional_artifacts includes %s and/or %s, format must not be %s", - costandusagereportservice.AdditionalArtifactQuicksight, - costandusagereportservice.AdditionalArtifactRedshift, - costandusagereportservice.ReportFormatParquet, - ) + if err != nil { + return err } - // end checks reportName := d.Get("report_name").(string) @@ -214,7 +164,7 @@ func resourceAwsCurReportDefinitionCreate(d *schema.ResourceData, meta interface } log.Printf("[DEBUG] Creating AWS Cost and Usage Report Definition : %v", reportDefinitionInput) - _, err := conn.PutReportDefinition(reportDefinitionInput) + _, err = conn.PutReportDefinition(reportDefinitionInput) if err != nil { return fmt.Errorf("Error creating AWS Cost And Usage Report Definition: %s", err) } @@ -283,3 +233,77 @@ func describeCurReportDefinition(conn *costandusagereportservice.CostandUsageRep } return matchingReportDefinition, nil } + +func checkAwsCurReportDefinitionPropertyCombination(additionalArtifacts []string, compression string, format string, prefix string, reportVersioning string) error { + // perform various combination checks, AWS API unhelpfully just returns an empty ValidationException + // these combinations have been determined from the Create Report AWS Console Web Form + + hasAthena := false + + for _, artifact := range additionalArtifacts { + if artifact == costandusagereportservice.AdditionalArtifactAthena { + hasAthena = true + break + } + } + + if hasAthena { + if len(additionalArtifacts) > 1 { + return fmt.Errorf( + "When %s exists within additional_artifacts, no other artifact type can be declared", + costandusagereportservice.AdditionalArtifactAthena, + ) + } + + if len(prefix) == 0 { + return fmt.Errorf( + "When %s exists within additional_artifacts, prefix cannot be empty", + costandusagereportservice.AdditionalArtifactAthena, + ) + } + + if reportVersioning != costandusagereportservice.ReportVersioningOverwriteReport { + return fmt.Errorf( + "When %s exists within additional_artifacts, report_versioning must be %s", + costandusagereportservice.AdditionalArtifactAthena, + costandusagereportservice.ReportVersioningOverwriteReport, + ) + } + + if format != costandusagereportservice.ReportFormatParquet { + return fmt.Errorf( + "When %s exists within additional_artifacts, both format and compression must be %s", + costandusagereportservice.AdditionalArtifactAthena, + costandusagereportservice.ReportFormatParquet, + ) + } + } else if len(additionalArtifacts) > 0 && (format == costandusagereportservice.ReportFormatParquet) { + return fmt.Errorf( + "When additional_artifacts includes %s and/or %s, format must not be %s", + costandusagereportservice.AdditionalArtifactQuicksight, + costandusagereportservice.AdditionalArtifactRedshift, + costandusagereportservice.ReportFormatParquet, + ) + } + + if format == costandusagereportservice.ReportFormatParquet { + if compression != costandusagereportservice.CompressionFormatParquet { + return fmt.Errorf( + "When format is %s, compression must also be %s", + costandusagereportservice.ReportFormatParquet, + costandusagereportservice.CompressionFormatParquet, + ) + } + } else { + if compression == costandusagereportservice.CompressionFormatParquet { + return fmt.Errorf( + "When format is %s, compression must not be %s", + format, + costandusagereportservice.CompressionFormatParquet, + ) + } + } + // end checks + + return nil +} diff --git a/aws/resource_aws_cur_report_definition_test.go b/aws/resource_aws_cur_report_definition_test.go index 784eabb3f27..15e518fbb9a 100644 --- a/aws/resource_aws_cur_report_definition_test.go +++ b/aws/resource_aws_cur_report_definition_test.go @@ -443,3 +443,217 @@ resource "aws_cur_report_definition" "test" { } `, reportName, bucketName, bucketPrefix, format, compression, artifactsStr, refreshClosedReports, reportVersioning) } + +func TestCheckAwsCurReportDefinitionPropertyCombination(t *testing.T) { + type propertyCombinationTestCase struct { + additionalArtifacts []string + compression string + format string + prefix string + reportVersioning string + shouldError bool + } + + testCases := map[string]propertyCombinationTestCase{ + "TestAthenaAndAdditionalArtifacts": { + additionalArtifacts: []string{ + costandusagereportservice.AdditionalArtifactAthena, + costandusagereportservice.AdditionalArtifactRedshift, + }, + compression: costandusagereportservice.CompressionFormatZip, + format: costandusagereportservice.ReportFormatTextOrcsv, + prefix: "prefix/", + reportVersioning: costandusagereportservice.ReportVersioningCreateNewReport, + shouldError: true, + }, + "TestAthenaAndEmptyPrefix": { + additionalArtifacts: []string{ + costandusagereportservice.AdditionalArtifactAthena, + }, + compression: costandusagereportservice.CompressionFormatZip, + format: costandusagereportservice.ReportFormatTextOrcsv, + prefix: "", + reportVersioning: costandusagereportservice.ReportVersioningCreateNewReport, + shouldError: true, + }, + "TestAthenaAndOverrideReport": { + additionalArtifacts: []string{ + costandusagereportservice.AdditionalArtifactAthena, + }, + compression: costandusagereportservice.CompressionFormatZip, + format: costandusagereportservice.ReportFormatTextOrcsv, + prefix: "prefix/", + reportVersioning: costandusagereportservice.ReportVersioningCreateNewReport, + shouldError: true, + }, + "TestAthenaWithNonParquetFormat": { + additionalArtifacts: []string{ + costandusagereportservice.AdditionalArtifactAthena, + }, + compression: costandusagereportservice.CompressionFormatParquet, + format: costandusagereportservice.ReportFormatTextOrcsv, + prefix: "prefix/", + reportVersioning: costandusagereportservice.ReportVersioningCreateNewReport, + shouldError: true, + }, + "TestParquetFormatWithoutParquetCompression": { + additionalArtifacts: []string{ + costandusagereportservice.AdditionalArtifactAthena, + }, + compression: costandusagereportservice.CompressionFormatZip, + format: costandusagereportservice.ReportFormatParquet, + prefix: "prefix/", + reportVersioning: costandusagereportservice.ReportVersioningCreateNewReport, + shouldError: true, + }, + "TestCSVFormatWithParquetCompression": { + additionalArtifacts: []string{ + costandusagereportservice.AdditionalArtifactAthena, + }, + compression: costandusagereportservice.CompressionFormatParquet, + format: costandusagereportservice.ReportFormatTextOrcsv, + prefix: "prefix/", + reportVersioning: costandusagereportservice.ReportVersioningCreateNewReport, + shouldError: true, + }, + "TestRedshiftWithParquetformat": { + additionalArtifacts: []string{ + costandusagereportservice.AdditionalArtifactRedshift, + }, + compression: costandusagereportservice.CompressionFormatParquet, + format: costandusagereportservice.ReportFormatParquet, + prefix: "prefix/", + reportVersioning: costandusagereportservice.ReportVersioningCreateNewReport, + shouldError: true, + }, + "TestQuicksightWithParquetformat": { + additionalArtifacts: []string{ + costandusagereportservice.AdditionalArtifactQuicksight, + }, + compression: costandusagereportservice.CompressionFormatParquet, + format: costandusagereportservice.ReportFormatParquet, + prefix: "prefix/", + reportVersioning: costandusagereportservice.ReportVersioningCreateNewReport, + shouldError: true, + }, + "TestAthenaValidCombination": { + additionalArtifacts: []string{ + costandusagereportservice.AdditionalArtifactAthena, + }, + compression: costandusagereportservice.CompressionFormatParquet, + format: costandusagereportservice.ReportFormatParquet, + prefix: "prefix/", + reportVersioning: costandusagereportservice.ReportVersioningOverwriteReport, + shouldError: false, + }, + "TestRedshiftWithGzipedOverwrite": { + additionalArtifacts: []string{ + costandusagereportservice.AdditionalArtifactRedshift, + }, + compression: costandusagereportservice.CompressionFormatGzip, + format: costandusagereportservice.ReportFormatTextOrcsv, + prefix: "prefix/", + reportVersioning: costandusagereportservice.ReportVersioningOverwriteReport, + shouldError: false, + }, + "TestRedshiftWithZippedOverwrite": { + additionalArtifacts: []string{ + costandusagereportservice.AdditionalArtifactRedshift, + }, + compression: costandusagereportservice.CompressionFormatZip, + format: costandusagereportservice.ReportFormatTextOrcsv, + prefix: "", + reportVersioning: costandusagereportservice.ReportVersioningOverwriteReport, + shouldError: false, + }, + "TestRedshiftWithGzipedCreateNew": { + additionalArtifacts: []string{ + costandusagereportservice.AdditionalArtifactRedshift, + }, + compression: costandusagereportservice.CompressionFormatGzip, + format: costandusagereportservice.ReportFormatTextOrcsv, + prefix: "", + reportVersioning: costandusagereportservice.ReportVersioningCreateNewReport, + shouldError: false, + }, + "TestRedshiftWithZippedCreateNew": { + additionalArtifacts: []string{ + costandusagereportservice.AdditionalArtifactRedshift, + }, + compression: costandusagereportservice.CompressionFormatZip, + format: costandusagereportservice.ReportFormatTextOrcsv, + prefix: "", + reportVersioning: costandusagereportservice.ReportVersioningCreateNewReport, + shouldError: false, + }, + "TestQuicksightWithGzipedOverwrite": { + additionalArtifacts: []string{ + costandusagereportservice.AdditionalArtifactQuicksight, + }, + compression: costandusagereportservice.CompressionFormatGzip, + format: costandusagereportservice.ReportFormatTextOrcsv, + prefix: "", + reportVersioning: costandusagereportservice.ReportVersioningOverwriteReport, + shouldError: false, + }, + "TestQuicksightWithZippedOverwrite": { + additionalArtifacts: []string{ + costandusagereportservice.AdditionalArtifactQuicksight, + }, + compression: costandusagereportservice.CompressionFormatZip, + format: costandusagereportservice.ReportFormatTextOrcsv, + prefix: "", + reportVersioning: costandusagereportservice.ReportVersioningOverwriteReport, + shouldError: false, + }, + "TestQuicksightWithGzipedCreateNew": { + additionalArtifacts: []string{ + costandusagereportservice.AdditionalArtifactQuicksight, + }, + compression: costandusagereportservice.CompressionFormatGzip, + format: costandusagereportservice.ReportFormatTextOrcsv, + prefix: "", + reportVersioning: costandusagereportservice.ReportVersioningCreateNewReport, + shouldError: false, + }, + "TestQuicksightWithZippedCreateNew": { + additionalArtifacts: []string{ + costandusagereportservice.AdditionalArtifactQuicksight, + }, + compression: costandusagereportservice.CompressionFormatZip, + format: costandusagereportservice.ReportFormatTextOrcsv, + prefix: "", + reportVersioning: costandusagereportservice.ReportVersioningCreateNewReport, + shouldError: false, + }, + "TestMultipleArtifacts": { + additionalArtifacts: []string{ + costandusagereportservice.AdditionalArtifactRedshift, + costandusagereportservice.AdditionalArtifactQuicksight, + }, + compression: costandusagereportservice.CompressionFormatGzip, + format: costandusagereportservice.ReportFormatTextOrcsv, + prefix: "prefix/", + reportVersioning: costandusagereportservice.ReportVersioningOverwriteReport, + shouldError: false, + }, + } + + for name, tCase := range testCases { + t.Run(name, func(t *testing.T) { + err := checkAwsCurReportDefinitionPropertyCombination( + tCase.additionalArtifacts, + tCase.compression, + tCase.format, + tCase.prefix, + tCase.reportVersioning, + ) + + if tCase.shouldError && err == nil { + t.Fail() + } else if !tCase.shouldError && err != nil { + t.Fail() + } + }) + } +} From 1db063b72c1d62b5ca1d2d56b06ff71c2803f40d Mon Sep 17 00:00:00 2001 From: Robert Bruce Date: Fri, 28 Aug 2020 18:41:23 +0100 Subject: [PATCH 10/10] Corrected golangci-lint issues --- aws/resource_aws_cur_report_definition.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/aws/resource_aws_cur_report_definition.go b/aws/resource_aws_cur_report_definition.go index 596205a550e..7be3a23fc90 100644 --- a/aws/resource_aws_cur_report_definition.go +++ b/aws/resource_aws_cur_report_definition.go @@ -123,12 +123,9 @@ func resourceAwsCurReportDefinitionCreate(d *schema.ResourceData, meta interface prefix := aws.String(d.Get("s3_prefix").(string)) reportVersioning := aws.String(d.Get("report_versioning").(string)) - // had some pointer addressing issues additionalArtifactsList := make([]string, 0) for i := 0; i < len(additionalArtifacts); i++ { - var artifact string - artifact = *additionalArtifacts[i] - additionalArtifactsList = append(additionalArtifactsList, artifact) + additionalArtifactsList = append(additionalArtifactsList, *additionalArtifacts[i]) } err := checkAwsCurReportDefinitionPropertyCombination(