Initial add of MSK resources #6809

jrefi · 2018-12-11T21:54:15Z

Changes proposed in this pull request:

Add aws_msk_cluster resource to support Managed Streams for Kafka.

TODO:

Bubble up cluster connection info to output
[x ] Flesh out acceptance tests
Handle resource updates (currently not supported in MSK API)

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSMskCluster'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSMskCluster -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSMskClusterDataSource
=== PAUSE TestAccAWSMskClusterDataSource
=== RUN   TestAccAWSMskCluster_basic
=== PAUSE TestAccAWSMskCluster_basic
=== RUN   TestAccAWSMskCluster_encryptAtRest
=== PAUSE TestAccAWSMskCluster_encryptAtRest
=== RUN   TestAccAWSMskCluster_brokerMonitoring
=== PAUSE TestAccAWSMskCluster_brokerMonitoring
=== CONT  TestAccAWSMskClusterDataSource
=== CONT  TestAccAWSMskCluster_brokerMonitoring
=== CONT  TestAccAWSMskCluster_encryptAtRest
=== CONT  TestAccAWSMskCluster_basic
--- PASS: TestAccAWSMskCluster_brokerMonitoring (912.39s)
--- PASS: TestAccAWSMskClusterDataSource (917.71s)
--- PASS: TestAccAWSMskCluster_basic (922.73s)
--- PASS: TestAccAWSMskCluster_encryptAtRest (1005.67s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1005.689s

Example Resource configuration:

resource "aws_msk_cluster" "test_cluster" {
	name = "terraform-msk-test-%d"
	broker_count = 3
	broker_instance_type = "kafka.m5.large"
	broker_volume_size = 10
	broker_security_groups =["${aws_security_group.test_sg_a.id}"]
	client_subnets = ["${aws_subnet.test_subnet_a.id}", "${aws_subnet.test_subnet_b.id}", "${aws_subnet.test_subnet_c.id}"]
}

bflad · 2018-12-11T23:49:30Z

Related: #6655

aws/config.go

tomelliff

Overall looks good to me, particularly for a first contribution but there's a few things I picked up on in a short review.

tomelliff · 2019-01-04T11:47:01Z

aws/resource_aws_msk_cluster.go

+}
+
+func resourceAwsMskClusterUpdate(d *schema.ResourceData, meta interface{}) error {
+	// TODO: Figure out update as API calls not yet implemented


If the API doesn't provide update calls then you need to ditch this and just use ForceNew on everything for now. If AWS eventually adds updating a MSK cluster then someone will be able to update this resource to support updates.

As an example, here's the ECS task definition resource before tagging was added (making it no longer just an immutable resource): https://github.com/terraform-providers/terraform-provider-aws/blob/f7c0899ca731c70659eb6b8e40ea520d095ffb1c/aws/resource_aws_ecs_task_definition.go

aws/data_source_aws_mks_cluster.go

aws/resource_aws_msk_cluster.go

tomelliff · 2019-01-04T11:57:27Z

aws/resource_aws_msk_cluster.go

+		Delete: resourceAwsMskClusterDelete,
+		Importer: &schema.ResourceImporter{
+			State: schema.ImportStatePassthrough,
+		},


You're missing a test for this.

Not sure what this actually does or how to test it. Didnt see an example elsewhere in the codebase.

@tomelliff there is only this issue before merging to master?

tomelliff · 2019-01-04T12:02:49Z

aws/data_source_aws_mks_cluster.go

+	d.Set("arn", state.arn)
+	d.Set("status", state.status)
+	d.Set("creation_timestamp", state.creationTimestamp)
+	d.Set("encrypt_rest_arn", state.encryptRestArn)


You've used encrypt_rest_key in the schema here but set this to encrypt_rest_arn which is what you're using elsewhere. Personally I find the encryptRestArn to be slightly confusing without any mention that it's the KMS key ARN and had to check the source to see that that was the case so I'd switch everything over to encrypt_rest_key or some variant on that.

Also it might help to add an acceptance test for this data source even if you are just exercising the same method as the read on the resource just to catch mistakes in the schema like this.

aws/data_source_aws_mks_cluster.go

jrefi · 2019-01-04T14:45:51Z

@tomelliff Thanks for the feedback. As this is my first contribution, I'm still trying to figure out how the whole system works. Your comments are very helpful.

I am a little ignorant about how ForceNew works. Is the user warned before action is taken? I'm concerned about the possibility of data loss occurring if the user changes something minor on their cluster.

tomelliff · 2019-01-04T18:37:50Z

The plan will show a +- against the resource to show that it needs to be destroyed and recreated. There is also a (forces new resource) against any changing attributes that cause Terraform to force a rebuild and it's a very common pattern across a wide set of resources so shouldn't come across as a surprise to the user.

Of course if you set everything as ForceNew and build it you should be able to see the results for yourself.

aws/resource_aws_msk_cluster.go

aws/resource_aws_msk_cluster_test.go

random1st · 2019-02-28T14:59:33Z

website/docs/r/msk_cluster.html.markdown

+* `broker_count` - (Required) Number of broker nodes you want to create in each Availability Zone.
+* `broker_instance_type` - (Required) Instance type for brokers from the m5 family. e.g. kafka.m5.large
+* `broker_volume_size` - (Required) The size of the drive in GiBs.
+* `broker_security_groups` - (Optional) Security groups to attach to broker nodes.


random1st · 2019-02-28T15:00:46Z

aws/resource_aws_msk_cluster.go

+		Delete: resourceAwsMskClusterDelete,
+		Importer: &schema.ResourceImporter{
+			State: schema.ImportStatePassthrough,
+		},


@tomelliff there is only this issue before merging to master?

gavinmbell · 2019-03-26T14:08:29Z

👍 this would be a great thing to have. I fully support this endeavor as I have a project coming on line that would be able to leverage this immediately.

kesensoy · 2019-03-26T16:52:43Z

@gavinmbell I agree, we need to find out how we can encourage the final reviews (like from @mrf and @tomelliff) before this becomes a dead PR. I've also tried posting in the IRC room to no avail.

landondao1 · 2019-03-26T17:40:52Z

+1 This is a really great feature. After AWS went live with MSK, I was pretty excited. I'm an SUPER excited to see this get implemented into terraform to make automated deployments work with MSK.

bflad

Hi @jrefi 👋 Thanks for submitting this. Please see the below initial feedback and let us know if you have any questions or do not have time to implement the items.

bflad · 2019-03-29T04:16:19Z

aws/config.go

@@ -159,6 +159,7 @@ type Config struct {
 	EsEndpoint               string
 	ElbEndpoint              string
 	IamEndpoint              string
+	KafkaEndpoint            string


There are some additional configuration steps required to support customizing service endpoints. We have opted to allow customizing all endpoints (#8096) so this change should be removed from this pull request. 👍

bflad · 2019-03-29T04:16:48Z

aws/provider.go

@@ -937,6 +939,7 @@ func providerConfigure(d *schema.ResourceData) (interface{}, error) {
 		config.ElbEndpoint = endpoints["elb"].(string)
 		config.EsEndpoint = endpoints["es"].(string)
 		config.IamEndpoint = endpoints["iam"].(string)
+		config.KafkaEndpoint = endpoints["kafka"].(string)


As noted above, this change should be removed from this pull request.

bflad · 2019-03-29T04:18:23Z

aws/data_source_aws_msk_cluster.go

+			},
+			"broker_node_group_info": {
+				Type:     schema.TypeString,
+				Optional: true,


Optional: true should be removed/replaced with only Computed: true for this and all the below attributes since they are not configurable to influence the data source lookup.

bflad · 2019-03-29T04:23:52Z

aws/data_source_aws_msk_cluster_test.go

+				Config: config,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckMskClusterExists("aws_msk_cluster.test_cluster", &cluster),
+					resource.TestCheckResourceAttrSet("data.aws_msk_cluster.test_cluster", "arn"),


Data source testing should preferably use resource.TestCheckResourceAttrPair() to ensure values match between the resource and data source, e.g.

Suggested change

resource.TestCheckResourceAttrSet("data.aws_msk_cluster.test_cluster", "arn"),

resource.TestCheckResourceAttrPair("data.aws_msk_cluster.test_cluster", "arn", "aws_msk_cluster.test_cluster", "arn"),

This should be done for all data source attribute checks.

bflad · 2019-03-29T04:27:59Z

aws/data_source_aws_msk_cluster_test.go

+resource "aws_subnet" "test_subnet_a" {
+	vpc_id = "${aws_vpc.test_vpc.id}"
+	cidr_block = "10.1.1.0/24"
+	availability_zone = "us-east-1a"


The acceptance testing framework for the provider runs in us-west-2 by default:

https://github.com/terraform-providers/terraform-provider-aws/blob/ffa6d70f31746ea626abe6a7d3a4398a9a77e4ef/aws/provider_test.go#L180-L186

Hardcoded availability zones should instead be replaced with the aws_availability_zones data source.

bflad · 2019-03-29T05:26:01Z

website/docs/r/msk_cluster.html.markdown

+---
+layout: "aws"
+page_title: "AWS: aws_msk_cluster"
+sidebar_current: "docs-aws-resource-msk-cluster"


This documentation page is missing a sidebar link in a new MSK Resources section of website/aws.erb

bflad · 2019-03-29T05:27:37Z

website/docs/d/msk_cluster.html.markdown

+
+## Attributes Reference
+
+See the [`aws_msk_cluster` resource](/docs/providers/aws/r/msk_cluster.html) for details on the returned attributes.


Data source attributes can easily become out of sync with their resource counterpart since they require a separate schema definition and appropriate d.Set() calls. We prefer to list out the attributes separately.

bflad · 2019-03-29T05:28:16Z

website/docs/r/msk_cluster.html.markdown

+* `broker_instance_type` - (Required) Instance type for brokers from the m5 family. e.g. kafka.m5.large
+* `broker_volume_size` - (Required) The size of the drive in GiBs.
+* `broker_security_groups` - (Required) Security groups to attach to broker nodes.
+* `encrypt_rest_arn` - (Optional) 


This argument is missing documentation.

bflad · 2019-03-29T05:30:32Z

aws/resource_aws_msk_cluster.go

+		},
+		Timeouts: &schema.ResourceTimeout{
+			Create: schema.DefaultTimeout(60 * time.Minute),
+			Update: schema.DefaultTimeout(120 * time.Minute),


This resource does not implement the Update function so this should be removed.

bflad · 2019-03-29T05:31:06Z

website/docs/r/msk_cluster.html.markdown

+* `encrypt_rest_key` - The AWS KMS key used for data encryption.
+* `zookeeper_connect` - Connection string for Zookeeper.
+* `bootstrap_brokers` - A list of brokers that a client application can use to bootstrap.
+


Documentation for the customizable timeouts is missing:

Suggested change

## Timeouts

`aws_msk_cluster` provides the following [Timeouts](/docs/configuration/resources.html#timeouts) configuration options:

* `create` - (Default `60m`) How long to wait for cluster creation.

* `delete` - (Default `120m`) How long to wait for cluster deletion.

kesensoy · 2019-03-29T14:43:16Z

@bflad thank you for the review! I would undertake a lot of the simple fixes myself but I don't want to mess anything up without go knowledge or a working testing env. I'll see if @jrefi still wants to take this one across the finish line!

dthtvwls · 2019-04-03T14:35:41Z

@jrefi If you are low on time/resources, please let me know I would be happy to take a handoff from you and address feedback myself.

jrefi · 2019-04-03T14:38:27Z

@dthtvwls Thanks. Yeah, actually my organization has decided that MSK is not yet a viable option for us and thus this is no longer a priority. If you don't mind cleaning the PR up, I would greatly appreciate it.

kesensoy · 2019-04-04T20:18:35Z

@dthtvwls Please let us know if you will be able to get this merged, else I will continue to search for more contributors! If anyone wants to be reminded of how much people are eagerly awaiting this feature, it is the second most 'reacted to' issue on the platform right now :) #6653

dthtvwls · 2019-04-05T15:29:58Z

@kesensoy I will certainly do what I can. I am working through @bflad's feedback on my own fork. Not really sure of the preferred way to handle the changes here though. Should I create a new PR? If anyone can say, please do, thanks.

bflad · 2019-04-05T18:07:41Z

@dthtvwls a new PR is totally fine (preserving the previous authors commits where possible) 👍

dthtvwls · 2019-04-11T20:50:44Z

@bflad I just left a review. One more question before I make the new PR. Thanks

move brokers call go fmt

dthtvwls · 2019-04-17T15:30:13Z

Please see #8357 which now supersedes this request.

move brokers call go fmt

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

We just merged in #8635 which will release with version 2.12.0 of the Terraform AWS Provider. Thanks @jrefi and @dthtvwls for your efforts here!

ghost · 2020-03-30T17:11:16Z

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!

bflad added new-resource Introduces a new resource. service/kafka Issues and PRs that pertain to the kafka service. labels Dec 11, 2018

jrefi changed the title ~~Initial add of MSK reources~~ Initial add of MSK resources Dec 12, 2018

jrefi mentioned this pull request Dec 12, 2018

Feature Request: Managed Streaming for Kafka #6653

Closed

mrf reviewed Dec 13, 2018

View reviewed changes

aws/config.go Outdated Show resolved Hide resolved

tomelliff reviewed Jan 4, 2019

View reviewed changes

mrf reviewed Jan 14, 2019

View reviewed changes

aws/resource_aws_msk_cluster.go Outdated Show resolved Hide resolved

mrf reviewed Jan 14, 2019

View reviewed changes

aws/resource_aws_msk_cluster.go Outdated Show resolved Hide resolved

mrf reviewed Jan 14, 2019

View reviewed changes

aws/resource_aws_msk_cluster_test.go Outdated Show resolved Hide resolved

ghost added the documentation Introduces or discusses updates to documentation. label Jan 23, 2019

matthewdooler approved these changes Feb 27, 2019

View reviewed changes

bflad mentioned this pull request Mar 11, 2019

Managed Streaming for Kafka Resource #6655

Closed

Justin Refi and others added 10 commits March 14, 2019 09:39

Initial add of MSK reources

0fa28a9

Initial MSK implementation with tests

674ca26

Working basic functionality

fe8dc82

Mostly working acceptance tests

440beab

Working acceptance tests

0dd35f3

Fix linting

260fb69

Respond to PR comments

a2038a6

More PR comment responses

ecd127f

Adding in website documentation for new MSK resource.

3d9a08b

Make KafkaVersion required. Make monitoring optional.

1296a3b

Justin Refi and others added 2 commits March 14, 2019 09:40

Make security groups required

8e09533

fix example description

9b21435

jrefi force-pushed the msk-cluster-resource branch from 3ba14b5 to 9b21435 Compare March 14, 2019 15:18

random1st approved these changes Mar 14, 2019

View reviewed changes

bflad reviewed Mar 29, 2019

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Mar 29, 2019

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Mar 29, 2019

dthtvwls added a commit to dthtvwls/terraform-provider-aws that referenced this pull request Apr 17, 2019

address most of the feedback from PR hashicorp#6809

a831096

move brokers call go fmt

dthtvwls mentioned this pull request Apr 17, 2019

MSK clusters #8357

Closed

ewbankkit pushed a commit to ewbankkit/terraform-provider-aws that referenced this pull request Apr 30, 2019

address most of the feedback from PR hashicorp#6809

5db1f84

move brokers call go fmt

nywilken assigned bflad May 21, 2019

bflad closed this May 22, 2019

ghost locked and limited conversation to collaborators Mar 30, 2020

	resource.TestCheckResourceAttrSet("data.aws_msk_cluster.test_cluster", "arn"),
	resource.TestCheckResourceAttrPair("data.aws_msk_cluster.test_cluster", "arn", "aws_msk_cluster.test_cluster", "arn"),


		## Attributes Reference

		See the [`aws_msk_cluster` resource](/docs/providers/aws/r/msk_cluster.html) for details on the returned attributes.

+## Timeouts
+`aws_msk_cluster` provides the following [Timeouts](/docs/configuration/resources.html#timeouts) configuration options:
+* `create` - (Default `60m`) How long to wait for cluster creation.
+* `delete` - (Default `120m`) How long to wait for cluster deletion.

Initial add of MSK resources #6809

Initial add of MSK resources #6809

Conversation

jrefi commented Dec 11, 2018 • edited Loading

bflad commented Dec 11, 2018

tomelliff 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

jrefi commented Jan 4, 2019

tomelliff commented Jan 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavinmbell commented Mar 26, 2019

kesensoy commented Mar 26, 2019

landondao1 commented Mar 26, 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

kesensoy commented Mar 29, 2019

dthtvwls commented Apr 3, 2019

jrefi commented Apr 3, 2019

kesensoy commented Apr 4, 2019

dthtvwls commented Apr 5, 2019

bflad commented Apr 5, 2019

dthtvwls commented Apr 11, 2019

dthtvwls commented Apr 17, 2019

bflad commented May 22, 2019

ghost commented Mar 30, 2020

jrefi commented Dec 11, 2018 •

edited

Loading