Support VPC config for Amazon Kinesis Data Firehose#13269
Conversation
DrFaust92
left a comment
There was a problem hiding this comment.
A few minor changes.
can you also add an example in the docs for this?
| return nil | ||
| } | ||
| vpcConfig := config[0].(map[string]interface{}) | ||
| s := vpcConfig["subnet_ids"].(*schema.Set).List() |
There was a problem hiding this comment.
helper func expandStringSet() can be used on vpcConfig["subnet_ids"].(*schema.Set) instead
|
I have addressed the PR comments. |
|
Verified acceptance tests: $ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSKinesisFirehoseDeliveryStream_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -count 1 -parallel 20 -run=TestAccAWSKinesisFirehoseDeliveryStream_basic -timeout 120m
=== RUN TestAccAWSKinesisFirehoseDeliveryStream_basic
=== PAUSE TestAccAWSKinesisFirehoseDeliveryStream_basic
=== CONT TestAccAWSKinesisFirehoseDeliveryStream_basic
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_basic (110.48s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 110.514s
$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -count 1 -parallel 20 -run=TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates -timeout 120m
=== RUN TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates
=== PAUSE TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates
=== CONT TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates
--- FAIL: TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates (2.45s)
resource_aws_elasticsearch_domain_test.go:991: missing IAM Service Linked Role (es.amazonaws.com), please create it in the AWS account and retry
FAIL
FAIL github.com/terraform-providers/terraform-provider-aws/aws 2.487s
$ aws iam create-service-linked-role --aws-service-name es.amazonaws.com
$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -count 1 -parallel 20 -run=TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates -timeout 120m
=== RUN TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates
=== PAUSE TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates
=== CONT TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates (1444.38s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 1444.406s |
|
when could I expect this PR to be merged ? |
|
@ewbankkit , Could you please let us know by when the PR would be merged ? |
|
Hello @ewbankkit @DrFaust92 |
|
Anxiously waiting for the merge too! :D |
|
Hello @breathingdust |
|
Hi @rajholla 👋 This is in our backlog of items to review and we are hoping to get to it soon. We are currently working through roadmap items for this quarter but once that has been completed we'll be in a better position to give feedback and hopefully merge. At the moment we can't give you a timeline, but stay tuned for an update. We appreciate your contributions and your patience! |
|
Hello, change to: The "plan" shows that it will be modified, but after "apply", the change doesn't happen. |
Hello @walterd1969 |
|
The resource should handle this appropriately with a taint (delete/create) when the vpc_config changes. According to the code, this should happen: Not sure why it didn't work as expected for @walterd1969 |
|
Hey folks, any updates on this? We've been using the compiled version of this branch and it seems to be working just fine, but we didn't test the resource recreation handling pointed by @dmccaffery |
|
@walterd1969 Thanks for reporting the issue. It should be fixed now. Can you rebuild and test again? |
|
@DrFaust92 thank you for the review. All comments are addressed. |
|
@breathingdust Since the roadmap you linked is for August - October and does not include this MR, is it safe to assume that this is not coming before end of October ? |
|
Hey @valorl 👋. The roadmap doesn't represent all that we will do in a quarter, but does represent what we are able to commit to. That said, this issue is on our radar and we hope to have an update for you soon. |
|
Hey guys, any updates about this? |
There was a problem hiding this comment.
@rajholla Could you rename this to vpc_configuration to match the AWS API? Thanks.
There was a problem hiding this comment.
@ewbankkit vpc_config is consistent with other resources like eks and lambda
Is there any advantage changing this ?
There was a problem hiding this comment.
For Lambda the AWS API names the field VpcConfig but for EKS it's named ResourceVpcConfig in the API so we are not consistently sticking to the API naming 😄.
Staying with vpc_config is fine.
There was a problem hiding this comment.
Can use flattenStringSet here for consistency with the expandStringSet below.
da0deef to
e09a126
Compare
e09a126 to
3f8303b
Compare
|
@ewbankkit requested changes are in now. |
|
@DrFaust92 Could you try verifying the |
|
Verified in $ AWS_DEFAULT_REGION=us-east-1 make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates -timeout 120m
=== RUN TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates
=== PAUSE TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates
=== CONT TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates (1548.08s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 1548.124s |
|
ah, im actually getting the following: does that even make sense? |
|
@DrFaust92 Yes, I got that originally. aws iam create-service-linked-role --aws-service-name es.amazonaws.comwill fix. |
us-west-2 |
bflad
left a comment
There was a problem hiding this comment.
Looks good 🚀
Output from acceptance testing:
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3KinesisStreamSource (102.19s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3basicWithTags (106.75s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_ParquetSerDe_Empty (118.90s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_OpenXJsonSerDe_Empty (123.17s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3basic (133.80s)
--- FAIL: TestAccAWSKinesisFirehoseDeliveryStream_basic (146.83s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_OrcSerDe_Empty (148.34s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_disappears (149.37s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_HiveJsonSerDe_Empty (149.70s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_Serializer_Update (152.19s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_ExternalUpdate (163.95s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3WithCloudwatchLogging (167.63s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3basic (168.05s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3KmsKeyArn (173.94s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3ConfigUpdates (175.01s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_ProcessingConfiguration_Empty (176.11s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_ErrorOutputPrefix (180.04s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_Deserializer_Update (187.17s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_Enabled (197.56s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_KinesisStreamSource (98.68s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3basicWithSSE (253.21s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_missingProcessingConfiguration (107.01s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_SplunkConfigUpdates (139.39s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3Updates (163.46s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_RedshiftConfigUpdates (340.62s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchConfigUpdates (991.76s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates (1574.83s)
|
This has been released in version 3.5.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks! |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |


Community Note
Closes #13015
Release note for CHANGELOG:
Output from acceptance testing: