-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Support VPC config for Amazon Kinesis Data Firehose #13269
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some changes to tests
@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? |
@@ -1275,6 +1291,39 @@ func resourceAwsKinesisFirehoseDeliveryStream() *schema.Resource { | |||
}, | |||
}, | |||
|
|||
"vpc_config": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rajholla Could you rename this to vpc_configuration
to match the AWS API? Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
"subnet_ids": flattenStringList(description.SubnetIds), | ||
"security_group_ids": flattenStringList(description.SecurityGroupIds), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
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.com will fix. |
us-west-2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: