Add AWS MSK cluster resource #8635

jesseaukeman · 2019-05-14T20:18:01Z

This is a new implementation of the AWS MSK cluster resource.

I realize this overlaps and duplicates #8357. This was a separate and parallel effort undertaken for the company I work for. I apologize if this is bad form, however I think this effort is a bit further along, as the other effort may be stalled on acceptance tests.

implement aws_msk_cluster resource with support for tags
implement acceptance tests

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Fixes #6653
Fixes #8429

Release note for CHANGELOG:

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSMskCluster'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSMskCluster -timeout 120m
=== RUN   TestAccAWSMskCluster_basic
=== PAUSE TestAccAWSMskCluster_basic
=== RUN   TestAccAWSMskCluster_kms
=== PAUSE TestAccAWSMskCluster_kms
=== RUN   TestAccAWSMskCluster_enhancedMonitoring
=== PAUSE TestAccAWSMskCluster_enhancedMonitoring
=== RUN   TestAccAWSMskCluster_tagsUpdate
=== PAUSE TestAccAWSMskCluster_tagsUpdate
=== RUN   TestAccAWSMskCluster_brokerNodes
=== PAUSE TestAccAWSMskCluster_brokerNodes
=== CONT  TestAccAWSMskCluster_basic
=== CONT  TestAccAWSMskCluster_tagsUpdate
=== CONT  TestAccAWSMskCluster_kms
=== CONT  TestAccAWSMskCluster_brokerNodes
=== CONT  TestAccAWSMskCluster_enhancedMonitoring
--- PASS: TestAccAWSMskCluster_enhancedMonitoring (958.48s)
--- PASS: TestAccAWSMskCluster_kms (1062.45s)
--- PASS: TestAccAWSMskCluster_basic (1063.50s)
--- PASS: TestAccAWSMskCluster_brokerNodes (1096.43s)
--- PASS: TestAccAWSMskCluster_tagsUpdate (1130.02s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       1130.671s
...

* implement aws_msk_cluster resource with support for tags * implement acceptance tests

KellerII · 2019-05-19T18:05:13Z

Any idea on when this will be ready/merged?

bflad

Hi @jesseaukeman 👋 Thanks so much for this contribution! It was in great shape. I left some feedback notes below during review but in the interest of getting this resource (and potentially a new resource for managing MSK Configurations) released this week, we handled the items in a followup commit (7cb443c). Thanks again, this will make a bunch of people happy. 🚀

Output from acceptance testing:

--- PASS: TestAccAWSMskCluster_basic (987.37s)
--- PASS: TestAccAWSMskCluster_tagsUpdate (999.03s)
--- PASS: TestAccAWSMskCluster_enhancedMonitoring (1018.97s)
--- PASS: TestAccAWSMskCluster_kms (1042.61s)
--- PASS: TestAccAWSMskCluster_brokerNodes (1074.66s)

bflad · 2019-05-22T09:36:00Z

aws/resource_aws_msk_cluster.go

+		info := v.([]interface{})
+		if len(info) == 1 {
+			if info[0] == nil {
+				return fmt.Errorf("At least one item is expected inside encryption_info")


Instead of returning an error here, we should be able to just move the nil check up a line and only include the EncryptionInfo then. 👍

bflad · 2019-05-22T09:44:08Z

aws/resource_aws_msk_cluster.go

+		out, err = conn.CreateCluster(input)
+
+		if err != nil {
+			if isAWSErr(err, kafka.ErrCodeTooManyRequestsException, "Too Many Requests") {


Rather than introducing resource.Retry() logic, which is missing a isResourceTimeoutError(err) check below (context: #7873), we should move this error code into our SDK client logic to automatically have the SDK retry these requests.

Examples of this code can be found here:

https://github.com/terraform-providers/terraform-provider-aws/blob/bdcf6a2e2fa396a09bd63fb1ce354398f89244aa/aws/config.go#L539-L544

An example implementation for here:

client.kafkaconn.Handlers.Retry.PushBack(func(r *request.Request) { if isAWSErr(r.Error, kafka.ErrCodeTooManyRequestsException, "Too Many Requests") { r.Retryable = aws.Bool(true) } })

We should likely also submit this upstream to the AWS Go SDK to automatically retry and throttle these as well.

bflad · 2019-05-22T09:45:19Z

aws/resource_aws_msk_cluster.go

+	return resourceAwsMskClusterRead(d, meta)
+}
+
+func waitForClusterCreation(conn *kafka.Kafka, arn string) error {


Nit: Since this project currently uses a very flat Go package structure, we should include the service name in the function name here to delineate which type of cluster we are referring to:

Suggested change

func waitForClusterCreation(conn *kafka.Kafka, arn string) error {

func waitForMskClusterCreation(conn *kafka.Kafka, arn string) error {

bflad · 2019-05-22T09:47:49Z

aws/resource_aws_msk_cluster.go

+		return err
+	}
+
+	d.Set("tags", tagsToMapMskCluster(tags))


Two notes:

The tags value is coming from the configuration above with d.Get(), which makes this call extraneous as Terraform will automatically pass through configuration information into the Terraform state. More importantly though, this information should be read back from the API to ensure it was properly configured in the API.

Setting information into the Terraform state should preferably happen during the Read function

Since the Read function does correctly call d.Set() via reading the information from the API for this attribute and the Create function correctly returns the Read function, this is extraneous. 😄

bflad · 2019-05-22T09:49:32Z

aws/resource_aws_msk_cluster.go

+	d.Set("arn", aws.StringValue(cluster.ClusterArn))
+	d.Set("bootstrap_brokers", brokerOut.BootstrapBrokerString)
+
+	d.Set("broker_node_group_info", flattenMskBrokerNodeGroupInfo(cluster.BrokerNodeGroupInfo))


When using d.Set() with aggregate types (TypeList, TypeSet, TypeMap), we should perform error checking to prevent issues where the code is not properly able to set the Terraform state. e.g.

Suggested change

d.Set("broker_node_group_info", flattenMskBrokerNodeGroupInfo(cluster.BrokerNodeGroupInfo))

if err := d.Set("broker_node_group_info", flattenMskBrokerNodeGroupInfo(cluster.BrokerNodeGroupInfo)); err != nil {

return fmt.Errorf("error setting broker_node_group_info: %s", err)

}

bflad · 2019-05-22T10:02:05Z

website/docs/r/msk_cluster.html.markdown

+  Terraform resource for managing an AWS Managed Streaming for Kafka cluster
+---
+
+# aws_msk_cluster


Nit: We're now including Resource: in resource titles 👍

Suggested change

# aws_msk_cluster

# Resource: aws_msk_cluster

bflad · 2019-05-22T10:02:54Z

website/docs/r/msk_cluster.html.markdown

+# aws_msk_cluster
+
+Manages AWS Managed Streaming for Kafka cluster
+


Maintainer Note: Adding the preview callout for this resource documentation

~> NOTE: This AWS service is in Preview and may change before General Availability release. Backwards compatibility is not guaranteed between Terraform AWS Provider releases.

bflad · 2019-05-22T10:03:55Z

website/aws.erb

@@ -1876,6 +1876,16 @@
                    </ul>
                </li>

+                <li>
+                    <a href="#">MSK Resources</a>


Nit: I'm thinking we might want to call out the full name here for folks searching for just "kafka", e.g.

Suggested change

<a href="#">MSK Resources</a>

<a href="#">Managed Streaming for Kafka (MSK) Resources</a>

bflad · 2019-05-22T10:09:45Z

aws/resource_aws_msk_cluster.go

+				MaxItems: 1,
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"encryption_at_rest_kms_id": {


Given the API always returns the ARN, we should just require inputting the ARN here to simplify attribute handling (removing the DiffSuppressFunc) and prevent potential "inconsistent plan" errors in Terraform 0.12 (the new "diffs didn't match during apply" error from Terraform 0.11).

Suggested change

"encryption_at_rest_kms_id": {

"encryption_at_rest_kms_key_arn": {

bflad · 2019-05-22T10:12:00Z

aws/resource_aws_msk_cluster.go

+			State: schema.ImportStatePassthrough,
+		},
+		Timeouts: &schema.ResourceTimeout{
+			Create: schema.DefaultTimeout(45 * time.Minute),


It appears these customizable timeouts are not passed into the waiter functions (e.g. waitForMskClusterCreation) so they should be removed. 👍

Reference: #8635 Output from acceptance testing: ``` --- PASS: TestAccAWSMskCluster_basic (987.37s) --- PASS: TestAccAWSMskCluster_tagsUpdate (999.03s) --- PASS: TestAccAWSMskCluster_enhancedMonitoring (1018.97s) --- PASS: TestAccAWSMskCluster_kms (1042.61s) --- PASS: TestAccAWSMskCluster_brokerNodes (1074.66s) ```

bflad · 2019-05-24T11:24:05Z

This has been released in version 2.12.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-03-29T17:58:15Z

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!

Add AWS MSK cluster resource

6f99432

* implement aws_msk_cluster resource with support for tags * implement acceptance tests

* remove unused test function

640d8a0

dthtvwls mentioned this pull request May 15, 2019

MSK clusters #8357

Closed

jesseaukeman added 2 commits May 15, 2019 14:36

* document custom timeouts for the resource

0b25d8f

* remove debugging comments in tags test

285c515

bflad added new-resource Introduces a new resource. service/kafka Issues and PRs that pertain to the kafka service. labels May 22, 2019

bflad self-assigned this May 22, 2019

bflad reviewed May 22, 2019

View reviewed changes

bflad added this to the v2.12.0 milestone May 22, 2019

bflad merged commit 285c515 into hashicorp:master May 22, 2019

bflad added a commit that referenced this pull request May 22, 2019

Update CHANGELOG for #8635

22be8f9

bflad mentioned this pull request May 22, 2019

Initial add of MSK resources #6809

Closed

2 tasks

ghost locked and limited conversation to collaborators Mar 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AWS MSK cluster resource #8635

Add AWS MSK cluster resource #8635

jesseaukeman commented May 14, 2019

KellerII commented May 19, 2019

bflad left a comment

bflad May 22, 2019

bflad May 22, 2019

bflad May 22, 2019

bflad May 22, 2019

bflad May 22, 2019

bflad May 22, 2019

bflad May 22, 2019

bflad May 22, 2019

bflad May 22, 2019

bflad May 22, 2019

bflad commented May 24, 2019

ghost commented Mar 29, 2020

	func waitForClusterCreation(conn *kafka.Kafka, arn string) error {
	func waitForMskClusterCreation(conn *kafka.Kafka, arn string) error {

-	d.Set("broker_node_group_info", flattenMskBrokerNodeGroupInfo(cluster.BrokerNodeGroupInfo))
+	if err := d.Set("broker_node_group_info", flattenMskBrokerNodeGroupInfo(cluster.BrokerNodeGroupInfo)); err != nil {
+		return fmt.Errorf("error setting broker_node_group_info: %s", err)
+	}

		# aws_msk_cluster

		Manages AWS Managed Streaming for Kafka cluster

	<a href="#">MSK Resources</a>
	<a href="#">Managed Streaming for Kafka (MSK) Resources</a>

	"encryption_at_rest_kms_id": {
	"encryption_at_rest_kms_key_arn": {

Add AWS MSK cluster resource #8635

Add AWS MSK cluster resource #8635

Conversation

jesseaukeman commented May 14, 2019

Community Note

KellerII commented May 19, 2019

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented May 24, 2019

ghost commented Mar 29, 2020