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.

Sign up for GitHub

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

resource_aws_kinesis_firehose_delivery_stream: Avoid invalid state on refresh (cont.) #9103

Merged
merged 4 commits into from
Aug 20, 2019
Merged

resource_aws_kinesis_firehose_delivery_stream: Avoid invalid state on refresh (cont.) #9103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ef059b2
resource_aws_kinesis_firehose_delivery_stream: Avoid invalid state on…
Jun 17, 2019
560cf07
Fix lint errors.
Jun 17, 2019
c821503
resource/aws_kinesis_firehose_delivery_stream: Changed a few API call…
Jun 24, 2019
d90aaf3
resource/aws_kinesis_firehose_delivery_stream: Added acceptance test …
Jun 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 50 additions & 9 deletions aws/resource_aws_kinesis_firehose_delivery_stream.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,11 +328,25 @@ func flattenFirehoseDataFormatConversionConfiguration(dfcc *firehose.DataFormatC
return []map[string]interface{}{}
}

enabled := aws.BoolValue(dfcc.Enabled)
ifc := flattenFirehoseInputFormatConfiguration(dfcc.InputFormatConfiguration)
ofc := flattenFirehoseOutputFormatConfiguration(dfcc.OutputFormatConfiguration)
sc := flattenFirehoseSchemaConfiguration(dfcc.SchemaConfiguration)

// The AWS SDK can represent "no data format conversion configuration" in two ways:
// 1. With a nil value
// 2. With enabled set to false and nil for ALL the config sections.
// These two cases are equivalent so we use the same state description, to avoid diffs.
if !enabled && len(ifc) == 0 && len(ofc) == 0 && len(sc) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

After various testing and investigation, even though we prefer to not introduce logic like this since it changes the behavior of the configuration when its returned from the API, this seems to be one of the few options we have for now since the configuration block enabled attribute has Default: true. 👍

log.Printf("Found ambiguous AWS response")
Copy link
Contributor

Choose a reason for hiding this comment

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

Extraneous log line can be removed

return []map[string]interface{}{}
}

m := map[string]interface{}{
"enabled": aws.BoolValue(dfcc.Enabled),
"input_format_configuration": flattenFirehoseInputFormatConfiguration(dfcc.InputFormatConfiguration),
"output_format_configuration": flattenFirehoseOutputFormatConfiguration(dfcc.OutputFormatConfiguration),
"schema_configuration": flattenFirehoseSchemaConfiguration(dfcc.SchemaConfiguration),
"enabled": enabled,
"input_format_configuration": ifc,
"output_format_configuration": ofc,
"schema_configuration": sc,
}

return []map[string]interface{}{m}
Expand Down Expand Up @@ -531,6 +545,16 @@ func flattenProcessingConfiguration(pc *firehose.ProcessingConfiguration, roleAr
return []map[string]interface{}{}
}

enabled := aws.BoolValue(pc.Enabled)

// The AWS SDK can represent "no processing configuration" in two ways:
// 1. With a nil value
// 2. With an empty processor list and enabled set to false.
// These are equivalent so we use the same state description, to avoid diffs.
if !enabled && len(pc.Processors) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typically, if the configuration block contains a nested "enabled" or similar attribute that we appropriately handle the default case, we prefer to actually pass through the API response into the state as-is (e.g. 1 list item and enabled = false) and then introduce DiffSuppressFunc: suppressMissingOptionalConfigurationBlock, on the configuration block attribute. This means the Terraform resource can accept a configuration with enabled = false and no processors without showing a difference, while also ignoring the configuration block if all the underlying attribute defaults match.

This change was good to revert back to how it was. If for some reason this does need to be changed, we should add a covering acceptance test to show the errant behavior in a future change request. 👍

return []map[string]interface{}{}
}

processingConfiguration := make([]map[string]interface{}, 1)

// It is necessary to explicitly filter this out
Expand Down Expand Up @@ -571,7 +595,7 @@ func flattenProcessingConfiguration(pc *firehose.ProcessingConfiguration, roleAr
}
}
processingConfiguration[0] = map[string]interface{}{
"enabled": aws.BoolValue(pc.Enabled),
"enabled": enabled,
"processors": processors,
}
return processingConfiguration
Expand Down Expand Up @@ -1402,8 +1426,12 @@ func createExtendedS3Config(d *schema.ResourceData) *firehose.ExtendedS3Destinat
configuration.CloudWatchLoggingOptions = extractCloudWatchLoggingConfiguration(s3)
}

