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.

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

MSK list_nodes ClientVpcIpAddress support #12821

Closed
wants to merge 1 commit into from

Conversation

carl34
Copy link

@carl34 carl34 commented Apr 14, 2020

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" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Example:

output "broker_client_vpc_ip_address" {
  description = "Broker IP Addresses"
  value       "${aws_msk_cluster.example.broker_client_vpc_ip_addresses}"
}

broker_client_vpc_ip_addresses = 10.0.0.1,10.0.0.2,10.0.0.3

Closes #11085

Release note for CHANGELOG:

Add list_nodes support to the aws_msk_cluster resource with a new output containing a comma separated list of all ClientVpcIpAddress values for a cluster.

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSMskCluster_NumberOfBrokerNodes'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -count 1 -parallel 20 -run=TestAccAWSMskCluster_NumberOfBrokerNodes -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSMskCluster_NumberOfBrokerNodes
=== PAUSE TestAccAWSMskCluster_NumberOfBrokerNodes
=== CONT  TestAccAWSMskCluster_NumberOfBrokerNodes
--- PASS: TestAccAWSMskCluster_NumberOfBrokerNodes (1357.48s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws   1358.889s
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws/internal/flatmap  0.471s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags 0.346s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws/internal/naming   0.345s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws/internal/service/batch/equivalency    0.502s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws/internal/service/eks/token    0.093s [no tests to run]
?       github.com/terraform-providers/terraform-provider-aws/awsproviderlint   [no test files]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/awsproviderlint/passes    0.374s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/awsproviderlint/passes/AWSAT001   0.229s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/awsproviderlint/passes/AWSR001    0.302s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/awsproviderlint/passes/fmtsprintfcallexpr 0.371s [no tests to run]

...

@carl34 carl34 requested a review from a team April 14, 2020 19:28
@ghost ghost added size/M Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. dependencies Used to indicate dependency changes. documentation Introduces or discusses updates to documentation. service/kafka Issues and PRs that pertain to the kafka service. labels Apr 14, 2020
@ghost ghost added size/S Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Apr 14, 2020
@ghost ghost added tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. size/M Managed by automation to categorize the size of a PR. and removed size/S Managed by automation to categorize the size of a PR. labels Apr 14, 2020
@carl34
Copy link
Author

carl34 commented Apr 14, 2020

Pushed candidate unit test updates, but they have not yet been run. Working out details on running testacc the the available environment. Please let me know if there are any suggestions.

@bflad bflad removed the dependencies Used to indicate dependency changes. label May 6, 2020
@carl34
Copy link
Author

carl34 commented Jul 15, 2020

Unit tests updated, output from TestAccAWSMskCluster_NumberOfBrokerNodes included above.

@carl34 carl34 changed the title [WIP] MSK list_nodes ClientVpcIpAddress support MSK list_nodes ClientVpcIpAddress support Jul 15, 2020
@carl34 carl34 force-pushed the msk_list_nodes branch 3 times, most recently from c83e673 to 5abf521 Compare July 30, 2020 09:22
Copy link
Collaborator

@DrFaust92 DrFaust92 left a comment

Choose a reason for hiding this comment

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

some comments, ill take a look regarding cluster expansion and readiness (maybe some wait logic is needed here)

aws/resource_aws_msk_cluster.go Outdated Show resolved Hide resolved
aws/resource_aws_msk_cluster.go Outdated Show resolved Hide resolved
aws/resource_aws_msk_cluster_test.go Outdated Show resolved Hide resolved
@ghost ghost added size/S Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Jul 30, 2020
@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Aug 18, 2020
@carl34
Copy link
Author

carl34 commented Aug 18, 2020

@bflad @DrFaust92 thanks for the guidance on this effort, please let me know if there are any new comments.

@carl34 carl34 requested a review from a team as a code owner November 12, 2020 00:41
@carl34
Copy link
Author

carl34 commented Nov 12, 2020

Please let me know if any steps remain before this code can be merged.

@dblooman
Copy link
Contributor

Can we get this merged in so we can have a bumper MSK release

Base automatically changed from master to main January 23, 2021 00:57
@sgLancelot
Copy link

Supporting this pull request. My current work around is doing my head in.

@maheshmadpathi
Copy link

Supporting this PR.

@rattboi
Copy link

rattboi commented Mar 13, 2021

Could really use this right now. Supporting this pr.

@carl34
Copy link
Author

carl34 commented Mar 13, 2021

Thanks for the support @bflad @DrFaust92 @dblooman @sgLancelot @maheshmadpathi @rattboi, I fixed the merge conflicts from when SASL/SCRAM was added.

@carl34 carl34 requested review from DrFaust92 and removed request for a team April 16, 2021 23:34
@carl34
Copy link
Author

carl34 commented May 22, 2021

Squashed commits

@carl34 carl34 marked this pull request as draft May 26, 2021 00:55
@carl34 carl34 marked this pull request as ready for review May 26, 2021 00:55
@carl34 carl34 force-pushed the msk_list_nodes branch 2 times, most recently from 8cd8689 to f2690a4 Compare June 12, 2021 23:37
@carl34
Copy link
Author

carl34 commented Jun 13, 2021

Requesting a maintainer review please.

@ewbankkit ewbankkit removed the needs-triage Waiting for first response or review from a maintainer. label Sep 17, 2021
@ewbankkit
Copy link
Contributor

@carl34 Thanks for the contribution 🎉 👏.
I think the best solution for this will be a new aws_msk_nodes data source as implemented in #20615.

@ewbankkit ewbankkit closed this Sep 17, 2021
@carl34
Copy link
Author

carl34 commented Sep 17, 2021

Thanks @ewbankkit for the reference to #20615, the additional resource is a great addition.

Is the list of clientVpcIpAddress values for a cluster or individual brokers present in a branch or planned to be made available to address #11085 (comment)? An example use-case where it would be convenient to have the full list of clientVpcIpAddress values at the cluster level is the creation of rr-dns entries which include all brokers in a cluster.

Using the BootstrapBrokers bootstrapBrokerString property and looking up the individual IP addresses is one option, but this property includes one broker per AZ rather than the full list.

If the endpoints property in #20615 has the hostname for each broker in a cluster, could a similar property with the list of all clientVpcIpAddress values be added?

If anyone has found a solution to the referenced issue without requiring external scripts, please let us know.

@carl34
Copy link
Author

carl34 commented Sep 17, 2021

Thanks @ewbankkit for adding client_vpc_ip_address per #20615 (comment), 60e8f5d

With this addition I am hoping that there is a way to obtain a list of all cluster IP addresses which would satisfy #11085 (comment) for use as records in https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/route53_record.

For example, given the variable that was planned in this PR,

output "broker_client_vpc_ip_address" {
  description = "Broker IP Addresses"
  value       "${aws_msk_cluster.example.broker_client_vpc_ip_addresses}"
}

broker_client_vpc_ip_addresses = 10.0.0.1,10.0.0.2,10.0.0.3

An A record like the following could be created,

resource "aws_route53_record" "kafka" {
  zone_id = aws_route53_zone.primary.zone_id
  name    = "kafka"
  type    = "A"
  ttl     = "300"
  records = [${aws_msk_cluster.example.broker_client_vpc_ip_addresses}]
}

I don't have a current TF dev environment available and I'm not sure how to perform the join using aws_msk_broker_nodes and client_vpc_ip_address with TF, if it is not too much trouble could we post an example of how to accomplish this with a wildcard, loop, or some other operation? Would something along these lines be feasible to obtain the desired list?

resource "aws_route53_record" "kafka" {
  zone_id = aws_route53_zone.primary.zone_id
  name    = "kafka"
  type    = "A"
  ttl     = "300"
  records = [${aws_msk_broker_nodes.example.*.client_vpc_ip_address}]
}

@ewbankkit
Copy link
Contributor

@carl34 You can use a Terraform for expression to loop over the node_info_list attribute:

data "aws_msk_broker_nodes" "example" {
  cluster_arn = "..."
}

output "client_vpc_ip_addresses" {
  value = join(",", [for node in data.aws_msk_broker_nodes.example.node_info_list : node.client_vpc_ip_address])
}
% terraform apply
...
Outputs:

client_vpc_ip_addresses = "192.168.1.52,192.168.0.248,192.168.2.11"

@carl34
Copy link
Author

carl34 commented Sep 21, 2021

@carl34 You can use a Terraform for expression to loop over the node_info_list attribute:

data "aws_msk_broker_nodes" "example" {
  cluster_arn = "..."
}

output "client_vpc_ip_addresses" {
  value = join(",", [for node in data.aws_msk_broker_nodes.example.node_info_list : node.client_vpc_ip_address])
}
% terraform apply
...
Outputs:

client_vpc_ip_addresses = "192.168.1.52,192.168.0.248,192.168.2.11"

Great, thank you very much @ewbankkit.

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. service/kafka Issues and PRs that pertain to the kafka service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose ClientVpcIpAddress for built Kafka clusters
8 participants