if v, ok := s3["error_output_prefix"]; ok && v.(string) != "" {
if v, ok := s3["error_output_prefix"]; ok {
configuration.ErrorOutputPrefix = aws.String(v.(string))
} else {
// It is possible to just pass nil here, but this seems to be the
Copy link
Contributor

Choose a reason for hiding this comment

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

We haven't found an AWS API that has required this additional logic so we would prefer to leave this in its original form. If there is some errant behavior with the original logic, let's add a covering acceptance test to show the errant behavior in a future change request. 👍

// canonical form that AWS uses, and is less likely to produce diffs.
configuration.ErrorOutputPrefix = aws.String("")
}

if s3BackupMode, ok := s3["s3_backup_mode"]; ok {
Expand Down Expand Up @@ -1487,8 +1515,12 @@ func updateExtendedS3Config(d *schema.ResourceData) *firehose.ExtendedS3Destinat
configuration.CloudWatchLoggingOptions = extractCloudWatchLoggingConfiguration(s3)
}

if v, ok := s3["error_output_prefix"]; ok && v.(string) != "" {
if v, ok := s3["error_output_prefix"]; ok {
configuration.ErrorOutputPrefix = aws.String(v.(string))
} else {
// It is possible to just pass nil here, but this seems to be the
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. 👍

// canonical form that AWS uses, and is less likely to produce diffs.
configuration.ErrorOutputPrefix = aws.String("")
}

if s3BackupMode, ok := s3["s3_backup_mode"]; ok {
Expand All @@ -1501,7 +1533,11 @@ func updateExtendedS3Config(d *schema.ResourceData) *firehose.ExtendedS3Destinat

func expandFirehoseDataFormatConversionConfiguration(l []interface{}) *firehose.DataFormatConversionConfiguration {
if len(l) == 0 || l[0] == nil {
return nil
// It is possible to just pass nil here, but this seems to be the
Copy link
Contributor

Choose a reason for hiding this comment

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

We typically do not need to normalize information when sending requests to the API. This doesn't appear to hurt anything here though, so will leave this in for now.

// canonical form that AWS uses, and is less likely to produce diffs.
return &firehose.DataFormatConversionConfiguration{
Enabled: aws.Bool(false),
}
}

m := l[0].(map[string]interface{})
Expand Down Expand Up @@ -1676,7 +1712,12 @@ func expandFirehoseSchemaConfiguration(l []interface{}) *firehose.SchemaConfigur
func extractProcessingConfiguration(s3 map[string]interface{}) *firehose.ProcessingConfiguration {
config := s3["processing_configuration"].([]interface{})
if len(config) == 0 {
return nil
// It is possible to just pass nil here, but this seems to be the
// canonical form that AWS uses, and is less likely to produce diffs.
return &firehose.ProcessingConfiguration{
Enabled: aws.Bool(false),
Processors: []*firehose.Processor{},
}
}

processingConfiguration := config[0].(map[string]interface{})
Expand Down
68 changes: 68 additions & 0 deletions aws/resource_aws_kinesis_firehose_delivery_stream_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,60 @@ func TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConf
})
}

func TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_ExternalUpdate(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you very much for adding this acceptance test! 💯

var stream firehose.DeliveryStreamDescription
rInt := acctest.RandInt()
rName := acctest.RandomWithPrefix("tf-acc-test")
resourceName := "aws_kinesis_firehose_delivery_stream.test"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckKinesisFirehoseDeliveryStreamDestroy_ExtendedS3,
Steps: []resource.TestStep{
{
Config: testAccKinesisFirehoseDeliveryStreamConfig_ExtendedS3_ExternalUpdate(rName, rInt),
Check: resource.ComposeTestCheckFunc(
testAccCheckKinesisFirehoseDeliveryStreamExists(resourceName, &stream),
resource.TestCheckResourceAttr(resourceName, "extended_s3_configuration.#", "1"),
resource.TestCheckResourceAttr(resourceName, "extended_s3_configuration.0.data_format_conversion_configuration.#", "0"),
resource.TestCheckResourceAttr(resourceName, "extended_s3_configuration.0.processing_configuration.#", "0"),
),
},
{
PreConfig: func() {
conn := testAccProvider.Meta().(*AWSClient).firehoseconn
udi := firehose.UpdateDestinationInput{
DeliveryStreamName: aws.String(rName),
DestinationId: aws.String("destinationId-000000000001"),
CurrentDeliveryStreamVersionId: aws.String("1"),
ExtendedS3DestinationUpdate: &firehose.ExtendedS3DestinationUpdate{
DataFormatConversionConfiguration: &firehose.DataFormatConversionConfiguration{
Enabled: aws.Bool(false),
},
ProcessingConfiguration: &firehose.ProcessingConfiguration{
Enabled: aws.Bool(false),
Processors: []*firehose.Processor{},
},
},
}
_, err := conn.UpdateDestination(&udi)
if err != nil {
t.Fatalf("Unable to update firehose destination: %s", err)
}
},
Config: testAccKinesisFirehoseDeliveryStreamConfig_ExtendedS3_ExternalUpdate(rName, rInt),
Check: resource.ComposeTestCheckFunc(
testAccCheckKinesisFirehoseDeliveryStreamExists(resourceName, &stream),
resource.TestCheckResourceAttr(resourceName, "extended_s3_configuration.#", "1"),
resource.TestCheckResourceAttr(resourceName, "extended_s3_configuration.0.data_format_conversion_configuration.#", "0"),
resource.TestCheckResourceAttr(resourceName, "extended_s3_configuration.0.processing_configuration.#", "0"),
),
},
},
})
}

func TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_Deserializer_Update(t *testing.T) {
var stream firehose.DeliveryStreamDescription
rInt := acctest.RandInt()
Expand Down Expand Up @@ -1640,6 +1694,20 @@ resource "aws_kinesis_firehose_delivery_stream" "test" {
`, rName, rName, rName)
}

func testAccKinesisFirehoseDeliveryStreamConfig_ExtendedS3_ExternalUpdate(rName string, rInt int) string {
return fmt.Sprintf(testAccKinesisFirehoseDeliveryStreamBaseConfig, rInt, rInt, rInt) + fmt.Sprintf(`
resource "aws_kinesis_firehose_delivery_stream" "test" {
destination = "extended_s3"
name = "%s"

extended_s3_configuration {
bucket_arn = "${aws_s3_bucket.bucket.arn}"
role_arn = "${aws_iam_role.firehose.arn}"
}
}
`, rName)
}

func testAccKinesisFirehoseDeliveryStreamConfig_ExtendedS3_DataFormatConversionConfiguration_OpenXJsonSerDe_Empty(rName string, rInt int) string {
return fmt.Sprintf(testAccKinesisFirehoseDeliveryStreamBaseConfig, rInt, rInt, rInt) + fmt.Sprintf(`
resource "aws_glue_catalog_database" "test" {
Expand Down